diff --git a/CLAUDE.md b/CLAUDE.md index 02ffa738..67f72b5c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -116,6 +116,8 @@ Vendor-agnostic, pluggable interfaces with implementations in subdirectories: 2. Implementations at `extension/{ext}/{impl}/` 3. Factory interface for dependency injection and lifecycle management +**Extensions hold contracts and impls only — never factory implementations or routing.** An `extension/{ext}` package owns the behavioral interface, its `Config`, the `Factory` *interface*, and impl constructors `New(...)` that return the interface. It must NOT contain `Factory` *implementations* (no `NewFactory()` constructors or factory structs) and no queue-selection/routing logic. The reason: an impl package (e.g. `scorer/heuristic`) cannot know the queue topology or the other impls, so any "which impl for which queue" decision baked there is wrong by construction. A `Factory` whose `For(cfg)` ignores `cfg.QueueName` is just an impl→Factory adapter — that adapting, and all per-queue routing, belongs in the **wiring/integrator layer** (e.g. `example/{domain}/{service}/server/main.go`), which is the one place that knows the queue set. There, adapt `New(...)` impls to the `Factory` contract and route on `Config.QueueName`. Keep this seam strict: if you're about to add a `NewFactory()` or a `map[queue]impl` under `extension/`, it belongs in the wiring layer instead. + **Design interfaces for the technology *space*, not the implementation in front of you.** The interface is a contract every backend will have to satisfy — SQL, key-value (DynamoDB, Bigtable), document, message queue, search, RPC, in-memory, mocks. If the contract assumes a capability that some plausible backend can't provide cheaply, you've baked the current impl's strengths into the API. Common over-constraints to avoid: diff --git a/example/submitqueue/gateway/server/queues.yaml b/example/submitqueue/gateway/server/queues.yaml index 5caa3219..df7571fe 100644 --- a/example/submitqueue/gateway/server/queues.yaml +++ b/example/submitqueue/gateway/server/queues.yaml @@ -7,3 +7,7 @@ queues: - name: test-queue - name: e2e-test-queue - name: e2e-cancel-queue + # Routes to an analyzer that always errors (conflictfake.FailAlways) so e2e can + # exercise the conflict-analysis error path. See newQueueRegistry in the + # orchestrator example server. + - name: e2e-conflict-error-queue diff --git a/example/submitqueue/orchestrator/server/BUILD.bazel b/example/submitqueue/orchestrator/server/BUILD.bazel index 5190e02e..2c19880b 100644 --- a/example/submitqueue/orchestrator/server/BUILD.bazel +++ b/example/submitqueue/orchestrator/server/BUILD.bazel @@ -21,16 +21,23 @@ go_library( "//submitqueue/core/consumer", "//submitqueue/entity", "//submitqueue/extension/buildrunner", - "//submitqueue/extension/buildrunner/noop", + "//submitqueue/extension/buildrunner/fake", "//submitqueue/extension/changeprovider", + "//submitqueue/extension/changeprovider/fake", "//submitqueue/extension/changeprovider/github", "//submitqueue/extension/conflict", "//submitqueue/extension/conflict/all", + "//submitqueue/extension/conflict/fake", + "//submitqueue/extension/conflict/none", "//submitqueue/extension/mergechecker", + "//submitqueue/extension/mergechecker/fake", "//submitqueue/extension/mergechecker/github", "//submitqueue/extension/pusher", + "//submitqueue/extension/pusher/fake", "//submitqueue/extension/pusher/git", "//submitqueue/extension/scorer", + "//submitqueue/extension/scorer/composite", + "//submitqueue/extension/scorer/fake", "//submitqueue/extension/scorer/heuristic", "//submitqueue/extension/storage", "//submitqueue/extension/storage/mysql", diff --git a/example/submitqueue/orchestrator/server/main.go b/example/submitqueue/orchestrator/server/main.go index 5595cc89..a4dbf09d 100644 --- a/example/submitqueue/orchestrator/server/main.go +++ b/example/submitqueue/orchestrator/server/main.go @@ -40,16 +40,23 @@ import ( "github.com/uber/submitqueue/submitqueue/core/consumer" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/buildrunner" - buildnoop "github.com/uber/submitqueue/submitqueue/extension/buildrunner/noop" + buildfake "github.com/uber/submitqueue/submitqueue/extension/buildrunner/fake" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" + cpfake "github.com/uber/submitqueue/submitqueue/extension/changeprovider/fake" githubprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/github" "github.com/uber/submitqueue/submitqueue/extension/conflict" "github.com/uber/submitqueue/submitqueue/extension/conflict/all" + conflictfake "github.com/uber/submitqueue/submitqueue/extension/conflict/fake" + "github.com/uber/submitqueue/submitqueue/extension/conflict/none" "github.com/uber/submitqueue/submitqueue/extension/mergechecker" + mcfake "github.com/uber/submitqueue/submitqueue/extension/mergechecker/fake" githubchecker "github.com/uber/submitqueue/submitqueue/extension/mergechecker/github" "github.com/uber/submitqueue/submitqueue/extension/pusher" + pushfake "github.com/uber/submitqueue/submitqueue/extension/pusher/fake" gitpusher "github.com/uber/submitqueue/submitqueue/extension/pusher/git" "github.com/uber/submitqueue/submitqueue/extension/scorer" + "github.com/uber/submitqueue/submitqueue/extension/scorer/composite" + scorerfake "github.com/uber/submitqueue/submitqueue/extension/scorer/fake" "github.com/uber/submitqueue/submitqueue/extension/scorer/heuristic" "github.com/uber/submitqueue/submitqueue/extension/storage" mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql" @@ -207,30 +214,26 @@ func run() error { mysqlerrs.Classifier, ) - // Create merge checker - mc, err := newMergeChecker(logger, scope) - if err != nil { - return fmt.Errorf("failed to create merge checker: %w", err) - } - - // Create change provider - cp, err := newChangeProvider(logger, scope) + // Build the per-queue extension registry: each queue resolves to its own + // set of extension implementations (scorer, conflict analyzer, …), falling + // back to a baseline profile for queues without an explicit entry. This is + // the single place queue topology is known; the extension packages stay + // queue-agnostic. + queues, err := newQueueRegistry(logger, scope) if err != nil { - return fmt.Errorf("failed to create change provider: %w", err) + return fmt.Errorf("failed to build queue registry: %w", err) } - // Create pusher - psh, err := newPusher(logger, scope) - if err != nil { - return fmt.Errorf("failed to create pusher: %w", err) - } - - // Create build runner. The noop runner is the pass-through default - // (every build immediately succeeds) until a real backend is wired in. - br := buildnoop.New() + // Per-extension factories all resolve against the registry by queue name. + mcf := mergeCheckerFactory{queues} + cpf := changeProviderFactory{queues} + pshf := pusherFactory{queues} + brf := buildRunnerFactory{queues} + scf := scorerFactory{queues} + cof := analyzerFactory{queues} // Register controllers - if err := registerControllers(c, logger.Sugar(), scope, registry, mc, cp, psh, br, cnt, store); err != nil { + if err := registerControllers(c, logger.Sugar(), scope, registry, mcf, cpf, pshf, brf, scf, cof, cnt, store); err != nil { return err } @@ -423,40 +426,87 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe // │ │ │ // └────────┴───────────────────────┘ -// Static per-extension factories for the example server: every queue resolves -// to the single configured implementation. A real deployment would vary the -// returned implementation by cfg.QueueName (and inject per-queue config). -type changeProviderFactory struct{ impl changeprovider.ChangeProvider } +// TODO(wiring abstraction): queueExtensions + queueRegistry currently live here +// as example-local wiring. Evaluate promoting them into a defined abstraction in +// the submitqueue domain layer (e.g. submitqueue/core/...) — NOT extension/* and +// NOT cross-domain core/, since the bundle names submitqueue-specific extensions. +// Do this only when a trigger lands: (1) a second consumer needs the same wiring +// (a real prod server, or an e2e harness building real per-queue profiles); +// (2) per-queue config becomes data-driven (build profiles from queueconfig.Store +// /queues.yaml instead of Go literals); or (3) the bundle grows lifecycle +// (Close/health/hot-reload). Until then, keep it local — extracting now adds +// indirection for one hardcoded consumer. See also queueconfig.Store, which holds +// the per-queue *data* half; a promoted Registry would build impl bundles from it. +// +// queueExtensions is the full set of extension implementations for a single +// queue. Grouping them per queue (rather than per extension) lets the wiring +// read as "for this queue, here are its scorer, analyzer, pusher, …", and lets +// a queue profile start from a baseline and override only what differs. +type queueExtensions struct { + mergeChecker mergechecker.MergeChecker + changeProvider changeprovider.ChangeProvider + pusher pusher.Pusher + buildRunner buildrunner.BuildRunner + scorer scorer.Scorer + analyzer conflict.Analyzer +} + +// queueRegistry maps a queue name to its extensions, falling back to a default +// profile for queues without an explicit entry. It is the single place that +// knows the queue topology; the extension packages remain queue-agnostic. +type queueRegistry struct { + byQueue map[string]queueExtensions + def queueExtensions +} + +// get returns the extensions for the named queue, or the default profile. +func (r queueRegistry) get(queue string) queueExtensions { + if e, ok := r.byQueue[queue]; ok { + return e + } + return r.def +} + +// The per-extension factories below are thin adapters: each satisfies its +// extension's Factory contract by resolving the queue's profile from the +// registry. All routing logic lives here in the wiring layer. +type mergeCheckerFactory struct{ reg queueRegistry } -func (f changeProviderFactory) For(changeprovider.Config) (changeprovider.ChangeProvider, error) { - return f.impl, nil +func (f mergeCheckerFactory) For(cfg mergechecker.Config) (mergechecker.MergeChecker, error) { + return f.reg.get(cfg.QueueName).mergeChecker, nil } -type mergeCheckerFactory struct{ impl mergechecker.MergeChecker } +type changeProviderFactory struct{ reg queueRegistry } -func (f mergeCheckerFactory) For(mergechecker.Config) (mergechecker.MergeChecker, error) { - return f.impl, nil +func (f changeProviderFactory) For(cfg changeprovider.Config) (changeprovider.ChangeProvider, error) { + return f.reg.get(cfg.QueueName).changeProvider, nil } -type pusherFactory struct{ impl pusher.Pusher } +type pusherFactory struct{ reg queueRegistry } -func (f pusherFactory) For(pusher.Config) (pusher.Pusher, error) { return f.impl, nil } +func (f pusherFactory) For(cfg pusher.Config) (pusher.Pusher, error) { + return f.reg.get(cfg.QueueName).pusher, nil +} -type buildRunnerFactory struct{ impl buildrunner.BuildRunner } +type buildRunnerFactory struct{ reg queueRegistry } -func (f buildRunnerFactory) For(buildrunner.Config) (buildrunner.BuildRunner, error) { - return f.impl, nil +func (f buildRunnerFactory) For(cfg buildrunner.Config) (buildrunner.BuildRunner, error) { + return f.reg.get(cfg.QueueName).buildRunner, nil } -type scorerFactory struct{ impl scorer.Scorer } +type scorerFactory struct{ reg queueRegistry } -func (f scorerFactory) For(scorer.Config) (scorer.Scorer, error) { return f.impl, nil } +func (f scorerFactory) For(cfg scorer.Config) (scorer.Scorer, error) { + return f.reg.get(cfg.QueueName).scorer, nil +} -type conflictFactory struct{ impl conflict.Analyzer } +type analyzerFactory struct{ reg queueRegistry } -func (f conflictFactory) For(conflict.Config) (conflict.Analyzer, error) { return f.impl, nil } +func (f analyzerFactory) For(cfg conflict.Config) (conflict.Analyzer, error) { + return f.reg.get(cfg.QueueName).analyzer, nil +} -func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope tally.Scope, registry consumer.TopicRegistry, mc mergechecker.MergeChecker, cp changeprovider.ChangeProvider, psh pusher.Pusher, br buildrunner.BuildRunner, cnt counter.Counter, store storage.Storage) error { +func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope tally.Scope, registry consumer.TopicRegistry, mcf mergechecker.Factory, cpf changeprovider.Factory, pshf pusher.Factory, brf buildrunner.Factory, scf scorer.Factory, cof conflict.Factory, cnt counter.Counter, store storage.Storage) error { requestController := start.NewController( logger, scope, @@ -486,8 +536,8 @@ func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope t scope, store, registry, - mergeCheckerFactory{impl: mc}, - changeProviderFactory{impl: cp}, + mcf, + cpf, consumer.TopicKeyValidate, "orchestrator-validate", ) @@ -501,9 +551,7 @@ func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope t registry, cnt, store, - // TODO: replace with a real conflict analyzer (e.g. one backed by - // Tango target analysis). The "all" stub serializes the queue. - conflictFactory{impl: all.New()}, + cof, consumer.TopicKeyBatch, "orchestrator-batch", ) @@ -515,20 +563,7 @@ func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope t logger, scope, store, - // Heuristic scorer: bucket the batch by total lines changed across all of - // its changes — larger batches are likelier to fail to land. - scorerFactory{impl: heuristic.New( - []heuristic.Bucket{ - {Min: 0, Max: 50, Score: 0.95}, - {Min: 51, Max: 250, Score: 0.80}, - {Min: 251, Max: 1000, Score: 0.60}, - {Min: 1001, Max: 1<<31 - 1, Score: 0.40}, - }, - func(_ context.Context, changes entity.BatchChanges) (int, error) { - return changes.TotalLinesChanged(), nil - }, - scope.SubScope("scorer"), - )}, + scf, registry, consumer.TopicKeyScore, "orchestrator-score", @@ -553,7 +588,7 @@ func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope t logger, scope, store, - buildRunnerFactory{impl: br}, + brf, registry, consumer.TopicKeyBuild, "orchestrator-build", @@ -566,7 +601,7 @@ func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope t logger, scope, store, - buildRunnerFactory{impl: br}, + brf, registry, consumer.TopicKeyBuildSignal, "orchestrator-buildsignal", @@ -580,7 +615,7 @@ func registerControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope t scope, store, registry, - pusherFactory{impl: psh}, + pshf, consumer.TopicKeyMerge, "orchestrator-merge", ) @@ -634,18 +669,24 @@ func parseTimeout(envVal string, defaultVal time.Duration) time.Duration { return defaultVal } -// newMergeChecker creates a MergeChecker for GitHub (github.com). -// Configured via GITHUB_BASE_URL, GITHUB_TOKEN, and GITHUB_TIMEOUT environment variables. +// newMergeChecker creates a MergeChecker for GitHub (github.com), configured via +// GITHUB_BASE_URL, GITHUB_TOKEN, and GITHUB_TIMEOUT. When GITHUB_TOKEN is unset +// it returns the fake merge checker (every change mergeable unless a URI carries +// a failure marker, see mergechecker/fake), keeping the example runnable without +// GitHub and letting e2e tests drive unmergeable scenarios via request payloads. func newMergeChecker(logger *zap.Logger, scope tally.Scope) (mergechecker.MergeChecker, error) { + if os.Getenv("GITHUB_TOKEN") == "" { + logger.Warn("GITHUB_TOKEN not set; using fake merge checker (every change mergeable unless URI-marked)") + return mcfake.New(), nil + } + client, err := httpclient.NewClient(getEnv("GITHUB_BASE_URL", "https://api.github.com")) if err != nil { return nil, fmt.Errorf("failed to build GitHub HTTP client: %w", err) } - if token := os.Getenv("GITHUB_TOKEN"); token != "" { - ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token}) - client.Transport = &oauth2.Transport{Source: ts, Base: client.Transport} - } + ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: os.Getenv("GITHUB_TOKEN")}) + client.Transport = &oauth2.Transport{Source: ts, Base: client.Transport} client.Timeout = parseTimeout(os.Getenv("GITHUB_TIMEOUT"), 30*time.Second) @@ -656,18 +697,23 @@ func newMergeChecker(logger *zap.Logger, scope tally.Scope) (mergechecker.MergeC }), nil } -// newChangeProvider creates a ChangeProvider for GitHub (github.com). -// Configured via GITHUB_BASE_URL, GITHUB_TOKEN, and GITHUB_TIMEOUT environment variables. +// newChangeProvider creates a ChangeProvider for GitHub (github.com), configured +// via GITHUB_BASE_URL, GITHUB_TOKEN, and GITHUB_TIMEOUT. When GITHUB_TOKEN is +// unset it returns the fake change provider (one empty ChangeInfo per URI unless +// a URI carries a failure marker, see changeprovider/fake). func newChangeProvider(logger *zap.Logger, scope tally.Scope) (changeprovider.ChangeProvider, error) { + if os.Getenv("GITHUB_TOKEN") == "" { + logger.Warn("GITHUB_TOKEN not set; using fake change provider (empty change info unless URI-marked)") + return cpfake.New(), nil + } + client, err := httpclient.NewClient(getEnv("GITHUB_BASE_URL", "https://api.github.com")) if err != nil { return nil, fmt.Errorf("failed to build GitHub HTTP client: %w", err) } - if token := os.Getenv("GITHUB_TOKEN"); token != "" { - ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token}) - client.Transport = &oauth2.Transport{Source: ts, Base: client.Transport} - } + ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: os.Getenv("GITHUB_TOKEN")}) + client.Transport = &oauth2.Transport{Source: ts, Base: client.Transport} client.Timeout = parseTimeout(os.Getenv("GITHUB_TIMEOUT"), 30*time.Second) @@ -678,19 +724,16 @@ func newChangeProvider(logger *zap.Logger, scope tally.Scope) (changeprovider.Ch }), nil } -// newPusher creates a git-backed Pusher bound to the configured checkout -// path, remote, and target branch. Configured via PUSHER_CHECKOUT_PATH, -// PUSHER_REMOTE (default "origin"), and PUSHER_TARGET (default "main"). -// -// If PUSHER_CHECKOUT_PATH is not set the orchestrator falls back to a -// no-op pusher that errors when invoked. This keeps the example server -// runnable in environments that don't exercise the merge controller -// (e.g. ping-only integration tests, local dev without a git checkout). +// newPusher creates a git-backed Pusher bound to the configured checkout path, +// remote, and target branch (PUSHER_CHECKOUT_PATH, PUSHER_REMOTE default +// "origin", PUSHER_TARGET default "main"). When PUSHER_CHECKOUT_PATH is unset it +// returns the fake pusher (commits succeed unless a change URI carries a failure +// marker, see pusher/fake), keeping the example runnable without a git checkout. func newPusher(logger *zap.Logger, scope tally.Scope) (pusher.Pusher, error) { checkout := os.Getenv("PUSHER_CHECKOUT_PATH") if checkout == "" { - logger.Warn("PUSHER_CHECKOUT_PATH not set; using no-op pusher (merge controller will fail if invoked)") - return noopPusher{}, nil + logger.Warn("PUSHER_CHECKOUT_PATH not set; using fake pusher (commits succeed unless URI-marked)") + return pushfake.New(), nil } return gitpusher.NewPusher(gitpusher.Params{ CheckoutPath: checkout, @@ -701,14 +744,94 @@ func newPusher(logger *zap.Logger, scope tally.Scope) (pusher.Pusher, error) { }), nil } -// noopPusher is a fallback Pusher used when PUSHER_CHECKOUT_PATH is not -// configured. It returns an error on every Push so the merge controller -// (which treats non-ErrConflict errors as transient and nacks the message) -// will not silently report success. It exists so the orchestrator can -// still start up — and serve Ping / accept enqueues — in environments -// that don't run the merge step. -type noopPusher struct{} - -func (noopPusher) Push(_ context.Context, _ []entity.Change) (pusher.Result, error) { - return pusher.Result{}, fmt.Errorf("pusher not configured: set PUSHER_CHECKOUT_PATH to enable pushing") +// newQueueRegistry builds the per-queue extension profiles for the example. +// Edge integrations (merge checker, change provider, pusher) and the build +// runner form a shared baseline; each per-queue profile starts from that +// baseline and overrides only the extensions that differ — here the scorer and +// conflict analyzer. Queues without an explicit profile fall back to the +// baseline. This is the one place queue topology lives; extension packages stay +// queue-agnostic. +func newQueueRegistry(logger *zap.Logger, scope tally.Scope) (queueRegistry, error) { + mc, err := newMergeChecker(logger, scope) + if err != nil { + return queueRegistry{}, fmt.Errorf("failed to create merge checker: %w", err) + } + cp, err := newChangeProvider(logger, scope) + if err != nil { + return queueRegistry{}, fmt.Errorf("failed to create change provider: %w", err) + } + psh, err := newPusher(logger, scope) + if err != nil { + return queueRegistry{}, fmt.Errorf("failed to create pusher: %w", err) + } + + // batchLines buckets a batch by total lines changed across all its changes — + // larger batches are likelier to fail to land. + batchLines := func(_ context.Context, changes entity.BatchChanges) (int, error) { + return changes.TotalLinesChanged(), nil + } + + // Baseline profile: shared edge integrations + a fake build runner (every + // build succeeds unless a head URI carries a failure marker), plus permissive + // defaults for scorer and conflict. The build runner instance is shared by + // the build and buildsignal controllers (same profile, same instance) so a + // build's recorded outcome survives across their separate factory lookups. + // + // The scorer is wrapped by scorerfake so a change URI carrying + // "sq-fake=score-error" forces a scoring error end-to-end; it is a pure + // passthrough otherwise. The analyzer is wrapped by conflictfake with a nil + // predicate (passthrough) — swap the predicate (e.g. conflictfake.FailAlways) + // on a queue to exercise the analyzer error path, as e2e-conflict-error-queue + // below does. + base := queueExtensions{ + mergeChecker: mc, + changeProvider: cp, + pusher: psh, + buildRunner: buildfake.New(), + scorer: scorerfake.New(heuristic.New( + []heuristic.Bucket{{Min: 0, Max: 1<<31 - 1, Score: 0.5}}, + batchLines, scope.SubScope("scorer.default"), + )), + // TODO: replace the delegate with a real analyzer (e.g. Tango target + // analysis). "all" serializes the queue conservatively. + analyzer: conflictfake.New(all.New(), nil), + } + + // test-queue: bucketed heuristic scorer; conservative (serialized) conflicts + // inherited from the baseline. + testQueue := base + testQueue.scorer = scorerfake.New(heuristic.New( + []heuristic.Bucket{ + {Min: 0, Max: 1, Score: 0.95}, + {Min: 2, Max: 5, Score: 0.80}, + {Min: 6, Max: 20, Score: 0.60}, + {Min: 21, Max: 1<<31 - 1, Score: 0.40}, + }, + batchLines, scope.SubScope("scorer.test-queue"), + )) + + // e2e-test-queue: composite scorer; no conflicts (maximum parallelism). + e2eQueue := base + e2eQueue.analyzer = conflictfake.New(none.New(), nil) + e2eQueue.scorer = scorerfake.New(composite.New( + map[string]scorer.Scorer{ + "size": heuristic.New([]heuristic.Bucket{{Min: 0, Max: 1<<31 - 1, Score: 0.8}}, batchLines, scope), + "flat": heuristic.New([]heuristic.Bucket{{Min: 0, Max: 1<<31 - 1, Score: 0.6}}, batchLines, scope), + }, + composite.Avg, scope.SubScope("scorer.e2e-test-queue"), + )) + + // e2e-conflict-error-queue: every conflict analysis fails, exercising the + // analyzer error path. Scorer/edge integrations inherit the baseline. + conflictErrQueue := base + conflictErrQueue.analyzer = conflictfake.New(all.New(), conflictfake.FailAlways) + + return queueRegistry{ + def: base, + byQueue: map[string]queueExtensions{ + "test-queue": testQueue, + "e2e-test-queue": e2eQueue, + "e2e-conflict-error-queue": conflictErrQueue, + }, + }, nil } diff --git a/submitqueue/extension/buildrunner/noop/BUILD.bazel b/submitqueue/extension/buildrunner/noop/BUILD.bazel deleted file mode 100644 index 00fef8af..00000000 --- a/submitqueue/extension/buildrunner/noop/BUILD.bazel +++ /dev/null @@ -1,24 +0,0 @@ -load("@rules_go//go:def.bzl", "go_library", "go_test") - -go_library( - name = "noop", - srcs = ["noop.go"], - importpath = "github.com/uber/submitqueue/submitqueue/extension/buildrunner/noop", - visibility = ["//visibility:public"], - deps = [ - "//submitqueue/entity", - "//submitqueue/extension/buildrunner", - ], -) - -go_test( - name = "noop_test", - srcs = ["noop_test.go"], - embed = [":noop"], - deps = [ - "//submitqueue/entity", - "//submitqueue/extension/buildrunner", - "@com_github_stretchr_testify//assert", - "@com_github_stretchr_testify//require", - ], -) diff --git a/submitqueue/extension/buildrunner/noop/noop.go b/submitqueue/extension/buildrunner/noop/noop.go deleted file mode 100644 index 97c6eab0..00000000 --- a/submitqueue/extension/buildrunner/noop/noop.go +++ /dev/null @@ -1,56 +0,0 @@ -// Copyright (c) 2025 Uber Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package noop provides a buildrunner.BuildRunner that performs no real -// work: every triggered build immediately succeeds. It is intended as a -// stub for wiring tests and as a best-case baseline where every build -// passes. -package noop - -import ( - "context" - "fmt" - "sync/atomic" - - "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/buildrunner" -) - -// runner is a buildrunner.BuildRunner that does no real work and reports -// every build as immediately succeeded. The atomic counter hands out -// unique build IDs and makes the type safe for concurrent use. -type runner struct { - counter atomic.Uint64 -} - -// New returns a buildrunner.BuildRunner that performs no real work. -func New() buildrunner.BuildRunner { - return &runner{} -} - -// Trigger returns a unique build ID without contacting any runner. -// Inputs are ignored. -func (r *runner) Trigger(_ context.Context, _ []entity.Change, _ []entity.Change, _ entity.BuildMetadata) (entity.BuildID, error) { - return entity.BuildID{ID: fmt.Sprintf("noop-%d", r.counter.Add(1))}, nil -} - -// Status always reports BuildStatusSucceeded with no metadata. -func (r *runner) Status(_ context.Context, _ entity.BuildID) (entity.BuildStatus, entity.BuildMetadata, error) { - return entity.BuildStatusSucceeded, nil, nil -} - -// Cancel is a no-op. -func (r *runner) Cancel(_ context.Context, _ entity.BuildID) error { - return nil -} diff --git a/submitqueue/extension/buildrunner/noop/noop_test.go b/submitqueue/extension/buildrunner/noop/noop_test.go deleted file mode 100644 index dff0d09b..00000000 --- a/submitqueue/extension/buildrunner/noop/noop_test.go +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright (c) 2025 Uber Technologies, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package noop - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/uber/submitqueue/submitqueue/entity" - "github.com/uber/submitqueue/submitqueue/extension/buildrunner" -) - -func TestNew_ImplementsInterface(t *testing.T) { - var _ buildrunner.BuildRunner = New() -} - -func TestRunner_Trigger(t *testing.T) { - r := New() - ctx := context.Background() - - id1, err := r.Trigger(ctx, - []entity.Change{{URIs: []string{"github://owner/repo/pull/1"}}}, - []entity.Change{{URIs: []string{"github://owner/repo/pull/2"}}}, - entity.BuildMetadata{"requester": "alice"}, - ) - require.NoError(t, err) - assert.NotEmpty(t, id1.ID) - - // IDs are unique across calls, even with empty inputs. - id2, err := r.Trigger(ctx, nil, nil, nil) - require.NoError(t, err) - assert.NotEqual(t, id1, id2) -} - -func TestRunner_Status(t *testing.T) { - r := New() - - status, meta, err := r.Status(context.Background(), entity.BuildID{ID: "any-id"}) - require.NoError(t, err) - assert.Equal(t, entity.BuildStatusSucceeded, status) - assert.Empty(t, meta) -} - -func TestRunner_Cancel(t *testing.T) { - r := New() - assert.NoError(t, r.Cancel(context.Background(), entity.BuildID{ID: "any-id"})) -} diff --git a/submitqueue/orchestrator/controller/build/BUILD.bazel b/submitqueue/orchestrator/controller/build/BUILD.bazel index 706a425f..c7da34b3 100644 --- a/submitqueue/orchestrator/controller/build/BUILD.bazel +++ b/submitqueue/orchestrator/controller/build/BUILD.bazel @@ -28,8 +28,8 @@ go_test( "//submitqueue/core/consumer", "//submitqueue/entity", "//submitqueue/extension/buildrunner", + "//submitqueue/extension/buildrunner/fake", "//submitqueue/extension/buildrunner/mock", - "//submitqueue/extension/buildrunner/noop", "//submitqueue/extension/storage", "//submitqueue/extension/storage/mock", "@com_github_stretchr_testify//assert", diff --git a/submitqueue/orchestrator/controller/build/build_test.go b/submitqueue/orchestrator/controller/build/build_test.go index 72ed0dc0..212c21b7 100644 --- a/submitqueue/orchestrator/controller/build/build_test.go +++ b/submitqueue/orchestrator/controller/build/build_test.go @@ -28,8 +28,8 @@ import ( "github.com/uber/submitqueue/submitqueue/core/consumer" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/buildrunner" + buildfake "github.com/uber/submitqueue/submitqueue/extension/buildrunner/fake" buildrunnermock "github.com/uber/submitqueue/submitqueue/extension/buildrunner/mock" - buildnoop "github.com/uber/submitqueue/submitqueue/extension/buildrunner/noop" "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" "go.uber.org/mock/gomock" @@ -74,7 +74,7 @@ func newMockStorage(ctrl *gomock.Controller, batch entity.Batch) *storagemock.Mo } // newTestController creates a controller with test dependencies. br is the -// build runner to inject; pass buildnoop.New() for the pass-through default. +// build runner to inject; pass buildfake.New() for the pass-through default. // staticBuildRunnerFactory is a test factory that returns a fixed BuildRunner // for any entityqueue. type staticBuildRunnerFactory struct{ r buildrunner.BuildRunner } @@ -111,7 +111,7 @@ func TestNewController(t *testing.T) { ctrl := gomock.NewController(t) batch := testBatch() store := newMockStorage(ctrl, batch) - controller := newTestController(t, ctrl, store, buildnoop.New(), nil) + controller := newTestController(t, ctrl, store, buildfake.New(), nil) require.NotNil(t, controller) assert.Equal(t, consumer.TopicKeyBuild, controller.TopicKey()) @@ -124,7 +124,7 @@ func TestController_Process_Success(t *testing.T) { batch := testBatch() store := newMockStorage(ctrl, batch) - controller := newTestController(t, ctrl, store, buildnoop.New(), nil) + controller := newTestController(t, ctrl, store, buildfake.New(), nil) msg := entityqueue.NewMessage(batch.ID, batchIDPayload(t, batch.ID), batch.Queue, nil) delivery := queuemock.NewMockDelivery(ctrl) @@ -315,7 +315,7 @@ func TestController_Process_StorageFailure(t *testing.T) { store.EXPECT().GetRequestStore().Return(storagemock.NewMockRequestStore(ctrl)).AnyTimes() store.EXPECT().GetBuildStore().Return(storagemock.NewMockBuildStore(ctrl)).AnyTimes() - controller := newTestController(t, ctrl, store, buildnoop.New(), nil) + controller := newTestController(t, ctrl, store, buildfake.New(), nil) msg := entityqueue.NewMessage("test-queue/batch/1", batchIDPayload(t, "test-queue/batch/1"), "test-queue", nil) delivery := queuemock.NewMockDelivery(ctrl) @@ -332,7 +332,7 @@ func TestController_Process_PublishFailure(t *testing.T) { batch := testBatch() store := newMockStorage(ctrl, batch) - controller := newTestController(t, ctrl, store, buildnoop.New(), fmt.Errorf("publish failed")) + controller := newTestController(t, ctrl, store, buildfake.New(), fmt.Errorf("publish failed")) msg := entityqueue.NewMessage(batch.ID, batchIDPayload(t, batch.ID), batch.Queue, nil) delivery := queuemock.NewMockDelivery(ctrl) @@ -347,7 +347,7 @@ func TestController_InterfaceImplementation(t *testing.T) { ctrl := gomock.NewController(t) batch := testBatch() store := newMockStorage(ctrl, batch) - controller := newTestController(t, ctrl, store, buildnoop.New(), nil) + controller := newTestController(t, ctrl, store, buildfake.New(), nil) var _ consumer.Controller = controller }