feat(extension): Add BuildManager interface for CI/CD integration by djuloori2794 · Pull Request #106 · uber/submitqueue

djuloori2794 · 2026-02-27T21:47:25Z

Add vendor-agnostic BuildManager interface for managing builds with external CI providers (BuildKite, Jenkins, etc.). Enables orchestrator to schedule builds, poll status, and cancel running builds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

behinddwalls · 2026-02-28T20:04:17Z

+	// Returns:
+	//   - string: Unique build ID that can be used with Poll and CancelBuild methods
+	//   - error: ErrInvalidRequest if validation fails
+	Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) (string, error)


does it make sense to say just "TriggerBuild", whether it's done right away vs scheduled...it's impl details?

I think this is another constraint that we need to put on the implementation by explicitly stating the expected behavior in comments. Which is: implementation should be asynchronous, return fast, and Poll is to be used to check statuses.

which means that "done right away" is not an option to the implementation.
If we believe that the implementation can deduce builds which already happen based on provided change, it can return the build status along with the buildID.
"123, accepted, nil"
"124, succeeded, nil"
So SQ will save on not doing the next polling call.

behinddwalls · 2026-02-28T20:04:41Z

+	//   - BuildStatus: Current state of the build
+	//   - BuildMetadata: Additional metadata about the build (e.g., build URL, commit SHA, duration)
+	//   - error: ErrBuildNotFound if the build doesn't exist
+	Poll(ctx context.Context, buildID string) (entity.BuildStatus, entity.BuildMetadata, error)


