From 343fd791fc6f28f4d270a4666416351b11ac568b Mon Sep 17 00:00:00 2001 From: djuloori Date: Tue, 24 Feb 2026 23:22:03 +0000 Subject: [PATCH 01/18] feat(extension): add BuildManager extension for CI/CD integration 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. Key components: - BuildManager interface with ScheduleBuild, Poll, CancelBuild methods - BuildID string type with URI format (provider://id) - BuildState enum with IsTerminal() for state management - BuildStatus with timestamps, URLs, and metadata - Sentinel errors for common failure modes Implementations: - mock/: mockgen-generated gomock mocks for unit testing - inmemory/: functional in-memory implementation for integration testing - Simulates async build execution with goroutines - Deterministic test behavior based on batch IDs - State callbacks for testing without time.Sleep Test coverage: - Complete coverage of BuildState.IsTerminal() business logic - BuildID parsing and construction - In-memory implementation with state transitions - Mock compilation and usage verification Co-Authored-By: Claude Opus 4.6 --- entity/build/BUILD.bazel | 25 + entity/build/build_id.go | 68 +++ entity/build/build_id_test.go | 132 +++++ entity/build/build_state.go | 40 ++ entity/build/build_state_test.go | 159 ++++++ entity/build/build_status.go | 40 ++ extension/build/BUILD.bazel | 15 + extension/build/README.md | 442 +++++++++++++++ extension/build/build_manager.go | 67 +++ extension/build/errors.go | 74 +++ extension/build/inmemory/BUILD.bazel | 26 + .../build/inmemory/in_memory_build_manager.go | 531 ++++++++++++++++++ .../inmemory/in_memory_build_manager_test.go | 323 +++++++++++ extension/build/mock/BUILD.bazel | 24 + extension/build/mock/build_manager.go | 101 ++++ extension/build/mock/build_manager_test.go | 39 ++ 16 files changed, 2106 insertions(+) create mode 100644 entity/build/BUILD.bazel create mode 100644 entity/build/build_id.go create mode 100644 entity/build/build_id_test.go create mode 100644 entity/build/build_state.go create mode 100644 entity/build/build_state_test.go create mode 100644 entity/build/build_status.go create mode 100644 extension/build/BUILD.bazel create mode 100644 extension/build/README.md create mode 100644 extension/build/build_manager.go create mode 100644 extension/build/errors.go create mode 100644 extension/build/inmemory/BUILD.bazel create mode 100644 extension/build/inmemory/in_memory_build_manager.go create mode 100644 extension/build/inmemory/in_memory_build_manager_test.go create mode 100644 extension/build/mock/BUILD.bazel create mode 100644 extension/build/mock/build_manager.go create mode 100644 extension/build/mock/build_manager_test.go diff --git a/entity/build/BUILD.bazel b/entity/build/BUILD.bazel new file mode 100644 index 00000000..8a1818c0 --- /dev/null +++ b/entity/build/BUILD.bazel @@ -0,0 +1,25 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "build", + srcs = [ + "build_id.go", + "build_state.go", + "build_status.go", + ], + importpath = "github.com/uber/submitqueue/entity/build", + visibility = ["//visibility:public"], +) + +go_test( + name = "build_test", + srcs = [ + "build_id_test.go", + "build_state_test.go", + ], + embed = [":build"], + deps = [ + "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", + ], +) diff --git a/entity/build/build_id.go b/entity/build/build_id.go new file mode 100644 index 00000000..b7e71315 --- /dev/null +++ b/entity/build/build_id.go @@ -0,0 +1,68 @@ +package build + +import "fmt" + +// BuildID uniquely identifies a build across all CI providers. +// Format: "provider://provider-specific-id" +// +// The BuildID uses a URI-like format that combines the provider name and +// provider-specific identifier into a single string for simplicity in storage, +// logging, and serialization. +// +// Examples: +// - "buildkite://uber/submitqueue-ci/123" +// - "jenkins://456" +// - "mock://1" +// +// For database storage, this single string format avoids the complexity of +// composite keys and makes it easy to use as a primary key or index. +type BuildID string + +// String returns the BuildID as a string. +// This is the canonical representation for logging, storage, and display. +func (b BuildID) String() string { + return string(b) +} + +// NewBuildID constructs a BuildID from a provider name and provider-specific ID. +// The provider should be a short identifier (e.g., "buildkite", "jenkins", "mock"). +// The id should be the provider's build identifier in whatever format they use. +func NewBuildID(provider, id string) BuildID { + return BuildID(fmt.Sprintf("%s://%s", provider, id)) +} + +// ParseBuildID extracts the provider and ID components from a BuildID string. +// Returns the provider name, provider-specific ID, and an error if the format is invalid. +// +// Expected format: "provider://id" +func ParseBuildID(buildID BuildID) (provider string, id string, err error) { + s := string(buildID) + + // Find the separator + sep := "://" + idx := len(s) + for i := 0; i < len(s)-len(sep)+1; i++ { + if s[i:i+len(sep)] == sep { + idx = i + break + } + } + + // Check if separator was found + if idx == len(s) { + return "", "", fmt.Errorf("invalid BuildID format: missing '://' separator in %q", s) + } + + provider = s[:idx] + id = s[idx+len(sep):] + + // Validate components are not empty + if provider == "" { + return "", "", fmt.Errorf("invalid BuildID format: empty provider in %q", s) + } + if id == "" { + return "", "", fmt.Errorf("invalid BuildID format: empty ID in %q", s) + } + + return provider, id, nil +} diff --git a/entity/build/build_id_test.go b/entity/build/build_id_test.go new file mode 100644 index 00000000..854cf4a6 --- /dev/null +++ b/entity/build/build_id_test.go @@ -0,0 +1,132 @@ +package build + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewBuildID(t *testing.T) { + testCases := []struct { + name string + provider string + id string + expected string + }{ + { + name: "buildkite with org/pipeline/number", + provider: "buildkite", + id: "uber/submitqueue-ci/123", + expected: "buildkite://uber/submitqueue-ci/123", + }, + { + name: "jenkins with build number", + provider: "jenkins", + id: "456", + expected: "jenkins://456", + }, + { + name: "mock with sequential number", + provider: "mock", + id: "1", + expected: "mock://1", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + buildID := NewBuildID(tc.provider, tc.id) + assert.Equal(t, tc.expected, string(buildID)) + assert.Equal(t, tc.expected, buildID.String()) + }) + } +} + +func TestParseBuildID(t *testing.T) { + testCases := []struct { + name string + buildID BuildID + expectedProvider string + expectedID string + expectError bool + }{ + { + name: "valid buildkite ID", + buildID: "buildkite://uber/submitqueue-ci/123", + expectedProvider: "buildkite", + expectedID: "uber/submitqueue-ci/123", + expectError: false, + }, + { + name: "valid jenkins ID", + buildID: "jenkins://456", + expectedProvider: "jenkins", + expectedID: "456", + expectError: false, + }, + { + name: "valid mock ID", + buildID: "mock://1", + expectedProvider: "mock", + expectedID: "1", + expectError: false, + }, + { + name: "missing separator", + buildID: "buildkite-123", + expectError: true, + }, + { + name: "empty provider", + buildID: "://123", + expectError: true, + }, + { + name: "empty ID", + buildID: "buildkite://", + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + provider, id, err := ParseBuildID(tc.buildID) + + if tc.expectError { + require.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, tc.expectedProvider, provider) + assert.Equal(t, tc.expectedID, id) + }) + } +} + +func TestBuildIDRoundTrip(t *testing.T) { + testCases := []struct { + provider string + id string + }{ + {"buildkite", "uber/submitqueue-ci/123"}, + {"jenkins", "456"}, + {"mock", "1"}, + } + + for _, tc := range testCases { + t.Run(tc.provider, func(t *testing.T) { + // Create BuildID + buildID := NewBuildID(tc.provider, tc.id) + + // Parse it back + provider, id, err := ParseBuildID(buildID) + require.NoError(t, err) + + // Verify round trip + assert.Equal(t, tc.provider, provider) + assert.Equal(t, tc.id, id) + }) + } +} diff --git a/entity/build/build_state.go b/entity/build/build_state.go new file mode 100644 index 00000000..b8faa0fd --- /dev/null +++ b/entity/build/build_state.go @@ -0,0 +1,40 @@ +package build + +// BuildState represents the execution state of a build. +// This is a string enum following SubmitQueue's pattern of using string enums with sentinel values. +type BuildState string + +const ( + // BuildStateUnknown is the sentinel value for unknown or unreachable build states. + // This should only occur if the provider returns an unexpected state value. + BuildStateUnknown BuildState = "" + + // BuildStateQueued indicates the build has been scheduled but not yet started. + BuildStateQueued BuildState = "queued" + + // BuildStateRunning indicates the build is currently executing. + BuildStateRunning BuildState = "running" + + // BuildStatePassed indicates the build completed successfully. + // This is a terminal state. + BuildStatePassed BuildState = "passed" + + // BuildStateFailed indicates the build completed with failures. + // This is a terminal state. + BuildStateFailed BuildState = "failed" + + // BuildStateCancelled indicates the build was cancelled before completion. + // This is a terminal state. + BuildStateCancelled BuildState = "cancelled" + + // BuildStateBlocked indicates the build is waiting for manual approval or unblocking. + // Some CI systems (like BuildKite) support manual approval steps. + BuildStateBlocked BuildState = "blocked" +) + +// IsTerminal returns true if the build state represents a final state (passed, failed, or cancelled). +// Terminal states indicate the build has finished and will not change state again. +// Note: BuildStateBlocked is NOT terminal as blocked builds can be unblocked and continue execution. +func (s BuildState) IsTerminal() bool { + return s == BuildStatePassed || s == BuildStateFailed || s == BuildStateCancelled +} diff --git a/entity/build/build_state_test.go b/entity/build/build_state_test.go new file mode 100644 index 00000000..75ecae99 --- /dev/null +++ b/entity/build/build_state_test.go @@ -0,0 +1,159 @@ +package build + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestBuildState_IsTerminal tests the IsTerminal method for all build states. +func TestBuildState_IsTerminal(t *testing.T) { + testCases := []struct { + name string + state BuildState + expected bool + }{ + { + name: "passed is terminal", + state: BuildStatePassed, + expected: true, + }, + { + name: "failed is terminal", + state: BuildStateFailed, + expected: true, + }, + { + name: "cancelled is terminal", + state: BuildStateCancelled, + expected: true, + }, + { + name: "queued is not terminal", + state: BuildStateQueued, + expected: false, + }, + { + name: "running is not terminal", + state: BuildStateRunning, + expected: false, + }, + { + name: "blocked is not terminal", + state: BuildStateBlocked, + expected: false, + }, + { + name: "unknown is not terminal", + state: BuildStateUnknown, + expected: false, + }, + { + name: "empty string is not terminal", + state: BuildState(""), + expected: false, + }, + { + name: "invalid state is not terminal", + state: BuildState("invalid"), + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := tc.state.IsTerminal() + assert.Equal(t, tc.expected, result, + "IsTerminal() for %s should return %v", tc.state, tc.expected) + }) + } +} + +// TestBuildState_Constants verifies the string values of all build state constants. +func TestBuildState_Constants(t *testing.T) { + testCases := []struct { + name string + state BuildState + expected string + }{ + { + name: "unknown", + state: BuildStateUnknown, + expected: "", + }, + { + name: "queued", + state: BuildStateQueued, + expected: "queued", + }, + { + name: "running", + state: BuildStateRunning, + expected: "running", + }, + { + name: "passed", + state: BuildStatePassed, + expected: "passed", + }, + { + name: "failed", + state: BuildStateFailed, + expected: "failed", + }, + { + name: "cancelled", + state: BuildStateCancelled, + expected: "cancelled", + }, + { + name: "blocked", + state: BuildStateBlocked, + expected: "blocked", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, string(tc.state), + "BuildState constant should have expected string value") + }) + } +} + +// TestBuildState_ZeroValue verifies that the zero value is BuildStateUnknown. +func TestBuildState_ZeroValue(t *testing.T) { + var state BuildState + assert.Equal(t, BuildStateUnknown, state, "Zero value should be BuildStateUnknown") + assert.Equal(t, "", string(state), "Zero value should be empty string") + assert.False(t, state.IsTerminal(), "Zero value should not be terminal") +} + +// TestBuildState_TerminalStates verifies all terminal states are accounted for. +func TestBuildState_TerminalStates(t *testing.T) { + terminalStates := []BuildState{ + BuildStatePassed, + BuildStateFailed, + BuildStateCancelled, + } + + for _, state := range terminalStates { + assert.True(t, state.IsTerminal(), + "State %s should be terminal", state) + } +} + +// TestBuildState_NonTerminalStates verifies all non-terminal states are accounted for. +func TestBuildState_NonTerminalStates(t *testing.T) { + nonTerminalStates := []BuildState{ + BuildStateUnknown, + BuildStateQueued, + BuildStateRunning, + BuildStateBlocked, + } + + for _, state := range nonTerminalStates { + assert.False(t, state.IsTerminal(), + "State %s should not be terminal", state) + } +} diff --git a/entity/build/build_status.go b/entity/build/build_status.go new file mode 100644 index 00000000..e3edc623 --- /dev/null +++ b/entity/build/build_status.go @@ -0,0 +1,40 @@ +package build + +// BuildStatus represents the current state of a build as returned by BuildManager.Poll(). +// It contains all information about the build's lifecycle, including timing, state, and URLs. +type BuildStatus struct { + // ID is the unique identifier for this build. + ID BuildID + + // State is the current execution state of the build (queued, running, passed, failed, etc.). + State BuildState + + // QueuedAt is the Unix timestamp in milliseconds when the build was queued. + // Zero if the build has not been queued yet. + QueuedAt int64 + + // StartedAt is the Unix timestamp in milliseconds when the build started executing. + // Zero if the build has not started yet. + StartedAt int64 + + // FinishedAt is the Unix timestamp in milliseconds when the build finished (passed, failed, or cancelled). + // Zero if the build has not finished yet. + FinishedAt int64 + + // WebURL is a link to view the build in the CI provider's web UI. + // Empty string if not available. + WebURL string + + // LogsURL is a direct link to the build's logs. + // Empty string if not available. + LogsURL string + + // ErrorMessage contains error details for failed builds. + // Empty string for successful builds or builds that haven't finished. + ErrorMessage string + + // Metadata contains provider-specific metadata that doesn't fit in the standard fields. + // Examples: commit author, branch, test results summary, etc. + // Never nil - always initialized to at least an empty map. + Metadata map[string]string +} diff --git a/extension/build/BUILD.bazel b/extension/build/BUILD.bazel new file mode 100644 index 00000000..88b91c1e --- /dev/null +++ b/extension/build/BUILD.bazel @@ -0,0 +1,15 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "build", + srcs = [ + "build_manager.go", + "errors.go", + ], + importpath = "github.com/uber/submitqueue/extension/build", + visibility = ["//visibility:public"], + deps = [ + "//entity", + "//entity/build", + ], +) diff --git a/extension/build/README.md b/extension/build/README.md new file mode 100644 index 00000000..df7dd605 --- /dev/null +++ b/extension/build/README.md @@ -0,0 +1,442 @@ +# BuildManager Extension + +Vendor-agnostic interface for managing builds with external CI/CD providers. + +## Overview + +The BuildManager extension provides a clean abstraction for integrating with CI/CD systems like BuildKite, Jenkins, and others. It allows the Orchestrator service to schedule builds, poll their status, and cancel running builds without being coupled to any specific CI provider. + +## Interfaces + +### BuildManager + +Main interface for interacting with CI providers. + +```go +type BuildManager interface { + // Schedule a new build with the CI provider for testing a batch + ScheduleBuild( + ctx context.Context, + baseSHA string, + speculatedBatchesToBeApplied []entity.BatchDependent, + batchToBeTest entity.Batch, + repoURL string, + branch string, + pipelineID string, + sqid string, + env map[string]string, + message string, + ) (BuildID, error) + + // Poll current status of a build + Poll(ctx context.Context, id BuildID) (BuildStatus, error) + + // Cancel a running or queued build + CancelBuild(ctx context.Context, id BuildID) error +} +``` + +**Thread-safety**: All implementations must be thread-safe and support concurrent operations. + +## Types + +### ScheduleBuild Parameters + +**baseSHA** (string, required): SHA of the main/base branch - the starting point for applying batches + +**speculatedBatchesToBeApplied** ([]entity.BatchDependent, optional): Batches that were applied on main before the test batch. + +**batchToBeTest** (entity.Batch, required): The batch being tested. + +**repoURL** (string, required): Repository URL (e.g., "https://github.com/uber/submitqueue") + +**branch** (string, required): Target branch name (e.g., "main") + +**pipelineID** (string, required): Provider-specific pipeline identifier +- BuildKite: "organization/pipeline-slug" +- Jenkins: "job-name" +- Mock: any string + +**sqid** (string, required): SubmitQueue request ID for correlation and tracing + +**env** (map[string]string, optional): Additional environment variables to pass to CI + +**message** (string, optional): Human-readable build description + +### BuildID + +Unique identifier for a build using a URI-like format: + +```go +type BuildID string + +// Constructor to create a BuildID from provider and ID components +func NewBuildID(provider, id string) BuildID + +// Parser to extract provider and ID from a BuildID +func ParseBuildID(buildID BuildID) (provider string, id string, err error) + +// String returns the BuildID as a string ("provider://id" format) +func (b BuildID) String() string +``` + +**Format**: `"provider://id"` + +**Examples**: +- `"buildkite://uber/submitqueue-ci/123"` +- `"jenkins://456"` +- `"mock://1"` + +**Usage**: +```go +// Create a BuildID +buildID := entitybuild.NewBuildID("buildkite", "uber/submitqueue-ci/123") +fmt.Println(buildID.String()) // "buildkite://uber/submitqueue-ci/123" + +// Parse a BuildID +provider, id, err := entitybuild.ParseBuildID(buildID) +// provider = "buildkite" +// id = "uber/submitqueue-ci/123" +``` + +### BuildStatus + +Output from polling a build: + +```go +type BuildStatus struct { + ID BuildID + State BuildState // Current execution state + QueuedAt int64 // Unix milliseconds + StartedAt int64 // Unix milliseconds + FinishedAt int64 // Unix milliseconds + WebURL string // Link to build UI + LogsURL string // Link to logs + ErrorMessage string // Error details for failures + Metadata map[string]string // Provider-specific metadata (never nil) +} +``` + +**Metadata guarantee**: The `Metadata` field is never nil. Implementations must always initialize it to at least an empty map. Consumers can safely iterate over it without nil checks. + +### BuildState + +Enum representing build execution state: + +```go +type BuildState string + +const ( + BuildStateUnknown BuildState = "" // Sentinel value + BuildStateQueued BuildState = "queued" // Scheduled but not started + BuildStateRunning BuildState = "running" // Currently executing + BuildStatePassed BuildState = "passed" // Completed successfully (terminal) + BuildStateFailed BuildState = "failed" // Completed with failures (terminal) + BuildStateCancelled BuildState = "cancelled" // Cancelled before completion (terminal) + BuildStateBlocked BuildState = "blocked" // Waiting for manual approval +) + +// IsTerminal returns true for passed/failed/cancelled states +func (s BuildState) IsTerminal() bool +``` + +## Error Handling + +The extension defines sentinel errors following the SubmitQueue pattern: + +- **`ErrBuildNotFound`** - Build doesn't exist or was deleted +- **`ErrBuildNotCancellable`** - Build has already finished +- **`ErrProviderUnavailable`** - CI provider unreachable or experiencing errors +- **`ErrInvalidRequest`** - Request validation failed + +Each error has helper functions: +- `Is{Error}(err)` - Check if error is of specific type +- `Wrap{Error}(err)` - Wrap provider-specific errors + +Example: +```go +status, err := buildMgr.Poll(ctx, buildID) +if build.IsBuildNotFound(err) { + // Handle missing build +} +``` + +## Usage + +### In-Memory Implementation + +For testing without external dependencies, use the in-memory implementation: + +```go +import ( + "github.com/uber/submitqueue/extension/build/inmemory" + "github.com/uber/submitqueue/entity" +) + +// Create in-memory build manager +mgr := inmemory.NewInMemoryBuildManager(inmemory.Params{ + BuildDelay: 100 * time.Millisecond, // How long simulated builds take +}) + +// Create test batch +testBatch := entity.Batch{ + ID: "queue-1/batch/5", + Queue: "queue-1", + Contains: []string{"queue-1/10", "queue-1/11"}, + State: entity.BatchStateUnknown, + Version: 1, +} + +// Create base batches (already applied on main) +baseBatches := []entity.BatchDependent{ + {BatchID: "queue-1/batch/1"}, + {BatchID: "queue-1/batch/2"}, +} + +// Schedule build +buildID, err := mgr.ScheduleBuild( + ctx, + "abc123def456", // baseSHA (main branch HEAD) + baseBatches, // base batches + testBatch, // batch to test + "https://github.com/uber/submitqueue", + "main", + "test-pipeline", + "queue-1/10", + map[string]string{"CUSTOM_VAR": "value"}, + "Testing batch queue-1/batch/5", +) +if err != nil { + // Handle error +} + +// Poll until complete +for { + status, err := mgr.Poll(ctx, buildID) + if err != nil { + // Handle error + } + + if status.State.IsTerminal() { + fmt.Printf("Build finished: %s\n", status.State) + fmt.Printf("Build metadata: %+v\n", status.Metadata) + break + } + + time.Sleep(1 * time.Second) +} +``` + +**Deterministic Testing**: The mock supports predictable behavior: +- Batch ID containing `"fail"` → `BuildStateFailed` +- Batch ID containing `"block"` → `BuildStateBlocked` +- All other batches → `BuildStatePassed` + +**State Change Callbacks**: For deterministic testing without `time.Sleep`, use callbacks: + +```go +// Create channel to track state changes +stateChanges := make(chan build.BuildState, 10) + +mgr := inmemory.NewInMemoryBuildManager(inmemory.Params{ + BuildDelay: 100 * time.Millisecond, + OnStateChange: func(buildID string, state build.BuildState) { + stateChanges <- state + }, +}) + +testBatch := entity.Batch{ + ID: "queue-1/batch/1", + Queue: "queue-1", + Contains: []string{"queue-1/1"}, +} + +buildID, _ := mgr.ScheduleBuild(ctx, "abc123", []entity.BatchDependent{}, testBatch, + "https://github.com/uber/submitqueue", "main", "test", "queue-1/1", nil, "") + +// Wait for running state +state := <-stateChanges // Will receive BuildStateRunning + +// Wait for terminal state +state = <-stateChanges // Will receive BuildStatePassed/Failed/Cancelled +``` + +**In-Memory Implementation Details**: +- PipelineID format: Any string (not validated) +- BuildID format: Sequential numbers ("1", "2", "3", etc.) +- BuildID.String() format: `"mock://1"`, `"mock://2"`, etc. + +### GoMock Mocks + +For unit testing with gomock, use the generated mock: + +```go +import ( + "testing" + + "github.com/uber/submitqueue/extension/build/mock" + "go.uber.org/mock/gomock" +) + +func TestMyController(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockBuildMgr := mock.NewMockBuildManager(ctrl) + + // Set up expectations + mockBuildMgr.EXPECT(). + ScheduleBuild(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), + gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), + gomock.Any(), gomock.Any()). + Return(entitybuild.NewBuildID("mock", "1"), nil) + + // Test your code that uses the mock +} +``` + +### Batch Metadata + +BuildManager uses batch information to enrich CI builds with metadata: + +**Environment Variables**: Automatically added to the build environment: +- `SQ_BASE_SHA`: SHA of the main branch +- `SQ_BASE_BATCHES`: Comma-separated list of base batch IDs (e.g., "queue-1/batch/1,queue-1/batch/2") +- `SQ_TEST_BATCH`: ID of the batch being tested + +**Build Message**: Automatically generated description like: +"Testing batch queue-1/batch/5 on top of queue-1/batch/1, queue-1/batch/2" + +**Metadata Map**: BuildStatus.Metadata includes: +- `base_sha`: Same as baseSHA parameter +- `base_batches`: Same as SQ_BASE_BATCHES +- `test_batch`: ID of the batch being tested +- `repo`: Repository URL +- `branch`: Target branch +- `sqid`: SubmitQueue request ID + +### Cancelling Builds + +```go +err := mgr.CancelBuild(ctx, buildID) +if build.IsBuildNotCancellable(err) { + // Build already finished +} else if err != nil { + // Other error +} +``` + +## Implementing a New Provider + +To add support for a new CI provider: + +1. **Create provider directory**: `extension/build/{provider}/` + +2. **Implement BuildManager interface**: + ```go + package jenkins + + import "github.com/uber/submitqueue/extension/build" + + type Params struct { + // Provider-specific configuration + BaseURL string + Username string + APIToken string + Logger *zap.Logger + MetricsScope tally.Scope + } + + func NewBuildManager(params Params) (build.BuildManager, error) { + // Validate params + // Create HTTP client + // Return implementation + } + ``` + +3. **Map provider states to BuildState enum**: + - Map provider's state values to the standard `BuildState` constants + - Use `BuildStateUnknown` for unexpected states + +4. **Initialize BuildStatus with non-nil Metadata**: + - Always initialize `Metadata` field to at least an empty map: `map[string]string{}` + - Never return a BuildStatus with nil Metadata - this is a documented guarantee + - Populate with provider-specific information (commit SHA, author, test counts, etc.) + +5. **Handle provider errors**: + - 404 errors → `build.WrapBuildNotFound()` + - 5xx errors → `build.WrapProviderUnavailable()` + - Validation errors → `build.WrapInvalidRequest()` + +6. **Define PipelineID format**: + - Document the expected format in provider README + - Examples: + - BuildKite: `"organization/pipeline-slug"` + - Jenkins: `"job-name"` + - Mock: `"any-string"` (not validated) + +7. **Add tests**: + - Unit tests with mock HTTP server + - Validation tests for all required fields + - Error mapping tests + - Thread-safety tests + +8. **Update BUILD.bazel**: + ```bazel + go_library( + name = "jenkins", + srcs = ["jenkins.go"], + importpath = "github.com/uber/submitqueue/extension/build/jenkins", + visibility = ["//visibility:public"], + deps = [ + "//extension/build", + "@org_uber_go_zap//:zap", + "@com_github_uber_go_tally_v4//:tally", + ], + ) + ``` + +## Architecture + +### Extension Pattern + +Following the established SubmitQueue extension pattern: + +``` +extension/build/ +├── build_manager.go # Interface definition +├── types.go # Request/Status/ID types +├── errors.go # Sentinel errors +├── README.md # This file +├── BUILD.bazel # Bazel configuration +├── mock/ # Generated gomock mocks +│ ├── build_manager.go # Generated by mockgen +│ └── BUILD.bazel +├── inmemory/ # In-memory implementation for testing +│ ├── in_memory_build_manager.go +│ ├── in_memory_build_manager_test.go +│ └── BUILD.bazel +└── {provider}/ # Provider implementations (future) + ├── {provider}.go + ├── {provider}_test.go + └── BUILD.bazel +``` + +### Design Principles + +1. **Vendor-agnostic**: Interface doesn't leak provider-specific details +2. **Immutable types**: BuildRequest and BuildStatus are value types +3. **Thread-safe**: All implementations support concurrent operations +4. **Error transparency**: Sentinel errors for common failure modes +5. **No persistent state**: BuildManager doesn't manage connections or require cleanup + +## Future Enhancements + +Potential improvements not in the current implementation: + +- **Webhook support**: Accept push notifications from CI providers instead of polling +- **Build artifacts**: Track and retrieve build artifacts +- **Retry logic**: Automatic retry for transient failures +- **Batch operations**: Schedule/poll/cancel multiple builds at once +- **Streaming logs**: Real-time log streaming from builds +- **Build caching**: Cache build status to reduce API calls diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go new file mode 100644 index 00000000..3a540625 --- /dev/null +++ b/extension/build/build_manager.go @@ -0,0 +1,67 @@ +package build + +//go:generate go run go.uber.org/mock/mockgen -source=build_manager.go -destination=mock/build_manager.go -package=mock + +import ( + "context" + + "github.com/uber/submitqueue/entity" + entitybuild "github.com/uber/submitqueue/entity/build" +) + +// 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. +// +// All implementations must be thread-safe and support concurrent operations. +type BuildManager interface { + // ScheduleBuild creates a new build with the CI provider for testing a batch. + // + // The baseSHA represents the starting point (main branch HEAD). + // The speculatedBatchesToBeApplied are batches already applied on main. + // The batchToBeTest is the batch being tested on top of those base batches. + // + // Batch information is used to: + // - Generate descriptive build messages + // - Add environment variables for CI scripts + // - Populate metadata for tracing and debugging + // + // Returns: + // - BuildID: Unique identifier for the scheduled build + // - error: ErrInvalidRequest if validation fails, ErrProviderUnavailable if CI provider is unreachable + ScheduleBuild( + ctx context.Context, + baseSHA string, + speculatedBatchesToBeApplied []entity.BatchDependent, + batchToBeTest entity.Batch, + repoURL string, + branch string, + pipelineID string, + sqid string, + env map[string]string, + message string, + ) (entitybuild.BuildID, error) + + // Poll retrieves the current status of a build from the CI provider. + // This is a synchronous call that queries the provider's API. + // + // Returns: + // - BuildStatus: Current state, timestamps, URLs, and metadata for the build + // - error: ErrBuildNotFound if the build doesn't exist, ErrProviderUnavailable if the CI provider is unreachable + Poll(ctx context.Context, id entitybuild.BuildID) (entitybuild.BuildStatus, error) + + // CancelBuild requests cancellation of a queued or running build. + // Builds that have already completed cannot be cancelled. + // + // Returns: + // - error: ErrBuildNotFound if the build doesn't exist, + // ErrBuildNotCancellable if the build has already finished, + // ErrProviderUnavailable if the CI provider is unreachable + CancelBuild(ctx context.Context, id entitybuild.BuildID) error + + // Close gracefully shuts down the build manager. + // Implementations should cancel pending requests, close HTTP clients, and clean up resources. + // After Close is called, all other methods should return errors. + // Close is idempotent and safe to call multiple times. + Close() error +} diff --git a/extension/build/errors.go b/extension/build/errors.go new file mode 100644 index 00000000..75ac8f2c --- /dev/null +++ b/extension/build/errors.go @@ -0,0 +1,74 @@ +package build + +import ( + "errors" + "fmt" +) + +// ErrBuildNotFound is returned when a build does not exist in the CI provider. +// This can occur if: +// - The build ID is invalid or malformed +// - The build was deleted from the provider +// - The build never existed +var ErrBuildNotFound = errors.New("build not found") + +// IsBuildNotFound returns true if any error in the error chain is ErrBuildNotFound. +func IsBuildNotFound(err error) bool { + return errors.Is(err, ErrBuildNotFound) +} + +// WrapBuildNotFound wraps ErrBuildNotFound with the original error from the build provider. +// This preserves the original error details while marking it as a "not found" error. +func WrapBuildNotFound(err error) error { + return fmt.Errorf("%w: %w", ErrBuildNotFound, err) +} + +// ErrBuildNotCancellable is returned when attempting to cancel a build that cannot be cancelled. +// This occurs when: +// - The build has already finished (passed, failed, or cancelled) +// - The provider does not support cancellation for this build type +var ErrBuildNotCancellable = errors.New("build not cancellable") + +// IsBuildNotCancellable returns true if any error in the error chain is ErrBuildNotCancellable. +func IsBuildNotCancellable(err error) bool { + return errors.Is(err, ErrBuildNotCancellable) +} + +// WrapBuildNotCancellable wraps ErrBuildNotCancellable with the original error from the build provider. +func WrapBuildNotCancellable(err error) error { + return fmt.Errorf("%w: %w", ErrBuildNotCancellable, err) +} + +// ErrProviderUnavailable is returned when the CI provider is unreachable or experiencing errors. +// This can occur due to: +// - Network connectivity issues +// - Provider service outages (5xx errors) +// - Authentication failures (invalid API tokens) +// - Rate limiting +var ErrProviderUnavailable = errors.New("provider unavailable") + +// IsProviderUnavailable returns true if any error in the error chain is ErrProviderUnavailable. +func IsProviderUnavailable(err error) bool { + return errors.Is(err, ErrProviderUnavailable) +} + +// WrapProviderUnavailable wraps ErrProviderUnavailable with the original error from the build provider. +func WrapProviderUnavailable(err error) error { + return fmt.Errorf("%w: %w", ErrProviderUnavailable, err) +} + +// ErrInvalidRequest is returned when ScheduleBuild parameters fail validation. +// This can occur when: +// - Required parameters are missing (baseSHA, batchToBeTest.ID, repoURL, branch, pipelineID, sqid) +// - Parameter values are malformed (invalid URLs, empty strings, etc.) +var ErrInvalidRequest = errors.New("invalid request") + +// IsInvalidRequest returns true if any error in the error chain is ErrInvalidRequest. +func IsInvalidRequest(err error) bool { + return errors.Is(err, ErrInvalidRequest) +} + +// WrapInvalidRequest wraps ErrInvalidRequest with a descriptive error message. +func WrapInvalidRequest(err error) error { + return fmt.Errorf("%w: %w", ErrInvalidRequest, err) +} diff --git a/extension/build/inmemory/BUILD.bazel b/extension/build/inmemory/BUILD.bazel new file mode 100644 index 00000000..e723dc86 --- /dev/null +++ b/extension/build/inmemory/BUILD.bazel @@ -0,0 +1,26 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "inmemory", + srcs = ["in_memory_build_manager.go"], + importpath = "github.com/uber/submitqueue/extension/build/inmemory", + visibility = ["//visibility:public"], + deps = [ + "//entity", + "//entity/build", + "//extension/build", + ], +) + +go_test( + name = "inmemory_test", + srcs = ["in_memory_build_manager_test.go"], + embed = [":inmemory"], + deps = [ + "//entity", + "//entity/build", + "//extension/build", + "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", + ], +) diff --git a/extension/build/inmemory/in_memory_build_manager.go b/extension/build/inmemory/in_memory_build_manager.go new file mode 100644 index 00000000..4814661d --- /dev/null +++ b/extension/build/inmemory/in_memory_build_manager.go @@ -0,0 +1,531 @@ +package inmemory + +import ( + "context" + "fmt" + "strings" + "sync" + "time" + + "github.com/uber/submitqueue/entity" + entitybuild "github.com/uber/submitqueue/entity/build" + "github.com/uber/submitqueue/extension/build" +) + +// inMemoryBuildManager is an in-memory implementation of build.BuildManager for testing. +// It simulates asynchronous build execution without making any external API calls. +// +// This is a functional test helper that simulates real CI behavior with goroutines, +// state transitions, and deterministic build outcomes based on batch IDs. +type inMemoryBuildManager struct { + // mu protects all fields below for thread-safe concurrent access + mu sync.RWMutex + + // builds stores all builds by their ID + // Key: build ID string, Value: pointer to buildData + builds map[string]*buildData + + // nextID is the counter for generating sequential build IDs + nextID int + + // buildDelay is how long simulated builds take to complete + // Default is 100ms for fast tests + buildDelay time.Duration + + // onStateChange is an optional callback invoked when a build changes state + // This is called while holding the lock, so callbacks should be fast + // Used for deterministic testing without time.Sleep + onStateChange func(buildID string, state entitybuild.BuildState) + + // closed tracks whether this manager has been closed + // After closing, all methods return errors + closed bool +} + +// buildData represents the internal state of a mock build +type buildData struct { + // baseSHA is the starting point (main branch SHA) + baseSHA string + + // speculatedBatchesToBeApplied are base batches applied on main + speculatedBatchesToBeApplied []entity.BatchDependent + + // batchToBeTest is the batch being tested + batchToBeTest entity.Batch + + // Other build parameters + repoURL string + branch string + pipelineID string + sqid string + env map[string]string + message string + + // status is the current BuildStatus + // This is updated by the background goroutine as the build progresses + status entitybuild.BuildStatus + + // cancel is called to stop the build execution goroutine + // nil if the build has already finished + cancel context.CancelFunc +} + +// Params contains configuration options for creating an in-memory BuildManager. +type Params struct { + // BuildDelay specifies how long simulated builds take to complete. + // If zero, defaults to 100ms. + // Set to a smaller value for faster tests, or larger for testing timeouts. + BuildDelay time.Duration + + // OnStateChange is an optional callback invoked when a build changes state. + // This is called while holding the manager's lock, so callbacks must be fast and non-blocking. + // Used for deterministic testing without time.Sleep - tests can wait on channels until specific states. + // The callback receives the build ID string and the new state. + OnStateChange func(buildID string, state entitybuild.BuildState) +} + +// NewInMemoryBuildManager creates a new in-memory BuildManager for testing. +// All builds are stored in memory and simulated with goroutines. +// +// The implementation supports deterministic test behavior: +// - Batch IDs containing "fail" will result in failed builds +// - Batch IDs containing "block" will result in blocked builds +// - All other batches will result in passed builds +// +// This is a functional test helper that simulates real CI behavior with +// async execution, state transitions, and configurable build delays. +func NewInMemoryBuildManager(params Params) build.BuildManager { + // Set default build delay if not specified + delay := params.BuildDelay + if delay == 0 { + // Default to 100ms for reasonably fast tests + delay = 100 * time.Millisecond + } + + return &inMemoryBuildManager{ + builds: make(map[string]*buildData), + nextID: 1, + buildDelay: delay, + onStateChange: params.OnStateChange, + closed: false, + } +} + +// ScheduleBuild creates a new in-memory build and starts a goroutine to simulate its execution. +func (m *inMemoryBuildManager) ScheduleBuild( + ctx context.Context, + baseSHA string, + speculatedBatchesToBeApplied []entity.BatchDependent, + batchToBeTest entity.Batch, + repoURL string, + branch string, + pipelineID string, + sqid string, + env map[string]string, + message string, +) (entitybuild.BuildID, error) { + // Validate the parameters first before doing any work + if err := m.validateParams(baseSHA, batchToBeTest, repoURL, branch, pipelineID, sqid); err != nil { + return "", err + } + + // Lock for reading/writing build state + m.mu.Lock() + defer m.mu.Unlock() + + // Check if manager has been closed + if m.closed { + return "", fmt.Errorf("manager is closed") + } + + // Generate a unique build ID + // Format: "mock://1", "mock://2", etc. + buildIDStr := fmt.Sprintf("%d", m.nextID) + m.nextID++ + + // Create the BuildID using the standard format + buildID := entitybuild.NewBuildID("mock", buildIDStr) + + // Record the current time for timestamps (Unix milliseconds) + now := time.Now().UnixMilli() + + // Generate build message from batches + buildMessage := m.generateBuildMessage(speculatedBatchesToBeApplied, batchToBeTest, message) + + // Enrich environment variables with batch metadata + enrichedEnv := m.enrichEnvironment(baseSHA, speculatedBatchesToBeApplied, batchToBeTest, env) + + // Create initial build status in "queued" state + status := entitybuild.BuildStatus{ + ID: buildID, + State: entitybuild.BuildStateQueued, + QueuedAt: now, + StartedAt: 0, // Not started yet + FinishedAt: 0, // Not finished yet + WebURL: fmt.Sprintf("https://mock-ci.example.com/builds/%s", buildIDStr), + LogsURL: fmt.Sprintf("https://mock-ci.example.com/builds/%s/logs", buildIDStr), + Metadata: map[string]string{ + "base_sha": baseSHA, + "base_batches": formatBatchDependentIDs(speculatedBatchesToBeApplied), + "test_batch": batchToBeTest.ID, + "repo": repoURL, + "branch": branch, + "sqid": sqid, + }, + } + + // Create a cancellable context for the build execution goroutine + buildCtx, cancel := context.WithCancel(context.Background()) + + // Store the build data + m.builds[buildIDStr] = &buildData{ + baseSHA: baseSHA, + speculatedBatchesToBeApplied: speculatedBatchesToBeApplied, + batchToBeTest: batchToBeTest, + repoURL: repoURL, + branch: branch, + pipelineID: pipelineID, + sqid: sqid, + env: enrichedEnv, + message: buildMessage, + status: status, + cancel: cancel, + } + + // Start a goroutine to simulate async build execution + go m.executeBuild(buildCtx, buildIDStr) + + return buildID, nil +} + +// executeBuild simulates the async execution of a build in a background goroutine. +// It transitions through states: queued -> running -> passed/failed/cancelled +func (m *inMemoryBuildManager) executeBuild(ctx context.Context, buildIDStr string) { + // Wait a small delay to simulate queueing time + // Use 10% of total build delay for queue time + queueDelay := m.buildDelay / 10 + select { + case <-time.After(queueDelay): + // Queue time elapsed, proceed to running state + case <-ctx.Done(): + // Build was cancelled while queued + m.updateBuildState(buildIDStr, entitybuild.BuildStateCancelled, "") + return + } + + // Transition to running state + m.mu.Lock() + data, exists := m.builds[buildIDStr] + if exists { + // Record when the build started executing + data.status.State = entitybuild.BuildStateRunning + data.status.StartedAt = time.Now().UnixMilli() + // Notify listeners that state changed + if m.onStateChange != nil { + m.onStateChange(buildIDStr, entitybuild.BuildStateRunning) + } + } + m.mu.Unlock() + + // Simulate build execution for the remaining delay + executionDelay := m.buildDelay - queueDelay + select { + case <-time.After(executionDelay): + // Build execution completed, determine final state + case <-ctx.Done(): + // Build was cancelled while running + m.updateBuildState(buildIDStr, entitybuild.BuildStateCancelled, "") + return + } + + // Get batch ID for deterministic testing + m.mu.RLock() + data, exists = m.builds[buildIDStr] + var batchID string + if exists { + batchID = data.batchToBeTest.ID + } + m.mu.RUnlock() + + // Determine final state based on batch ID for deterministic testing + var finalState entitybuild.BuildState + var errorMsg string + + // Check batch ID for test markers using strings.Contains + // This allows tests to control build outcomes deterministically + if strings.Contains(batchID, "fail") { + // Batch ID contains "fail" -> build fails + finalState = entitybuild.BuildStateFailed + errorMsg = "Build failed: tests did not pass" + } else if strings.Contains(batchID, "block") { + // Batch ID contains "block" -> build is blocked waiting for approval + finalState = entitybuild.BuildStateBlocked + errorMsg = "" + } else { + // Default case -> build passes + finalState = entitybuild.BuildStatePassed + errorMsg = "" + } + + // Update to final state + m.updateBuildState(buildIDStr, finalState, errorMsg) +} + +// updateBuildState updates a build's state and finished timestamp atomically. +func (m *inMemoryBuildManager) updateBuildState(buildIDStr string, state entitybuild.BuildState, errorMsg string) { + m.mu.Lock() + defer m.mu.Unlock() + + // Find the build data + data, exists := m.builds[buildIDStr] + if !exists { + // Build was deleted, nothing to update + return + } + + // Update the state + data.status.State = state + data.status.ErrorMessage = errorMsg + + // If this is a terminal state, record the finish time and clear cancel function + if state.IsTerminal() { + data.status.FinishedAt = time.Now().UnixMilli() + // Clear the cancel function since build is done + // This is safe because we hold the lock during both read and write + data.cancel = nil + } + + // Notify listeners that state changed + // This is called while holding the lock, so callbacks must be fast + if m.onStateChange != nil { + m.onStateChange(buildIDStr, state) + } +} + +// Poll retrieves the current status of a build. +func (m *inMemoryBuildManager) Poll(ctx context.Context, id entitybuild.BuildID) (entitybuild.BuildStatus, error) { + m.mu.RLock() + defer m.mu.RUnlock() + + // Check if manager has been closed + if m.closed { + return entitybuild.BuildStatus{}, fmt.Errorf("manager is closed") + } + + // Parse the BuildID to extract provider and ID + provider, idStr, err := entitybuild.ParseBuildID(id) + if err != nil { + return entitybuild.BuildStatus{}, build.WrapInvalidRequest(err) + } + + // Verify this is a mock build + if provider != "mock" { + return entitybuild.BuildStatus{}, build.WrapInvalidRequest( + fmt.Errorf("provider mismatch: expected 'mock', got '%s'", provider), + ) + } + + // Look up the build + data, exists := m.builds[idStr] + if !exists { + // Build not found + return entitybuild.BuildStatus{}, build.WrapBuildNotFound( + fmt.Errorf("build %s does not exist", idStr), + ) + } + + // Return a copy of the current status + // Copy the status to avoid race conditions if caller modifies it + status := data.status + + // Deep copy the metadata map + // Metadata is guaranteed to be non-nil, so we always create a copy + status.Metadata = make(map[string]string, len(data.status.Metadata)) + for k, v := range data.status.Metadata { + status.Metadata[k] = v + } + + return status, nil +} + +// CancelBuild requests cancellation of a queued or running build. +func (m *inMemoryBuildManager) CancelBuild(ctx context.Context, id entitybuild.BuildID) error { + m.mu.Lock() + defer m.mu.Unlock() + + // Check if manager has been closed + if m.closed { + return fmt.Errorf("manager is closed") + } + + // Parse the BuildID to extract provider and ID + provider, idStr, err := entitybuild.ParseBuildID(id) + if err != nil { + return build.WrapInvalidRequest(err) + } + + // Verify this is a mock build + if provider != "mock" { + return build.WrapInvalidRequest( + fmt.Errorf("provider mismatch: expected 'mock', got '%s'", provider), + ) + } + + // Look up the build + data, exists := m.builds[idStr] + if !exists { + // Build not found + return build.WrapBuildNotFound( + fmt.Errorf("build %s does not exist", idStr), + ) + } + + // Check if build is still cancellable + // Can only cancel if build is not in a terminal state + if data.status.State.IsTerminal() { + return build.WrapBuildNotCancellable( + fmt.Errorf("build %s is already in terminal state: %s", idStr, data.status.State), + ) + } + + // Cancel the build execution goroutine if it's still running + // Check for nil to handle race where build just finished + // This is safe because we hold the lock during both read and call + if data.cancel != nil { + // Calling cancel() will cause the executeBuild goroutine to exit + // and update the state to BuildStateCancelled via updateBuildState + data.cancel() + } + + return nil +} + +// Close shuts down the in-memory build manager. +// After Close is called, all operations will return errors. +func (m *inMemoryBuildManager) Close() error { + m.mu.Lock() + defer m.mu.Unlock() + + if m.closed { + // Already closed, this is idempotent + return nil + } + + // Mark as closed + m.closed = true + + // Cancel all running builds + for _, data := range m.builds { + if data.cancel != nil { + data.cancel() + } + } + + return nil +} + +// validateParams checks that all required parameters are present. +func (m *inMemoryBuildManager) validateParams( + baseSHA string, + batchToBeTest entity.Batch, + repoURL string, + branch string, + pipelineID string, + sqid string, +) error { + // Check baseSHA is not empty + if baseSHA == "" { + return build.WrapInvalidRequest(fmt.Errorf("baseSHA is required")) + } + + // Check batchToBeTest.ID is not empty + if batchToBeTest.ID == "" { + return build.WrapInvalidRequest(fmt.Errorf("batchToBeTest.ID is required")) + } + + // Check repoURL is not empty + if repoURL == "" { + return build.WrapInvalidRequest(fmt.Errorf("repoURL is required")) + } + + // Check branch is not empty + if branch == "" { + return build.WrapInvalidRequest(fmt.Errorf("branch is required")) + } + + // Check pipelineID is not empty + if pipelineID == "" { + return build.WrapInvalidRequest(fmt.Errorf("pipelineID is required")) + } + + // Check sqid is not empty + if sqid == "" { + return build.WrapInvalidRequest(fmt.Errorf("sqid is required")) + } + + // Note: speculatedBatchesToBeApplied can be empty (single batch test) + // env and message are optional + + // All required parameters are present + return nil +} + +// generateBuildMessage creates a descriptive build message from batch information. +func (m *inMemoryBuildManager) generateBuildMessage( + baseBatches []entity.BatchDependent, + testBatch entity.Batch, + userMessage string, +) string { + var parts []string + + // Start with the test batch + if len(baseBatches) == 0 { + // No base batches, simple message + parts = append(parts, fmt.Sprintf("Testing batch %s", testBatch.ID)) + } else { + // Include base batches in message + baseBatchIDs := formatBatchDependentIDs(baseBatches) + parts = append(parts, fmt.Sprintf("Testing batch %s on top of %s", testBatch.ID, baseBatchIDs)) + } + + // Append user message if provided + if userMessage != "" { + parts = append(parts, userMessage) + } + + return strings.Join(parts, " - ") +} + +// enrichEnvironment adds batch metadata to environment variables. +func (m *inMemoryBuildManager) enrichEnvironment( + baseSHA string, + baseBatches []entity.BatchDependent, + testBatch entity.Batch, + userEnv map[string]string, +) map[string]string { + // Copy user env and add batch metadata + env := make(map[string]string, len(userEnv)+3) + for k, v := range userEnv { + env[k] = v + } + + // Add batch metadata as environment variables + env["SQ_BASE_SHA"] = baseSHA + env["SQ_BASE_BATCHES"] = formatBatchDependentIDs(baseBatches) + env["SQ_TEST_BATCH"] = testBatch.ID + + return env +} + +// formatBatchDependentIDs converts a slice of batch dependents to comma-separated IDs. +func formatBatchDependentIDs(batches []entity.BatchDependent) string { + if len(batches) == 0 { + return "" + } + ids := make([]string, len(batches)) + for i, batch := range batches { + ids[i] = batch.BatchID + } + return strings.Join(ids, ",") +} + diff --git a/extension/build/inmemory/in_memory_build_manager_test.go b/extension/build/inmemory/in_memory_build_manager_test.go new file mode 100644 index 00000000..e4cd361f --- /dev/null +++ b/extension/build/inmemory/in_memory_build_manager_test.go @@ -0,0 +1,323 @@ +package inmemory + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/entity" + entitybuild "github.com/uber/submitqueue/entity/build" + "github.com/uber/submitqueue/extension/build" +) + +// TestInMemoryBuildManager_ScheduleBuild_Success tests successful build scheduling. +func TestInMemoryBuildManager_ScheduleBuild_Success(t *testing.T) { + mgr := NewInMemoryBuildManager(Params{ + BuildDelay: 50 * time.Millisecond, + }) + + testBatch := entity.Batch{ + ID: "queue-1/batch/1", + Queue: "queue-1", + Contains: []string{"queue-1/1", "queue-1/2"}, + State: entity.BatchStateUnknown, + Version: 1, + } + + buildID, err := mgr.ScheduleBuild( + context.Background(), + "abc123def456", + []entity.BatchDependent{}, + testBatch, + "https://github.com/uber/submitqueue", + "main", + "test-pipeline", + "queue-1/1", + map[string]string{"TEST_VAR": "value"}, + "Test build", + ) + + require.NoError(t, err) + provider, id, err := entitybuild.ParseBuildID(buildID) + require.NoError(t, err) + assert.Equal(t, "mock", provider) + assert.NotEmpty(t, id) + assert.Contains(t, buildID.String(), "mock://") +} + +// TestInMemoryBuildManager_ScheduleBuild_ValidationErrors tests parameter validation. +func TestInMemoryBuildManager_ScheduleBuild_ValidationErrors(t *testing.T) { + mgr := NewInMemoryBuildManager(Params{}) + + validBatch := entity.Batch{ + ID: "queue-1/batch/1", + Queue: "queue-1", + Contains: []string{"queue-1/1"}, + State: entity.BatchStateUnknown, + Version: 1, + } + + testCases := []struct { + name string + baseSHA string + batch entity.Batch + repoURL string + branch string + pipelineID string + sqid string + wantErr string + }{ + { + name: "missing baseSHA", + baseSHA: "", + batch: validBatch, + repoURL: "https://github.com/uber/submitqueue", + branch: "main", + pipelineID: "test", + sqid: "queue-1/1", + wantErr: "baseSHA is required", + }, + { + name: "missing batchToBeTest.ID", + baseSHA: "abc123", + batch: entity.Batch{ + ID: "", + }, + repoURL: "https://github.com/uber/submitqueue", + branch: "main", + pipelineID: "test", + sqid: "queue-1/1", + wantErr: "batchToBeTest.ID is required", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + _, err := mgr.ScheduleBuild( + context.Background(), + tc.baseSHA, + []entity.BatchDependent{}, + tc.batch, + tc.repoURL, + tc.branch, + tc.pipelineID, + tc.sqid, + nil, + "", + ) + + require.Error(t, err) + assert.True(t, build.IsInvalidRequest(err), "expected ErrInvalidRequest") + assert.Contains(t, err.Error(), tc.wantErr) + }) + } +} + +// TestInMemoryBuildManager_Poll_BuildLifecycle tests polling a build through its lifecycle. +func TestInMemoryBuildManager_Poll_BuildLifecycle(t *testing.T) { + stateChanges := make(chan entitybuild.BuildState, 10) + + mgr := NewInMemoryBuildManager(Params{ + BuildDelay: 100 * time.Millisecond, + OnStateChange: func(buildID string, state entitybuild.BuildState) { + stateChanges <- state + }, + }) + + testBatch := entity.Batch{ + ID: "queue-1/batch/1", + Queue: "queue-1", + Contains: []string{"queue-1/1"}, + State: entity.BatchStateUnknown, + Version: 1, + } + + buildID, err := mgr.ScheduleBuild( + context.Background(), + "abc123def456", + []entity.BatchDependent{}, + testBatch, + "https://github.com/uber/submitqueue", + "main", + "test-pipeline", + "queue-1/1", + nil, + "", + ) + require.NoError(t, err) + + // Immediately poll - should be queued + status, err := mgr.Poll(context.Background(), buildID) + require.NoError(t, err) + assert.Equal(t, entitybuild.BuildStateQueued, status.State) + assert.Greater(t, status.QueuedAt, int64(0)) + + // Wait for running state + state := <-stateChanges + assert.Equal(t, entitybuild.BuildStateRunning, state) + + // Wait for terminal state + state = <-stateChanges + assert.Equal(t, entitybuild.BuildStatePassed, state) + + // Poll for final status + status, err = mgr.Poll(context.Background(), buildID) + require.NoError(t, err) + assert.Equal(t, entitybuild.BuildStatePassed, status.State) + assert.True(t, status.State.IsTerminal()) + assert.Greater(t, status.FinishedAt, int64(0)) +} + +// TestInMemoryBuildManager_Poll_BuildNotFound tests polling a non-existent build. +func TestInMemoryBuildManager_Poll_BuildNotFound(t *testing.T) { + mgr := NewInMemoryBuildManager(Params{}) + + _, err := mgr.Poll(context.Background(), entitybuild.NewBuildID("mock", "99999")) + + require.Error(t, err) + assert.True(t, build.IsBuildNotFound(err)) +} + +// TestInMemoryBuildManager_Close tests closing the manager. +func TestInMemoryBuildManager_Close(t *testing.T) { + mgr := NewInMemoryBuildManager(Params{BuildDelay: 200 * time.Millisecond}) + + testBatch := entity.Batch{ + ID: "queue-1/batch/1", + Queue: "queue-1", + Contains: []string{"queue-1/1"}, + State: entity.BatchStateUnknown, + Version: 1, + } + + buildID, err := mgr.ScheduleBuild( + context.Background(), + "abc123", + []entity.BatchDependent{}, + testBatch, + "https://github.com/uber/submitqueue", + "main", + "test", + "queue-1/1", + nil, + "", + ) + require.NoError(t, err) + + // Close the manager + err = mgr.Close() + require.NoError(t, err) + + // Verify operations fail after close + _, err = mgr.Poll(context.Background(), buildID) + require.Error(t, err) + + // Close should be idempotent + err = mgr.Close() + require.NoError(t, err) +} + +// TestInMemoryBuildManager_ConcurrentOperations tests thread safety. +func TestInMemoryBuildManager_ConcurrentOperations(t *testing.T) { + mgr := NewInMemoryBuildManager(Params{BuildDelay: 100 * time.Millisecond}) + + numBuilds := 10 + buildIDs := make([]entitybuild.BuildID, numBuilds) + errors := make([]error, numBuilds) + + var wg sync.WaitGroup + for i := 0; i < numBuilds; i++ { + wg.Add(1) + go func(index int) { + defer wg.Done() + testBatch := entity.Batch{ + ID: "queue-1/batch/1", + Queue: "queue-1", + Contains: []string{"queue-1/1"}, + State: entity.BatchStateUnknown, + Version: 1, + } + + buildID, err := mgr.ScheduleBuild( + context.Background(), + "concurrent-test", + []entity.BatchDependent{}, + testBatch, + "https://github.com/uber/submitqueue", + "main", + "test", + "queue-1/1", + nil, + "", + ) + buildIDs[index] = buildID + errors[index] = err + }(i) + } + + wg.Wait() + + // Verify all builds were scheduled successfully + for i := 0; i < numBuilds; i++ { + require.NoError(t, errors[i]) + assert.NotEmpty(t, buildIDs[i].String()) + } + + // Verify all build IDs are unique + seenIDs := make(map[string]bool) + for _, buildID := range buildIDs { + assert.False(t, seenIDs[buildID.String()]) + seenIDs[buildID.String()] = true + } +} + +// TestInMemoryBuildManager_MetadataNeverNil verifies the guarantee that Metadata is never nil. +func TestInMemoryBuildManager_MetadataNeverNil(t *testing.T) { + mgr := NewInMemoryBuildManager(Params{BuildDelay: 50 * time.Millisecond}) + + testBatch := entity.Batch{ + ID: "queue-1/batch/1", + Queue: "queue-1", + Contains: []string{"queue-1/1"}, + State: entity.BatchStateUnknown, + Version: 1, + } + + // Schedule build with nil env (optional parameter) + buildID, err := mgr.ScheduleBuild( + context.Background(), + "abc123", + []entity.BatchDependent{}, + testBatch, + "https://github.com/uber/submitqueue", + "main", + "test", + "queue-1/1", + nil, // nil env to test edge case + "", + ) + require.NoError(t, err) + + // Poll immediately - Metadata should never be nil + status, err := mgr.Poll(context.Background(), buildID) + require.NoError(t, err) + assert.NotNil(t, status.Metadata, "Metadata must never be nil") + + // Verify we can safely iterate without nil check + count := 0 + for range status.Metadata { + count++ + } + assert.GreaterOrEqual(t, count, 0, "Should be able to iterate over Metadata") + + // Wait for build to complete + time.Sleep(100 * time.Millisecond) + + // Poll again - Metadata should still not be nil in terminal state + status, err = mgr.Poll(context.Background(), buildID) + require.NoError(t, err) + assert.NotNil(t, status.Metadata, "Metadata must never be nil, even in terminal state") +} diff --git a/extension/build/mock/BUILD.bazel b/extension/build/mock/BUILD.bazel new file mode 100644 index 00000000..b11b508d --- /dev/null +++ b/extension/build/mock/BUILD.bazel @@ -0,0 +1,24 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "mock", + srcs = ["build_manager.go"], + importpath = "github.com/uber/submitqueue/extension/build/mock", + visibility = ["//visibility:public"], + deps = [ + "//entity", + "//entity/build", + "@org_uber_go_mock//gomock", + ], +) + +go_test( + name = "mock_test", + srcs = ["build_manager_test.go"], + embed = [":mock"], + deps = [ + "//entity", + "//entity/build", + "@org_uber_go_mock//gomock", + ], +) diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go new file mode 100644 index 00000000..a39c5cab --- /dev/null +++ b/extension/build/mock/build_manager.go @@ -0,0 +1,101 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: extension/build/build_manager.go +// +// Generated by this command: +// +// mockgen -source=extension/build/build_manager.go -destination=extension/build/mock/build_manager.go -package=mock +// + +// Package mock is a generated GoMock package. +package mock + +import ( + context "context" + reflect "reflect" + + entity "github.com/uber/submitqueue/entity" + build "github.com/uber/submitqueue/entity/build" + gomock "go.uber.org/mock/gomock" +) + +// MockBuildManager is a mock of BuildManager interface. +type MockBuildManager struct { + ctrl *gomock.Controller + recorder *MockBuildManagerMockRecorder + isgomock struct{} +} + +// MockBuildManagerMockRecorder is the mock recorder for MockBuildManager. +type MockBuildManagerMockRecorder struct { + mock *MockBuildManager +} + +// NewMockBuildManager creates a new mock instance. +func NewMockBuildManager(ctrl *gomock.Controller) *MockBuildManager { + mock := &MockBuildManager{ctrl: ctrl} + mock.recorder = &MockBuildManagerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockBuildManager) EXPECT() *MockBuildManagerMockRecorder { + return m.recorder +} + +// CancelBuild mocks base method. +func (m *MockBuildManager) CancelBuild(ctx context.Context, id build.BuildID) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CancelBuild", ctx, id) + ret0, _ := ret[0].(error) + return ret0 +} + +// CancelBuild indicates an expected call of CancelBuild. +func (mr *MockBuildManagerMockRecorder) CancelBuild(ctx, id any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CancelBuild", reflect.TypeOf((*MockBuildManager)(nil).CancelBuild), ctx, id) +} + +// Close mocks base method. +func (m *MockBuildManager) Close() error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Close") + ret0, _ := ret[0].(error) + return ret0 +} + +// Close indicates an expected call of Close. +func (mr *MockBuildManagerMockRecorder) Close() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockBuildManager)(nil).Close)) +} + +// Poll mocks base method. +func (m *MockBuildManager) Poll(ctx context.Context, id build.BuildID) (build.BuildStatus, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Poll", ctx, id) + ret0, _ := ret[0].(build.BuildStatus) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Poll indicates an expected call of Poll. +func (mr *MockBuildManagerMockRecorder) Poll(ctx, id any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Poll", reflect.TypeOf((*MockBuildManager)(nil).Poll), ctx, id) +} + +// ScheduleBuild mocks base method. +func (m *MockBuildManager) ScheduleBuild(ctx context.Context, baseSHA string, speculatedBatchesToBeApplied []entity.BatchDependent, batchToBeTest entity.Batch, repoURL, branch, pipelineID, sqid string, env map[string]string, message string) (build.BuildID, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ScheduleBuild", ctx, baseSHA, speculatedBatchesToBeApplied, batchToBeTest, repoURL, branch, pipelineID, sqid, env, message) + ret0, _ := ret[0].(build.BuildID) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ScheduleBuild indicates an expected call of ScheduleBuild. +func (mr *MockBuildManagerMockRecorder) ScheduleBuild(ctx, baseSHA, speculatedBatchesToBeApplied, batchToBeTest, repoURL, branch, pipelineID, sqid, env, message any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ScheduleBuild", reflect.TypeOf((*MockBuildManager)(nil).ScheduleBuild), ctx, baseSHA, speculatedBatchesToBeApplied, batchToBeTest, repoURL, branch, pipelineID, sqid, env, message) +} diff --git a/extension/build/mock/build_manager_test.go b/extension/build/mock/build_manager_test.go new file mode 100644 index 00000000..e7ece7fd --- /dev/null +++ b/extension/build/mock/build_manager_test.go @@ -0,0 +1,39 @@ +package mock + +import ( + "context" + "testing" + + "github.com/uber/submitqueue/entity" + entitybuild "github.com/uber/submitqueue/entity/build" + "go.uber.org/mock/gomock" +) + +// TestMockBuildManager_Compilation verifies the mock compiles and basic setup works. +func TestMockBuildManager_Compilation(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockBuildMgr := NewMockBuildManager(ctrl) + + // Verify mock implements the interface by calling a method with expectations + buildID := entitybuild.NewBuildID("mock", "1") + mockBuildMgr.EXPECT(). + ScheduleBuild(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), + gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), + gomock.Any(), gomock.Any()). + Return(buildID, nil) + + testBatch := entity.Batch{ID: "test"} + result, err := mockBuildMgr.ScheduleBuild( + context.Background(), "sha", nil, testBatch, + "repo", "main", "pipeline", "sqid", nil, "msg", + ) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != buildID { + t.Fatalf("expected %v, got %v", buildID, result) + } +} From 6ca9bc9742be1ff7db0b318ae675b469f1d336a3 Mon Sep 17 00:00:00 2001 From: djuloori Date: Wed, 25 Feb 2026 03:13:05 +0000 Subject: [PATCH 02/18] refactor(extension): simplify BuildManager interface and remove unused code - Simplify ScheduleBuild to 3 parameters (head, base, jobName) - Change all BuildID types to plain string - Return entity.BuildStatus enum directly from Poll - Remove BuildStatusBlocked state - Remove in-memory test implementation (use gomock instead) - Remove unused entity/build package (BuildID, BuildStatus struct) - Consolidate build state into entity.BuildStatus enum - Fix go:generate directive to use mockgen directly This reduces the interface from 10 parameters to 3, removes ~1000 lines of unused code, and eliminates custom types in favor of primitives. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 5 - entity/build/BUILD.bazel | 25 - entity/build/build_id.go | 68 --- entity/build/build_id_test.go | 132 ----- entity/build/build_state.go | 40 -- entity/build/build_state_test.go | 159 ------ entity/build/build_status.go | 40 -- entity/build_test.go | 5 - extension/build/BUILD.bazel | 1 - extension/build/README.md | 264 +-------- extension/build/build_manager.go | 45 +- extension/build/inmemory/BUILD.bazel | 26 - .../build/inmemory/in_memory_build_manager.go | 531 ------------------ .../inmemory/in_memory_build_manager_test.go | 323 ----------- extension/build/mock/BUILD.bazel | 2 - extension/build/mock/build_manager.go | 31 +- extension/build/mock/build_manager_test.go | 30 +- 17 files changed, 84 insertions(+), 1643 deletions(-) delete mode 100644 entity/build/BUILD.bazel delete mode 100644 entity/build/build_id.go delete mode 100644 entity/build/build_id_test.go delete mode 100644 entity/build/build_state.go delete mode 100644 entity/build/build_state_test.go delete mode 100644 entity/build/build_status.go delete mode 100644 extension/build/inmemory/BUILD.bazel delete mode 100644 extension/build/inmemory/in_memory_build_manager.go delete mode 100644 extension/build/inmemory/in_memory_build_manager_test.go diff --git a/entity/build.go b/entity/build.go index c6253502..1dee6bf9 100644 --- a/entity/build.go +++ b/entity/build.go @@ -24,15 +24,10 @@ const ( // BuildStatusCancelled indicates the build was cancelled before completion. // This is a terminal state. BuildStatusCancelled BuildStatus = "cancelled" - - // BuildStatusBlocked indicates the build is waiting for manual approval or unblocking. - // Some CI systems (like BuildKite) support manual approval steps. - BuildStatusBlocked BuildStatus = "blocked" ) // IsTerminal returns true if the build state represents a final state (passed, failed, or cancelled). // Terminal states indicate the build has finished and will not change state again. -// Note: BuildStatusBlocked is NOT terminal as blocked builds can be unblocked and continue execution. func (s BuildStatus) IsTerminal() bool { return s == BuildStatusPassed || s == BuildStatusFailed || s == BuildStatusCancelled } diff --git a/entity/build/BUILD.bazel b/entity/build/BUILD.bazel deleted file mode 100644 index 8a1818c0..00000000 --- a/entity/build/BUILD.bazel +++ /dev/null @@ -1,25 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library", "go_test") - -go_library( - name = "build", - srcs = [ - "build_id.go", - "build_state.go", - "build_status.go", - ], - importpath = "github.com/uber/submitqueue/entity/build", - visibility = ["//visibility:public"], -) - -go_test( - name = "build_test", - srcs = [ - "build_id_test.go", - "build_state_test.go", - ], - embed = [":build"], - deps = [ - "@com_github_stretchr_testify//assert", - "@com_github_stretchr_testify//require", - ], -) diff --git a/entity/build/build_id.go b/entity/build/build_id.go deleted file mode 100644 index b7e71315..00000000 --- a/entity/build/build_id.go +++ /dev/null @@ -1,68 +0,0 @@ -package build - -import "fmt" - -// BuildID uniquely identifies a build across all CI providers. -// Format: "provider://provider-specific-id" -// -// The BuildID uses a URI-like format that combines the provider name and -// provider-specific identifier into a single string for simplicity in storage, -// logging, and serialization. -// -// Examples: -// - "buildkite://uber/submitqueue-ci/123" -// - "jenkins://456" -// - "mock://1" -// -// For database storage, this single string format avoids the complexity of -// composite keys and makes it easy to use as a primary key or index. -type BuildID string - -// String returns the BuildID as a string. -// This is the canonical representation for logging, storage, and display. -func (b BuildID) String() string { - return string(b) -} - -// NewBuildID constructs a BuildID from a provider name and provider-specific ID. -// The provider should be a short identifier (e.g., "buildkite", "jenkins", "mock"). -// The id should be the provider's build identifier in whatever format they use. -func NewBuildID(provider, id string) BuildID { - return BuildID(fmt.Sprintf("%s://%s", provider, id)) -} - -// ParseBuildID extracts the provider and ID components from a BuildID string. -// Returns the provider name, provider-specific ID, and an error if the format is invalid. -// -// Expected format: "provider://id" -func ParseBuildID(buildID BuildID) (provider string, id string, err error) { - s := string(buildID) - - // Find the separator - sep := "://" - idx := len(s) - for i := 0; i < len(s)-len(sep)+1; i++ { - if s[i:i+len(sep)] == sep { - idx = i - break - } - } - - // Check if separator was found - if idx == len(s) { - return "", "", fmt.Errorf("invalid BuildID format: missing '://' separator in %q", s) - } - - provider = s[:idx] - id = s[idx+len(sep):] - - // Validate components are not empty - if provider == "" { - return "", "", fmt.Errorf("invalid BuildID format: empty provider in %q", s) - } - if id == "" { - return "", "", fmt.Errorf("invalid BuildID format: empty ID in %q", s) - } - - return provider, id, nil -} diff --git a/entity/build/build_id_test.go b/entity/build/build_id_test.go deleted file mode 100644 index 854cf4a6..00000000 --- a/entity/build/build_id_test.go +++ /dev/null @@ -1,132 +0,0 @@ -package build - -import ( - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestNewBuildID(t *testing.T) { - testCases := []struct { - name string - provider string - id string - expected string - }{ - { - name: "buildkite with org/pipeline/number", - provider: "buildkite", - id: "uber/submitqueue-ci/123", - expected: "buildkite://uber/submitqueue-ci/123", - }, - { - name: "jenkins with build number", - provider: "jenkins", - id: "456", - expected: "jenkins://456", - }, - { - name: "mock with sequential number", - provider: "mock", - id: "1", - expected: "mock://1", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - buildID := NewBuildID(tc.provider, tc.id) - assert.Equal(t, tc.expected, string(buildID)) - assert.Equal(t, tc.expected, buildID.String()) - }) - } -} - -func TestParseBuildID(t *testing.T) { - testCases := []struct { - name string - buildID BuildID - expectedProvider string - expectedID string - expectError bool - }{ - { - name: "valid buildkite ID", - buildID: "buildkite://uber/submitqueue-ci/123", - expectedProvider: "buildkite", - expectedID: "uber/submitqueue-ci/123", - expectError: false, - }, - { - name: "valid jenkins ID", - buildID: "jenkins://456", - expectedProvider: "jenkins", - expectedID: "456", - expectError: false, - }, - { - name: "valid mock ID", - buildID: "mock://1", - expectedProvider: "mock", - expectedID: "1", - expectError: false, - }, - { - name: "missing separator", - buildID: "buildkite-123", - expectError: true, - }, - { - name: "empty provider", - buildID: "://123", - expectError: true, - }, - { - name: "empty ID", - buildID: "buildkite://", - expectError: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - provider, id, err := ParseBuildID(tc.buildID) - - if tc.expectError { - require.Error(t, err) - return - } - - require.NoError(t, err) - assert.Equal(t, tc.expectedProvider, provider) - assert.Equal(t, tc.expectedID, id) - }) - } -} - -func TestBuildIDRoundTrip(t *testing.T) { - testCases := []struct { - provider string - id string - }{ - {"buildkite", "uber/submitqueue-ci/123"}, - {"jenkins", "456"}, - {"mock", "1"}, - } - - for _, tc := range testCases { - t.Run(tc.provider, func(t *testing.T) { - // Create BuildID - buildID := NewBuildID(tc.provider, tc.id) - - // Parse it back - provider, id, err := ParseBuildID(buildID) - require.NoError(t, err) - - // Verify round trip - assert.Equal(t, tc.provider, provider) - assert.Equal(t, tc.id, id) - }) - } -} diff --git a/entity/build/build_state.go b/entity/build/build_state.go deleted file mode 100644 index b8faa0fd..00000000 --- a/entity/build/build_state.go +++ /dev/null @@ -1,40 +0,0 @@ -package build - -// BuildState represents the execution state of a build. -// This is a string enum following SubmitQueue's pattern of using string enums with sentinel values. -type BuildState string - -const ( - // BuildStateUnknown is the sentinel value for unknown or unreachable build states. - // This should only occur if the provider returns an unexpected state value. - BuildStateUnknown BuildState = "" - - // BuildStateQueued indicates the build has been scheduled but not yet started. - BuildStateQueued BuildState = "queued" - - // BuildStateRunning indicates the build is currently executing. - BuildStateRunning BuildState = "running" - - // BuildStatePassed indicates the build completed successfully. - // This is a terminal state. - BuildStatePassed BuildState = "passed" - - // BuildStateFailed indicates the build completed with failures. - // This is a terminal state. - BuildStateFailed BuildState = "failed" - - // BuildStateCancelled indicates the build was cancelled before completion. - // This is a terminal state. - BuildStateCancelled BuildState = "cancelled" - - // BuildStateBlocked indicates the build is waiting for manual approval or unblocking. - // Some CI systems (like BuildKite) support manual approval steps. - BuildStateBlocked BuildState = "blocked" -) - -// IsTerminal returns true if the build state represents a final state (passed, failed, or cancelled). -// Terminal states indicate the build has finished and will not change state again. -// Note: BuildStateBlocked is NOT terminal as blocked builds can be unblocked and continue execution. -func (s BuildState) IsTerminal() bool { - return s == BuildStatePassed || s == BuildStateFailed || s == BuildStateCancelled -} diff --git a/entity/build/build_state_test.go b/entity/build/build_state_test.go deleted file mode 100644 index 75ecae99..00000000 --- a/entity/build/build_state_test.go +++ /dev/null @@ -1,159 +0,0 @@ -package build - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -// TestBuildState_IsTerminal tests the IsTerminal method for all build states. -func TestBuildState_IsTerminal(t *testing.T) { - testCases := []struct { - name string - state BuildState - expected bool - }{ - { - name: "passed is terminal", - state: BuildStatePassed, - expected: true, - }, - { - name: "failed is terminal", - state: BuildStateFailed, - expected: true, - }, - { - name: "cancelled is terminal", - state: BuildStateCancelled, - expected: true, - }, - { - name: "queued is not terminal", - state: BuildStateQueued, - expected: false, - }, - { - name: "running is not terminal", - state: BuildStateRunning, - expected: false, - }, - { - name: "blocked is not terminal", - state: BuildStateBlocked, - expected: false, - }, - { - name: "unknown is not terminal", - state: BuildStateUnknown, - expected: false, - }, - { - name: "empty string is not terminal", - state: BuildState(""), - expected: false, - }, - { - name: "invalid state is not terminal", - state: BuildState("invalid"), - expected: false, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - result := tc.state.IsTerminal() - assert.Equal(t, tc.expected, result, - "IsTerminal() for %s should return %v", tc.state, tc.expected) - }) - } -} - -// TestBuildState_Constants verifies the string values of all build state constants. -func TestBuildState_Constants(t *testing.T) { - testCases := []struct { - name string - state BuildState - expected string - }{ - { - name: "unknown", - state: BuildStateUnknown, - expected: "", - }, - { - name: "queued", - state: BuildStateQueued, - expected: "queued", - }, - { - name: "running", - state: BuildStateRunning, - expected: "running", - }, - { - name: "passed", - state: BuildStatePassed, - expected: "passed", - }, - { - name: "failed", - state: BuildStateFailed, - expected: "failed", - }, - { - name: "cancelled", - state: BuildStateCancelled, - expected: "cancelled", - }, - { - name: "blocked", - state: BuildStateBlocked, - expected: "blocked", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.expected, string(tc.state), - "BuildState constant should have expected string value") - }) - } -} - -// TestBuildState_ZeroValue verifies that the zero value is BuildStateUnknown. -func TestBuildState_ZeroValue(t *testing.T) { - var state BuildState - assert.Equal(t, BuildStateUnknown, state, "Zero value should be BuildStateUnknown") - assert.Equal(t, "", string(state), "Zero value should be empty string") - assert.False(t, state.IsTerminal(), "Zero value should not be terminal") -} - -// TestBuildState_TerminalStates verifies all terminal states are accounted for. -func TestBuildState_TerminalStates(t *testing.T) { - terminalStates := []BuildState{ - BuildStatePassed, - BuildStateFailed, - BuildStateCancelled, - } - - for _, state := range terminalStates { - assert.True(t, state.IsTerminal(), - "State %s should be terminal", state) - } -} - -// TestBuildState_NonTerminalStates verifies all non-terminal states are accounted for. -func TestBuildState_NonTerminalStates(t *testing.T) { - nonTerminalStates := []BuildState{ - BuildStateUnknown, - BuildStateQueued, - BuildStateRunning, - BuildStateBlocked, - } - - for _, state := range nonTerminalStates { - assert.False(t, state.IsTerminal(), - "State %s should not be terminal", state) - } -} diff --git a/entity/build/build_status.go b/entity/build/build_status.go deleted file mode 100644 index e3edc623..00000000 --- a/entity/build/build_status.go +++ /dev/null @@ -1,40 +0,0 @@ -package build - -// BuildStatus represents the current state of a build as returned by BuildManager.Poll(). -// It contains all information about the build's lifecycle, including timing, state, and URLs. -type BuildStatus struct { - // ID is the unique identifier for this build. - ID BuildID - - // State is the current execution state of the build (queued, running, passed, failed, etc.). - State BuildState - - // QueuedAt is the Unix timestamp in milliseconds when the build was queued. - // Zero if the build has not been queued yet. - QueuedAt int64 - - // StartedAt is the Unix timestamp in milliseconds when the build started executing. - // Zero if the build has not started yet. - StartedAt int64 - - // FinishedAt is the Unix timestamp in milliseconds when the build finished (passed, failed, or cancelled). - // Zero if the build has not finished yet. - FinishedAt int64 - - // WebURL is a link to view the build in the CI provider's web UI. - // Empty string if not available. - WebURL string - - // LogsURL is a direct link to the build's logs. - // Empty string if not available. - LogsURL string - - // ErrorMessage contains error details for failed builds. - // Empty string for successful builds or builds that haven't finished. - ErrorMessage string - - // Metadata contains provider-specific metadata that doesn't fit in the standard fields. - // Examples: commit author, branch, test results summary, etc. - // Never nil - always initialized to at least an empty map. - Metadata map[string]string -} diff --git a/entity/build_test.go b/entity/build_test.go index cf0aa0ac..17324954 100644 --- a/entity/build_test.go +++ b/entity/build_test.go @@ -37,11 +37,6 @@ func TestBuildStatus_IsTerminal(t *testing.T) { status: BuildStatusRunning, expected: false, }, - { - name: "blocked is not terminal", - status: BuildStatusBlocked, - expected: false, - }, { name: "unknown is not terminal", status: BuildStatusUnknown, diff --git a/extension/build/BUILD.bazel b/extension/build/BUILD.bazel index 88b91c1e..b5f597ae 100644 --- a/extension/build/BUILD.bazel +++ b/extension/build/BUILD.bazel @@ -10,6 +10,5 @@ go_library( visibility = ["//visibility:public"], deps = [ "//entity", - "//entity/build", ], ) diff --git a/extension/build/README.md b/extension/build/README.md index df7dd605..bad0768d 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -17,22 +17,16 @@ type BuildManager interface { // Schedule a new build with the CI provider for testing a batch ScheduleBuild( ctx context.Context, - baseSHA string, - speculatedBatchesToBeApplied []entity.BatchDependent, - batchToBeTest entity.Batch, - repoURL string, - branch string, - pipelineID string, - sqid string, - env map[string]string, - message string, - ) (BuildID, error) + head string, + base []string, + jobName string, + ) (string, error) // Poll current status of a build - Poll(ctx context.Context, id BuildID) (BuildStatus, error) + Poll(ctx context.Context, id string) (entity.BuildStatus, error) // Cancel a running or queued build - CancelBuild(ctx context.Context, id BuildID) error + CancelBuild(ctx context.Context, id string) error } ``` @@ -42,102 +36,41 @@ type BuildManager interface { ### ScheduleBuild Parameters -**baseSHA** (string, required): SHA of the main/base branch - the starting point for applying batches +**head** (string, required): BatchID of the batch being tested -**speculatedBatchesToBeApplied** ([]entity.BatchDependent, optional): Batches that were applied on main before the test batch. +**base** ([]string, required): List of BatchIDs (in order) that have been applied on main. Order matters. -**batchToBeTest** (entity.Batch, required): The batch being tested. - -**repoURL** (string, required): Repository URL (e.g., "https://github.com/uber/submitqueue") - -**branch** (string, required): Target branch name (e.g., "main") - -**pipelineID** (string, required): Provider-specific pipeline identifier +**jobName** (string, required): Pipeline/job name to be called on the CI provider - BuildKite: "organization/pipeline-slug" - Jenkins: "job-name" -- Mock: any string - -**sqid** (string, required): SubmitQueue request ID for correlation and tracing -**env** (map[string]string, optional): Additional environment variables to pass to CI +### Build ID Format -**message** (string, optional): Human-readable build description - -### BuildID - -Unique identifier for a build using a URI-like format: - -```go -type BuildID string - -// Constructor to create a BuildID from provider and ID components -func NewBuildID(provider, id string) BuildID - -// Parser to extract provider and ID from a BuildID -func ParseBuildID(buildID BuildID) (provider string, id string, err error) - -// String returns the BuildID as a string ("provider://id" format) -func (b BuildID) String() string -``` - -**Format**: `"provider://id"` +Build IDs returned by `ScheduleBuild` should use a URI-like format: `"provider://id"` **Examples**: - `"buildkite://uber/submitqueue-ci/123"` - `"jenkins://456"` - `"mock://1"` -**Usage**: -```go -// Create a BuildID -buildID := entitybuild.NewBuildID("buildkite", "uber/submitqueue-ci/123") -fmt.Println(buildID.String()) // "buildkite://uber/submitqueue-ci/123" - -// Parse a BuildID -provider, id, err := entitybuild.ParseBuildID(buildID) -// provider = "buildkite" -// id = "uber/submitqueue-ci/123" -``` +This format allows the implementation to encode both the provider name and provider-specific build identifier in a single string. -### BuildStatus - -Output from polling a build: - -```go -type BuildStatus struct { - ID BuildID - State BuildState // Current execution state - QueuedAt int64 // Unix milliseconds - StartedAt int64 // Unix milliseconds - FinishedAt int64 // Unix milliseconds - WebURL string // Link to build UI - LogsURL string // Link to logs - ErrorMessage string // Error details for failures - Metadata map[string]string // Provider-specific metadata (never nil) -} -``` - -**Metadata guarantee**: The `Metadata` field is never nil. Implementations must always initialize it to at least an empty map. Consumers can safely iterate over it without nil checks. - -### BuildState - -Enum representing build execution state: +### Build State Enum ```go -type BuildState string +type BuildStatus string const ( - BuildStateUnknown BuildState = "" // Sentinel value - BuildStateQueued BuildState = "queued" // Scheduled but not started - BuildStateRunning BuildState = "running" // Currently executing - BuildStatePassed BuildState = "passed" // Completed successfully (terminal) - BuildStateFailed BuildState = "failed" // Completed with failures (terminal) - BuildStateCancelled BuildState = "cancelled" // Cancelled before completion (terminal) - BuildStateBlocked BuildState = "blocked" // Waiting for manual approval + BuildStatusUnknown BuildStatus = "" // Sentinel value + BuildStatusQueued BuildStatus = "queued" // Scheduled but not started + BuildStatusRunning BuildStatus = "running" // Currently executing + BuildStatusPassed BuildStatus = "passed" // Completed successfully (terminal) + BuildStatusFailed BuildStatus = "failed" // Completed with failures (terminal) + BuildStatusCancelled BuildStatus = "cancelled" // Cancelled before completion (terminal) ) // IsTerminal returns true for passed/failed/cancelled states -func (s BuildState) IsTerminal() bool +func (s BuildStatus) IsTerminal() bool ``` ## Error Handling @@ -163,109 +96,6 @@ if build.IsBuildNotFound(err) { ## Usage -### In-Memory Implementation - -For testing without external dependencies, use the in-memory implementation: - -```go -import ( - "github.com/uber/submitqueue/extension/build/inmemory" - "github.com/uber/submitqueue/entity" -) - -// Create in-memory build manager -mgr := inmemory.NewInMemoryBuildManager(inmemory.Params{ - BuildDelay: 100 * time.Millisecond, // How long simulated builds take -}) - -// Create test batch -testBatch := entity.Batch{ - ID: "queue-1/batch/5", - Queue: "queue-1", - Contains: []string{"queue-1/10", "queue-1/11"}, - State: entity.BatchStateUnknown, - Version: 1, -} - -// Create base batches (already applied on main) -baseBatches := []entity.BatchDependent{ - {BatchID: "queue-1/batch/1"}, - {BatchID: "queue-1/batch/2"}, -} - -// Schedule build -buildID, err := mgr.ScheduleBuild( - ctx, - "abc123def456", // baseSHA (main branch HEAD) - baseBatches, // base batches - testBatch, // batch to test - "https://github.com/uber/submitqueue", - "main", - "test-pipeline", - "queue-1/10", - map[string]string{"CUSTOM_VAR": "value"}, - "Testing batch queue-1/batch/5", -) -if err != nil { - // Handle error -} - -// Poll until complete -for { - status, err := mgr.Poll(ctx, buildID) - if err != nil { - // Handle error - } - - if status.State.IsTerminal() { - fmt.Printf("Build finished: %s\n", status.State) - fmt.Printf("Build metadata: %+v\n", status.Metadata) - break - } - - time.Sleep(1 * time.Second) -} -``` - -**Deterministic Testing**: The mock supports predictable behavior: -- Batch ID containing `"fail"` → `BuildStateFailed` -- Batch ID containing `"block"` → `BuildStateBlocked` -- All other batches → `BuildStatePassed` - -**State Change Callbacks**: For deterministic testing without `time.Sleep`, use callbacks: - -```go -// Create channel to track state changes -stateChanges := make(chan build.BuildState, 10) - -mgr := inmemory.NewInMemoryBuildManager(inmemory.Params{ - BuildDelay: 100 * time.Millisecond, - OnStateChange: func(buildID string, state build.BuildState) { - stateChanges <- state - }, -}) - -testBatch := entity.Batch{ - ID: "queue-1/batch/1", - Queue: "queue-1", - Contains: []string{"queue-1/1"}, -} - -buildID, _ := mgr.ScheduleBuild(ctx, "abc123", []entity.BatchDependent{}, testBatch, - "https://github.com/uber/submitqueue", "main", "test", "queue-1/1", nil, "") - -// Wait for running state -state := <-stateChanges // Will receive BuildStateRunning - -// Wait for terminal state -state = <-stateChanges // Will receive BuildStatePassed/Failed/Cancelled -``` - -**In-Memory Implementation Details**: -- PipelineID format: Any string (not validated) -- BuildID format: Sequential numbers ("1", "2", "3", etc.) -- BuildID.String() format: `"mock://1"`, `"mock://2"`, etc. - ### GoMock Mocks For unit testing with gomock, use the generated mock: @@ -286,35 +116,13 @@ func TestMyController(t *testing.T) { // Set up expectations mockBuildMgr.EXPECT(). - ScheduleBuild(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), - gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), - gomock.Any(), gomock.Any()). - Return(entitybuild.NewBuildID("mock", "1"), nil) + ScheduleBuild(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Return("mock://1", nil) // Test your code that uses the mock } ``` -### Batch Metadata - -BuildManager uses batch information to enrich CI builds with metadata: - -**Environment Variables**: Automatically added to the build environment: -- `SQ_BASE_SHA`: SHA of the main branch -- `SQ_BASE_BATCHES`: Comma-separated list of base batch IDs (e.g., "queue-1/batch/1,queue-1/batch/2") -- `SQ_TEST_BATCH`: ID of the batch being tested - -**Build Message**: Automatically generated description like: -"Testing batch queue-1/batch/5 on top of queue-1/batch/1, queue-1/batch/2" - -**Metadata Map**: BuildStatus.Metadata includes: -- `base_sha`: Same as baseSHA parameter -- `base_batches`: Same as SQ_BASE_BATCHES -- `test_batch`: ID of the batch being tested -- `repo`: Repository URL -- `branch`: Target branch -- `sqid`: SubmitQueue request ID - ### Cancelling Builds ```go @@ -354,34 +162,28 @@ To add support for a new CI provider: } ``` -3. **Map provider states to BuildState enum**: - - Map provider's state values to the standard `BuildState` constants - - Use `BuildStateUnknown` for unexpected states - -4. **Initialize BuildStatus with non-nil Metadata**: - - Always initialize `Metadata` field to at least an empty map: `map[string]string{}` - - Never return a BuildStatus with nil Metadata - this is a documented guarantee - - Populate with provider-specific information (commit SHA, author, test counts, etc.) +3. **Map provider states to entity.BuildStatus enum**: + - Map provider's state values to the standard `entity.BuildStatus` constants + - Use `entity.BuildStatusUnknown` for unexpected states -5. **Handle provider errors**: +4. **Handle provider errors**: - 404 errors → `build.WrapBuildNotFound()` - 5xx errors → `build.WrapProviderUnavailable()` - Validation errors → `build.WrapInvalidRequest()` -6. **Define PipelineID format**: +5. **Define jobName format**: - Document the expected format in provider README - Examples: - BuildKite: `"organization/pipeline-slug"` - Jenkins: `"job-name"` - - Mock: `"any-string"` (not validated) -7. **Add tests**: +6. **Add tests**: - Unit tests with mock HTTP server - Validation tests for all required fields - Error mapping tests - Thread-safety tests -8. **Update BUILD.bazel**: +7. **Update BUILD.bazel**: ```bazel go_library( name = "jenkins", @@ -405,16 +207,12 @@ Following the established SubmitQueue extension pattern: ``` extension/build/ ├── build_manager.go # Interface definition -├── types.go # Request/Status/ID types ├── errors.go # Sentinel errors ├── README.md # This file ├── BUILD.bazel # Bazel configuration ├── mock/ # Generated gomock mocks │ ├── build_manager.go # Generated by mockgen -│ └── BUILD.bazel -├── inmemory/ # In-memory implementation for testing -│ ├── in_memory_build_manager.go -│ ├── in_memory_build_manager_test.go +│ ├── build_manager_test.go │ └── BUILD.bazel └── {provider}/ # Provider implementations (future) ├── {provider}.go diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 3a540625..09fb3203 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -1,12 +1,11 @@ package build -//go:generate go run go.uber.org/mock/mockgen -source=build_manager.go -destination=mock/build_manager.go -package=mock +//go:generate mockgen -source=build_manager.go -destination=mock/build_manager.go -package=mock import ( "context" "github.com/uber/submitqueue/entity" - entitybuild "github.com/uber/submitqueue/entity/build" ) // BuildManager is a vendor-agnostic interface for managing builds with external CI/CD providers. @@ -17,47 +16,43 @@ import ( type BuildManager interface { // ScheduleBuild creates a new build with the CI provider for testing a batch. // - // The baseSHA represents the starting point (main branch HEAD). - // The speculatedBatchesToBeApplied are batches already applied on main. - // The batchToBeTest is the batch being tested on top of those base batches. - // - // Batch information is used to: - // - Generate descriptive build messages - // - Add environment variables for CI scripts - // - Populate metadata for tracing and debugging + // Parameters: + // - head: BatchID of the batch being tested + // - base: List of BatchIDs (in order) that have been applied on main. Order matters. + // - jobName: Pipeline/job name to be called on the CI provider // // Returns: - // - BuildID: Unique identifier for the scheduled build + // - string: Unique build ID // - error: ErrInvalidRequest if validation fails, ErrProviderUnavailable if CI provider is unreachable ScheduleBuild( ctx context.Context, - baseSHA string, - speculatedBatchesToBeApplied []entity.BatchDependent, - batchToBeTest entity.Batch, - repoURL string, - branch string, - pipelineID string, - sqid string, - env map[string]string, - message string, - ) (entitybuild.BuildID, error) + head string, + base []string, + jobName string, + ) (string, error) // Poll retrieves the current status of a build from the CI provider. // This is a synchronous call that queries the provider's API. // + // Parameters: + // - id: Build ID string + // // Returns: - // - BuildStatus: Current state, timestamps, URLs, and metadata for the build - // - error: ErrBuildNotFound if the build doesn't exist, ErrProviderUnavailable if the CI provider is unreachable - Poll(ctx context.Context, id entitybuild.BuildID) (entitybuild.BuildStatus, error) + // - BuildStatus: Current state of the build + // - error: ErrBuildNotFound if the build doesn't exist, ErrProviderUnavailable if CI provider is unreachable + Poll(ctx context.Context, id string) (entity.BuildStatus, error) // CancelBuild requests cancellation of a queued or running build. // Builds that have already completed cannot be cancelled. // + // Parameters: + // - id: Build ID string + // // Returns: // - error: ErrBuildNotFound if the build doesn't exist, // ErrBuildNotCancellable if the build has already finished, // ErrProviderUnavailable if the CI provider is unreachable - CancelBuild(ctx context.Context, id entitybuild.BuildID) error + CancelBuild(ctx context.Context, id string) error // Close gracefully shuts down the build manager. // Implementations should cancel pending requests, close HTTP clients, and clean up resources. diff --git a/extension/build/inmemory/BUILD.bazel b/extension/build/inmemory/BUILD.bazel deleted file mode 100644 index e723dc86..00000000 --- a/extension/build/inmemory/BUILD.bazel +++ /dev/null @@ -1,26 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library", "go_test") - -go_library( - name = "inmemory", - srcs = ["in_memory_build_manager.go"], - importpath = "github.com/uber/submitqueue/extension/build/inmemory", - visibility = ["//visibility:public"], - deps = [ - "//entity", - "//entity/build", - "//extension/build", - ], -) - -go_test( - name = "inmemory_test", - srcs = ["in_memory_build_manager_test.go"], - embed = [":inmemory"], - deps = [ - "//entity", - "//entity/build", - "//extension/build", - "@com_github_stretchr_testify//assert", - "@com_github_stretchr_testify//require", - ], -) diff --git a/extension/build/inmemory/in_memory_build_manager.go b/extension/build/inmemory/in_memory_build_manager.go deleted file mode 100644 index 4814661d..00000000 --- a/extension/build/inmemory/in_memory_build_manager.go +++ /dev/null @@ -1,531 +0,0 @@ -package inmemory - -import ( - "context" - "fmt" - "strings" - "sync" - "time" - - "github.com/uber/submitqueue/entity" - entitybuild "github.com/uber/submitqueue/entity/build" - "github.com/uber/submitqueue/extension/build" -) - -// inMemoryBuildManager is an in-memory implementation of build.BuildManager for testing. -// It simulates asynchronous build execution without making any external API calls. -// -// This is a functional test helper that simulates real CI behavior with goroutines, -// state transitions, and deterministic build outcomes based on batch IDs. -type inMemoryBuildManager struct { - // mu protects all fields below for thread-safe concurrent access - mu sync.RWMutex - - // builds stores all builds by their ID - // Key: build ID string, Value: pointer to buildData - builds map[string]*buildData - - // nextID is the counter for generating sequential build IDs - nextID int - - // buildDelay is how long simulated builds take to complete - // Default is 100ms for fast tests - buildDelay time.Duration - - // onStateChange is an optional callback invoked when a build changes state - // This is called while holding the lock, so callbacks should be fast - // Used for deterministic testing without time.Sleep - onStateChange func(buildID string, state entitybuild.BuildState) - - // closed tracks whether this manager has been closed - // After closing, all methods return errors - closed bool -} - -// buildData represents the internal state of a mock build -type buildData struct { - // baseSHA is the starting point (main branch SHA) - baseSHA string - - // speculatedBatchesToBeApplied are base batches applied on main - speculatedBatchesToBeApplied []entity.BatchDependent - - // batchToBeTest is the batch being tested - batchToBeTest entity.Batch - - // Other build parameters - repoURL string - branch string - pipelineID string - sqid string - env map[string]string - message string - - // status is the current BuildStatus - // This is updated by the background goroutine as the build progresses - status entitybuild.BuildStatus - - // cancel is called to stop the build execution goroutine - // nil if the build has already finished - cancel context.CancelFunc -} - -// Params contains configuration options for creating an in-memory BuildManager. -type Params struct { - // BuildDelay specifies how long simulated builds take to complete. - // If zero, defaults to 100ms. - // Set to a smaller value for faster tests, or larger for testing timeouts. - BuildDelay time.Duration - - // OnStateChange is an optional callback invoked when a build changes state. - // This is called while holding the manager's lock, so callbacks must be fast and non-blocking. - // Used for deterministic testing without time.Sleep - tests can wait on channels until specific states. - // The callback receives the build ID string and the new state. - OnStateChange func(buildID string, state entitybuild.BuildState) -} - -// NewInMemoryBuildManager creates a new in-memory BuildManager for testing. -// All builds are stored in memory and simulated with goroutines. -// -// The implementation supports deterministic test behavior: -// - Batch IDs containing "fail" will result in failed builds -// - Batch IDs containing "block" will result in blocked builds -// - All other batches will result in passed builds -// -// This is a functional test helper that simulates real CI behavior with -// async execution, state transitions, and configurable build delays. -func NewInMemoryBuildManager(params Params) build.BuildManager { - // Set default build delay if not specified - delay := params.BuildDelay - if delay == 0 { - // Default to 100ms for reasonably fast tests - delay = 100 * time.Millisecond - } - - return &inMemoryBuildManager{ - builds: make(map[string]*buildData), - nextID: 1, - buildDelay: delay, - onStateChange: params.OnStateChange, - closed: false, - } -} - -// ScheduleBuild creates a new in-memory build and starts a goroutine to simulate its execution. -func (m *inMemoryBuildManager) ScheduleBuild( - ctx context.Context, - baseSHA string, - speculatedBatchesToBeApplied []entity.BatchDependent, - batchToBeTest entity.Batch, - repoURL string, - branch string, - pipelineID string, - sqid string, - env map[string]string, - message string, -) (entitybuild.BuildID, error) { - // Validate the parameters first before doing any work - if err := m.validateParams(baseSHA, batchToBeTest, repoURL, branch, pipelineID, sqid); err != nil { - return "", err - } - - // Lock for reading/writing build state - m.mu.Lock() - defer m.mu.Unlock() - - // Check if manager has been closed - if m.closed { - return "", fmt.Errorf("manager is closed") - } - - // Generate a unique build ID - // Format: "mock://1", "mock://2", etc. - buildIDStr := fmt.Sprintf("%d", m.nextID) - m.nextID++ - - // Create the BuildID using the standard format - buildID := entitybuild.NewBuildID("mock", buildIDStr) - - // Record the current time for timestamps (Unix milliseconds) - now := time.Now().UnixMilli() - - // Generate build message from batches - buildMessage := m.generateBuildMessage(speculatedBatchesToBeApplied, batchToBeTest, message) - - // Enrich environment variables with batch metadata - enrichedEnv := m.enrichEnvironment(baseSHA, speculatedBatchesToBeApplied, batchToBeTest, env) - - // Create initial build status in "queued" state - status := entitybuild.BuildStatus{ - ID: buildID, - State: entitybuild.BuildStateQueued, - QueuedAt: now, - StartedAt: 0, // Not started yet - FinishedAt: 0, // Not finished yet - WebURL: fmt.Sprintf("https://mock-ci.example.com/builds/%s", buildIDStr), - LogsURL: fmt.Sprintf("https://mock-ci.example.com/builds/%s/logs", buildIDStr), - Metadata: map[string]string{ - "base_sha": baseSHA, - "base_batches": formatBatchDependentIDs(speculatedBatchesToBeApplied), - "test_batch": batchToBeTest.ID, - "repo": repoURL, - "branch": branch, - "sqid": sqid, - }, - } - - // Create a cancellable context for the build execution goroutine - buildCtx, cancel := context.WithCancel(context.Background()) - - // Store the build data - m.builds[buildIDStr] = &buildData{ - baseSHA: baseSHA, - speculatedBatchesToBeApplied: speculatedBatchesToBeApplied, - batchToBeTest: batchToBeTest, - repoURL: repoURL, - branch: branch, - pipelineID: pipelineID, - sqid: sqid, - env: enrichedEnv, - message: buildMessage, - status: status, - cancel: cancel, - } - - // Start a goroutine to simulate async build execution - go m.executeBuild(buildCtx, buildIDStr) - - return buildID, nil -} - -// executeBuild simulates the async execution of a build in a background goroutine. -// It transitions through states: queued -> running -> passed/failed/cancelled -func (m *inMemoryBuildManager) executeBuild(ctx context.Context, buildIDStr string) { - // Wait a small delay to simulate queueing time - // Use 10% of total build delay for queue time - queueDelay := m.buildDelay / 10 - select { - case <-time.After(queueDelay): - // Queue time elapsed, proceed to running state - case <-ctx.Done(): - // Build was cancelled while queued - m.updateBuildState(buildIDStr, entitybuild.BuildStateCancelled, "") - return - } - - // Transition to running state - m.mu.Lock() - data, exists := m.builds[buildIDStr] - if exists { - // Record when the build started executing - data.status.State = entitybuild.BuildStateRunning - data.status.StartedAt = time.Now().UnixMilli() - // Notify listeners that state changed - if m.onStateChange != nil { - m.onStateChange(buildIDStr, entitybuild.BuildStateRunning) - } - } - m.mu.Unlock() - - // Simulate build execution for the remaining delay - executionDelay := m.buildDelay - queueDelay - select { - case <-time.After(executionDelay): - // Build execution completed, determine final state - case <-ctx.Done(): - // Build was cancelled while running - m.updateBuildState(buildIDStr, entitybuild.BuildStateCancelled, "") - return - } - - // Get batch ID for deterministic testing - m.mu.RLock() - data, exists = m.builds[buildIDStr] - var batchID string - if exists { - batchID = data.batchToBeTest.ID - } - m.mu.RUnlock() - - // Determine final state based on batch ID for deterministic testing - var finalState entitybuild.BuildState - var errorMsg string - - // Check batch ID for test markers using strings.Contains - // This allows tests to control build outcomes deterministically - if strings.Contains(batchID, "fail") { - // Batch ID contains "fail" -> build fails - finalState = entitybuild.BuildStateFailed - errorMsg = "Build failed: tests did not pass" - } else if strings.Contains(batchID, "block") { - // Batch ID contains "block" -> build is blocked waiting for approval - finalState = entitybuild.BuildStateBlocked - errorMsg = "" - } else { - // Default case -> build passes - finalState = entitybuild.BuildStatePassed - errorMsg = "" - } - - // Update to final state - m.updateBuildState(buildIDStr, finalState, errorMsg) -} - -// updateBuildState updates a build's state and finished timestamp atomically. -func (m *inMemoryBuildManager) updateBuildState(buildIDStr string, state entitybuild.BuildState, errorMsg string) { - m.mu.Lock() - defer m.mu.Unlock() - - // Find the build data - data, exists := m.builds[buildIDStr] - if !exists { - // Build was deleted, nothing to update - return - } - - // Update the state - data.status.State = state - data.status.ErrorMessage = errorMsg - - // If this is a terminal state, record the finish time and clear cancel function - if state.IsTerminal() { - data.status.FinishedAt = time.Now().UnixMilli() - // Clear the cancel function since build is done - // This is safe because we hold the lock during both read and write - data.cancel = nil - } - - // Notify listeners that state changed - // This is called while holding the lock, so callbacks must be fast - if m.onStateChange != nil { - m.onStateChange(buildIDStr, state) - } -} - -// Poll retrieves the current status of a build. -func (m *inMemoryBuildManager) Poll(ctx context.Context, id entitybuild.BuildID) (entitybuild.BuildStatus, error) { - m.mu.RLock() - defer m.mu.RUnlock() - - // Check if manager has been closed - if m.closed { - return entitybuild.BuildStatus{}, fmt.Errorf("manager is closed") - } - - // Parse the BuildID to extract provider and ID - provider, idStr, err := entitybuild.ParseBuildID(id) - if err != nil { - return entitybuild.BuildStatus{}, build.WrapInvalidRequest(err) - } - - // Verify this is a mock build - if provider != "mock" { - return entitybuild.BuildStatus{}, build.WrapInvalidRequest( - fmt.Errorf("provider mismatch: expected 'mock', got '%s'", provider), - ) - } - - // Look up the build - data, exists := m.builds[idStr] - if !exists { - // Build not found - return entitybuild.BuildStatus{}, build.WrapBuildNotFound( - fmt.Errorf("build %s does not exist", idStr), - ) - } - - // Return a copy of the current status - // Copy the status to avoid race conditions if caller modifies it - status := data.status - - // Deep copy the metadata map - // Metadata is guaranteed to be non-nil, so we always create a copy - status.Metadata = make(map[string]string, len(data.status.Metadata)) - for k, v := range data.status.Metadata { - status.Metadata[k] = v - } - - return status, nil -} - -// CancelBuild requests cancellation of a queued or running build. -func (m *inMemoryBuildManager) CancelBuild(ctx context.Context, id entitybuild.BuildID) error { - m.mu.Lock() - defer m.mu.Unlock() - - // Check if manager has been closed - if m.closed { - return fmt.Errorf("manager is closed") - } - - // Parse the BuildID to extract provider and ID - provider, idStr, err := entitybuild.ParseBuildID(id) - if err != nil { - return build.WrapInvalidRequest(err) - } - - // Verify this is a mock build - if provider != "mock" { - return build.WrapInvalidRequest( - fmt.Errorf("provider mismatch: expected 'mock', got '%s'", provider), - ) - } - - // Look up the build - data, exists := m.builds[idStr] - if !exists { - // Build not found - return build.WrapBuildNotFound( - fmt.Errorf("build %s does not exist", idStr), - ) - } - - // Check if build is still cancellable - // Can only cancel if build is not in a terminal state - if data.status.State.IsTerminal() { - return build.WrapBuildNotCancellable( - fmt.Errorf("build %s is already in terminal state: %s", idStr, data.status.State), - ) - } - - // Cancel the build execution goroutine if it's still running - // Check for nil to handle race where build just finished - // This is safe because we hold the lock during both read and call - if data.cancel != nil { - // Calling cancel() will cause the executeBuild goroutine to exit - // and update the state to BuildStateCancelled via updateBuildState - data.cancel() - } - - return nil -} - -// Close shuts down the in-memory build manager. -// After Close is called, all operations will return errors. -func (m *inMemoryBuildManager) Close() error { - m.mu.Lock() - defer m.mu.Unlock() - - if m.closed { - // Already closed, this is idempotent - return nil - } - - // Mark as closed - m.closed = true - - // Cancel all running builds - for _, data := range m.builds { - if data.cancel != nil { - data.cancel() - } - } - - return nil -} - -// validateParams checks that all required parameters are present. -func (m *inMemoryBuildManager) validateParams( - baseSHA string, - batchToBeTest entity.Batch, - repoURL string, - branch string, - pipelineID string, - sqid string, -) error { - // Check baseSHA is not empty - if baseSHA == "" { - return build.WrapInvalidRequest(fmt.Errorf("baseSHA is required")) - } - - // Check batchToBeTest.ID is not empty - if batchToBeTest.ID == "" { - return build.WrapInvalidRequest(fmt.Errorf("batchToBeTest.ID is required")) - } - - // Check repoURL is not empty - if repoURL == "" { - return build.WrapInvalidRequest(fmt.Errorf("repoURL is required")) - } - - // Check branch is not empty - if branch == "" { - return build.WrapInvalidRequest(fmt.Errorf("branch is required")) - } - - // Check pipelineID is not empty - if pipelineID == "" { - return build.WrapInvalidRequest(fmt.Errorf("pipelineID is required")) - } - - // Check sqid is not empty - if sqid == "" { - return build.WrapInvalidRequest(fmt.Errorf("sqid is required")) - } - - // Note: speculatedBatchesToBeApplied can be empty (single batch test) - // env and message are optional - - // All required parameters are present - return nil -} - -// generateBuildMessage creates a descriptive build message from batch information. -func (m *inMemoryBuildManager) generateBuildMessage( - baseBatches []entity.BatchDependent, - testBatch entity.Batch, - userMessage string, -) string { - var parts []string - - // Start with the test batch - if len(baseBatches) == 0 { - // No base batches, simple message - parts = append(parts, fmt.Sprintf("Testing batch %s", testBatch.ID)) - } else { - // Include base batches in message - baseBatchIDs := formatBatchDependentIDs(baseBatches) - parts = append(parts, fmt.Sprintf("Testing batch %s on top of %s", testBatch.ID, baseBatchIDs)) - } - - // Append user message if provided - if userMessage != "" { - parts = append(parts, userMessage) - } - - return strings.Join(parts, " - ") -} - -// enrichEnvironment adds batch metadata to environment variables. -func (m *inMemoryBuildManager) enrichEnvironment( - baseSHA string, - baseBatches []entity.BatchDependent, - testBatch entity.Batch, - userEnv map[string]string, -) map[string]string { - // Copy user env and add batch metadata - env := make(map[string]string, len(userEnv)+3) - for k, v := range userEnv { - env[k] = v - } - - // Add batch metadata as environment variables - env["SQ_BASE_SHA"] = baseSHA - env["SQ_BASE_BATCHES"] = formatBatchDependentIDs(baseBatches) - env["SQ_TEST_BATCH"] = testBatch.ID - - return env -} - -// formatBatchDependentIDs converts a slice of batch dependents to comma-separated IDs. -func formatBatchDependentIDs(batches []entity.BatchDependent) string { - if len(batches) == 0 { - return "" - } - ids := make([]string, len(batches)) - for i, batch := range batches { - ids[i] = batch.BatchID - } - return strings.Join(ids, ",") -} - diff --git a/extension/build/inmemory/in_memory_build_manager_test.go b/extension/build/inmemory/in_memory_build_manager_test.go deleted file mode 100644 index e4cd361f..00000000 --- a/extension/build/inmemory/in_memory_build_manager_test.go +++ /dev/null @@ -1,323 +0,0 @@ -package inmemory - -import ( - "context" - "sync" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/uber/submitqueue/entity" - entitybuild "github.com/uber/submitqueue/entity/build" - "github.com/uber/submitqueue/extension/build" -) - -// TestInMemoryBuildManager_ScheduleBuild_Success tests successful build scheduling. -func TestInMemoryBuildManager_ScheduleBuild_Success(t *testing.T) { - mgr := NewInMemoryBuildManager(Params{ - BuildDelay: 50 * time.Millisecond, - }) - - testBatch := entity.Batch{ - ID: "queue-1/batch/1", - Queue: "queue-1", - Contains: []string{"queue-1/1", "queue-1/2"}, - State: entity.BatchStateUnknown, - Version: 1, - } - - buildID, err := mgr.ScheduleBuild( - context.Background(), - "abc123def456", - []entity.BatchDependent{}, - testBatch, - "https://github.com/uber/submitqueue", - "main", - "test-pipeline", - "queue-1/1", - map[string]string{"TEST_VAR": "value"}, - "Test build", - ) - - require.NoError(t, err) - provider, id, err := entitybuild.ParseBuildID(buildID) - require.NoError(t, err) - assert.Equal(t, "mock", provider) - assert.NotEmpty(t, id) - assert.Contains(t, buildID.String(), "mock://") -} - -// TestInMemoryBuildManager_ScheduleBuild_ValidationErrors tests parameter validation. -func TestInMemoryBuildManager_ScheduleBuild_ValidationErrors(t *testing.T) { - mgr := NewInMemoryBuildManager(Params{}) - - validBatch := entity.Batch{ - ID: "queue-1/batch/1", - Queue: "queue-1", - Contains: []string{"queue-1/1"}, - State: entity.BatchStateUnknown, - Version: 1, - } - - testCases := []struct { - name string - baseSHA string - batch entity.Batch - repoURL string - branch string - pipelineID string - sqid string - wantErr string - }{ - { - name: "missing baseSHA", - baseSHA: "", - batch: validBatch, - repoURL: "https://github.com/uber/submitqueue", - branch: "main", - pipelineID: "test", - sqid: "queue-1/1", - wantErr: "baseSHA is required", - }, - { - name: "missing batchToBeTest.ID", - baseSHA: "abc123", - batch: entity.Batch{ - ID: "", - }, - repoURL: "https://github.com/uber/submitqueue", - branch: "main", - pipelineID: "test", - sqid: "queue-1/1", - wantErr: "batchToBeTest.ID is required", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - _, err := mgr.ScheduleBuild( - context.Background(), - tc.baseSHA, - []entity.BatchDependent{}, - tc.batch, - tc.repoURL, - tc.branch, - tc.pipelineID, - tc.sqid, - nil, - "", - ) - - require.Error(t, err) - assert.True(t, build.IsInvalidRequest(err), "expected ErrInvalidRequest") - assert.Contains(t, err.Error(), tc.wantErr) - }) - } -} - -// TestInMemoryBuildManager_Poll_BuildLifecycle tests polling a build through its lifecycle. -func TestInMemoryBuildManager_Poll_BuildLifecycle(t *testing.T) { - stateChanges := make(chan entitybuild.BuildState, 10) - - mgr := NewInMemoryBuildManager(Params{ - BuildDelay: 100 * time.Millisecond, - OnStateChange: func(buildID string, state entitybuild.BuildState) { - stateChanges <- state - }, - }) - - testBatch := entity.Batch{ - ID: "queue-1/batch/1", - Queue: "queue-1", - Contains: []string{"queue-1/1"}, - State: entity.BatchStateUnknown, - Version: 1, - } - - buildID, err := mgr.ScheduleBuild( - context.Background(), - "abc123def456", - []entity.BatchDependent{}, - testBatch, - "https://github.com/uber/submitqueue", - "main", - "test-pipeline", - "queue-1/1", - nil, - "", - ) - require.NoError(t, err) - - // Immediately poll - should be queued - status, err := mgr.Poll(context.Background(), buildID) - require.NoError(t, err) - assert.Equal(t, entitybuild.BuildStateQueued, status.State) - assert.Greater(t, status.QueuedAt, int64(0)) - - // Wait for running state - state := <-stateChanges - assert.Equal(t, entitybuild.BuildStateRunning, state) - - // Wait for terminal state - state = <-stateChanges - assert.Equal(t, entitybuild.BuildStatePassed, state) - - // Poll for final status - status, err = mgr.Poll(context.Background(), buildID) - require.NoError(t, err) - assert.Equal(t, entitybuild.BuildStatePassed, status.State) - assert.True(t, status.State.IsTerminal()) - assert.Greater(t, status.FinishedAt, int64(0)) -} - -// TestInMemoryBuildManager_Poll_BuildNotFound tests polling a non-existent build. -func TestInMemoryBuildManager_Poll_BuildNotFound(t *testing.T) { - mgr := NewInMemoryBuildManager(Params{}) - - _, err := mgr.Poll(context.Background(), entitybuild.NewBuildID("mock", "99999")) - - require.Error(t, err) - assert.True(t, build.IsBuildNotFound(err)) -} - -// TestInMemoryBuildManager_Close tests closing the manager. -func TestInMemoryBuildManager_Close(t *testing.T) { - mgr := NewInMemoryBuildManager(Params{BuildDelay: 200 * time.Millisecond}) - - testBatch := entity.Batch{ - ID: "queue-1/batch/1", - Queue: "queue-1", - Contains: []string{"queue-1/1"}, - State: entity.BatchStateUnknown, - Version: 1, - } - - buildID, err := mgr.ScheduleBuild( - context.Background(), - "abc123", - []entity.BatchDependent{}, - testBatch, - "https://github.com/uber/submitqueue", - "main", - "test", - "queue-1/1", - nil, - "", - ) - require.NoError(t, err) - - // Close the manager - err = mgr.Close() - require.NoError(t, err) - - // Verify operations fail after close - _, err = mgr.Poll(context.Background(), buildID) - require.Error(t, err) - - // Close should be idempotent - err = mgr.Close() - require.NoError(t, err) -} - -// TestInMemoryBuildManager_ConcurrentOperations tests thread safety. -func TestInMemoryBuildManager_ConcurrentOperations(t *testing.T) { - mgr := NewInMemoryBuildManager(Params{BuildDelay: 100 * time.Millisecond}) - - numBuilds := 10 - buildIDs := make([]entitybuild.BuildID, numBuilds) - errors := make([]error, numBuilds) - - var wg sync.WaitGroup - for i := 0; i < numBuilds; i++ { - wg.Add(1) - go func(index int) { - defer wg.Done() - testBatch := entity.Batch{ - ID: "queue-1/batch/1", - Queue: "queue-1", - Contains: []string{"queue-1/1"}, - State: entity.BatchStateUnknown, - Version: 1, - } - - buildID, err := mgr.ScheduleBuild( - context.Background(), - "concurrent-test", - []entity.BatchDependent{}, - testBatch, - "https://github.com/uber/submitqueue", - "main", - "test", - "queue-1/1", - nil, - "", - ) - buildIDs[index] = buildID - errors[index] = err - }(i) - } - - wg.Wait() - - // Verify all builds were scheduled successfully - for i := 0; i < numBuilds; i++ { - require.NoError(t, errors[i]) - assert.NotEmpty(t, buildIDs[i].String()) - } - - // Verify all build IDs are unique - seenIDs := make(map[string]bool) - for _, buildID := range buildIDs { - assert.False(t, seenIDs[buildID.String()]) - seenIDs[buildID.String()] = true - } -} - -// TestInMemoryBuildManager_MetadataNeverNil verifies the guarantee that Metadata is never nil. -func TestInMemoryBuildManager_MetadataNeverNil(t *testing.T) { - mgr := NewInMemoryBuildManager(Params{BuildDelay: 50 * time.Millisecond}) - - testBatch := entity.Batch{ - ID: "queue-1/batch/1", - Queue: "queue-1", - Contains: []string{"queue-1/1"}, - State: entity.BatchStateUnknown, - Version: 1, - } - - // Schedule build with nil env (optional parameter) - buildID, err := mgr.ScheduleBuild( - context.Background(), - "abc123", - []entity.BatchDependent{}, - testBatch, - "https://github.com/uber/submitqueue", - "main", - "test", - "queue-1/1", - nil, // nil env to test edge case - "", - ) - require.NoError(t, err) - - // Poll immediately - Metadata should never be nil - status, err := mgr.Poll(context.Background(), buildID) - require.NoError(t, err) - assert.NotNil(t, status.Metadata, "Metadata must never be nil") - - // Verify we can safely iterate without nil check - count := 0 - for range status.Metadata { - count++ - } - assert.GreaterOrEqual(t, count, 0, "Should be able to iterate over Metadata") - - // Wait for build to complete - time.Sleep(100 * time.Millisecond) - - // Poll again - Metadata should still not be nil in terminal state - status, err = mgr.Poll(context.Background(), buildID) - require.NoError(t, err) - assert.NotNil(t, status.Metadata, "Metadata must never be nil, even in terminal state") -} diff --git a/extension/build/mock/BUILD.bazel b/extension/build/mock/BUILD.bazel index b11b508d..3de8b8ff 100644 --- a/extension/build/mock/BUILD.bazel +++ b/extension/build/mock/BUILD.bazel @@ -7,7 +7,6 @@ go_library( visibility = ["//visibility:public"], deps = [ "//entity", - "//entity/build", "@org_uber_go_mock//gomock", ], ) @@ -18,7 +17,6 @@ go_test( embed = [":mock"], deps = [ "//entity", - "//entity/build", "@org_uber_go_mock//gomock", ], ) diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go index a39c5cab..c17a3de4 100644 --- a/extension/build/mock/build_manager.go +++ b/extension/build/mock/build_manager.go @@ -1,10 +1,5 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: extension/build/build_manager.go -// -// Generated by this command: -// -// mockgen -source=extension/build/build_manager.go -destination=extension/build/mock/build_manager.go -package=mock -// +// Source: build_manager.go // Package mock is a generated GoMock package. package mock @@ -13,16 +8,14 @@ import ( context "context" reflect "reflect" - entity "github.com/uber/submitqueue/entity" - build "github.com/uber/submitqueue/entity/build" gomock "go.uber.org/mock/gomock" + entity "github.com/uber/submitqueue/entity" ) // MockBuildManager is a mock of BuildManager interface. type MockBuildManager struct { ctrl *gomock.Controller recorder *MockBuildManagerMockRecorder - isgomock struct{} } // MockBuildManagerMockRecorder is the mock recorder for MockBuildManager. @@ -43,7 +36,7 @@ func (m *MockBuildManager) EXPECT() *MockBuildManagerMockRecorder { } // CancelBuild mocks base method. -func (m *MockBuildManager) CancelBuild(ctx context.Context, id build.BuildID) error { +func (m *MockBuildManager) CancelBuild(ctx context.Context, id string) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "CancelBuild", ctx, id) ret0, _ := ret[0].(error) @@ -51,7 +44,7 @@ func (m *MockBuildManager) CancelBuild(ctx context.Context, id build.BuildID) er } // CancelBuild indicates an expected call of CancelBuild. -func (mr *MockBuildManagerMockRecorder) CancelBuild(ctx, id any) *gomock.Call { +func (mr *MockBuildManagerMockRecorder) CancelBuild(ctx, id interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CancelBuild", reflect.TypeOf((*MockBuildManager)(nil).CancelBuild), ctx, id) } @@ -71,31 +64,31 @@ func (mr *MockBuildManagerMockRecorder) Close() *gomock.Call { } // Poll mocks base method. -func (m *MockBuildManager) Poll(ctx context.Context, id build.BuildID) (build.BuildStatus, error) { +func (m *MockBuildManager) Poll(ctx context.Context, id string) (entity.BuildStatus, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Poll", ctx, id) - ret0, _ := ret[0].(build.BuildStatus) + ret0, _ := ret[0].(entity.BuildStatus) ret1, _ := ret[1].(error) return ret0, ret1 } // Poll indicates an expected call of Poll. -func (mr *MockBuildManagerMockRecorder) Poll(ctx, id any) *gomock.Call { +func (mr *MockBuildManagerMockRecorder) Poll(ctx, id interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Poll", reflect.TypeOf((*MockBuildManager)(nil).Poll), ctx, id) } // ScheduleBuild mocks base method. -func (m *MockBuildManager) ScheduleBuild(ctx context.Context, baseSHA string, speculatedBatchesToBeApplied []entity.BatchDependent, batchToBeTest entity.Batch, repoURL, branch, pipelineID, sqid string, env map[string]string, message string) (build.BuildID, error) { +func (m *MockBuildManager) ScheduleBuild(ctx context.Context, head string, base []string, jobName string) (string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ScheduleBuild", ctx, baseSHA, speculatedBatchesToBeApplied, batchToBeTest, repoURL, branch, pipelineID, sqid, env, message) - ret0, _ := ret[0].(build.BuildID) + ret := m.ctrl.Call(m, "ScheduleBuild", ctx, head, base, jobName) + ret0, _ := ret[0].(string) ret1, _ := ret[1].(error) return ret0, ret1 } // ScheduleBuild indicates an expected call of ScheduleBuild. -func (mr *MockBuildManagerMockRecorder) ScheduleBuild(ctx, baseSHA, speculatedBatchesToBeApplied, batchToBeTest, repoURL, branch, pipelineID, sqid, env, message any) *gomock.Call { +func (mr *MockBuildManagerMockRecorder) ScheduleBuild(ctx, head, base, jobName interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ScheduleBuild", reflect.TypeOf((*MockBuildManager)(nil).ScheduleBuild), ctx, baseSHA, speculatedBatchesToBeApplied, batchToBeTest, repoURL, branch, pipelineID, sqid, env, message) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ScheduleBuild", reflect.TypeOf((*MockBuildManager)(nil).ScheduleBuild), ctx, head, base, jobName) } diff --git a/extension/build/mock/build_manager_test.go b/extension/build/mock/build_manager_test.go index e7ece7fd..3a2ebcc8 100644 --- a/extension/build/mock/build_manager_test.go +++ b/extension/build/mock/build_manager_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/uber/submitqueue/entity" - entitybuild "github.com/uber/submitqueue/entity/build" "go.uber.org/mock/gomock" ) @@ -16,18 +15,18 @@ func TestMockBuildManager_Compilation(t *testing.T) { mockBuildMgr := NewMockBuildManager(ctrl) - // Verify mock implements the interface by calling a method with expectations - buildID := entitybuild.NewBuildID("mock", "1") + // Test ScheduleBuild + buildID := "mock://1" mockBuildMgr.EXPECT(). - ScheduleBuild(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), - gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), - gomock.Any(), gomock.Any()). + ScheduleBuild(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return(buildID, nil) - testBatch := entity.Batch{ID: "test"} + head := "queue-1/batch/5" + base := []string{"queue-1/batch/1", "queue-1/batch/2"} + jobName := "test-pipeline" + result, err := mockBuildMgr.ScheduleBuild( - context.Background(), "sha", nil, testBatch, - "repo", "main", "pipeline", "sqid", nil, "msg", + context.Background(), head, base, jobName, ) if err != nil { @@ -36,4 +35,17 @@ func TestMockBuildManager_Compilation(t *testing.T) { if result != buildID { t.Fatalf("expected %v, got %v", buildID, result) } + + // Test Poll + mockBuildMgr.EXPECT(). + Poll(gomock.Any(), gomock.Any()). + Return(entity.BuildStatusPassed, nil) + + status, err := mockBuildMgr.Poll(context.Background(), buildID) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if status != entity.BuildStatusPassed { + t.Fatalf("expected %v, got %v", entity.BuildStatusPassed, status) + } } From 936177c539c681b3c4141ec6d9ea09791862fd25 Mon Sep 17 00:00:00 2001 From: djuloori Date: Wed, 25 Feb 2026 19:00:45 +0000 Subject: [PATCH 03/18] refactor(extension): redesign BuildManager.Schedule to accept changes with actions Refactor BuildManager interface based on review feedback to operate on individual changes (diff IDs, PR numbers) rather than batches, and to accept explicit actions for each change. Changes: - Add BuildAction enum (validate, apply) in entity/build.go - Add BuildChange struct with ChangeID and Action fields - Replace BuildManager.ScheduleBuild() with Schedule() - Takes queueName parameter for job configuration lookup - Takes []BuildChange instead of batch-focused parameters - Returns only error (implementations track builds internally) - Update mocks and tests to match new interface signature The new interface allows callers to specify what action to perform on each change (validate for testing, apply for landing), with the implementation responsible for looking up job configuration based on queue name. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 23 +++++++++++ entity/build_test.go | 44 ++++++++++++++++++++++ extension/build/build_manager.go | 23 +++++------ extension/build/mock/build_manager.go | 17 ++++----- extension/build/mock/build_manager_test.go | 25 ++++++------ 5 files changed, 99 insertions(+), 33 deletions(-) diff --git a/entity/build.go b/entity/build.go index 1dee6bf9..7bdbf60a 100644 --- a/entity/build.go +++ b/entity/build.go @@ -56,3 +56,26 @@ type Build struct { // Status represents the state of the build lifecycle this build is in. Status BuildStatus } + +// BuildAction defines the action to perform on a change submitted to the build system. +type BuildAction string + +const ( + // BuildActionUnknown is the sentinel value for uninitialized actions. + BuildActionUnknown BuildAction = "" + // BuildActionValidate runs validation/testing on the change without applying it. + BuildActionValidate BuildAction = "validate" + // BuildActionApply applies the change to the target branch. + BuildActionApply BuildAction = "apply" +) + +// BuildChange represents a code change to be processed by the build system. +// This is used by BuildManager to specify what changes to build and what action to perform. +type BuildChange struct { + // ChangeID is the unique identifier for this change. + // This is typically a diff ID (e.g., "D12345") or PR number (e.g., "42"), + // depending on the source control provider. + ChangeID string + // Action specifies what operation to perform on this change. + Action BuildAction +} diff --git a/entity/build_test.go b/entity/build_test.go index 17324954..e377820b 100644 --- a/entity/build_test.go +++ b/entity/build_test.go @@ -50,3 +50,47 @@ func TestBuildStatus_IsTerminal(t *testing.T) { }) } } + +func TestBuildChange_Creation(t *testing.T) { + tests := []struct { + name string + change BuildChange + wantID string + wantAction BuildAction + }{ + { + name: "validate action", + change: BuildChange{ + ChangeID: "D12345", + Action: BuildActionValidate, + }, + wantID: "D12345", + wantAction: BuildActionValidate, + }, + { + name: "apply action", + change: BuildChange{ + ChangeID: "PR-42", + Action: BuildActionApply, + }, + wantID: "PR-42", + wantAction: BuildActionApply, + }, + { + name: "unknown action", + change: BuildChange{ + ChangeID: "123", + Action: BuildActionUnknown, + }, + wantID: "123", + wantAction: BuildActionUnknown, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.wantID, tt.change.ChangeID) + assert.Equal(t, tt.wantAction, tt.change.Action) + }) + } +} diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 09fb3203..61167656 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -14,22 +14,23 @@ import ( // // All implementations must be thread-safe and support concurrent operations. type BuildManager interface { - // ScheduleBuild creates a new build with the CI provider for testing a batch. + // Schedule submits a list of changes to the CI provider for processing. + // Each change specifies an action (validate or apply) to perform. + // + // 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) + // - Tracking build IDs internally for subsequent Poll/CancelBuild calls // // Parameters: - // - head: BatchID of the batch being tested - // - base: List of BatchIDs (in order) that have been applied on main. Order matters. - // - jobName: Pipeline/job name to be called on the CI provider + // - ctx: Request context for cancellation and timeouts + // - queueName: Name of the queue processing these changes. Used to look up job configuration. + // - changes: List of changes to process. Order may be significant for dependencies. // // Returns: - // - string: Unique build ID // - error: ErrInvalidRequest if validation fails, ErrProviderUnavailable if CI provider is unreachable - ScheduleBuild( - ctx context.Context, - head string, - base []string, - jobName string, - ) (string, error) + Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) error // Poll retrieves the current status of a build from the CI provider. // This is a synchronous call that queries the provider's API. diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go index c17a3de4..30286eee 100644 --- a/extension/build/mock/build_manager.go +++ b/extension/build/mock/build_manager.go @@ -78,17 +78,16 @@ func (mr *MockBuildManagerMockRecorder) Poll(ctx, id interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Poll", reflect.TypeOf((*MockBuildManager)(nil).Poll), ctx, id) } -// ScheduleBuild mocks base method. -func (m *MockBuildManager) ScheduleBuild(ctx context.Context, head string, base []string, jobName string) (string, error) { +// Schedule mocks base method. +func (m *MockBuildManager) Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ScheduleBuild", ctx, head, base, jobName) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret := m.ctrl.Call(m, "Schedule", ctx, queueName, changes) + ret0, _ := ret[0].(error) + return ret0 } -// ScheduleBuild indicates an expected call of ScheduleBuild. -func (mr *MockBuildManagerMockRecorder) ScheduleBuild(ctx, head, base, jobName interface{}) *gomock.Call { +// Schedule indicates an expected call of Schedule. +func (mr *MockBuildManagerMockRecorder) Schedule(ctx, queueName, changes interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ScheduleBuild", reflect.TypeOf((*MockBuildManager)(nil).ScheduleBuild), ctx, head, base, jobName) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Schedule", reflect.TypeOf((*MockBuildManager)(nil).Schedule), ctx, queueName, changes) } diff --git a/extension/build/mock/build_manager_test.go b/extension/build/mock/build_manager_test.go index 3a2ebcc8..722decc5 100644 --- a/extension/build/mock/build_manager_test.go +++ b/extension/build/mock/build_manager_test.go @@ -4,8 +4,8 @@ import ( "context" "testing" - "github.com/uber/submitqueue/entity" "go.uber.org/mock/gomock" + "github.com/uber/submitqueue/entity" ) // TestMockBuildManager_Compilation verifies the mock compiles and basic setup works. @@ -15,28 +15,27 @@ func TestMockBuildManager_Compilation(t *testing.T) { mockBuildMgr := NewMockBuildManager(ctrl) - // Test ScheduleBuild - buildID := "mock://1" + // Test Schedule mockBuildMgr.EXPECT(). - ScheduleBuild(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). - Return(buildID, nil) + Schedule(gomock.Any(), gomock.Any(), gomock.Any()). + Return(nil) - head := "queue-1/batch/5" - base := []string{"queue-1/batch/1", "queue-1/batch/2"} - jobName := "test-pipeline" + queueName := "test-queue" + changes := []entity.BuildChange{ + {ChangeID: "D12345", Action: entity.BuildActionValidate}, + {ChangeID: "D12346", Action: entity.BuildActionApply}, + } - result, err := mockBuildMgr.ScheduleBuild( - context.Background(), head, base, jobName, + err := mockBuildMgr.Schedule( + context.Background(), queueName, changes, ) if err != nil { t.Fatalf("unexpected error: %v", err) } - if result != buildID { - t.Fatalf("expected %v, got %v", buildID, result) - } // Test Poll + buildID := "mock://1" mockBuildMgr.EXPECT(). Poll(gomock.Any(), gomock.Any()). Return(entity.BuildStatusPassed, nil) From bfb2ba5525b83d4c0f47b55d13f5e0bc9761e4c2 Mon Sep 17 00:00:00 2001 From: djuloori Date: Wed, 25 Feb 2026 19:27:25 +0000 Subject: [PATCH 04/18] feat(extension): enhance BuildManager interface and add BuildStatusAccepted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback to improve BuildManager interface design and add missing build status for better lifecycle tracking. Changes: - Add BuildStatusAccepted status to entity/build.go - Represents build accepted by CI provider (between queued and running) - Non-terminal state in the build lifecycle - Added test coverage in entity/build_test.go - Update BuildManager.Schedule to return build ID - Returns (string, error) instead of just error - Build ID enables callers to poll and cancel builds - Updated mocks and tests to match new signature - Enhance BuildManager interface documentation - Clarify implementations can be singletons or on-demand instances - Allows flexibility based on CI provider requirements - Clarify CancelBuild behavior - Document asynchronous operation (fire-and-forget) - Remove restriction on terminal state cancellation - Implementation decides how to handle terminal state requests - Callers should use Poll to verify cancellation Build lifecycle now: queued → accepted → running → passed/failed/cancelled Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 3 +++ entity/build_test.go | 5 +++++ extension/build/build_manager.go | 22 +++++++++++++++++----- extension/build/mock/build_manager.go | 7 ++++--- extension/build/mock/build_manager_test.go | 9 ++++++--- 5 files changed, 35 insertions(+), 11 deletions(-) diff --git a/entity/build.go b/entity/build.go index 7bdbf60a..3bd40cf7 100644 --- a/entity/build.go +++ b/entity/build.go @@ -10,6 +10,9 @@ const ( // BuildStatusQueued indicates the build has been scheduled but not yet started. BuildStatusQueued BuildStatus = "queued" + // BuildStatusAccepted indicates the build has been accepted by the CI provider. + BuildStatusAccepted BuildStatus = "accepted" + // BuildStatusRunning indicates the build is currently executing. BuildStatusRunning BuildStatus = "running" diff --git a/entity/build_test.go b/entity/build_test.go index e377820b..10aed153 100644 --- a/entity/build_test.go +++ b/entity/build_test.go @@ -32,6 +32,11 @@ func TestBuildStatus_IsTerminal(t *testing.T) { status: BuildStatusQueued, expected: false, }, + { + name: "accepted is not terminal", + status: BuildStatusAccepted, + expected: false, + }, { name: "running is not terminal", status: BuildStatusRunning, diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 61167656..46235c9c 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -12,6 +12,10 @@ import ( // Implementations provide integration with specific CI systems (BuildKite, Jenkins, etc.) // to schedule builds, poll their status, and cancel running builds. // +// Implementations may be designed as heavy singletons (with connection pooling and caching) +// or lightweight instances created on-demand, depending on the CI provider's requirements +// and implementation strategy. +// // 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. @@ -21,7 +25,6 @@ type BuildManager interface { // - 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) - // - Tracking build IDs internally for subsequent Poll/CancelBuild calls // // Parameters: // - ctx: Request context for cancellation and timeouts @@ -29,8 +32,9 @@ type BuildManager interface { // - 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 // - error: ErrInvalidRequest if validation fails, ErrProviderUnavailable if CI provider is unreachable - Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) error + Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) (string, error) // Poll retrieves the current status of a build from the CI provider. // This is a synchronous call that queries the provider's API. @@ -43,15 +47,23 @@ type BuildManager interface { // - error: ErrBuildNotFound if the build doesn't exist, ErrProviderUnavailable if CI provider is unreachable Poll(ctx context.Context, id string) (entity.BuildStatus, error) - // CancelBuild requests cancellation of a queued or running build. - // Builds that have already completed cannot be cancelled. + // CancelBuild requests cancellation of a build. + // + // This operation is asynchronous and does not wait for the cancellation to complete. + // The implementation should initiate the cancellation request with the CI provider + // and return immediately. Use Poll to check if the build has transitioned to + // BuildStatusCancelled. + // + // The implementation decides how to handle cancellation requests for builds in + // terminal states (passed, failed, cancelled). It may return an error, silently + // ignore the request, or handle it in a provider-specific way. // // Parameters: // - id: Build ID string // // Returns: // - error: ErrBuildNotFound if the build doesn't exist, - // ErrBuildNotCancellable if the build has already finished, + // ErrBuildNotCancellable if the build cannot be cancelled (implementation-specific), // ErrProviderUnavailable if the CI provider is unreachable CancelBuild(ctx context.Context, id string) error diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go index 30286eee..a019a33c 100644 --- a/extension/build/mock/build_manager.go +++ b/extension/build/mock/build_manager.go @@ -79,11 +79,12 @@ func (mr *MockBuildManagerMockRecorder) Poll(ctx, id interface{}) *gomock.Call { } // Schedule mocks base method. -func (m *MockBuildManager) Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) error { +func (m *MockBuildManager) Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) (string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Schedule", ctx, queueName, changes) - ret0, _ := ret[0].(error) - return ret0 + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(error) + return ret0, ret1 } // Schedule indicates an expected call of Schedule. diff --git a/extension/build/mock/build_manager_test.go b/extension/build/mock/build_manager_test.go index 722decc5..d7933eee 100644 --- a/extension/build/mock/build_manager_test.go +++ b/extension/build/mock/build_manager_test.go @@ -16,9 +16,10 @@ func TestMockBuildManager_Compilation(t *testing.T) { mockBuildMgr := NewMockBuildManager(ctrl) // Test Schedule + expectedBuildID := "mock://test-build-123" mockBuildMgr.EXPECT(). Schedule(gomock.Any(), gomock.Any(), gomock.Any()). - Return(nil) + Return(expectedBuildID, nil) queueName := "test-queue" changes := []entity.BuildChange{ @@ -26,16 +27,18 @@ func TestMockBuildManager_Compilation(t *testing.T) { {ChangeID: "D12346", Action: entity.BuildActionApply}, } - err := mockBuildMgr.Schedule( + buildID, err := mockBuildMgr.Schedule( context.Background(), queueName, changes, ) if err != nil { t.Fatalf("unexpected error: %v", err) } + if buildID != expectedBuildID { + t.Fatalf("expected build ID %v, got %v", expectedBuildID, buildID) + } // Test Poll - buildID := "mock://1" mockBuildMgr.EXPECT(). Poll(gomock.Any(), gomock.Any()). Return(entity.BuildStatusPassed, nil) From a10c7f93e76b58beabe823f74d302ebfa4c2d192 Mon Sep 17 00:00:00 2001 From: djuloori Date: Wed, 25 Feb 2026 19:39:30 +0000 Subject: [PATCH 05/18] docs(extension): update BuildManager README with latest interface changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update README.md to reflect all recent changes to BuildManager interface and entity types. Changes: - Update interface signature from ScheduleBuild to Schedule - New parameters: queueName and []BuildChange instead of head, base, jobName - Document Close() method - Add documentation for new types - BuildChange struct (ChangeID, Action) - BuildAction enum (validate, apply) - Update BuildStatus lifecycle - Add BuildStatusAccepted state - Document lifecycle: queued → accepted → running → passed/failed/cancelled - Enhance interface documentation - Clarify singleton vs on-demand implementation flexibility - Document CancelBuild asynchronous behavior - Update terminal state cancellation handling - Update usage examples - Show new Schedule API with BuildChange - Update mock examples - Add cancellation workflow example - Update provider implementation guide - Add queue config integration steps - Document action handling (validate vs apply) - Add implementation design choice guidance - Revise design principles - Replace "No persistent state" with "Implementation flexibility" - Add "Change-focused" and "Asynchronous cancellation" Co-Authored-By: Claude Opus 4.6 --- extension/build/README.md | 153 ++++++++++++++++++++++++++++++-------- 1 file changed, 122 insertions(+), 31 deletions(-) diff --git a/extension/build/README.md b/extension/build/README.md index bad0768d..4139946c 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -14,39 +14,70 @@ Main interface for interacting with CI providers. ```go type BuildManager interface { - // Schedule a new build with the CI provider for testing a batch - ScheduleBuild( + // Schedule submits a list of changes to the CI provider for processing + Schedule( ctx context.Context, - head string, - base []string, - jobName string, + queueName string, + changes []entity.BuildChange, ) (string, error) - // Poll current status of a build + // Poll retrieves the current status of a build from the CI provider Poll(ctx context.Context, id string) (entity.BuildStatus, error) - // Cancel a running or queued build + // CancelBuild requests cancellation of a build (asynchronous operation) CancelBuild(ctx context.Context, id string) error + + // Close gracefully shuts down the build manager + Close() error } ``` **Thread-safety**: All implementations must be thread-safe and support concurrent operations. +**Implementation Design**: Implementations may be designed as heavy singletons (with connection pooling and caching) or lightweight instances created on-demand, depending on the CI provider's requirements and implementation strategy. + ## Types -### ScheduleBuild Parameters +### BuildChange + +Represents a code change to be processed by the build system. + +```go +type BuildChange struct { + // ChangeID is the unique identifier for this change (diff ID, PR number, etc.) + ChangeID string + // Action specifies what operation to perform on this change + Action BuildAction +} +``` + +### BuildAction + +Defines the action to perform on a change. -**head** (string, required): BatchID of the batch being tested +```go +type BuildAction string + +const ( + BuildActionUnknown BuildAction = "" // Sentinel value + BuildActionValidate BuildAction = "validate" // Run validation/testing without applying + BuildActionApply BuildAction = "apply" // Apply the change to the target branch +) +``` -**base** ([]string, required): List of BatchIDs (in order) that have been applied on main. Order matters. +### Schedule Parameters -**jobName** (string, required): Pipeline/job name to be called on the CI provider -- BuildKite: "organization/pipeline-slug" -- Jenkins: "job-name" +**queueName** (string, required): Name of the queue processing these changes. Used to look up job configuration from queue config. + +**changes** ([]entity.BuildChange, required): List of changes to process. Each change includes: +- **ChangeID**: Unique identifier (e.g., "D12345" for Phabricator, "42" for GitHub PR) +- **Action**: What to do with the change (validate or apply) + +Order of changes may be significant for dependencies. ### Build ID Format -Build IDs returned by `ScheduleBuild` should use a URI-like format: `"provider://id"` +Build IDs returned by `Schedule` should use a URI-like format: `"provider://id"` **Examples**: - `"buildkite://uber/submitqueue-ci/123"` @@ -55,7 +86,7 @@ Build IDs returned by `ScheduleBuild` should use a URI-like format: `"provider:/ This format allows the implementation to encode both the provider name and provider-specific build identifier in a single string. -### Build State Enum +### Build Status Enum ```go type BuildStatus string @@ -63,6 +94,7 @@ type BuildStatus string const ( BuildStatusUnknown BuildStatus = "" // Sentinel value BuildStatusQueued BuildStatus = "queued" // Scheduled but not started + BuildStatusAccepted BuildStatus = "accepted" // Accepted by CI provider BuildStatusRunning BuildStatus = "running" // Currently executing BuildStatusPassed BuildStatus = "passed" // Completed successfully (terminal) BuildStatusFailed BuildStatus = "failed" // Completed with failures (terminal) @@ -73,6 +105,8 @@ const ( func (s BuildStatus) IsTerminal() bool ``` +**Build Lifecycle**: `queued` → `accepted` → `running` → `passed`/`failed`/`cancelled` + ## Error Handling The extension defines sentinel errors following the SubmitQueue pattern: @@ -96,6 +130,32 @@ if build.IsBuildNotFound(err) { ## Usage +### Basic Workflow + +```go +// 1. Schedule a build with changes +changes := []entity.BuildChange{ + {ChangeID: "D12345", Action: entity.BuildActionValidate}, + {ChangeID: "D12346", Action: entity.BuildActionApply}, +} + +buildID, err := buildMgr.Schedule(ctx, "my-queue", changes) +if err != nil { + // Handle error +} + +// 2. Poll for build status +status, err := buildMgr.Poll(ctx, buildID) +if err != nil { + // Handle error +} + +// 3. Check if build is done +if status.IsTerminal() { + // Build finished: status is passed, failed, or cancelled +} +``` + ### GoMock Mocks For unit testing with gomock, use the generated mock: @@ -104,6 +164,7 @@ For unit testing with gomock, use the generated mock: import ( "testing" + "github.com/uber/submitqueue/entity" "github.com/uber/submitqueue/extension/build/mock" "go.uber.org/mock/gomock" ) @@ -115,8 +176,12 @@ func TestMyController(t *testing.T) { mockBuildMgr := mock.NewMockBuildManager(ctrl) // Set up expectations + changes := []entity.BuildChange{ + {ChangeID: "D12345", Action: entity.BuildActionValidate}, + } + mockBuildMgr.EXPECT(). - ScheduleBuild(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). + Schedule(gomock.Any(), "test-queue", changes). Return("mock://1", nil) // Test your code that uses the mock @@ -125,15 +190,25 @@ func TestMyController(t *testing.T) { ### Cancelling Builds +Cancellation is **asynchronous** - the method initiates the cancellation request and returns immediately. Use `Poll` to check if the build has transitioned to `BuildStatusCancelled`. + ```go err := mgr.CancelBuild(ctx, buildID) if build.IsBuildNotCancellable(err) { - // Build already finished + // Build cannot be cancelled (implementation-specific) } else if err != nil { // Other error } + +// Poll to verify cancellation +status, _ := mgr.Poll(ctx, buildID) +if status == entity.BuildStatusCancelled { + // Cancellation successful +} ``` +**Note**: The implementation decides how to handle cancellation requests for builds in terminal states (passed, failed, cancelled). It may return an error, silently ignore the request, or handle it in a provider-specific way. + ## Implementing a New Provider To add support for a new CI provider: @@ -148,10 +223,11 @@ To add support for a new CI provider: type Params struct { // Provider-specific configuration - BaseURL string - Username string - APIToken string - Logger *zap.Logger + BaseURL string + Username string + APIToken string + QueueConfig QueueConfigStore // For looking up job names + Logger *zap.Logger MetricsScope tally.Scope } @@ -164,6 +240,7 @@ To add support for a new CI provider: 3. **Map provider states to entity.BuildStatus enum**: - Map provider's state values to the standard `entity.BuildStatus` constants + - Include mapping for `BuildStatusAccepted` if provider supports it - Use `entity.BuildStatusUnknown` for unexpected states 4. **Handle provider errors**: @@ -171,19 +248,29 @@ To add support for a new CI provider: - 5xx errors → `build.WrapProviderUnavailable()` - Validation errors → `build.WrapInvalidRequest()` -5. **Define jobName format**: - - Document the expected format in provider README - - Examples: - - BuildKite: `"organization/pipeline-slug"` - - Jenkins: `"job-name"` +5. **Implement Schedule method**: + - Look up job name from queue config using `queueName` parameter + - Handle both `BuildActionValidate` and `BuildActionApply` actions + - Create appropriate builds/jobs for each change + - Return unique build ID + +6. **Implement CancelBuild**: + - Make asynchronous cancellation request to CI provider + - Return immediately after initiating request + - Decide how to handle terminal state cancellations + +7. **Choose implementation design**: + - **Singleton**: If provider needs connection pooling, auth token caching, rate limiting + - **Lightweight**: If provider SDK handles resource management or implementation is stateless -6. **Add tests**: +8. **Add tests**: - Unit tests with mock HTTP server - Validation tests for all required fields - Error mapping tests - Thread-safety tests + - Test both validate and apply actions -7. **Update BUILD.bazel**: +9. **Update BUILD.bazel**: ```bazel go_library( name = "jenkins", @@ -191,7 +278,9 @@ To add support for a new CI provider: importpath = "github.com/uber/submitqueue/extension/build/jenkins", visibility = ["//visibility:public"], deps = [ + "//entity", "//extension/build", + "//extension/queueconfig", "@org_uber_go_zap//:zap", "@com_github_uber_go_tally_v4//:tally", ], @@ -223,10 +312,11 @@ extension/build/ ### Design Principles 1. **Vendor-agnostic**: Interface doesn't leak provider-specific details -2. **Immutable types**: BuildRequest and BuildStatus are value types +2. **Change-focused**: Operates on individual changes (diff IDs, PR numbers) with explicit actions 3. **Thread-safe**: All implementations support concurrent operations 4. **Error transparency**: Sentinel errors for common failure modes -5. **No persistent state**: BuildManager doesn't manage connections or require cleanup +5. **Implementation flexibility**: Supports both heavy singleton and lightweight on-demand designs +6. **Asynchronous cancellation**: CancelBuild initiates request and returns immediately ## Future Enhancements @@ -235,6 +325,7 @@ Potential improvements not in the current implementation: - **Webhook support**: Accept push notifications from CI providers instead of polling - **Build artifacts**: Track and retrieve build artifacts - **Retry logic**: Automatic retry for transient failures -- **Batch operations**: Schedule/poll/cancel multiple builds at once +- **Batch operations**: Poll/cancel multiple builds at once - **Streaming logs**: Real-time log streaming from builds - **Build caching**: Cache build status to reduce API calls +- **Partial cancellation**: Cancel individual changes in a multi-change build From 025cf2af8c587d4f4b257a8edb71748f74391d77 Mon Sep 17 00:00:00 2001 From: djuloori Date: Wed, 25 Feb 2026 23:01:57 +0000 Subject: [PATCH 06/18] refactor(extension): simplify BuildStatus enum and error contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify BuildManager interface by removing unnecessary build statuses and removing unexpected errors from the interface contract. Changes to BuildStatus enum: - Remove BuildStatusQueued - no longer needed - Remove BuildStatusRunning - no longer needed - Keep only 4 statuses: accepted, passed, failed, cancelled - Update lifecycle: accepted → passed/failed/cancelled - Update tests to remove queued and running test cases Changes to BuildManager interface: - Remove ErrProviderUnavailable from all method contracts - Schedule: only ErrInvalidRequest expected - Poll: only ErrBuildNotFound expected - CancelBuild: only ErrBuildNotFound and ErrBuildNotCancellable expected - Implementations can still handle provider errors internally - Only expected/actionable errors are part of the interface contract Documentation updates: - Update BuildStatus enum in README - Update build lifecycle diagram - Remove ErrProviderUnavailable from error handling section - Remove 5xx error mapping from implementation guide - Update ErrBuildNotCancellable description The simplified interface makes it clearer what errors callers need to handle explicitly, while implementations retain flexibility to handle provider-specific issues internally. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 6 ------ entity/build_test.go | 10 ---------- extension/build/README.md | 8 ++------ extension/build/build_manager.go | 7 +++---- 4 files changed, 5 insertions(+), 26 deletions(-) diff --git a/entity/build.go b/entity/build.go index 3bd40cf7..93f15bab 100644 --- a/entity/build.go +++ b/entity/build.go @@ -7,15 +7,9 @@ const ( // BuildStatusUnknown is the unreachable state. It is set by default when the structure is initialized. It should never be seen in the system. BuildStatusUnknown BuildStatus = "" - // BuildStatusQueued indicates the build has been scheduled but not yet started. - BuildStatusQueued BuildStatus = "queued" - // BuildStatusAccepted indicates the build has been accepted by the CI provider. BuildStatusAccepted BuildStatus = "accepted" - // BuildStatusRunning indicates the build is currently executing. - BuildStatusRunning BuildStatus = "running" - // BuildStatusPassed indicates the build completed successfully. // This is a terminal state. BuildStatusPassed BuildStatus = "passed" diff --git a/entity/build_test.go b/entity/build_test.go index 10aed153..47fe143d 100644 --- a/entity/build_test.go +++ b/entity/build_test.go @@ -27,21 +27,11 @@ func TestBuildStatus_IsTerminal(t *testing.T) { status: BuildStatusCancelled, expected: true, }, - { - name: "queued is not terminal", - status: BuildStatusQueued, - expected: false, - }, { name: "accepted is not terminal", status: BuildStatusAccepted, expected: false, }, - { - name: "running is not terminal", - status: BuildStatusRunning, - expected: false, - }, { name: "unknown is not terminal", status: BuildStatusUnknown, diff --git a/extension/build/README.md b/extension/build/README.md index 4139946c..5a10d634 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -93,9 +93,7 @@ type BuildStatus string const ( BuildStatusUnknown BuildStatus = "" // Sentinel value - BuildStatusQueued BuildStatus = "queued" // Scheduled but not started BuildStatusAccepted BuildStatus = "accepted" // Accepted by CI provider - BuildStatusRunning BuildStatus = "running" // Currently executing BuildStatusPassed BuildStatus = "passed" // Completed successfully (terminal) BuildStatusFailed BuildStatus = "failed" // Completed with failures (terminal) BuildStatusCancelled BuildStatus = "cancelled" // Cancelled before completion (terminal) @@ -105,15 +103,14 @@ const ( func (s BuildStatus) IsTerminal() bool ``` -**Build Lifecycle**: `queued` → `accepted` → `running` → `passed`/`failed`/`cancelled` +**Build Lifecycle**: `accepted` → `passed`/`failed`/`cancelled` ## Error Handling The extension defines sentinel errors following the SubmitQueue pattern: - **`ErrBuildNotFound`** - Build doesn't exist or was deleted -- **`ErrBuildNotCancellable`** - Build has already finished -- **`ErrProviderUnavailable`** - CI provider unreachable or experiencing errors +- **`ErrBuildNotCancellable`** - Build cannot be cancelled (implementation-specific) - **`ErrInvalidRequest`** - Request validation failed Each error has helper functions: @@ -245,7 +242,6 @@ To add support for a new CI provider: 4. **Handle provider errors**: - 404 errors → `build.WrapBuildNotFound()` - - 5xx errors → `build.WrapProviderUnavailable()` - Validation errors → `build.WrapInvalidRequest()` 5. **Implement Schedule method**: diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 46235c9c..5e6d4544 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -33,7 +33,7 @@ type BuildManager interface { // // Returns: // - string: Unique build ID that can be used with Poll and CancelBuild methods - // - error: ErrInvalidRequest if validation fails, ErrProviderUnavailable if CI provider is unreachable + // - error: ErrInvalidRequest if validation fails Schedule(ctx context.Context, queueName string, changes []entity.BuildChange) (string, error) // Poll retrieves the current status of a build from the CI provider. @@ -44,7 +44,7 @@ type BuildManager interface { // // Returns: // - BuildStatus: Current state of the build - // - error: ErrBuildNotFound if the build doesn't exist, ErrProviderUnavailable if CI provider is unreachable + // - error: ErrBuildNotFound if the build doesn't exist Poll(ctx context.Context, id string) (entity.BuildStatus, error) // CancelBuild requests cancellation of a build. @@ -63,8 +63,7 @@ type BuildManager interface { // // Returns: // - error: ErrBuildNotFound if the build doesn't exist, - // ErrBuildNotCancellable if the build cannot be cancelled (implementation-specific), - // ErrProviderUnavailable if the CI provider is unreachable + // ErrBuildNotCancellable if the build cannot be cancelled (implementation-specific) CancelBuild(ctx context.Context, id string) error // Close gracefully shuts down the build manager. From bf095cab33cdbbee4f129bb06c85a7a310e30707 Mon Sep 17 00:00:00 2001 From: djuloori Date: Wed, 25 Feb 2026 23:31:04 +0000 Subject: [PATCH 07/18] feat(extension): add metadata to Poll method and reorder BuildAction enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enhance BuildManager.Poll to return build metadata and reorder BuildAction constants for better semantic ordering. Changes to Poll method: - Add map[string]string return value for build metadata - Document common metadata keys (build_url, commit_sha, duration_ms, etc.) - Update interface signature: Poll(ctx, id) → (BuildStatus, map[string]string, error) - Update mocks and tests to handle metadata return value Changes to BuildAction enum: - Reorder constants: BuildActionApply now comes before BuildActionValidate - Apply is the primary action, validate is secondary - Update all tests and examples to reflect new ordering Documentation updates: - Add Build Metadata section explaining common keys - Update all Poll usage examples to show metadata handling - Update mock examples with metadata assertions - Clarify that metadata keys are implementation-defined The metadata map allows implementations to return provider-specific information (build URLs, commit SHAs, timestamps, etc.) without changing the interface. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 4 +-- entity/build_test.go | 18 ++++++------ extension/build/README.md | 32 +++++++++++++++++----- extension/build/build_manager.go | 3 +- extension/build/mock/build_manager.go | 7 +++-- extension/build/mock/build_manager_test.go | 15 +++++++--- 6 files changed, 53 insertions(+), 26 deletions(-) diff --git a/entity/build.go b/entity/build.go index 93f15bab..02283432 100644 --- a/entity/build.go +++ b/entity/build.go @@ -60,10 +60,10 @@ type BuildAction string const ( // BuildActionUnknown is the sentinel value for uninitialized actions. BuildActionUnknown BuildAction = "" - // BuildActionValidate runs validation/testing on the change without applying it. - BuildActionValidate BuildAction = "validate" // BuildActionApply applies the change to the target branch. BuildActionApply BuildAction = "apply" + // BuildActionValidate runs validation/testing on the change without applying it. + BuildActionValidate BuildAction = "validate" ) // BuildChange represents a code change to be processed by the build system. diff --git a/entity/build_test.go b/entity/build_test.go index 47fe143d..b01fd5c0 100644 --- a/entity/build_test.go +++ b/entity/build_test.go @@ -53,15 +53,6 @@ func TestBuildChange_Creation(t *testing.T) { wantID string wantAction BuildAction }{ - { - name: "validate action", - change: BuildChange{ - ChangeID: "D12345", - Action: BuildActionValidate, - }, - wantID: "D12345", - wantAction: BuildActionValidate, - }, { name: "apply action", change: BuildChange{ @@ -71,6 +62,15 @@ func TestBuildChange_Creation(t *testing.T) { wantID: "PR-42", wantAction: BuildActionApply, }, + { + name: "validate action", + change: BuildChange{ + ChangeID: "D12345", + Action: BuildActionValidate, + }, + wantID: "D12345", + wantAction: BuildActionValidate, + }, { name: "unknown action", change: BuildChange{ diff --git a/extension/build/README.md b/extension/build/README.md index 5a10d634..f5c27dbb 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -22,7 +22,7 @@ type BuildManager interface { ) (string, error) // Poll retrieves the current status of a build from the CI provider - Poll(ctx context.Context, id string) (entity.BuildStatus, error) + Poll(ctx context.Context, id string) (entity.BuildStatus, map[string]string, error) // CancelBuild requests cancellation of a build (asynchronous operation) CancelBuild(ctx context.Context, id string) error @@ -60,8 +60,8 @@ type BuildAction string const ( BuildActionUnknown BuildAction = "" // Sentinel value - BuildActionValidate BuildAction = "validate" // Run validation/testing without applying BuildActionApply BuildAction = "apply" // Apply the change to the target branch + BuildActionValidate BuildAction = "validate" // Run validation/testing without applying ) ``` @@ -105,6 +105,20 @@ func (s BuildStatus) IsTerminal() bool **Build Lifecycle**: `accepted` → `passed`/`failed`/`cancelled` +### Build Metadata + +The `Poll` method returns a `map[string]string` containing additional metadata about the build. The specific keys and values are implementation-defined, but common examples include: + +**Common metadata keys:** +- `build_url` - Direct link to the build in the CI provider's UI +- `commit_sha` - Git commit SHA being tested +- `duration_ms` - Build duration in milliseconds +- `started_at` - Build start timestamp +- `finished_at` - Build completion timestamp +- `error_message` - Error details for failed builds + +Implementations may include additional provider-specific metadata. Consumers should handle missing keys gracefully. + ## Error Handling The extension defines sentinel errors following the SubmitQueue pattern: @@ -119,7 +133,7 @@ Each error has helper functions: Example: ```go -status, err := buildMgr.Poll(ctx, buildID) +status, metadata, err := buildMgr.Poll(ctx, buildID) if build.IsBuildNotFound(err) { // Handle missing build } @@ -132,8 +146,8 @@ if build.IsBuildNotFound(err) { ```go // 1. Schedule a build with changes changes := []entity.BuildChange{ - {ChangeID: "D12345", Action: entity.BuildActionValidate}, - {ChangeID: "D12346", Action: entity.BuildActionApply}, + {ChangeID: "D12345", Action: entity.BuildActionApply}, + {ChangeID: "D12346", Action: entity.BuildActionValidate}, } buildID, err := buildMgr.Schedule(ctx, "my-queue", changes) @@ -142,11 +156,15 @@ if err != nil { } // 2. Poll for build status -status, err := buildMgr.Poll(ctx, buildID) +status, metadata, err := buildMgr.Poll(ctx, buildID) if err != nil { // Handle error } +// Access build metadata +buildURL := metadata["build_url"] +commitSHA := metadata["commit_sha"] + // 3. Check if build is done if status.IsTerminal() { // Build finished: status is passed, failed, or cancelled @@ -246,7 +264,7 @@ To add support for a new CI provider: 5. **Implement Schedule method**: - Look up job name from queue config using `queueName` parameter - - Handle both `BuildActionValidate` and `BuildActionApply` actions + - Handle both `BuildActionApply` and `BuildActionValidate` actions - Create appropriate builds/jobs for each change - Return unique build ID diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 5e6d4544..611cfae0 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -44,8 +44,9 @@ type BuildManager interface { // // Returns: // - BuildStatus: Current state of the build + // - map[string]string: Additional metadata about the build (e.g., build URL, commit SHA, duration) // - error: ErrBuildNotFound if the build doesn't exist - Poll(ctx context.Context, id string) (entity.BuildStatus, error) + Poll(ctx context.Context, id string) (entity.BuildStatus, map[string]string, error) // CancelBuild requests cancellation of a build. // diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go index a019a33c..2af1718a 100644 --- a/extension/build/mock/build_manager.go +++ b/extension/build/mock/build_manager.go @@ -64,12 +64,13 @@ func (mr *MockBuildManagerMockRecorder) Close() *gomock.Call { } // Poll mocks base method. -func (m *MockBuildManager) Poll(ctx context.Context, id string) (entity.BuildStatus, error) { +func (m *MockBuildManager) Poll(ctx context.Context, id string) (entity.BuildStatus, map[string]string, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Poll", ctx, id) ret0, _ := ret[0].(entity.BuildStatus) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret1, _ := ret[1].(map[string]string) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 } // Poll indicates an expected call of Poll. diff --git a/extension/build/mock/build_manager_test.go b/extension/build/mock/build_manager_test.go index d7933eee..0e569661 100644 --- a/extension/build/mock/build_manager_test.go +++ b/extension/build/mock/build_manager_test.go @@ -23,8 +23,8 @@ func TestMockBuildManager_Compilation(t *testing.T) { queueName := "test-queue" changes := []entity.BuildChange{ - {ChangeID: "D12345", Action: entity.BuildActionValidate}, - {ChangeID: "D12346", Action: entity.BuildActionApply}, + {ChangeID: "D12345", Action: entity.BuildActionApply}, + {ChangeID: "D12346", Action: entity.BuildActionValidate}, } buildID, err := mockBuildMgr.Schedule( @@ -39,15 +39,22 @@ func TestMockBuildManager_Compilation(t *testing.T) { } // Test Poll + expectedMetadata := map[string]string{ + "build_url": "https://ci.example.com/builds/123", + "commit_sha": "abc123", + } mockBuildMgr.EXPECT(). Poll(gomock.Any(), gomock.Any()). - Return(entity.BuildStatusPassed, nil) + Return(entity.BuildStatusPassed, expectedMetadata, nil) - status, err := mockBuildMgr.Poll(context.Background(), buildID) + status, metadata, err := mockBuildMgr.Poll(context.Background(), buildID) if err != nil { t.Fatalf("unexpected error: %v", err) } if status != entity.BuildStatusPassed { t.Fatalf("expected %v, got %v", entity.BuildStatusPassed, status) } + if metadata["build_url"] != expectedMetadata["build_url"] { + t.Fatalf("expected metadata %v, got %v", expectedMetadata, metadata) + } } From 3da9f04c2aad72e77a6b4b71bb98230387490848 Mon Sep 17 00:00:00 2001 From: djuloori Date: Wed, 25 Feb 2026 23:37:11 +0000 Subject: [PATCH 08/18] refactor(extension): remove ErrProviderUnavailable from errors.go Remove ErrProviderUnavailable and its helper functions since it's no longer part of the BuildManager interface contract. Update remaining error comments to reflect current interface. Changes: - Remove ErrProviderUnavailable, IsProviderUnavailable, WrapProviderUnavailable - Update ErrInvalidRequest comment to reference Schedule parameters (queueName, changes) instead of outdated parameters (baseSHA, batchToBeTest.ID, etc.) - Update ErrBuildNotCancellable comment to clarify conditions are implementation-defined Only expected errors (ErrBuildNotFound, ErrBuildNotCancellable, ErrInvalidRequest) remain in errors.go, matching the BuildManager interface contract. Co-Authored-By: Claude Opus 4.6 --- extension/build/errors.go | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-) diff --git a/extension/build/errors.go b/extension/build/errors.go index 75ac8f2c..0ef84b85 100644 --- a/extension/build/errors.go +++ b/extension/build/errors.go @@ -24,7 +24,7 @@ func WrapBuildNotFound(err error) error { } // ErrBuildNotCancellable is returned when attempting to cancel a build that cannot be cancelled. -// This occurs when: +// The specific conditions are implementation-defined and may include: // - The build has already finished (passed, failed, or cancelled) // - The provider does not support cancellation for this build type var ErrBuildNotCancellable = errors.New("build not cancellable") @@ -39,28 +39,11 @@ func WrapBuildNotCancellable(err error) error { return fmt.Errorf("%w: %w", ErrBuildNotCancellable, err) } -// ErrProviderUnavailable is returned when the CI provider is unreachable or experiencing errors. -// This can occur due to: -// - Network connectivity issues -// - Provider service outages (5xx errors) -// - Authentication failures (invalid API tokens) -// - Rate limiting -var ErrProviderUnavailable = errors.New("provider unavailable") - -// IsProviderUnavailable returns true if any error in the error chain is ErrProviderUnavailable. -func IsProviderUnavailable(err error) bool { - return errors.Is(err, ErrProviderUnavailable) -} - -// WrapProviderUnavailable wraps ErrProviderUnavailable with the original error from the build provider. -func WrapProviderUnavailable(err error) error { - return fmt.Errorf("%w: %w", ErrProviderUnavailable, err) -} - -// ErrInvalidRequest is returned when ScheduleBuild parameters fail validation. +// ErrInvalidRequest is returned when Schedule parameters fail validation. // This can occur when: -// - Required parameters are missing (baseSHA, batchToBeTest.ID, repoURL, branch, pipelineID, sqid) -// - Parameter values are malformed (invalid URLs, empty strings, etc.) +// - queueName is empty or invalid +// - changes list is empty +// - changes contain invalid ChangeIDs or Actions var ErrInvalidRequest = errors.New("invalid request") // IsInvalidRequest returns true if any error in the error chain is ErrInvalidRequest. From d009f946e557a699cf575de3ee0547529b325401 Mon Sep 17 00:00:00 2001 From: djuloori Date: Wed, 25 Feb 2026 23:54:13 +0000 Subject: [PATCH 09/18] refactor(entity): rename BuildStatusPassed to BuildStatusSucceeded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename BuildStatusPassed to BuildStatusSucceeded for better semantic clarity. Change the status value from "passed" to "succeeded". Changes: - Rename BuildStatusPassed → BuildStatusSucceeded - Change status value "passed" → "succeeded" - Update IsTerminal() method comment and implementation - Update all test cases from "passed is terminal" → "succeeded is terminal" - Update all documentation references from passed → succeeded - Update mock tests to use BuildStatusSucceeded - Update error comments to reference succeeded instead of passed Build lifecycle now: accepted → succeeded/failed/cancelled The term "succeeded" is more consistent with standard CI/CD terminology and provides clearer semantics for the successful completion state. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 8 ++++---- entity/build_test.go | 4 ++-- extension/build/README.md | 18 +++++++++--------- extension/build/build_manager.go | 2 +- extension/build/errors.go | 2 +- extension/build/mock/build_manager_test.go | 6 +++--- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/entity/build.go b/entity/build.go index 02283432..991e52e9 100644 --- a/entity/build.go +++ b/entity/build.go @@ -10,9 +10,9 @@ const ( // BuildStatusAccepted indicates the build has been accepted by the CI provider. BuildStatusAccepted BuildStatus = "accepted" - // BuildStatusPassed indicates the build completed successfully. + // BuildStatusSucceeded indicates the build completed successfully. // This is a terminal state. - BuildStatusPassed BuildStatus = "passed" + BuildStatusSucceeded BuildStatus = "succeeded" // BuildStatusFailed indicates the build completed with failures. // This is a terminal state. @@ -23,10 +23,10 @@ const ( BuildStatusCancelled BuildStatus = "cancelled" ) -// IsTerminal returns true if the build state represents a final state (passed, failed, or cancelled). +// IsTerminal returns true if the build state represents a final state (succeeded, failed, or cancelled). // Terminal states indicate the build has finished and will not change state again. func (s BuildStatus) IsTerminal() bool { - return s == BuildStatusPassed || s == BuildStatusFailed || s == BuildStatusCancelled + return s == BuildStatusSucceeded || s == BuildStatusFailed || s == BuildStatusCancelled } diff --git a/entity/build_test.go b/entity/build_test.go index b01fd5c0..2e403ecd 100644 --- a/entity/build_test.go +++ b/entity/build_test.go @@ -13,8 +13,8 @@ func TestBuildStatus_IsTerminal(t *testing.T) { expected bool }{ { - name: "passed is terminal", - status: BuildStatusPassed, + name: "succeeded is terminal", + status: BuildStatusSucceeded, expected: true, }, { diff --git a/extension/build/README.md b/extension/build/README.md index f5c27dbb..03fb467d 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -92,18 +92,18 @@ This format allows the implementation to encode both the provider name and provi type BuildStatus string const ( - BuildStatusUnknown BuildStatus = "" // Sentinel value - BuildStatusAccepted BuildStatus = "accepted" // Accepted by CI provider - BuildStatusPassed BuildStatus = "passed" // Completed successfully (terminal) - BuildStatusFailed BuildStatus = "failed" // Completed with failures (terminal) - BuildStatusCancelled BuildStatus = "cancelled" // Cancelled before completion (terminal) + BuildStatusUnknown BuildStatus = "" // Sentinel value + BuildStatusAccepted BuildStatus = "accepted" // Accepted by CI provider + BuildStatusSucceeded BuildStatus = "succeeded" // Completed successfully (terminal) + BuildStatusFailed BuildStatus = "failed" // Completed with failures (terminal) + BuildStatusCancelled BuildStatus = "cancelled" // Cancelled before completion (terminal) ) -// IsTerminal returns true for passed/failed/cancelled states +// IsTerminal returns true for succeeded/failed/cancelled states func (s BuildStatus) IsTerminal() bool ``` -**Build Lifecycle**: `accepted` → `passed`/`failed`/`cancelled` +**Build Lifecycle**: `accepted` → `succeeded`/`failed`/`cancelled` ### Build Metadata @@ -167,7 +167,7 @@ commitSHA := metadata["commit_sha"] // 3. Check if build is done if status.IsTerminal() { - // Build finished: status is passed, failed, or cancelled + // Build finished: status is succeeded, failed, or cancelled } ``` @@ -222,7 +222,7 @@ if status == entity.BuildStatusCancelled { } ``` -**Note**: The implementation decides how to handle cancellation requests for builds in terminal states (passed, failed, cancelled). It may return an error, silently ignore the request, or handle it in a provider-specific way. +**Note**: The implementation decides how to handle cancellation requests for builds in terminal states (succeeded, failed, cancelled). It may return an error, silently ignore the request, or handle it in a provider-specific way. ## Implementing a New Provider diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 611cfae0..3b09129d 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -56,7 +56,7 @@ type BuildManager interface { // BuildStatusCancelled. // // The implementation decides how to handle cancellation requests for builds in - // terminal states (passed, failed, cancelled). It may return an error, silently + // terminal states (succeeded, failed, cancelled). It may return an error, silently // ignore the request, or handle it in a provider-specific way. // // Parameters: diff --git a/extension/build/errors.go b/extension/build/errors.go index 0ef84b85..749aadea 100644 --- a/extension/build/errors.go +++ b/extension/build/errors.go @@ -25,7 +25,7 @@ func WrapBuildNotFound(err error) error { // ErrBuildNotCancellable is returned when attempting to cancel a build that cannot be cancelled. // The specific conditions are implementation-defined and may include: -// - The build has already finished (passed, failed, or cancelled) +// - The build has already finished (succeeded, failed, or cancelled) // - The provider does not support cancellation for this build type var ErrBuildNotCancellable = errors.New("build not cancellable") diff --git a/extension/build/mock/build_manager_test.go b/extension/build/mock/build_manager_test.go index 0e569661..de597b42 100644 --- a/extension/build/mock/build_manager_test.go +++ b/extension/build/mock/build_manager_test.go @@ -45,14 +45,14 @@ func TestMockBuildManager_Compilation(t *testing.T) { } mockBuildMgr.EXPECT(). Poll(gomock.Any(), gomock.Any()). - Return(entity.BuildStatusPassed, expectedMetadata, nil) + Return(entity.BuildStatusSucceeded, expectedMetadata, nil) status, metadata, err := mockBuildMgr.Poll(context.Background(), buildID) if err != nil { t.Fatalf("unexpected error: %v", err) } - if status != entity.BuildStatusPassed { - t.Fatalf("expected %v, got %v", entity.BuildStatusPassed, status) + if status != entity.BuildStatusSucceeded { + t.Fatalf("expected %v, got %v", entity.BuildStatusSucceeded, status) } if metadata["build_url"] != expectedMetadata["build_url"] { t.Fatalf("expected metadata %v, got %v", expectedMetadata, metadata) From fc385f03145492f03980a304d9c1844db756ba3c Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 00:00:37 +0000 Subject: [PATCH 10/18] refactor(extension): rename parameter from id to buildID in Poll and CancelBuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename parameter from generic 'id' to more descriptive 'buildID' in Poll and CancelBuild methods for better clarity and consistency. Changes: - Poll(ctx, id) → Poll(ctx, buildID) - CancelBuild(ctx, id) → CancelBuild(ctx, buildID) - Update parameter documentation from "id:" to "buildID:" - Update README interface signature - Regenerate mocks with updated parameter names The more descriptive parameter name makes the API clearer and more self-documenting. Co-Authored-By: Claude Opus 4.6 --- extension/build/README.md | 4 ++-- extension/build/build_manager.go | 8 ++++---- extension/build/mock/build_manager.go | 16 ++++++++-------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/extension/build/README.md b/extension/build/README.md index 03fb467d..3ec5fab6 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -22,10 +22,10 @@ type BuildManager interface { ) (string, error) // Poll retrieves the current status of a build from the CI provider - Poll(ctx context.Context, id string) (entity.BuildStatus, map[string]string, error) + Poll(ctx context.Context, buildID string) (entity.BuildStatus, map[string]string, error) // CancelBuild requests cancellation of a build (asynchronous operation) - CancelBuild(ctx context.Context, id string) error + CancelBuild(ctx context.Context, buildID string) error // Close gracefully shuts down the build manager Close() error diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 3b09129d..61c1ef2d 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -40,13 +40,13 @@ type BuildManager interface { // This is a synchronous call that queries the provider's API. // // Parameters: - // - id: Build ID string + // - buildID: Build ID string // // Returns: // - BuildStatus: Current state of the build // - map[string]string: Additional metadata about the build (e.g., build URL, commit SHA, duration) // - error: ErrBuildNotFound if the build doesn't exist - Poll(ctx context.Context, id string) (entity.BuildStatus, map[string]string, error) + Poll(ctx context.Context, buildID string) (entity.BuildStatus, map[string]string, error) // CancelBuild requests cancellation of a build. // @@ -60,12 +60,12 @@ type BuildManager interface { // ignore the request, or handle it in a provider-specific way. // // Parameters: - // - id: Build ID string + // - buildID: Build ID string // // Returns: // - error: ErrBuildNotFound if the build doesn't exist, // ErrBuildNotCancellable if the build cannot be cancelled (implementation-specific) - CancelBuild(ctx context.Context, id string) error + CancelBuild(ctx context.Context, buildID string) error // Close gracefully shuts down the build manager. // Implementations should cancel pending requests, close HTTP clients, and clean up resources. diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go index 2af1718a..09ffdbed 100644 --- a/extension/build/mock/build_manager.go +++ b/extension/build/mock/build_manager.go @@ -36,17 +36,17 @@ func (m *MockBuildManager) EXPECT() *MockBuildManagerMockRecorder { } // CancelBuild mocks base method. -func (m *MockBuildManager) CancelBuild(ctx context.Context, id string) error { +func (m *MockBuildManager) CancelBuild(ctx context.Context, buildID string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CancelBuild", ctx, id) + ret := m.ctrl.Call(m, "CancelBuild", ctx, buildID) ret0, _ := ret[0].(error) return ret0 } // CancelBuild indicates an expected call of CancelBuild. -func (mr *MockBuildManagerMockRecorder) CancelBuild(ctx, id interface{}) *gomock.Call { +func (mr *MockBuildManagerMockRecorder) CancelBuild(ctx, buildID interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CancelBuild", reflect.TypeOf((*MockBuildManager)(nil).CancelBuild), ctx, id) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CancelBuild", reflect.TypeOf((*MockBuildManager)(nil).CancelBuild), ctx, buildID) } // Close mocks base method. @@ -64,9 +64,9 @@ func (mr *MockBuildManagerMockRecorder) Close() *gomock.Call { } // Poll mocks base method. -func (m *MockBuildManager) Poll(ctx context.Context, id string) (entity.BuildStatus, map[string]string, error) { +func (m *MockBuildManager) Poll(ctx context.Context, buildID string) (entity.BuildStatus, map[string]string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Poll", ctx, id) + ret := m.ctrl.Call(m, "Poll", ctx, buildID) ret0, _ := ret[0].(entity.BuildStatus) ret1, _ := ret[1].(map[string]string) ret2, _ := ret[2].(error) @@ -74,9 +74,9 @@ func (m *MockBuildManager) Poll(ctx context.Context, id string) (entity.BuildSta } // Poll indicates an expected call of Poll. -func (mr *MockBuildManagerMockRecorder) Poll(ctx, id interface{}) *gomock.Call { +func (mr *MockBuildManagerMockRecorder) Poll(ctx, buildID interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Poll", reflect.TypeOf((*MockBuildManager)(nil).Poll), ctx, id) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Poll", reflect.TypeOf((*MockBuildManager)(nil).Poll), ctx, buildID) } // Schedule mocks base method. From 539423f8b9f7961b2341480add744b26021b83fa Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 00:06:25 +0000 Subject: [PATCH 11/18] refactor(entity): rename BuildAction to ChangeAction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename BuildAction type to ChangeAction for better semantic clarity and consistency with the domain model. Changes: - Rename type: BuildAction → ChangeAction - Rename constants: - BuildActionUnknown → ChangeActionUnknown - BuildActionApply → ChangeActionApply - BuildActionValidate → ChangeActionValidate - Update all references in tests and documentation - Update BuildChange struct to use ChangeAction type - Update all usage examples in README - Update implementation guide references The term "ChangeAction" is more semantically accurate as it describes the action to perform on a change, not on a build. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 18 +++++++++--------- entity/build_test.go | 14 +++++++------- extension/build/README.md | 20 ++++++++++---------- extension/build/mock/build_manager_test.go | 4 ++-- 4 files changed, 28 insertions(+), 28 deletions(-) diff --git a/entity/build.go b/entity/build.go index 991e52e9..ff96ac71 100644 --- a/entity/build.go +++ b/entity/build.go @@ -54,16 +54,16 @@ type Build struct { Status BuildStatus } -// BuildAction defines the action to perform on a change submitted to the build system. -type BuildAction string +// ChangeAction defines the action to perform on a change submitted to the build system. +type ChangeAction string const ( - // BuildActionUnknown is the sentinel value for uninitialized actions. - BuildActionUnknown BuildAction = "" - // BuildActionApply applies the change to the target branch. - BuildActionApply BuildAction = "apply" - // BuildActionValidate runs validation/testing on the change without applying it. - BuildActionValidate BuildAction = "validate" + // ChangeActionUnknown is the sentinel value for uninitialized actions. + ChangeActionUnknown ChangeAction = "" + // ChangeActionApply applies the change to the target branch. + ChangeActionApply ChangeAction = "apply" + // ChangeActionValidate runs validation/testing on the change without applying it. + ChangeActionValidate ChangeAction = "validate" ) // BuildChange represents a code change to be processed by the build system. @@ -74,5 +74,5 @@ type BuildChange struct { // depending on the source control provider. ChangeID string // Action specifies what operation to perform on this change. - Action BuildAction + Action ChangeAction } diff --git a/entity/build_test.go b/entity/build_test.go index 2e403ecd..93c71f0e 100644 --- a/entity/build_test.go +++ b/entity/build_test.go @@ -51,34 +51,34 @@ func TestBuildChange_Creation(t *testing.T) { name string change BuildChange wantID string - wantAction BuildAction + wantAction ChangeAction }{ { name: "apply action", change: BuildChange{ ChangeID: "PR-42", - Action: BuildActionApply, + Action: ChangeActionApply, }, wantID: "PR-42", - wantAction: BuildActionApply, + wantAction: ChangeActionApply, }, { name: "validate action", change: BuildChange{ ChangeID: "D12345", - Action: BuildActionValidate, + Action: ChangeActionValidate, }, wantID: "D12345", - wantAction: BuildActionValidate, + wantAction: ChangeActionValidate, }, { name: "unknown action", change: BuildChange{ ChangeID: "123", - Action: BuildActionUnknown, + Action: ChangeActionUnknown, }, wantID: "123", - wantAction: BuildActionUnknown, + wantAction: ChangeActionUnknown, }, } diff --git a/extension/build/README.md b/extension/build/README.md index 3ec5fab6..5a2fec58 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -47,21 +47,21 @@ type BuildChange struct { // ChangeID is the unique identifier for this change (diff ID, PR number, etc.) ChangeID string // Action specifies what operation to perform on this change - Action BuildAction + Action ChangeAction } ``` -### BuildAction +### ChangeAction Defines the action to perform on a change. ```go -type BuildAction string +type ChangeAction string const ( - BuildActionUnknown BuildAction = "" // Sentinel value - BuildActionApply BuildAction = "apply" // Apply the change to the target branch - BuildActionValidate BuildAction = "validate" // Run validation/testing without applying + ChangeActionUnknown ChangeAction = "" // Sentinel value + ChangeActionApply ChangeAction = "apply" // Apply the change to the target branch + ChangeActionValidate ChangeAction = "validate" // Run validation/testing without applying ) ``` @@ -146,8 +146,8 @@ if build.IsBuildNotFound(err) { ```go // 1. Schedule a build with changes changes := []entity.BuildChange{ - {ChangeID: "D12345", Action: entity.BuildActionApply}, - {ChangeID: "D12346", Action: entity.BuildActionValidate}, + {ChangeID: "D12345", Action: entity.ChangeActionApply}, + {ChangeID: "D12346", Action: entity.ChangeActionValidate}, } buildID, err := buildMgr.Schedule(ctx, "my-queue", changes) @@ -192,7 +192,7 @@ func TestMyController(t *testing.T) { // Set up expectations changes := []entity.BuildChange{ - {ChangeID: "D12345", Action: entity.BuildActionValidate}, + {ChangeID: "D12345", Action: entity.ChangeActionValidate}, } mockBuildMgr.EXPECT(). @@ -264,7 +264,7 @@ To add support for a new CI provider: 5. **Implement Schedule method**: - Look up job name from queue config using `queueName` parameter - - Handle both `BuildActionApply` and `BuildActionValidate` actions + - Handle both `ChangeActionApply` and `ChangeActionValidate` actions - Create appropriate builds/jobs for each change - Return unique build ID diff --git a/extension/build/mock/build_manager_test.go b/extension/build/mock/build_manager_test.go index de597b42..fbd2dcaa 100644 --- a/extension/build/mock/build_manager_test.go +++ b/extension/build/mock/build_manager_test.go @@ -23,8 +23,8 @@ func TestMockBuildManager_Compilation(t *testing.T) { queueName := "test-queue" changes := []entity.BuildChange{ - {ChangeID: "D12345", Action: entity.BuildActionApply}, - {ChangeID: "D12346", Action: entity.BuildActionValidate}, + {ChangeID: "D12345", Action: entity.ChangeActionApply}, + {ChangeID: "D12346", Action: entity.ChangeActionValidate}, } buildID, err := mockBuildMgr.Schedule( From 4998f0b9807631fb5079dbba2609ada363954cdc Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 00:42:43 +0000 Subject: [PATCH 12/18] refactor(entity): introduce BuildMetadata type for Poll return value Replace raw map[string]string with named BuildMetadata type in Poll method. This provides better type safety and allows for future extension with methods. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 4 ++++ extension/build/build_manager.go | 4 ++-- extension/build/mock/build_manager.go | 4 ++-- extension/build/mock/build_manager_test.go | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/entity/build.go b/entity/build.go index ff96ac71..9d659ba0 100644 --- a/entity/build.go +++ b/entity/build.go @@ -76,3 +76,7 @@ type BuildChange struct { // Action specifies what operation to perform on this change. Action ChangeAction } + +// BuildMetadata contains additional metadata about a build returned by the build system. +// The specific keys and values are implementation-defined. +type BuildMetadata map[string]string diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 61c1ef2d..02f37dd4 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -44,9 +44,9 @@ type BuildManager interface { // // Returns: // - BuildStatus: Current state of the build - // - map[string]string: Additional metadata about the build (e.g., build URL, commit SHA, duration) + // - 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, map[string]string, error) + Poll(ctx context.Context, buildID string) (entity.BuildStatus, entity.BuildMetadata, error) // CancelBuild requests cancellation of a build. // diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go index 09ffdbed..dc78da71 100644 --- a/extension/build/mock/build_manager.go +++ b/extension/build/mock/build_manager.go @@ -64,11 +64,11 @@ func (mr *MockBuildManagerMockRecorder) Close() *gomock.Call { } // Poll mocks base method. -func (m *MockBuildManager) Poll(ctx context.Context, buildID string) (entity.BuildStatus, map[string]string, error) { +func (m *MockBuildManager) Poll(ctx context.Context, buildID string) (entity.BuildStatus, entity.BuildMetadata, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "Poll", ctx, buildID) ret0, _ := ret[0].(entity.BuildStatus) - ret1, _ := ret[1].(map[string]string) + ret1, _ := ret[1].(entity.BuildMetadata) ret2, _ := ret[2].(error) return ret0, ret1, ret2 } diff --git a/extension/build/mock/build_manager_test.go b/extension/build/mock/build_manager_test.go index fbd2dcaa..56cdb0bf 100644 --- a/extension/build/mock/build_manager_test.go +++ b/extension/build/mock/build_manager_test.go @@ -39,7 +39,7 @@ func TestMockBuildManager_Compilation(t *testing.T) { } // Test Poll - expectedMetadata := map[string]string{ + expectedMetadata := entity.BuildMetadata{ "build_url": "https://ci.example.com/builds/123", "commit_sha": "abc123", } From 0bf45a8db49d775962369dcef52c457f017ecf2b Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 00:43:33 +0000 Subject: [PATCH 13/18] docs(build): update README to reference BuildMetadata type Update documentation to use entity.BuildMetadata instead of map[string]string. Co-Authored-By: Claude Opus 4.6 --- extension/build/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extension/build/README.md b/extension/build/README.md index 5a2fec58..f5999052 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -22,7 +22,7 @@ type BuildManager interface { ) (string, error) // Poll retrieves the current status of a build from the CI provider - Poll(ctx context.Context, buildID string) (entity.BuildStatus, map[string]string, error) + Poll(ctx context.Context, buildID string) (entity.BuildStatus, entity.BuildMetadata, error) // CancelBuild requests cancellation of a build (asynchronous operation) CancelBuild(ctx context.Context, buildID string) error @@ -107,7 +107,7 @@ func (s BuildStatus) IsTerminal() bool ### Build Metadata -The `Poll` method returns a `map[string]string` containing additional metadata about the build. The specific keys and values are implementation-defined, but common examples include: +The `Poll` method returns `entity.BuildMetadata` containing additional metadata about the build. The specific keys and values are implementation-defined, but common examples include: **Common metadata keys:** - `build_url` - Direct link to the build in the CI provider's UI From 9e1340378af145abd8119f02fa6980be3b82f859 Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 23:01:17 +0000 Subject: [PATCH 14/18] refactor(build): refine BuildManager interface and documentation - Change BuildChange to use Change entity instead of string ChangeID for better type safety - Add context parameter to Close() method for shutdown timeout control - Remove ErrBuildNotCancellable error - CancelBuild now only returns ErrBuildNotFound - Clarify BuildStatusCancelled is set immediately by SubmitQueue without waiting for build system confirmation - Update ChangeActionValidate comment to reflect it applies the change first, then validates - Clarify implementations are long-lived singletons initialized at service startup - Update README with all interface changes and usage examples Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 14 +++--- extension/build/README.md | 64 ++++++++++++++------------- extension/build/build_manager.go | 24 +++++----- extension/build/errors.go | 18 +------- extension/build/mock/build_manager.go | 10 ++--- 5 files changed, 59 insertions(+), 71 deletions(-) diff --git a/entity/build.go b/entity/build.go index 9d659ba0..eb0938d0 100644 --- a/entity/build.go +++ b/entity/build.go @@ -18,8 +18,10 @@ const ( // This is a terminal state. BuildStatusFailed BuildStatus = "failed" - // BuildStatusCancelled indicates the build was cancelled before completion. + // BuildStatusCancelled indicates the build was cancelled by SubmitQueue via CancelBuild. // This is a terminal state. + // Note: If the build system cancels a build for external reasons (e.g., timeout, resource limits), + // this should be reported as BuildStatusFailed, not BuildStatusCancelled. BuildStatusCancelled BuildStatus = "cancelled" ) @@ -62,17 +64,17 @@ const ( ChangeActionUnknown ChangeAction = "" // ChangeActionApply applies the change to the target branch. ChangeActionApply ChangeAction = "apply" - // ChangeActionValidate runs validation/testing on the change without applying it. + // ChangeActionValidate applies the change and runs validation/testing on it. ChangeActionValidate ChangeAction = "validate" ) // BuildChange represents a code change to be processed by the build system. // This is used by BuildManager to specify what changes to build and what action to perform. type BuildChange struct { - // ChangeID is the unique identifier for this change. - // This is typically a diff ID (e.g., "D12345") or PR number (e.g., "42"), - // depending on the source control provider. - ChangeID string + // Change is the code change to process. + // This references the same Change entity used in Request, containing the source provider + // and list of change IDs (e.g., PR numbers, diff IDs). + Change Change // Action specifies what operation to perform on this change. Action ChangeAction } diff --git a/extension/build/README.md b/extension/build/README.md index f5999052..59706a48 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -28,13 +28,13 @@ type BuildManager interface { CancelBuild(ctx context.Context, buildID string) error // Close gracefully shuts down the build manager - Close() error + Close(ctx context.Context) error } ``` **Thread-safety**: All implementations must be thread-safe and support concurrent operations. -**Implementation Design**: Implementations may be designed as heavy singletons (with connection pooling and caching) or lightweight instances created on-demand, depending on the CI provider's requirements and implementation strategy. +**Implementation 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. ## Types @@ -44,8 +44,10 @@ Represents a code change to be processed by the build system. ```go type BuildChange struct { - // ChangeID is the unique identifier for this change (diff ID, PR number, etc.) - ChangeID string + // Change is the code change to process. + // This references the same Change entity used in Request, containing the source provider + // and list of change IDs (e.g., PR numbers, diff IDs). + Change Change // Action specifies what operation to perform on this change Action ChangeAction } @@ -61,7 +63,7 @@ type ChangeAction string const ( ChangeActionUnknown ChangeAction = "" // Sentinel value ChangeActionApply ChangeAction = "apply" // Apply the change to the target branch - ChangeActionValidate ChangeAction = "validate" // Run validation/testing without applying + ChangeActionValidate ChangeAction = "validate" // Apply the change and run validation/testing on it ) ``` @@ -70,7 +72,7 @@ const ( **queueName** (string, required): Name of the queue processing these changes. Used to look up job configuration from queue config. **changes** ([]entity.BuildChange, required): List of changes to process. Each change includes: -- **ChangeID**: Unique identifier (e.g., "D12345" for Phabricator, "42" for GitHub PR) +- **Change**: The code change entity containing source provider and list of change IDs (e.g., "D12345" for Phabricator, "42" for GitHub PR) - **Action**: What to do with the change (validate or apply) Order of changes may be significant for dependencies. @@ -96,7 +98,8 @@ const ( BuildStatusAccepted BuildStatus = "accepted" // Accepted by CI provider BuildStatusSucceeded BuildStatus = "succeeded" // Completed successfully (terminal) BuildStatusFailed BuildStatus = "failed" // Completed with failures (terminal) - BuildStatusCancelled BuildStatus = "cancelled" // Cancelled before completion (terminal) + BuildStatusCancelled BuildStatus = "cancelled" // Cancelled by SubmitQueue via CancelBuild (terminal) + // Note: Build system cancellations should be reported as BuildStatusFailed ) // IsTerminal returns true for succeeded/failed/cancelled states @@ -124,7 +127,6 @@ Implementations may include additional provider-specific metadata. Consumers sho The extension defines sentinel errors following the SubmitQueue pattern: - **`ErrBuildNotFound`** - Build doesn't exist or was deleted -- **`ErrBuildNotCancellable`** - Build cannot be cancelled (implementation-specific) - **`ErrInvalidRequest`** - Request validation failed Each error has helper functions: @@ -146,8 +148,14 @@ if build.IsBuildNotFound(err) { ```go // 1. Schedule a build with changes changes := []entity.BuildChange{ - {ChangeID: "D12345", Action: entity.ChangeActionApply}, - {ChangeID: "D12346", Action: entity.ChangeActionValidate}, + { + Change: entity.Change{Source: "github", IDs: []string{"123"}}, + Action: entity.ChangeActionApply, + }, + { + Change: entity.Change{Source: "github", IDs: []string{"124"}}, + Action: entity.ChangeActionValidate, + }, } buildID, err := buildMgr.Schedule(ctx, "my-queue", changes) @@ -192,7 +200,10 @@ func TestMyController(t *testing.T) { // Set up expectations changes := []entity.BuildChange{ - {ChangeID: "D12345", Action: entity.ChangeActionValidate}, + { + Change: entity.Change{Source: "github", IDs: []string{"123"}}, + Action: entity.ChangeActionValidate, + }, } mockBuildMgr.EXPECT(). @@ -205,25 +216,22 @@ func TestMyController(t *testing.T) { ### Cancelling Builds -Cancellation is **asynchronous** - the method initiates the cancellation request and returns immediately. Use `Poll` to check if the build has transitioned to `BuildStatusCancelled`. +Cancellation is **asynchronous** - the method initiates the cancellation request and returns immediately. + +**Important**: SubmitQueue sets `BuildStatusCancelled` immediately upon calling `CancelBuild`, without waiting for confirmation from the build system. The build status reflects SubmitQueue's decision to cancel, not the build system's actual state. ```go err := mgr.CancelBuild(ctx, buildID) -if build.IsBuildNotCancellable(err) { - // Build cannot be cancelled (implementation-specific) +if build.IsBuildNotFound(err) { + // Build doesn't exist } else if err != nil { // Other error } -// Poll to verify cancellation -status, _ := mgr.Poll(ctx, buildID) -if status == entity.BuildStatusCancelled { - // Cancellation successful -} +// SubmitQueue will have already marked the build as BuildStatusCancelled +// No need to poll for confirmation ``` -**Note**: The implementation decides how to handle cancellation requests for builds in terminal states (succeeded, failed, cancelled). It may return an error, silently ignore the request, or handle it in a provider-specific way. - ## Implementing a New Provider To add support for a new CI provider: @@ -271,20 +279,15 @@ To add support for a new CI provider: 6. **Implement CancelBuild**: - Make asynchronous cancellation request to CI provider - Return immediately after initiating request - - Decide how to handle terminal state cancellations - -7. **Choose implementation design**: - - **Singleton**: If provider needs connection pooling, auth token caching, rate limiting - - **Lightweight**: If provider SDK handles resource management or implementation is stateless -8. **Add tests**: +7. **Add tests**: - Unit tests with mock HTTP server - Validation tests for all required fields - Error mapping tests - Thread-safety tests - Test both validate and apply actions -9. **Update BUILD.bazel**: +8. **Update BUILD.bazel**: ```bazel go_library( name = "jenkins", @@ -326,10 +329,10 @@ extension/build/ ### Design Principles 1. **Vendor-agnostic**: Interface doesn't leak provider-specific details -2. **Change-focused**: Operates on individual changes (diff IDs, PR numbers) with explicit actions +2. **Change-focused**: Operates on individual changes with explicit actions 3. **Thread-safe**: All implementations support concurrent operations 4. **Error transparency**: Sentinel errors for common failure modes -5. **Implementation flexibility**: Supports both heavy singleton and lightweight on-demand designs +5. **Long-lived singletons**: Implementations are initialized once per build provider at service startup 6. **Asynchronous cancellation**: CancelBuild initiates request and returns immediately ## Future Enhancements @@ -342,4 +345,3 @@ Potential improvements not in the current implementation: - **Batch operations**: Poll/cancel multiple builds at once - **Streaming logs**: Real-time log streaming from builds - **Build caching**: Cache build status to reduce API calls -- **Partial cancellation**: Cancel individual changes in a multi-change build diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 02f37dd4..ab8342c0 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -12,9 +12,9 @@ import ( // Implementations provide integration with specific CI systems (BuildKite, Jenkins, etc.) // to schedule builds, poll their status, and cancel running builds. // -// Implementations may be designed as heavy singletons (with connection pooling and caching) -// or lightweight instances created on-demand, depending on the CI provider's requirements -// and implementation strategy. +// 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. // // All implementations must be thread-safe and support concurrent operations. type BuildManager interface { @@ -52,24 +52,24 @@ type BuildManager interface { // // This operation is asynchronous and does not wait for the cancellation to complete. // The implementation should initiate the cancellation request with the CI provider - // and return immediately. Use Poll to check if the build has transitioned to - // BuildStatusCancelled. + // and return immediately. // - // The implementation decides how to handle cancellation requests for builds in - // terminal states (succeeded, failed, cancelled). It may return an error, silently - // ignore the request, or handle it in a provider-specific way. + // SubmitQueue will mark the build as BuildStatusCancelled immediately without waiting + // for confirmation from the build system. // // Parameters: // - buildID: Build ID string // // Returns: - // - error: ErrBuildNotFound if the build doesn't exist, - // ErrBuildNotCancellable if the build cannot be cancelled (implementation-specific) + // - error: ErrBuildNotFound if the build doesn't exist CancelBuild(ctx context.Context, buildID string) error // Close gracefully shuts down the build manager. - // Implementations should cancel pending requests, close HTTP clients, and clean up resources. + // Implementations should close HTTP clients and clean up resources. // After Close is called, all other methods should return errors. // Close is idempotent and safe to call multiple times. - Close() error + // + // The context controls the shutdown timeout. If the context is cancelled before + // cleanup completes, Close should stop cleanup operations and return the context error. + Close(ctx context.Context) error } diff --git a/extension/build/errors.go b/extension/build/errors.go index 749aadea..00727e37 100644 --- a/extension/build/errors.go +++ b/extension/build/errors.go @@ -23,27 +23,11 @@ func WrapBuildNotFound(err error) error { return fmt.Errorf("%w: %w", ErrBuildNotFound, err) } -// ErrBuildNotCancellable is returned when attempting to cancel a build that cannot be cancelled. -// The specific conditions are implementation-defined and may include: -// - The build has already finished (succeeded, failed, or cancelled) -// - The provider does not support cancellation for this build type -var ErrBuildNotCancellable = errors.New("build not cancellable") - -// IsBuildNotCancellable returns true if any error in the error chain is ErrBuildNotCancellable. -func IsBuildNotCancellable(err error) bool { - return errors.Is(err, ErrBuildNotCancellable) -} - -// WrapBuildNotCancellable wraps ErrBuildNotCancellable with the original error from the build provider. -func WrapBuildNotCancellable(err error) error { - return fmt.Errorf("%w: %w", ErrBuildNotCancellable, err) -} - // ErrInvalidRequest is returned when Schedule parameters fail validation. // This can occur when: // - queueName is empty or invalid // - changes list is empty -// - changes contain invalid ChangeIDs or Actions +// - changes contain invalid Change entities or Actions var ErrInvalidRequest = errors.New("invalid request") // IsInvalidRequest returns true if any error in the error chain is ErrInvalidRequest. diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go index dc78da71..99786fe2 100644 --- a/extension/build/mock/build_manager.go +++ b/extension/build/mock/build_manager.go @@ -8,7 +8,7 @@ import ( context "context" reflect "reflect" - gomock "go.uber.org/mock/gomock" + gomock "github.com/golang/mock/gomock" entity "github.com/uber/submitqueue/entity" ) @@ -50,17 +50,17 @@ func (mr *MockBuildManagerMockRecorder) CancelBuild(ctx, buildID interface{}) *g } // Close mocks base method. -func (m *MockBuildManager) Close() error { +func (m *MockBuildManager) Close(ctx context.Context) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Close") + ret := m.ctrl.Call(m, "Close", ctx) ret0, _ := ret[0].(error) return ret0 } // Close indicates an expected call of Close. -func (mr *MockBuildManagerMockRecorder) Close() *gomock.Call { +func (mr *MockBuildManagerMockRecorder) Close(ctx interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockBuildManager)(nil).Close)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockBuildManager)(nil).Close), ctx) } // Poll mocks base method. From 58cebbeb6a508a73e33803d866dade9179851987 Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 23:08:26 +0000 Subject: [PATCH 15/18] docs(build): improve ChangeActionValidate comment clarity Update ChangeActionValidate comment to explicitly describe the sequence: applies the change first, then validates by running respective validation/test suites. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 2 +- extension/build/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/entity/build.go b/entity/build.go index eb0938d0..af115d5d 100644 --- a/entity/build.go +++ b/entity/build.go @@ -64,7 +64,7 @@ const ( ChangeActionUnknown ChangeAction = "" // ChangeActionApply applies the change to the target branch. ChangeActionApply ChangeAction = "apply" - // ChangeActionValidate applies the change and runs validation/testing on it. + // ChangeActionValidate applies the change first, and then validates the change by running respective validation/test suites. ChangeActionValidate ChangeAction = "validate" ) diff --git a/extension/build/README.md b/extension/build/README.md index 59706a48..fac290ba 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -63,7 +63,7 @@ type ChangeAction string const ( ChangeActionUnknown ChangeAction = "" // Sentinel value ChangeActionApply ChangeAction = "apply" // Apply the change to the target branch - ChangeActionValidate ChangeAction = "validate" // Apply the change and run validation/testing on it + ChangeActionValidate ChangeAction = "validate" // Applies the change first, and then validates the change by running respective validation/test suites ) ``` From 9b862dc74c21d4ed57163c57ef65da0c89b7f15a Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 23:24:29 +0000 Subject: [PATCH 16/18] docs(build): simplify BuildStatusCancelled comment Remove redundant "via CancelBuild" from comment - "cancelled by SubmitQueue" is sufficient. Co-Authored-By: Claude Opus 4.6 --- entity/build.go | 2 +- extension/build/README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/entity/build.go b/entity/build.go index af115d5d..039b182a 100644 --- a/entity/build.go +++ b/entity/build.go @@ -18,7 +18,7 @@ const ( // This is a terminal state. BuildStatusFailed BuildStatus = "failed" - // BuildStatusCancelled indicates the build was cancelled by SubmitQueue via CancelBuild. + // BuildStatusCancelled indicates the build was cancelled by SubmitQueue. // This is a terminal state. // Note: If the build system cancels a build for external reasons (e.g., timeout, resource limits), // this should be reported as BuildStatusFailed, not BuildStatusCancelled. diff --git a/extension/build/README.md b/extension/build/README.md index fac290ba..f92912e1 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -98,7 +98,7 @@ const ( BuildStatusAccepted BuildStatus = "accepted" // Accepted by CI provider BuildStatusSucceeded BuildStatus = "succeeded" // Completed successfully (terminal) BuildStatusFailed BuildStatus = "failed" // Completed with failures (terminal) - BuildStatusCancelled BuildStatus = "cancelled" // Cancelled by SubmitQueue via CancelBuild (terminal) + BuildStatusCancelled BuildStatus = "cancelled" // Cancelled by SubmitQueue (terminal) // Note: Build system cancellations should be reported as BuildStatusFailed ) From 34d58d9c4b55d83b3768b82f946ef9791d4d3c3c Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 23:28:54 +0000 Subject: [PATCH 17/18] docs(build): remove context timeout explanation from Close method Remove the explanatory comment about context controlling shutdown timeout from the Close method documentation. Co-Authored-By: Claude Opus 4.6 --- extension/build/build_manager.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index ab8342c0..99bf3794 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -68,8 +68,5 @@ type BuildManager interface { // Implementations should close HTTP clients and clean up resources. // After Close is called, all other methods should return errors. // Close is idempotent and safe to call multiple times. - // - // The context controls the shutdown timeout. If the context is cancelled before - // cleanup completes, Close should stop cleanup operations and return the context error. Close(ctx context.Context) error } From 1f9d780ce302fa1255cb9571b567d9424486ec9f Mon Sep 17 00:00:00 2001 From: djuloori Date: Thu, 26 Feb 2026 23:50:56 +0000 Subject: [PATCH 18/18] refactor(build): remove context parameter from Close method Remove context parameter from Close() to be consistent with other extensions (Storage, Queue, Publisher, Subscriber) which all use the standard io.Closer pattern: Close() error Co-Authored-By: Claude Opus 4.6 --- extension/build/README.md | 2 +- extension/build/build_manager.go | 2 +- extension/build/mock/build_manager.go | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/extension/build/README.md b/extension/build/README.md index f92912e1..8ce239be 100644 --- a/extension/build/README.md +++ b/extension/build/README.md @@ -28,7 +28,7 @@ type BuildManager interface { CancelBuild(ctx context.Context, buildID string) error // Close gracefully shuts down the build manager - Close(ctx context.Context) error + Close() error } ``` diff --git a/extension/build/build_manager.go b/extension/build/build_manager.go index 99bf3794..83bac312 100644 --- a/extension/build/build_manager.go +++ b/extension/build/build_manager.go @@ -68,5 +68,5 @@ type BuildManager interface { // Implementations should close HTTP clients and clean up resources. // After Close is called, all other methods should return errors. // Close is idempotent and safe to call multiple times. - Close(ctx context.Context) error + Close() error } diff --git a/extension/build/mock/build_manager.go b/extension/build/mock/build_manager.go index 99786fe2..2f46269a 100644 --- a/extension/build/mock/build_manager.go +++ b/extension/build/mock/build_manager.go @@ -50,17 +50,17 @@ func (mr *MockBuildManagerMockRecorder) CancelBuild(ctx, buildID interface{}) *g } // Close mocks base method. -func (m *MockBuildManager) Close(ctx context.Context) error { +func (m *MockBuildManager) Close() error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Close", ctx) + ret := m.ctrl.Call(m, "Close") ret0, _ := ret[0].(error) return ret0 } // Close indicates an expected call of Close. -func (mr *MockBuildManagerMockRecorder) Close(ctx interface{}) *gomock.Call { +func (mr *MockBuildManagerMockRecorder) Close() *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockBuildManager)(nil).Close), ctx) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Close", reflect.TypeOf((*MockBuildManager)(nil).Close)) } // Poll mocks base method.