Skip to content

Commit c19d95e

Browse files
fix(runway): ISS-011 fail fast without Git configuration (#677)
## Summary Intent: - Prevent an explicit Git merger selection from silently starting with the noop merger. - Keep automatic noop fallback and explicit noop/fake overrides unchanged. Changes: - Validate startup configuration when MERGER=git and require at least one usable Git target. - Cover environment and file-based Git configuration, Git-less files, noop, fake, unset, and invalid selections. - Document the explicit Git startup contract. Reproduction: - Deploy Runway with MERGER=git while omitting both MERGE_CONFIG_PATH and MERGE_CHECKOUT_PATH. - Previously the service started with the noop merger and could publish synthetic successful merge results without changing Git. - Startup now fails with: MERGER="git" requires usable Git configuration: set MERGE_CHECKOUT_PATH or set MERGE_CONFIG_PATH to a config containing at least one git merger. --- <sub>Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace</sub> ## Test Plan ## AI Verification > Validated at `492963f` on Sep 4 22:15 UTC · 4 files analyzed · 1s | Validator | Status | Issues | |----------|--------|--------| | java-lint | not_applicable | 0 | | java-coverage | not_applicable | 0 | | ios-lint | not_applicable | 0 | | fix-disclosure | not_applicable | 0 | | merge-conflict | not_applicable | 0 | | visual-web | not_applicable | 0 | | go-thrift-lint | not_applicable | 0 | | web-coverage | not_applicable | 0 | | web-unit | not_applicable | 0 | | diff-template | not_applicable | 0 | | go-gazelle | not_applicable | 0 | | arc-unit | not_applicable | 0 | | arc-lint | not_applicable | 0 | | android-coverage | not_applicable | 0 | | web-typecheck | not_applicable | 0 | | web-repocheck | not_applicable | 0 | | go-lint | not_applicable | 0 | | go-coverage | not_applicable | 0 | | visual-ios | not_applicable | 0 | | android-lint | not_applicable | 0 | | ios-test | not_applicable | 0 | | uber-one | not_applicable | 0 | | web-lint | not_applicable | 0 | | visual-android | not_applicable | 0 | | go-proto-lint | not_applicable | 0 | | custom | not_applicable | 0 | | ureview | completed | 0 | **0** issues detected <sub>Skipped validators: claude · [EngWiki](http://t.uber.com/ai-verification)</sub> ## Issues T3-ISS-011 Co-authored-by: sergeyb <sergeyb@uber.com>
1 parent f3541fa commit c19d95e

4 files changed

Lines changed: 172 additions & 15 deletions

File tree

service/runway/README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ These topic keys and their wire contracts are owned by the queue's producer side
1515

1616
### Merger backend
1717

18-
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).
18+
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.
1919

2020
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.
2121