I would rename this to `BuildStatus".. poller is just an impl detail?

Not going to nitpick names, but here again is where behavior matters. We can say in the comments, that unlike "Schedule" this implementation might be synchronous and potentially lengthy.
Full abstractions are not efficient unless they are implemented according to expected usage.
Unfortunately, the language itself has limited ways to contract this so we have to rely on conventions (explicit in comments).

behinddwalls · 2026-02-28T20:05:30Z

+	//
+	// Returns:
+	//   - error: ErrBuildNotFound if the build doesn't exist
+	CancelBuild(ctx context.Context, buildID string) error


we can drop Build and say Cancel

behinddwalls · 2026-02-28T20:08:27Z

+	"github.com/uber/submitqueue/entity"
+)
+
+// BuildManager is a vendor-agnostic interface for managing builds with external CI/CD providers.


can we avoid saying CI/CD completely in this interface as it can be anything as per impl...

behinddwalls · 2026-02-28T20:08:53Z

+
+// BuildManager is a vendor-agnostic interface for managing builds with external CI/CD providers.
+// Implementations provide integration with specific CI systems (BuildKite, Jenkins, etc.)
+// to schedule builds, poll their status, and cancel running builds.


poll is again a semantics of impl

Yet we made it a part of the interface.

behinddwalls · 2026-02-28T20:11:47Z

+// Implementations are long-lived singletons (one per build provider) initialized at service
+// startup, similar to Storage and other extension components. They should manage connection
+// pooling, caching, and other resources for the lifetime of the service.
+//


i would avoid saying this, maybe simplify it.

BuildManager is responsible for triggering, cancelling and checking status of builds generated by underlying implementations.

Implementations must be thread-safe.

This is what I asked for so I take this comment.
I think comments need to provide an instruction of how the implementation will be used - as it will ultimately dictate the implementation itself. Can the implementation hold heavy state in the object, or should it use a shared state elsewhere?
A requirement on just thread safety may not be enough to create efficient implementations, as someone implementing them may oversynchronize for the sake of safety.

behinddwalls · 2026-02-28T20:12:34Z

+//
+// All implementations must be thread-safe and support concurrent operations.
+type BuildManager interface {
+	// Schedule submits a list of changes to the CI provider for processing.


no CI in docs

behinddwalls · 2026-02-28T20:13:05Z

+	// The implementation is responsible for:
+	//   - Looking up the job name from the queue configuration
+	//   - Creating appropriate builds/jobs for each change based on its action
+	//   - Handling dependencies between changes (order may be significant)


generally order is always important

behinddwalls · 2026-02-28T20:13:20Z

+	//
+	// The implementation is responsible for:
+	//   - Looking up the job name from the queue configuration
+	//   - Creating appropriate builds/jobs for each change based on its action


jobs is very impl specific? so we should just say build

behinddwalls · 2026-02-28T20:13:49Z

+	//   - changes: List of changes to process. Order may be significant for dependencies.
+	//
+	// Returns:
+	//   - string: Unique build ID that can be used with Poll and CancelBuild methods


replace Poll with BuildStatus or for checking the build status

sbalabanov · 2026-03-03T02:00:38Z

+
+// BuildManager is a vendor-agnostic interface for managing builds with external CI/CD providers.
+// Implementations provide integration with specific CI systems (BuildKite, Jenkins, etc.)
+// to schedule builds, poll their status, and cancel running builds.


Yet we made it a part of the interface.

sbalabanov · 2026-03-03T02:04:17Z

+// Implementations are long-lived singletons (one per build provider) initialized at service
+// startup, similar to Storage and other extension components. They should manage connection
+// pooling, caching, and other resources for the lifetime of the service.
+//


This is what I asked for so I take this comment.
I think comments need to provide an instruction of how the implementation will be used - as it will ultimately dictate the implementation itself. Can the implementation hold heavy state in the object, or should it use a shared state elsewhere?
A requirement on just thread safety may not be enough to create efficient implementations, as someone implementing them may oversynchronize for the sake of safety.

sbalabanov · 2026-03-03T02:06:51Z

+	// Returns:
+	//   - string: Unique build ID that can be used with Poll and CancelBuild methods
+	//   - error: ErrInvalidRequest if validation fails
+	Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) (string, error)


I think this is another constraint that we need to put on the implementation by explicitly stating the expected behavior in comments. Which is: implementation should be asynchronous, return fast, and Poll is to be used to check statuses.

sbalabanov · 2026-03-03T02:10:21Z

+	// Returns:
+	//   - string: Unique build ID that can be used with Poll and CancelBuild methods
+	//   - error: ErrInvalidRequest if validation fails
+	Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) (string, error)


which means that "done right away" is not an option to the implementation.
If we believe that the implementation can deduce builds which already happen based on provided change, it can return the build status along with the buildID.
"123, accepted, nil"
"124, succeeded, nil"
So SQ will save on not doing the next polling call.

sbalabanov · 2026-03-03T02:14:13Z

+	//   - BuildStatus: Current state of the build
+	//   - BuildMetadata: Additional metadata about the build (e.g., build URL, commit SHA, duration)
+	//   - error: ErrBuildNotFound if the build doesn't exist
+	Poll(ctx context.Context, buildID string) (entity.BuildStatus, entity.BuildMetadata, error)


Not going to nitpick names, but here again is where behavior matters. We can say in the comments, that unlike "Schedule" this implementation might be synchronous and potentially lengthy.
Full abstractions are not efficient unless they are implemented according to expected usage.
Unfortunately, the language itself has limited ways to contract this so we have to rely on conventions (explicit in comments).

sbalabanov · 2026-03-03T02:15:50Z

+Schedules builds, polls their status, and cancels running builds.
+
+```go
+type BuildManager interface {


plz avoid copying the code to the docs.

examples on "how to" are ok

sbalabanov · 2026-03-03T02:17:38Z

+
+**Thread-safety**: All implementations must be thread-safe and support concurrent operations.
+
+**Singleton Design**: Implementations are long-lived singletons (one per build provider) initialized at service startup, similar to Storage and other extension components. They should manage connection pooling, caching, and other resources for the lifetime of the service.


Also important to note how transient errors are handled. Are the implementation expected to reestablish the connection if it is lost, and give back errors on interface requests in the interim?

## Summary Adds a vendor-agnostic `BuildManager` interface under `extension/build` so the orchestrator can drive builds against an external provider (BuildKite, Jenkins, an internal job runner, etc.) without coupling to any one of them. Ports the closed upstream PR #106, refined per its review feedback (naming, contract clarity, no CI/CD or caller-specific wording, no premature error sentinels). ### Interface `BuildManager` exposes four verbs — `Trigger`, `Status`, `Cancel`, `Close`: - `Trigger` returns `(buildID, status, error)`; an implementation that can deduce an already-finished build returns its terminal status so the caller can skip the first `Status` poll, otherwise `Accepted`. - `Status` may be synchronous and lengthy (a provider round trip); `Trigger`/`Cancel` must return promptly. - The godoc spells out what is required of an implementation: concurrency safety, internal recovery from transient connectivity failures, transient-only local state, and plain errors left for the controller to classify per `core/errs`. No custom error sentinels ship yet — they land with the first implementation that needs them. ### Entity changes - `entity.BuildStatus` collapses to `{Unknown, Accepted, Succeeded, Failed, Cancelled}` — drops `Queued`/`Running`/`Blocked` (provider-specific stages) and renames `Passed → Succeeded`. Orchestrator build/buildsignal references move to `Accepted`. - Adds `entity.ChangeAction`, `entity.BuildChange`, `entity.BuildMetadata` as `Trigger` inputs/outputs. ### Out of scope No backend implementation and no orchestrator wiring — those are follow-ups. ## Test Plan - ✅ \`make fmt && make lint\` - ✅ \`make check-tidy && make check-gazelle && make check-mocks\` - ✅ \`bazel build //...\` - ✅ \`bazel test //... --test_tag_filters=-integration\` — 33/33 (integration tests require Docker, not run here) ## Issues (none)

## Summary Adds a vendor-agnostic `BuildManager` interface under `extension/build` so the orchestrator can drive builds against an external provider (BuildKite, Jenkins, an internal job runner, etc.) without coupling to any one of them. Ports the closed upstream PR #106, refined per its review feedback (naming, contract clarity, no CI/CD or caller-specific wording, no premature error sentinels). ### Interface `BuildManager` exposes four verbs — `Trigger`, `Status`, `Cancel`, `Close`: - `Trigger` returns `(buildID, status, error)`; an implementation that can deduce an already-finished build returns its terminal status so the caller can skip the first `Status` poll, otherwise `Accepted`. - `Status` may be synchronous and lengthy (a provider round trip); `Trigger`/`Cancel` must return promptly. - The godoc spells out what is required of an implementation: concurrency safety, internal recovery from transient connectivity failures, transient-only local state, and plain errors left for the controller to classify per `core/errs`. No custom error sentinels ship yet — they land with the first implementation that needs them. ### Entity changes - `entity.BuildStatus` is `{Unknown, Accepted, Running, Succeeded, Failed, Cancelled}`. This drops the provider-specific `Queued`/`Blocked` stages and renames `Passed → Succeeded`, while keeping `Running` as a first-class non-terminal state so callers can distinguish a queued/accepted build from one that is actively executing. Orchestrator build/buildsignal references use `Accepted`. - Adds `entity.ChangeAction`, `entity.BuildChange`, `entity.BuildMetadata` as `Trigger` inputs/outputs. ### Out of scope No backend implementation and no orchestrator wiring — those are follow-ups. ## Test Plan - ✅ \`make fmt && make lint\` - ✅ \`make check-tidy && make check-gazelle && make check-mocks\` - ✅ \`bazel build //...\` - ✅ \`bazel test //... --test_tag_filters=-integration\` (integration tests require Docker, not run here) ## Issues (none)

feat(extension): Add BuildManager interface for CI/CD integration

e7a2024

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

djuloori2794 requested review from a team, behinddwalls and sbalabanov as code owners February 27, 2026 21:47

behinddwalls reviewed Feb 28, 2026

View reviewed changes

sbalabanov reviewed Mar 3, 2026

View reviewed changes

sbalabanov requested changes Mar 4, 2026

View reviewed changes

behinddwalls closed this May 28, 2026

behinddwalls mentioned this pull request May 29, 2026

feat(build): add BuildManager extension and trim BuildStatus enum #167

Closed

behinddwalls deleted the djuloori/build branch June 2, 2026 18:40


		Thread-safety: All implementations must be thread-safe and support concurrent operations.

		Singleton Design: Implementations are long-lived singletons (one per build provider) initialized at service startup, similar to Storage and other extension components. They should manage connection pooling, caching, and other resources for the lifetime of the service.

Conversation

djuloori2794 commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

djuloori2794 commented Feb 27, 2026 •

edited

Loading