diff --git a/service/runway/README.md b/service/runway/README.md index 99b152aea..28710a624 100644 --- a/service/runway/README.md +++ b/service/runway/README.md @@ -15,7 +15,7 @@ These topic keys and their wire contracts are owned by the queue's producer side ### Merger backend -The merge work is done by the [`merger`](../../runway/extension/merger) extension, resolved **per queue** — so one Runway can serve several repositories by giving each queue its own merge target. By default every queue gets the **noop** merger (always succeeds — for local dev and compose). Point `MERGE_CONFIG_PATH` at a merge configuration file to wire real **git** merge targets, or set `MERGE_CHECKOUT_PATH` to configure a single one from the environment (see Configuration). +The merge work is done by the [`merger`](../../runway/extension/merger) extension, resolved **per queue** — so one Runway can serve several repositories by giving each queue its own merge target. By default every queue gets the **noop** merger (always succeeds — for local dev and compose). Point `MERGE_CONFIG_PATH` at a merge configuration file to wire real **git** merge targets, or set `MERGE_CHECKOUT_PATH` to configure a single one from the environment (see Configuration). Setting `MERGER=git` makes Git configuration mandatory: startup fails unless one of those sources defines at least one Git target. Two queues naming the same checkout resolve to the *same* merger instance, which is what serializes them against each other: a git merger locks the working tree it owns, and two instances over one tree would reset it out from under each other mid-merge. Naming one checkout for two *different* targets is rejected at startup. @@ -71,6 +71,7 @@ The Runway controllers themselves live under [`runway/controller/`](../../runway | `QUEUE_MYSQL_DSN` | yes | Queue database DSN | — | | `PORT` | no | gRPC listen address | `:8086` | | `HOSTNAME` | no | Subscriber name for the queue consumer | `runway-` | +| `MERGER` | no | Explicit merger selection. `git` requires `MERGE_CHECKOUT_PATH` or a `MERGE_CONFIG_PATH` file containing at least one Git target; `noop` forces synthetic success; `fake` is test-only. Unset resolves from the merge configuration and retains the noop fallback. | — | | `MERGE_CONFIG_PATH` | no | Path to the per-queue merge configuration file (see above). Takes precedence over the `MERGE_*` variables below, which configure a single target. | — | | `MERGE_CHECKOUT_PATH` | no | Absolute path to the git checkout the merger owns. When unset (and no config file), the noop merger is used. | — (noop) | | `MERGE_REMOTE` | no | Git remote to fetch/push | `origin` | diff --git a/service/runway/server/config_test.go b/service/runway/server/config_test.go index 63e3fb186..6178bbcf2 100644 --- a/service/runway/server/config_test.go +++ b/service/runway/server/config_test.go @@ -15,6 +15,7 @@ package main import ( + "errors" "os" "path/filepath" "strings" @@ -22,6 +23,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" ) @@ -113,6 +115,133 @@ func TestLoadMergeConfig_EmptyFileIsNoop(t *testing.T) { assert.False(t, cfg.usesGit()) } +func TestResolveMergerStartupConfig(t *testing.T) { + tests := []struct { + name string + merger string + checkoutPath string + configPath string + configContents string + hasConfigFile bool + wantSelection string + wantDefaultType string + wantUsesGit bool + wantErr error + wantUnclassified bool + }{ + { + name: "unset without config uses automatic noop fallback", + wantDefaultType: mergerTypeNoop, + }, + { + name: "explicit git without config fails", + merger: mergerTypeGit, + wantErr: errExplicitGitConfigurationRequired, + }, + { + name: "explicit git with checkout environment is configured", + merger: mergerTypeGit, + checkoutPath: "/var/checkouts/r", + wantSelection: mergerTypeGit, + wantDefaultType: mergerTypeGit, + wantUsesGit: true, + }, + { + name: "explicit git with config file containing a git target is configured", + merger: mergerTypeGit, + configContents: gitTargetConfig(), + hasConfigFile: true, + wantSelection: mergerTypeGit, + wantDefaultType: mergerTypeNoop, + wantUsesGit: true, + }, + { + name: "explicit git with config file containing no git target fails", + merger: mergerTypeGit, + configContents: noopTargetConfig(), + hasConfigFile: true, + wantErr: errExplicitGitConfigurationRequired, + }, + { + name: "explicit noop bypasses merge configuration", + merger: mergerTypeNoop, + configPath: "/missing/merge.yaml", + wantSelection: mergerTypeNoop, + }, + { + name: "explicit fake preserves test behavior", + merger: mergerOverrideFake, + configPath: "/missing/merge.yaml", + wantSelection: mergerOverrideFake, + }, + { + name: "invalid merger fails", + merger: "magic", + wantUnclassified: true, + }, + { + name: "invalid merge config still fails", + configContents: "defaults:\n merger: {type: magic}\n", + hasConfigFile: true, + wantUnclassified: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setMergerStartupEnv(t, tt.merger, tt.checkoutPath) + switch { + case tt.hasConfigFile: + t.Setenv("MERGE_CONFIG_PATH", writeConfig(t, tt.configContents)) + case tt.configPath != "": + t.Setenv("MERGE_CONFIG_PATH", tt.configPath) + } + + startup, err := resolveMergerStartupConfig(zaptest.NewLogger(t)) + if tt.wantErr != nil { + require.ErrorIs(t, err, tt.wantErr) + return + } + if tt.wantUnclassified { + require.Error(t, err) + assert.False(t, errors.Is(err, errExplicitGitConfigurationRequired)) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantSelection, startup.selection) + assert.Equal(t, tt.wantDefaultType, startup.targets.Defaults.Merger.Type) + assert.Equal(t, tt.wantUsesGit, startup.targets.usesGit()) + }) + } +} + +func setMergerStartupEnv(t *testing.T, merger, checkoutPath string) { + t.Helper() + t.Setenv("MERGER", merger) + t.Setenv("MERGE_CONFIG_PATH", "") + t.Setenv("MERGE_CHECKOUT_PATH", checkoutPath) +} + +func gitTargetConfig() string { + return ` +defaults: + merger: {type: noop} +queues: + - name: demo + merger: {type: git, checkoutPath: /var/checkouts/r} +` +} + +func noopTargetConfig() string { + return ` +defaults: + merger: {type: noop} +queues: + - name: demo + merger: {type: noop} +` +} + func TestLoadMergeConfig_SharedCheckoutForSameTargetIsAllowed(t *testing.T) { // Two queues landing on one target must resolve to one merger instance, so // sharing a checkout is the correct configuration rather than a mistake. diff --git a/service/runway/server/docker-compose.yml b/service/runway/server/docker-compose.yml index 1706886dc..0b536d619 100644 --- a/service/runway/server/docker-compose.yml +++ b/service/runway/server/docker-compose.yml @@ -45,6 +45,7 @@ services: - PORT=:8080 # Merger implementation. Empty (the default) resolves from the merge # environment: git when MERGE_CHECKOUT_PATH is set, noop otherwise. The + # explicit git value fails startup without a configured Git target. The # e2e suite sets SQ_RUNWAY_MERGER=fake to drive outcomes from the payload. - MERGER=${SQ_RUNWAY_MERGER:-} # Queue infrastructure connection diff --git a/service/runway/server/main.go b/service/runway/server/main.go index dc9985001..0d50c9816 100644 --- a/service/runway/server/main.go +++ b/service/runway/server/main.go @@ -62,6 +62,10 @@ type RunwayServer struct { pingController *controller.PingController } +const mergerOverrideFake = "fake" + +var errExplicitGitConfigurationRequired = errors.New(`MERGER="git" requires usable Git configuration: set MERGE_CHECKOUT_PATH or set MERGE_CONFIG_PATH to a config containing at least one git merger`) + // Ping delegates to the controller. func (s *RunwayServer) Ping(ctx context.Context, req *pb.PingRequest) (*pb.PingResponse, error) { return s.pingController.Ping(ctx, req) @@ -307,17 +311,21 @@ func run() error { // newMergerFactory builds the mergers for the server. // -// MERGER pins every queue to one implementation explicitly, which is how a test -// holds the service to a fake without a git checkout. Left unset, each queue -// resolves its own merge target through the merge configuration, so a -// deployment can serve several repositories from one Runway. +// MERGER=fake and MERGER=noop pin every queue to one implementation. +// MERGER=git requires the resolved merge configuration to contain a Git target. +// Left unset, each queue resolves its own merge target through configuration. // // The fake is reachable only through MERGER, never through the configuration // file: an implementation whose outcomes are steered by markers in a change URI // has no business being selectable by a production config. func newMergerFactory(ctx context.Context, logger *zap.Logger, scope tally.Scope) (merger.Factory, error) { - switch impl := strings.ToLower(strings.TrimSpace(os.Getenv("MERGER"))); impl { - case "fake": + startup, err := resolveMergerStartupConfig(logger) + if err != nil { + return nil, err + } + + switch startup.selection { + case mergerOverrideFake: // Marker-driven outcomes, for e2e tests that need Runway to fail on // demand without a git checkout. Never production. logger.Info("MERGER=fake; using marker-driven fake merger for every queue") @@ -325,16 +333,9 @@ func newMergerFactory(ctx context.Context, logger *zap.Logger, scope tally.Scope case "noop": logger.Info("MERGER=noop; using noop merger for every queue") return &noopMergerFactory{seq: new(atomic.Uint64)}, nil - case "", "git": - // Fall through to the configured per-queue merge targets. - default: - return nil, fmt.Errorf("invalid MERGER %q", impl) } - cfg, err := loadMergeConfigFromEnv(logger) - if err != nil { - return nil, err - } + cfg := startup.targets // The git runtime is resolved only when something actually needs it, so a // deployment running nothing but the noop merger does not require git to be @@ -385,6 +386,31 @@ func newMergerFactory(ctx context.Context, logger *zap.Logger, scope tally.Scope return mergerRegistry{byQueue: byQueue, fallback: fallback}, nil } +type mergerStartupConfig struct { + selection string + targets mergeConfig +} + +func resolveMergerStartupConfig(logger *zap.Logger) (mergerStartupConfig, error) { + selection := strings.ToLower(strings.TrimSpace(os.Getenv("MERGER"))) + switch selection { + case mergerOverrideFake, mergerTypeNoop: + return mergerStartupConfig{selection: selection}, nil + case "", mergerTypeGit: + default: + return mergerStartupConfig{}, fmt.Errorf("invalid MERGER %q", selection) + } + + cfg, err := loadMergeConfigFromEnv(logger) + if err != nil { + return mergerStartupConfig{}, err + } + if selection == mergerTypeGit && !cfg.usesGit() { + return mergerStartupConfig{}, errExplicitGitConfigurationRequired + } + return mergerStartupConfig{selection: selection, targets: cfg}, nil +} + // loadMergeConfigFromEnv reads the merge configuration file when one is // configured, and otherwise reconstructs the equivalent single-queue // configuration from the MERGE_* environment.