@@ -71,6 +71,7 @@ The Runway controllers themselves live under [`runway/controller/`](../../runway
7171
| `QUEUE_MYSQL_DSN` | yes | Queue database DSN | — |
7272
| `PORT` | no | gRPC listen address | `:8086` |
7373
| `HOSTNAME` | no | Subscriber name for the queue consumer | `runway-<unix_ts>` |
74+
| `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. | — |
7475
| `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. | — |
7576
| `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) |
7677
| `MERGE_REMOTE` | no | Git remote to fetch/push | `origin` |

service/runway/server/config_test.go

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@
1515
package main
1616

1717
import (
18+
"errors"
1819
"os"
1920
"path/filepath"
2021
"strings"
2122
"testing"
2223

2324
"github.com/stretchr/testify/assert"
2425
"github.com/stretchr/testify/require"
26+
"go.uber.org/zap/zaptest"
2527

2628
mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb"
2729
)
@@ -113,6 +115,133 @@ func TestLoadMergeConfig_EmptyFileIsNoop(t *testing.T) {
113115
assert.False(t, cfg.usesGit())
114116
}
115117

118+
func TestResolveMergerStartupConfig(t *testing.T) {
119+
tests := []struct {
120+
name string
121+
merger string
122+
checkoutPath string
123+
configPath string
124+
configContents string
125+
hasConfigFile bool
126+
wantSelection string
127+
wantDefaultType string
128+
wantUsesGit bool
129+
wantErr error
130+
wantUnclassified bool
131+
}{
132+
{
133+
name: "unset without config uses automatic noop fallback",
134+
wantDefaultType: mergerTypeNoop,
135+
},
136+
{
137+
name: "explicit git without config fails",
138+
merger: mergerTypeGit,
139+
wantErr: errExplicitGitConfigurationRequired,
140+
},
141+
{
142+
name: "explicit git with checkout environment is configured",
143+
merger: mergerTypeGit,
144+
checkoutPath: "/var/checkouts/r",
145+
wantSelection: mergerTypeGit,
146+
wantDefaultType: mergerTypeGit,
147+
wantUsesGit: true,
148+
},
149+
{
150+
name: "explicit git with config file containing a git target is configured",
151+
merger: mergerTypeGit,
152+
configContents: gitTargetConfig(),
153+
hasConfigFile: true,
154+
wantSelection: mergerTypeGit,
155+
wantDefaultType: mergerTypeNoop,
156+
wantUsesGit: true,
157+
},
158+
{
159+
name: "explicit git with config file containing no git target fails",
160+
merger: mergerTypeGit,
161+
configContents: noopTargetConfig(),
162+
hasConfigFile: true,
163+
wantErr: errExplicitGitConfigurationRequired,
164+
},
165+
{
166+
name: "explicit noop bypasses merge configuration",
167+
merger: mergerTypeNoop,
168+
configPath: "/missing/merge.yaml",
169+
wantSelection: mergerTypeNoop,
170+
},
171+
{
172+
name: "explicit fake preserves test behavior",
173+
merger: mergerOverrideFake,
174+
configPath: "/missing/merge.yaml",
175+
wantSelection: mergerOverrideFake,
176+
},
177+
{
178+
name: "invalid merger fails",
179+
merger: "magic",
180+
wantUnclassified: true,
181+
},
182+
{
183+
name: "invalid merge config still fails",
184+
configContents: "defaults:\n merger: {type: magic}\n",
185+
hasConfigFile: true,
186+
wantUnclassified: true,
187+
},
188+
}
189+
190+
for _, tt := range tests {
191+
t.Run(tt.name, func(t *testing.T) {
192+
setMergerStartupEnv(t, tt.merger, tt.checkoutPath)
193+
switch {
194+
case tt.hasConfigFile:
195+
t.Setenv("MERGE_CONFIG_PATH", writeConfig(t, tt.configContents))
196+
case tt.configPath != "":
197+
t.Setenv("MERGE_CONFIG_PATH", tt.configPath)
198+
}
199+
200+
startup, err := resolveMergerStartupConfig(zaptest.NewLogger(t))
201+
if tt.wantErr != nil {
202+
require.ErrorIs(t, err, tt.wantErr)
203+
return
204+
}
205+
if tt.wantUnclassified {
206+
require.Error(t, err)
207+
assert.False(t, errors.Is(err, errExplicitGitConfigurationRequired))
208+
return
209+
}
210+
require.NoError(t, err)
211+
assert.Equal(t, tt.wantSelection, startup.selection)
212+
assert.Equal(t, tt.wantDefaultType, startup.targets.Defaults.Merger.Type)
213+
assert.Equal(t, tt.wantUsesGit, startup.targets.usesGit())
214+
})
215+
}
216+
}
217+
218+
func setMergerStartupEnv(t *testing.T, merger, checkoutPath string) {
219+
t.Helper()
220+
t.Setenv("MERGER", merger)
221+
t.Setenv("MERGE_CONFIG_PATH", "")
222+
t.Setenv("MERGE_CHECKOUT_PATH", checkoutPath)
223+
}
224+
225+
func gitTargetConfig() string {
226+
return `
227+
defaults:
228+
merger: {type: noop}
229+
queues:
230+
- name: demo
231+
merger: {type: git, checkoutPath: /var/checkouts/r}
232+
`
233+
}
234+
235+
func noopTargetConfig() string {
236+
return `
237+
defaults:
238+
merger: {type: noop}
239+
queues:
240+
- name: demo
241+
merger: {type: noop}
242+
`
243+
}
244+
116245
func TestLoadMergeConfig_SharedCheckoutForSameTargetIsAllowed(t *testing.T) {
117246
// Two queues landing on one target must resolve to one merger instance, so
118247
// sharing a checkout is the correct configuration rather than a mistake.

service/runway/server/docker-compose.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ services:
4545
- PORT=:8080
4646
# Merger implementation. Empty (the default) resolves from the merge
4747
# environment: git when MERGE_CHECKOUT_PATH is set, noop otherwise. The
48+
# explicit git value fails startup without a configured Git target. The
4849
# e2e suite sets SQ_RUNWAY_MERGER=fake to drive outcomes from the payload.
4950
- MERGER=${SQ_RUNWAY_MERGER:-}
5051
# Queue infrastructure connection

service/runway/server/main.go

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,10 @@ type RunwayServer struct {
6262
pingController *controller.PingController
6363
}
6464

65+
const mergerOverrideFake = "fake"
66+
67+
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`)
68+
6569
// Ping delegates to the controller.
6670
func (s *RunwayServer) Ping(ctx context.Context, req *pb.PingRequest) (*pb.PingResponse, error) {
6771
return s.pingController.Ping(ctx, req)
@@ -313,34 +317,31 @@ func logQueueInitialized(logger *zap.Logger) {
313317

314318
// newMergerFactory builds the mergers for the server.
315319
//
316-
// MERGER pins every queue to one implementation explicitly, which is how a test
317-
// holds the service to a fake without a git checkout. Left unset, each queue
318-
// resolves its own merge target through the merge configuration, so a
319-
// deployment can serve several repositories from one Runway.
320+
// MERGER=fake and MERGER=noop pin every queue to one implementation.
321+
// MERGER=git requires the resolved merge configuration to contain a Git target.
322+
// Left unset, each queue resolves its own merge target through configuration.
320323
//
321324
// The fake is reachable only through MERGER, never through the configuration
322325
// file: an implementation whose outcomes are steered by markers in a change URI
323326
// has no business being selectable by a production config.
324327
func newMergerFactory(ctx context.Context, logger *zap.Logger, scope tally.Scope) (merger.Factory, error) {
325-
switch impl := strings.ToLower(strings.TrimSpace(os.Getenv("MERGER"))); impl {
326-
case "fake":
328+
startup, err := resolveMergerStartupConfig(logger)
329+
if err != nil {
330+
return nil, err
331+
}
332+
333+
switch startup.selection {
334+
case mergerOverrideFake:
327335
// Marker-driven outcomes, for e2e tests that need Runway to fail on
328336
// demand without a git checkout. Never production.
329337
logger.Info("MERGER=fake; using marker-driven fake merger for every queue")
330338
return &fakeMergerFactory{seq: new(atomic.Uint64)}, nil
331339
case "noop":
332340
logger.Info("MERGER=noop; using noop merger for every queue")
333341
return &noopMergerFactory{seq: new(atomic.Uint64)}, nil
334-
case "", "git":
335-
// Fall through to the configured per-queue merge targets.
336-
default:
337-
return nil, fmt.Errorf("invalid MERGER %q", impl)
338342
}
339343

340-
cfg, err := loadMergeConfigFromEnv(logger)
341-
if err != nil {
342-
return nil, err
343-
}
344+
cfg := startup.targets
344345

345346
// The git runtime is resolved only when something actually needs it, so a
346347
// deployment running nothing but the noop merger does not require git to be
@@ -391,6 +392,31 @@ func newMergerFactory(ctx context.Context, logger *zap.Logger, scope tally.Scope
391392
return mergerRegistry{byQueue: byQueue, fallback: fallback}, nil
392393
}
393394

395+
type mergerStartupConfig struct {
396+
selection string
397+
targets mergeConfig
398+
}
399+
400+
func resolveMergerStartupConfig(logger *zap.Logger) (mergerStartupConfig, error) {
401+
selection := strings.ToLower(strings.TrimSpace(os.Getenv("MERGER")))
402+
switch selection {
403+
case mergerOverrideFake, mergerTypeNoop:
404+
return mergerStartupConfig{selection: selection}, nil
405+
case "", mergerTypeGit:
406+
default:
407+
return mergerStartupConfig{}, fmt.Errorf("invalid MERGER %q", selection)
408+
}
409+
410+
cfg, err := loadMergeConfigFromEnv(logger)
411+
if err != nil {
412+
return mergerStartupConfig{}, err
413+
}
414+
if selection == mergerTypeGit && !cfg.usesGit() {
415+
return mergerStartupConfig{}, errExplicitGitConfigurationRequired
416+
}
417+
return mergerStartupConfig{selection: selection, targets: cfg}, nil
418+
}
419+
394420
// loadMergeConfigFromEnv reads the merge configuration file when one is
395421
// configured, and otherwise reconstructs the equivalent single-queue
396422
// configuration from the MERGE_* environment.

0 commit comments

Comments
 (0)