From b5eb21fb83027cdba1943619699e330bc77a383e Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Sun, 16 Aug 2026 13:37:36 -0700 Subject: [PATCH 1/4] refactor(test): one helper for resolving the pinned git MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? Five test files each carried their own copy of the same twenty lines: read `SUBMITQUEUE_TEST_GIT`, notice that rules_go expanded `$(location)` to an execroot-relative path, and re-root it under `TEST_SRCDIR` so the binary can actually be executed. They had already drifted. Two spellings handled a leading `external/` as well as an embedded `/external/`; three handled only the embedded form. Two verified the resolved path with `os.Stat`; one returned it unchecked. Two were named for what they returned (`testGit`, `pinnedGit`), two for how they worked (`runfilePath`, `absoluteTestPath`). One carried a comment pointing at another copy as the explanation for why any of it was necessary. None of that is behaviour anyone chose — it is what five independent transcriptions of the same workaround look like after a while. The next test that shells out to git was going to be a sixth. ### What? `platform/gitexec/gitexectest` — `Git(t)` for the common case, `Runfile(t, name)` for a target that pins something else as well, which today is the merger's commit-template directory. Colocated with `gitexec` and named for it, following `httptest`: the thing a caller wants is the binary `gitexec` will run, and the runfiles indirection is Bazel's business rather than theirs. The promoted implementation is the most complete of the five (`service/runway/server`'s): it accepts both spellings of an external path and stats what it resolves, so a misconfigured target fails saying so rather than handing back a path that does not exist. The three thinner copies gain that; nothing loses anything. Deliberately not in `test/testutil`. That package is the Docker Compose harness and pulls in the MySQL driver and gRPC — dependencies a unit test that only needs a git binary should not acquire to find one. Pure refactor: no test changes what it asserts, and no production code is touched. ## Test Plan - ✅ all five converted targets pass — `//tool/gitsandbox`, `//service/submitqueue/demo/requests`, `//runway/extension/merger/git`, `//service/runway/server`, and `make e2e-git-test` - ✅ `make fmt`, `make gazelle` The E2E is the one that matters here: it runs the pinned git both from the test process and inside the containers, so a helper that resolved the wrong path would fail it rather than silently fall back to the host's git. Local note, unrelated to this change: `make e2e-git-test` needs `--sandbox_writable_path=$HOME/.docker` on macOS, or the build fails on `open ~/.docker/buildx/activity/…: operation not permitted` before any test runs. --- platform/gitexec/gitexectest/BUILD.bazel | 9 ++ platform/gitexec/gitexectest/gitexectest.go | 86 +++++++++++++++++++ runway/extension/merger/git/BUILD.bazel | 1 + .../extension/merger/git/git_merger_test.go | 44 +--------- service/runway/server/BUILD.bazel | 1 + service/runway/server/checkout_test.go | 35 +------- service/submitqueue/demo/requests/BUILD.bazel | 1 + .../submitqueue/demo/requests/source_test.go | 33 ++----- test/e2e/submitqueue/BUILD.bazel | 1 + test/e2e/submitqueue/git_suite_test.go | 39 +-------- tool/gitsandbox/BUILD.bazel | 1 + tool/gitsandbox/main_test.go | 53 +++--------- 12 files changed, 126 insertions(+), 178 deletions(-) create mode 100644 platform/gitexec/gitexectest/BUILD.bazel create mode 100644 platform/gitexec/gitexectest/gitexectest.go diff --git a/platform/gitexec/gitexectest/BUILD.bazel b/platform/gitexec/gitexectest/BUILD.bazel new file mode 100644 index 00000000..7ec28413 --- /dev/null +++ b/platform/gitexec/gitexectest/BUILD.bazel @@ -0,0 +1,9 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["gitexectest.go"], + importpath = "github.com/uber/submitqueue/platform/gitexec/gitexectest", + visibility = ["//visibility:public"], + deps = ["@com_github_stretchr_testify//require:go_default_library"], +) diff --git a/platform/gitexec/gitexectest/gitexectest.go b/platform/gitexec/gitexectest/gitexectest.go new file mode 100644 index 00000000..8e7fec6d --- /dev/null +++ b/platform/gitexec/gitexectest/gitexectest.go @@ -0,0 +1,86 @@ +// 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 gitexectest resolves the Bazel-pinned git a test target supplies. +// +// Tests that shell out to git run against the same pinned build the services +// use, rather than whatever the host happens to have, so a developer's git +// version cannot change what a test proves. A target opts in by depending on +// the binary and naming it in the environment: +// +// go_test( +// data = ["@git"], +// env = {"SUBMITQUEUE_TEST_GIT": "$(location @git//:git)"}, +// ) +// +// The indirection through an environment variable is Bazel's: rules_go expands +// $(location) to an execroot-relative path, which for an external output has to +// be re-rooted under the runfiles tree the test actually runs from. +package gitexectest + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// GitEnv is the variable a test target sets to the pinned git binary. +const GitEnv = "SUBMITQUEUE_TEST_GIT" + +// Git returns an absolute path to the pinned git binary, failing the test if +// the target did not supply one. +func Git(t *testing.T) string { + t.Helper() + return Runfile(t, GitEnv) +} + +// Runfile resolves the $(location)-expanded path held by the named environment +// variable, failing the test if it is unset or does not resolve. +// +// A path that is already usable is returned as-is, which is what happens under +// `bazel run`, where the process already starts inside the runfiles tree. +func Runfile(t *testing.T, name string) string { + t.Helper() + + path := os.Getenv(name) + require.NotEmpty(t, path, "%s must be set by the test target", name) + + if absolute, err := filepath.Abs(path); err == nil { + if _, err := os.Stat(absolute); err == nil { + return absolute + } + } + + // Both spellings occur: the path is execroot-relative for a target in this + // repository, and carries a leading segment for one reached through another. + slashed := filepath.ToSlash(path) + external := "" + if strings.HasPrefix(slashed, "external/") { + external = strings.TrimPrefix(slashed, "external/") + } else if i := strings.Index(slashed, "/external/"); i >= 0 { + external = slashed[i+len("/external/"):] + } + require.NotEmpty(t, external, "%s=%q is not a runfile", name, path) + + root := os.Getenv("TEST_SRCDIR") + require.NotEmpty(t, root, "TEST_SRCDIR must be set when %s is a runfile", name) + + candidate := filepath.Join(root, filepath.FromSlash(external)) + _, err := os.Stat(candidate) + require.NoError(t, err, "%s=%q does not resolve under TEST_SRCDIR", name, path) + return candidate +} diff --git a/runway/extension/merger/git/BUILD.bazel b/runway/extension/merger/git/BUILD.bazel index 4633a955..9d876bf3 100644 --- a/runway/extension/merger/git/BUILD.bazel +++ b/runway/extension/merger/git/BUILD.bazel @@ -48,6 +48,7 @@ go_test( "//api/base/mergestrategy/protopb:go_default_library", "//api/runway/messagequeue:go_default_library", "//api/runway/messagequeue/protopb:go_default_library", + "//platform/gitexec/gitexectest:go_default_library", "//runway/extension/merger:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/runway/extension/merger/git/git_merger_test.go b/runway/extension/merger/git/git_merger_test.go index 0e8e4b30..8dc3763b 100644 --- a/runway/extension/merger/git/git_merger_test.go +++ b/runway/extension/merger/git/git_merger_test.go @@ -35,6 +35,7 @@ import ( mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" runwaymq "github.com/uber/submitqueue/api/runway/messagequeue" runwaypb "github.com/uber/submitqueue/api/runway/messagequeue/protopb" + "github.com/uber/submitqueue/platform/gitexec/gitexectest" "github.com/uber/submitqueue/runway/extension/merger" ) @@ -1782,8 +1783,8 @@ func mustGitOutput(t *testing.T, dir string, args ...string) []byte { func testGitRuntime(t *testing.T) GitRuntime { t.Helper() - executable := absoluteTestPath(t, "SUBMITQUEUE_TEST_GIT") - templateDescription := absoluteTestPath(t, "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION") + executable := gitexectest.Git(t) + templateDescription := gitexectest.Runfile(t, "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION") return GitRuntime{ Executable: executable, ExecPath: filepath.Dir(executable), @@ -1791,45 +1792,6 @@ func testGitRuntime(t *testing.T) GitRuntime { } } -func absoluteTestPath(t *testing.T, name string) string { - t.Helper() - path := os.Getenv(name) - require.NotEmpty(t, path) - if filepath.IsAbs(path) { - _, err := os.Stat(path) - require.NoError(t, err) - return path - } - if absolute, err := filepath.Abs(path); err == nil { - if _, err := os.Stat(absolute); err == nil { - return absolute - } - } - - // rules_go expands $(location) to an execroot-relative path. Tests run from - // their main-repository runfiles directory, so translate an external output - // to its canonical repository path under TEST_SRCDIR. - const externalMarker = "/external/" - slashed := filepath.ToSlash(path) - externalPath := "" - if strings.HasPrefix(slashed, "external/") { - externalPath = strings.TrimPrefix(slashed, "external/") - } else if i := strings.Index(slashed, externalMarker); i >= 0 { - externalPath = slashed[i+len(externalMarker):] - } - if externalPath != "" { - runfilesRoot := os.Getenv("TEST_SRCDIR") - require.NotEmpty(t, runfilesRoot) - candidate := filepath.Join(runfilesRoot, filepath.FromSlash(externalPath)) - _, err := os.Stat(candidate) - require.NoError(t, err) - return candidate - } - - require.FailNowf(t, "resolve test path", "%s=%q is not a runfile", name, path) - return "" -} - func writeFile(path, contents string) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return err diff --git a/service/runway/server/BUILD.bazel b/service/runway/server/BUILD.bazel index 70d0b1c9..d0c21693 100644 --- a/service/runway/server/BUILD.bazel +++ b/service/runway/server/BUILD.bazel @@ -96,6 +96,7 @@ go_test( }, deps = [ "//api/base/mergestrategy/protopb:go_default_library", + "//platform/gitexec/gitexectest:go_default_library", "//runway/extension/merger/git:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/service/runway/server/checkout_test.go b/service/runway/server/checkout_test.go index a2e4962c..9a11501d 100644 --- a/service/runway/server/checkout_test.go +++ b/service/runway/server/checkout_test.go @@ -26,6 +26,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/zap/zaptest" + "github.com/uber/submitqueue/platform/gitexec/gitexectest" gitmerger "github.com/uber/submitqueue/runway/extension/merger/git" ) @@ -33,8 +34,8 @@ import ( // runs real git, so these tests use the same runtime the merger will. func testRuntime(t *testing.T) gitmerger.GitRuntime { t.Helper() - executable := runfilePath(t, "SUBMITQUEUE_TEST_GIT") - templateDescription := runfilePath(t, "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION") + executable := gitexectest.Git(t) + templateDescription := gitexectest.Runfile(t, "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION") return gitmerger.GitRuntime{ Executable: executable, ExecPath: filepath.Dir(executable), @@ -42,36 +43,6 @@ func testRuntime(t *testing.T) gitmerger.GitRuntime { } } -// runfilePath resolves a $(location)-expanded path from the test environment. -// rules_go emits an execroot-relative path, which for an external output has to -// be re-rooted under the runfiles directory the test actually runs from. -func runfilePath(t *testing.T, name string) string { - t.Helper() - path := os.Getenv(name) - require.NotEmpty(t, path, "%s must be set by the test target", name) - if absolute, err := filepath.Abs(path); err == nil { - if _, err := os.Stat(absolute); err == nil { - return absolute - } - } - - slashed := filepath.ToSlash(path) - external := "" - if strings.HasPrefix(slashed, "external/") { - external = strings.TrimPrefix(slashed, "external/") - } else if i := strings.Index(slashed, "/external/"); i >= 0 { - external = slashed[i+len("/external/"):] - } - require.NotEmpty(t, external, "%s=%q is not a runfile", name, path) - - root := os.Getenv("TEST_SRCDIR") - require.NotEmpty(t, root) - candidate := filepath.Join(root, filepath.FromSlash(external)) - _, err := os.Stat(candidate) - require.NoError(t, err) - return candidate -} - // seedBareRepo creates a bare repository holding one commit on branch and // returns its path — the shape of the remote a merge target points at. func seedBareRepo(t *testing.T, branch string) string { diff --git a/service/submitqueue/demo/requests/BUILD.bazel b/service/submitqueue/demo/requests/BUILD.bazel index 6189c153..e5ffc347 100644 --- a/service/submitqueue/demo/requests/BUILD.bazel +++ b/service/submitqueue/demo/requests/BUILD.bazel @@ -49,6 +49,7 @@ go_test( "//platform/base/change/git:go_default_library", "//platform/fakemarker:go_default_library", "//platform/gitexec:go_default_library", + "//platform/gitexec/gitexectest:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", ], diff --git a/service/submitqueue/demo/requests/source_test.go b/service/submitqueue/demo/requests/source_test.go index 8f45cde2..9577fe1b 100644 --- a/service/submitqueue/demo/requests/source_test.go +++ b/service/submitqueue/demo/requests/source_test.go @@ -28,6 +28,7 @@ import ( gitchange "github.com/uber/submitqueue/platform/base/change/git" "github.com/uber/submitqueue/platform/fakemarker" "github.com/uber/submitqueue/platform/gitexec" + "github.com/uber/submitqueue/platform/gitexec/gitexectest" ) // quietSpec is a changeSpec whose progress notes go nowhere, for tests that @@ -134,28 +135,6 @@ func TestFakeSource_IsReproducible(t *testing.T) { assert.Equal(t, first.uri, again.uri, "replaying a run must submit the same change") } -// testGit resolves the pinned git the test target supplies. See the identical -// helper in tool/gitsandbox for why the runfile has to be re-rooted. -func testGit(t *testing.T) string { - t.Helper() - - supplied := os.Getenv("SUBMITQUEUE_TEST_GIT") - require.NotEmpty(t, supplied, "the test target must supply SUBMITQUEUE_TEST_GIT") - if git, err := gitexec.Resolve(supplied); err == nil { - return git - } - - slashed := filepath.ToSlash(supplied) - index := strings.Index(slashed, "/external/") - require.GreaterOrEqual(t, index, 0, "SUBMITQUEUE_TEST_GIT=%q is not a runfile", supplied) - root := os.Getenv("TEST_SRCDIR") - require.NotEmpty(t, root) - - git, err := gitexec.Resolve(filepath.Join(root, filepath.FromSlash(slashed[index+len("/external/"):]))) - require.NoError(t, err) - return git -} - // sandbox creates a bare repository with one commit on main, standing in for // what tool/gitsandbox provisions. func sandbox(t *testing.T, git string) string { @@ -176,7 +155,7 @@ func sandbox(t *testing.T, git string) string { } func TestGitSource_PushesABranchWithACommitPerFile(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := sandbox(t, git) @@ -208,7 +187,7 @@ func TestGitSource_PushesABranchWithACommitPerFile(t *testing.T) { } func TestGitSource_MintsAURIPinningTheHead(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() src, err := newGitSource(ctx, git, sandbox(t, git), "sandbox") require.NoError(t, err) @@ -232,7 +211,7 @@ func TestGitSource_MintsAURIPinningTheHead(t *testing.T) { // A stack is cut from the previous change's head rather than the base, which is // what makes the chain land in the order it was built. func TestGitSource_StacksOnAPreviousChange(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := sandbox(t, git) src, err := newGitSource(ctx, git, bare, "sandbox") @@ -256,7 +235,7 @@ func TestGitSource_StacksOnAPreviousChange(t *testing.T) { // runs up to -concurrency of these at once, so the source has to serialize them // itself; without the lock this corrupts the index or races the branch. func TestGitSource_SerializesConcurrentChanges(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := sandbox(t, git) src, err := newGitSource(ctx, git, bare, "sandbox") @@ -293,7 +272,7 @@ func TestGitSource_SerializesConcurrentChanges(t *testing.T) { func TestNewChangeSource_GitRejectsAMissingSandbox(t *testing.T) { _, _, err := newChangeSource(context.Background(), config{ provider: providerGit, - git: testGit(t), + git: gitexectest.Git(t), sandboxDir: filepath.Join(t.TempDir(), "nothing-here"), }) require.Error(t, err, "a missing sandbox must say so rather than fail later mid-run") diff --git a/test/e2e/submitqueue/BUILD.bazel b/test/e2e/submitqueue/BUILD.bazel index 73bed50c..6097a9ab 100644 --- a/test/e2e/submitqueue/BUILD.bazel +++ b/test/e2e/submitqueue/BUILD.bazel @@ -43,6 +43,7 @@ go_test( "//platform/extension/consumergate:go_default_library", "//platform/extension/consumergate/file:go_default_library", "//platform/extension/messagequeue/mysql:go_default_library", + "//platform/gitexec/gitexectest:go_default_library", "//platform/publish:go_default_library", "//submitqueue/core/batch:go_default_library", "//submitqueue/core/topickey:go_default_library", diff --git a/test/e2e/submitqueue/git_suite_test.go b/test/e2e/submitqueue/git_suite_test.go index dd508476..094a9838 100644 --- a/test/e2e/submitqueue/git_suite_test.go +++ b/test/e2e/submitqueue/git_suite_test.go @@ -44,6 +44,7 @@ import ( changepb "github.com/uber/submitqueue/api/base/change/protopb" mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb" gatewaypb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" + "github.com/uber/submitqueue/platform/gitexec/gitexectest" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/test/testutil" "google.golang.org/grpc" @@ -85,7 +86,7 @@ func (s *GitMergeSuite) SetupSuite() { s.ctx = context.Background() s.log = testutil.NewTestLogger(t) - s.git = pinnedGit(t) + s.git = gitexectest.Git(t) containerUser := dockerContainerUser(t) t.Setenv("SQ_CONTAINER_USER", containerUser) t.Setenv("SQ_CONSUMER_GATE_DIR", t.TempDir()) @@ -307,42 +308,6 @@ func (s *GitMergeSuite) stageProviderConfig() string { return staged } -// pinnedGit resolves the git build the test target supplies, rather than -// whatever git the host has. The assertions here depend on repository -// mechanics, and an ambient git brings ambient configuration — hooks, signing, -// templates — with it. -// -// rules_go expands $(location) to an execroot-relative path, which for an -// external output has to be re-rooted under the runfiles directory the test -// runs from. -func pinnedGit(t *testing.T) string { - t.Helper() - path := os.Getenv("SUBMITQUEUE_TEST_GIT") - require.NotEmpty(t, path, "SUBMITQUEUE_TEST_GIT must be set by the test target") - - if absolute, err := filepath.Abs(path); err == nil { - if _, err := os.Stat(absolute); err == nil { - return absolute - } - } - - slashed := filepath.ToSlash(path) - external := "" - if strings.HasPrefix(slashed, "external/") { - external = strings.TrimPrefix(slashed, "external/") - } else if i := strings.Index(slashed, "/external/"); i >= 0 { - external = slashed[i+len("/external/"):] - } - require.NotEmpty(t, external, "SUBMITQUEUE_TEST_GIT=%q is not a runfile", path) - - root := os.Getenv("TEST_SRCDIR") - require.NotEmpty(t, root) - candidate := filepath.Join(root, filepath.FromSlash(external)) - _, err := os.Stat(candidate) - require.NoError(t, err) - return candidate -} - // seedRepository creates the bare repository Runway merges into, plus a working // clone the test authors changes in. func (s *GitMergeSuite) seedRepository() { diff --git a/tool/gitsandbox/BUILD.bazel b/tool/gitsandbox/BUILD.bazel index d01f72c7..f33138dc 100644 --- a/tool/gitsandbox/BUILD.bazel +++ b/tool/gitsandbox/BUILD.bazel @@ -32,6 +32,7 @@ go_test( }, deps = [ "//platform/gitexec:go_default_library", + "//platform/gitexec/gitexectest:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", ], diff --git a/tool/gitsandbox/main_test.go b/tool/gitsandbox/main_test.go index e3ce8a80..b9a762a3 100644 --- a/tool/gitsandbox/main_test.go +++ b/tool/gitsandbox/main_test.go @@ -24,40 +24,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/submitqueue/platform/gitexec" + "github.com/uber/submitqueue/platform/gitexec/gitexectest" ) -// testGit resolves the pinned git the test target supplies, so these assertions -// exercise the same build the sandbox is seeded with rather than the host's. -// -// rules_go expands $(location) to an execroot-relative path, which for an -// external output has to be re-rooted under the runfiles directory a test runs -// from — the same re-rooting the git E2E does. Under `bazel run` the binary -// needs none of this, because it already runs from the runfiles tree. -func testGit(t *testing.T) string { - t.Helper() - - supplied := os.Getenv("SUBMITQUEUE_TEST_GIT") - require.NotEmpty(t, supplied, "the test target must supply SUBMITQUEUE_TEST_GIT") - - if git, err := gitexec.Resolve(supplied); err == nil { - return git - } - - slashed := filepath.ToSlash(supplied) - index := strings.Index(slashed, "/external/") - require.GreaterOrEqual(t, index, 0, "SUBMITQUEUE_TEST_GIT=%q is not a runfile", supplied) - external := slashed[index+len("/external/"):] - - root := os.Getenv("TEST_SRCDIR") - require.NotEmpty(t, root) - - git, err := gitexec.Resolve(filepath.Join(root, filepath.FromSlash(external))) - require.NoError(t, err, "the test target must supply a git binary") - return git -} - func TestProvision_SeedsABareRepositoryOnTheTargetBranch(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := filepath.Join(t.TempDir(), "sandbox.git") @@ -79,7 +50,7 @@ func TestProvision_SeedsABareRepositoryOnTheTargetBranch(t *testing.T) { } func TestProvision_LeavesAnExistingRepositoryAlone(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := filepath.Join(t.TempDir(), "sandbox.git") @@ -101,7 +72,7 @@ func TestProvision_LeavesAnExistingRepositoryAlone(t *testing.T) { // A restarted stack must not lose the history someone is looking at, which is // the whole reason provisioning is idempotent rather than a reset. func TestProvision_PreservesCommitsLandedAfterSeeding(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := filepath.Join(t.TempDir(), "sandbox.git") require.NoError(t, func() error { _, err := provision(ctx, git, bare, "main"); return err }()) @@ -124,7 +95,7 @@ func TestProvision_PreservesCommitsLandedAfterSeeding(t *testing.T) { } func TestProvision_HonorsTheBranchName(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := filepath.Join(t.TempDir(), "sandbox.git") @@ -138,7 +109,7 @@ func TestProvision_HonorsTheBranchName(t *testing.T) { // The reflog is how a reader confirms a stack landed in one push, and bare // repositories do not keep one unless asked. func TestProvision_EnablesTheReflog(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := filepath.Join(t.TempDir(), "sandbox.git") @@ -156,7 +127,7 @@ func TestProvision_EnablesTheReflog(t *testing.T) { // can catch the file mid-name and fail a land for no reason. Nothing to repack, // nothing to catch. func TestProvision_DisablesAutomaticHousekeeping(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := filepath.Join(t.TempDir(), "sandbox.git") @@ -178,7 +149,7 @@ func TestProvision_DisablesAutomaticHousekeeping(t *testing.T) { // so it has to gain them on the next start instead of staying broken until // someone deletes it. func TestProvision_AppliesSettingsToAnExistingSandbox(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := filepath.Join(t.TempDir(), "sandbox.git") @@ -196,14 +167,14 @@ func TestProvision_AppliesSettingsToAnExistingSandbox(t *testing.T) { } func TestRun_RequiresASandboxDirectory(t *testing.T) { - err := run(context.Background(), testGit(t), "", "", "sandbox", "main") + err := run(context.Background(), gitexectest.Git(t), "", "", "sandbox", "main") require.Error(t, err) } func TestRun_CreatesTheCheckoutDirectory(t *testing.T) { checkout := filepath.Join(t.TempDir(), "checkouts") - err := run(context.Background(), testGit(t), t.TempDir(), checkout, "sandbox", "main") + err := run(context.Background(), gitexectest.Git(t), t.TempDir(), checkout, "sandbox", "main") require.NoError(t, err) info, err := os.Stat(checkout) @@ -218,7 +189,7 @@ func TestProvision_LeavesNothingBehindWhenCreationFails(t *testing.T) { bare := filepath.Join(t.TempDir(), "sandbox.git") // A branch name git refuses, so creation fails partway. - _, err := provision(ctx, testGit(t), bare, "refs/heads/") + _, err := provision(ctx, gitexectest.Git(t), bare, "refs/heads/") require.Error(t, err) _, statErr := os.Stat(bare) @@ -230,7 +201,7 @@ func TestProvision_LeavesNothingBehindWhenCreationFails(t *testing.T) { // The failure above must be recoverable: a corrected run provisions cleanly // rather than tripping over remnants of the one before it. func TestProvision_SucceedsAfterAFailedAttempt(t *testing.T) { - git := testGit(t) + git := gitexectest.Git(t) ctx := context.Background() bare := filepath.Join(t.TempDir(), "sandbox.git") From f5ca76e30d4e77fc32f5240c86df3cf48c888940 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Sun, 16 Aug 2026 13:45:06 -0700 Subject: [PATCH 2/4] feat(changeprovider): read change metadata from a git remote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? Every change provider so far asks a service what a change contains. GitHub and Phabricator both have an API that already knows; `fake` invents an answer; `routing` picks between the first two. A plain git remote has no such service, so there has been no way to run the queue against one and have it know what a change touched. That gap is visible in the demo's git rung, where the merge is real and everything upstream of it is not. Because the fake provider cannot read a repository, `make demo-requests` writes the paths it committed onto the change URI itself (`sq-files=…`) and the fake reads them back, so the conflict analyzer has something to key on. It works for changes the demo creates and for nothing else: a change pushed by hand carries no marker and conflicts with nothing. It is also why change URIs run into the 255-byte storage limit and had to be budgeted down to one path per directory. The queue's own logic — batching, conflict analysis, scoring — is built on what a change touched. Deriving that from git makes a plain remote a first-class source rather than a rung where those features are simulated. ### What? `submitqueue/extension/changeprovider/git` keeps its own copy of a remote and computes each change from the commits: `--numstat` for files and line counts, the commit itself for the author. **The baseline chains through a stack, and this is the part worth reviewing.** A `git://` URI names a commit and a ref and nothing else — unlike a pull request it carries no base, so the baseline has to be derived. It cannot be the target branch: a stack's changes are cut one from the next, so measuring each against the target reports the second change as containing the first, and anything summing line counts across a batch counts them twice. The first change is measured from where it diverged from the target, each one after it from where it diverged from its predecessor. The order of `Change.URIs` is the stack order and is load-bearing. This is wrong by default and fails silently — no error, just inflated numbers and scores that look slightly off — so it is the first thing the tests pin. **Authentication is injected, never derived.** The provider takes an `Auth` implementation and calls it before each fetch. It never reads an environment variable, never encodes a token, never decides what a credential is. `tokenEnv` in configuration is one implementation of that interface, supplied by the wiring layer; an integrator with a secrets manager or short-lived minted tokens supplies a different one and nothing in the extension changes. Calling it per fetch rather than once is what lets an expiring credential be refreshed. A nil `Auth` means the remote needs none, which covers a local path and an SSH remote served by the host's own SSH config and agent. The environment a fetch needs to reach a remote is passed through; the configuration that could change what a diff says is not. **The copy is bare and its own.** Nothing is ever checked out — the provider answers questions about commits and produces none — so there is no working tree to leave dirty and no index to corrupt. It is independent of any checkout a merger keeps, which is the point: each service configures its own remote for a queue, and a bind-mounted bare repository and `https://github.com/…` are the same code path. Provisioning runs at wiring time rather than on first use. Resolving a provider happens once per message on the validate path, so provisioning there would put a clone inside a retry loop and hide an unreachable remote behind queue processing rather than failing the service that owns the configuration. Nothing is wired to this yet — no queue selects `type: git`, and the orchestrator image still has no git binary. Those are the next steps, kept separate so this one is reviewable on its own. ## Test Plan Hermetic tests driving the pinned `@git//:git` against throwaway repositories — a bare "remote", a working clone that authors changes, and the provider reading through its own third copy, which is the same three-way arrangement the deployment has. - ✅ **three-step stack reports per change, not cumulatively** — `pkg/a` / `pkg/b` / `pkg/c` and 1 / 2 / 3 lines, each change carrying only its own - ✅ **mutation-tested that assertion**: reverting the baseline to always use the target fails it on exactly the two claims it exists for ("the second change must not carry the first's files", "nor the third the first two's") and nothing else fails — so it is not passing by construction - ✅ a multi-commit change reports every file across its commits, not just its tip - ✅ a binary file is reported as touched with no line counts, rather than dropped or refused — `--numstat` gives `-` for both counts, which a plain `Atoi` would fail on - ✅ a rename is reported at its new path, which is the case that splits one record across extra NUL-delimited fields and that a naive parse turns into an empty path - ✅ a commit the copy has not seen is fetched — the normal case, since the copy is its own - ✅ an unknown commit, a malformed URI, and a change sharing no history with the target are each errors; the last is deliberately not reported as a change that touches everything - ✅ four concurrent `Get`s against one copy return the right answer each, exercising the shared lock - ✅ re-provisioning keeps objects already fetched - ✅ `make fmt`, `make gazelle`, `make lint` The rename token layout was the one thing not taken from documentation: the test against a real renamed file is what pinned it. --- .../extension/changeprovider/git/BUILD.bazel | 43 ++ .../extension/changeprovider/git/README.md | 33 ++ .../extension/changeprovider/git/auth.go | 36 ++ .../extension/changeprovider/git/numstat.go | 92 +++++ .../extension/changeprovider/git/provider.go | 181 +++++++++ .../changeprovider/git/provider_test.go | 378 ++++++++++++++++++ .../extension/changeprovider/git/repo.go | 295 ++++++++++++++ 7 files changed, 1058 insertions(+) create mode 100644 submitqueue/extension/changeprovider/git/BUILD.bazel create mode 100644 submitqueue/extension/changeprovider/git/README.md create mode 100644 submitqueue/extension/changeprovider/git/auth.go create mode 100644 submitqueue/extension/changeprovider/git/numstat.go create mode 100644 submitqueue/extension/changeprovider/git/provider.go create mode 100644 submitqueue/extension/changeprovider/git/provider_test.go create mode 100644 submitqueue/extension/changeprovider/git/repo.go diff --git a/submitqueue/extension/changeprovider/git/BUILD.bazel b/submitqueue/extension/changeprovider/git/BUILD.bazel new file mode 100644 index 00000000..6d2e25c0 --- /dev/null +++ b/submitqueue/extension/changeprovider/git/BUILD.bazel @@ -0,0 +1,43 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "auth.go", + "numstat.go", + "provider.go", + "repo.go", + ], + importpath = "github.com/uber/submitqueue/submitqueue/extension/changeprovider/git", + visibility = ["//visibility:public"], + deps = [ + "//platform/base/change/git:go_default_library", + "//platform/metrics:go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/changeprovider:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + "@org_uber_go_zap//:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["provider_test.go"], + # The pinned git, so these assertions describe the build the services run + # rather than whatever the host happens to have. + data = ["@git"], + embed = [":go_default_library"], + env = { + "SUBMITQUEUE_TEST_GIT": "$(location @git//:git)", + }, + deps = [ + "//platform/base/change:go_default_library", + "//platform/gitexec/gitexectest:go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/changeprovider:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@com_github_uber_go_tally//:go_default_library", + "@org_uber_go_zap//:go_default_library", + ], +) diff --git a/submitqueue/extension/changeprovider/git/README.md b/submitqueue/extension/changeprovider/git/README.md new file mode 100644 index 00000000..e91baf1d --- /dev/null +++ b/submitqueue/extension/changeprovider/git/README.md @@ -0,0 +1,33 @@ +# Git change provider + +Reads change metadata — files, line counts, author — out of a git repository, for a remote that offers no API to ask. + +The GitHub and Phabricator providers query a service that already knows what a change contains. This one derives it. It keeps its own copy of the remote and computes each change from the commits themselves, which makes a plain git remote a first-class source of change metadata with nothing in front of it: an internal host, a mirror, or a bare repository on disk. + +## What a change is measured against + +A `git://` change URI names a commit and the ref it lives on, and nothing else. A pull request carries a base; this does not, so the baseline has to be derived — and for a stack it cannot be the target branch. + +A stack's changes are cut one from the next. Measuring every change against the target would report the second as containing the first, and any consumer that sums line counts across a batch would count them twice. So the first change in a request is measured from where it diverged from the target, and each one after it from where it diverged from the change before it. The order of the URIs is the stack order, and it is load-bearing. + +A change that shares no history with what it claims to land on is an error, not a change that touches nothing. + +## Its own copy + +Each service keeps its own copy of a queue's repository and configures its own remote for it, so this provider's copy is independent of anything a merger keeps. Where that copy fetches from is configuration: a bind-mounted bare repository and a remote host are the same code path, differing only in the URL. + +The copy is bare. Nothing here checks anything out — the provider answers questions about commits and never produces one — so there is no working tree to leave dirty and no index to corrupt. + +Git commands against one repository cannot safely interleave, so every provider sharing a copy shares its lock. + +Provisioning happens once, at wiring time, rather than on first use: resolving a provider happens per message on the validate path, so a copy created there would put a clone inside a retry loop and hide an unreachable remote behind queue processing instead of failing the service that owns the configuration. + +## Authentication + +The provider does not decide what a credential is. It takes an `Auth` implementation and calls it before each fetch; an integrator supplies one that reads an environment variable, calls a secrets manager, or mints a short-lived token, and only that implementation changes when the answer does. `Auth` is called per fetch rather than once so an expiring credential can be refreshed. + +A nil `Auth` means the remote needs none. That covers a local path, and an SSH remote served by the host's own SSH configuration and agent — the environment a fetch needs to reach a remote is passed through, while the configuration that could change what a diff says is not. + +## Tests + +Hermetic, against throwaway repositories, driving the Bazel-pinned git rather than the host's. The test that matters most is the three-step stack: reporting a stack cumulatively is wrong by default, never fails loudly, and shows up only as odd-looking scores. diff --git a/submitqueue/extension/changeprovider/git/auth.go b/submitqueue/extension/changeprovider/git/auth.go new file mode 100644 index 00000000..bb98f116 --- /dev/null +++ b/submitqueue/extension/changeprovider/git/auth.go @@ -0,0 +1,36 @@ +// 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 git + +import "context" + +// Auth prepares a local repository to authenticate to its remote. +// +// This provider never decides what a credential is, where it comes from, or how +// long it lives. An integrator wires an implementation in — reading an +// environment variable, calling a secrets manager, minting a short-lived token — +// and only that implementation changes when the answer does. +// +// Apply runs immediately before every fetch rather than once at provisioning, +// so an implementation backed by an expiring credential can refresh it. It must +// therefore be cheap and idempotent. +// +// A nil Auth means the remote needs none, which covers a local path and an SSH +// remote served by the host's own SSH configuration and agent. +type Auth interface { + // Apply configures repoPath so that git commands run against remoteURL from + // inside it can authenticate. + Apply(ctx context.Context, repoPath, remoteURL string) error +} diff --git a/submitqueue/extension/changeprovider/git/numstat.go b/submitqueue/extension/changeprovider/git/numstat.go new file mode 100644 index 00000000..2a1213ec --- /dev/null +++ b/submitqueue/extension/changeprovider/git/numstat.go @@ -0,0 +1,92 @@ +// 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 git + +import ( + "fmt" + "strconv" + "strings" + + "github.com/uber/submitqueue/submitqueue/entity" +) + +// parseNumstat reads the output of `git diff --numstat -z`. +// +// The NUL-delimited form is used rather than the line-based one because it is +// the only one that survives a path containing a newline, and because it is how +// a rename becomes unambiguous: a normal record is one NUL-terminated field +// holding "added\tdeleted\tpath", while a rename leaves the path empty and +// follows the record with the old and new paths as two further fields. +// +// A binary file reports "-" for both counts. Those are recorded as a changed +// path with no line counts, which is true — a binary has no lines — rather than +// dropped, since a path-keyed conflict analyzer still needs to see it. +func parseNumstat(out string) ([]entity.ChangedFile, error) { + fields := strings.Split(out, "\x00") + var files []entity.ChangedFile + + for i := 0; i < len(fields); i++ { + record := fields[i] + if record == "" { + continue + } + + parts := strings.SplitN(record, "\t", 3) + if len(parts) != 3 { + return nil, fmt.Errorf("unparseable numstat record %q", record) + } + + added, err := parseCount(parts[0]) + if err != nil { + return nil, fmt.Errorf("numstat record %q: %w", record, err) + } + deleted, err := parseCount(parts[1]) + if err != nil { + return nil, fmt.Errorf("numstat record %q: %w", record, err) + } + + path := parts[2] + if path == "" { + // A rename or copy. The two fields that follow are the old path and + // the new one; only the new one is kept, because a ChangedFile names + // one path and the new one is where the content now lives. + if i+2 >= len(fields) { + return nil, fmt.Errorf("numstat rename record %q is missing its paths", record) + } + path = fields[i+2] + i += 2 + } + + files = append(files, entity.ChangedFile{ + Path: path, + LinesAdded: added, + LinesDeleted: deleted, + }) + } + return files, nil +} + +// parseCount reads one side of a numstat record. "-" means the file is binary, +// which is reported as zero rather than refused. +func parseCount(field string) (int, error) { + if field == "-" { + return 0, nil + } + n, err := strconv.Atoi(field) + if err != nil { + return 0, fmt.Errorf("%q is not a line count: %w", field, err) + } + return n, nil +} diff --git a/submitqueue/extension/changeprovider/git/provider.go b/submitqueue/extension/changeprovider/git/provider.go new file mode 100644 index 00000000..be07be37 --- /dev/null +++ b/submitqueue/extension/changeprovider/git/provider.go @@ -0,0 +1,181 @@ +// 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 git provides a changeprovider.ChangeProvider that reads change +// metadata out of a git repository, for a remote that offers no API to ask. +// +// Where the GitHub and Phabricator providers query a service that already knows +// what a change contains, this one derives it: it keeps its own copy of the +// remote and computes each change's files, line counts and author from the +// commits themselves. That makes a plain git remote — an internal host, a +// mirror, a bare repository on disk — a first-class source of change metadata +// with no service in front of it. +// +// # What a change is measured against +// +// A git:// change URI names a commit and the ref it lives on, and nothing else. +// Unlike a pull request it carries no base, so the baseline has to be derived, +// and for a stack it cannot be the target branch: a stack's changes are cut one +// from the next, so measuring each against the target would report the second +// change as containing the first as well. Each change is therefore measured +// from where it diverged from the change before it, and only the first from the +// target. Callers get per-change numbers that sum, which is what any consumer +// aggregating over a batch depends on. +package git + +import ( + "context" + "fmt" + + "github.com/uber-go/tally" + "go.uber.org/zap" + + changegit "github.com/uber/submitqueue/platform/base/change/git" + coremetrics "github.com/uber/submitqueue/platform/metrics" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/changeprovider" +) + +const opName = "git_changeprovider" + +// Params carries what a provider needs. The Repo is built once per repository +// and shared by every queue reading it. +type Params struct { + Config changeprovider.Config + Repo *Repo + Logger *zap.SugaredLogger + MetricsScope tally.Scope +} + +// provider reads change metadata from a local copy of a git remote. +type provider struct { + cfg changeprovider.Config + repo *Repo + logger *zap.SugaredLogger + metricsScope tally.Scope +} + +// New returns a changeprovider.ChangeProvider reading from repo. +func New(params Params) changeprovider.ChangeProvider { + return &provider{ + cfg: params.Config, + repo: params.Repo, + logger: params.Logger.Named(opName), + metricsScope: params.MetricsScope.SubScope(opName), + } +} + +// Get returns one ChangeInfo per URI, in the order the URIs were given. +// +// The order is load-bearing: it is the stack order, and each change after the +// first is measured from the one before it. +func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity.ChangeInfo, retErr error) { + op := coremetrics.Begin(p.metricsScope, "get", coremetrics.LongLatencyBuckets) + defer func() { op.Complete(retErr) }() + + uris := request.Change.URIs + infos := make([]entity.ChangeInfo, 0, len(uris)) + + p.repo.mu.Lock() + defer p.repo.mu.Unlock() + + if err := p.repo.fetchTarget(ctx); err != nil { + coremetrics.NamedCounter(p.metricsScope, "get", "fetch_errors", 1) + return nil, fmt.Errorf("failed to update target branch %s: %w", p.repo.cfg.Target, err) + } + + previous := "" + for _, uri := range uris { + id, err := changegit.ParseChangeID(uri) + if err != nil { + return nil, fmt.Errorf("failed to parse change URI: %w", err) + } + if err := p.repo.ensureCommit(ctx, id.CommitSHA, id.Ref); err != nil { + coremetrics.NamedCounter(p.metricsScope, "get", "commit_unavailable", 1) + return nil, err + } + + // The first change stands on the target; each one after it stands on the + // change before it. + against := p.repo.cfg.Remote + "/" + p.repo.cfg.Target + if previous != "" { + against = previous + } + + details, err := p.describe(ctx, against, id.CommitSHA) + if err != nil { + return nil, fmt.Errorf("failed to describe change %s: %w", uri, err) + } + + infos = append(infos, entity.ChangeInfo{URI: uri, Details: details}) + previous = id.CommitSHA + } + return infos, nil +} + +// describe reports what sha changed relative to where it diverged from against. +func (p *provider) describe(ctx context.Context, against, sha string) (entity.ChangeDetails, error) { + base, err := p.repo.mergeBase(ctx, against, sha) + if err != nil { + return entity.ChangeDetails{}, err + } + + // -M so a rename reads as one moved file rather than a whole file deleted + // and another added; the scrubbed environment leaves git's own default off. + raw, err := p.repo.output(ctx, "diff", "--numstat", "-M", "-z", base, sha) + if err != nil { + return entity.ChangeDetails{}, err + } + files, err := parseNumstat(raw) + if err != nil { + return entity.ChangeDetails{}, err + } + + author, err := p.author(ctx, sha) + if err != nil { + return entity.ChangeDetails{}, err + } + return entity.ChangeDetails{Author: author, ChangedFiles: files}, nil +} + +// author reads the commit's author, NUL-separated because a display name can +// contain anything a friendlier separator would collide with. +func (p *provider) author(ctx context.Context, sha string) (entity.Author, error) { + out, err := p.repo.output(ctx, "show", "--no-patch", "--format=%an%x00%ae", sha) + if err != nil { + return entity.Author{}, err + } + name, email, found := cut(out) + if !found { + return entity.Author{}, fmt.Errorf("unreadable author for commit %s", sha) + } + return entity.Author{Name: name, Email: email}, nil +} + +// cut splits the author format's two fields, trimming the newline git appends. +func cut(out string) (name, email string, found bool) { + for i := 0; i < len(out); i++ { + if out[i] == 0 { + return out[:i], trimNewline(out[i+1:]), true + } + } + return "", "", false +} + +func trimNewline(s string) string { + for len(s) > 0 && (s[len(s)-1] == '\n' || s[len(s)-1] == '\r') { + s = s[:len(s)-1] + } + return s +} diff --git a/submitqueue/extension/changeprovider/git/provider_test.go b/submitqueue/extension/changeprovider/git/provider_test.go new file mode 100644 index 00000000..f973dbe1 --- /dev/null +++ b/submitqueue/extension/changeprovider/git/provider_test.go @@ -0,0 +1,378 @@ +// 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 git + +import ( + "context" + "fmt" + "net/url" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + "go.uber.org/zap" + + "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/platform/gitexec/gitexectest" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/changeprovider" +) + +// fixture is a bare "remote" plus a working clone the test authors changes in, +// and a provider reading through its own separate copy of that remote — the +// same three-way arrangement the real deployment has. +type fixture struct { + t *testing.T + git string + remote string + work string + provider changeprovider.ChangeProvider + repo *Repo +} + +func newFixture(t *testing.T) *fixture { + t.Helper() + ctx := context.Background() + git := gitexectest.Git(t) + + root := t.TempDir() + f := &fixture{ + t: t, + git: git, + remote: filepath.Join(root, "remote.git"), + work: filepath.Join(root, "work"), + } + + f.run("", "init", "--bare", "-b", "main", f.remote) + f.run("", "clone", f.remote, f.work) + f.run(f.work, "config", "user.name", "Test") + f.run(f.work, "config", "user.email", "test@example.invalid") + f.write("seed.txt", "seed\n") + f.run(f.work, "add", ".") + f.run(f.work, "commit", "-m", "seed") + f.run(f.work, "push", "origin", "main") + + repo, err := NewRepo(RepoConfig{ + Git: git, + Path: filepath.Join(root, "copy.git"), + RemoteURL: f.remote, + Target: "main", + }) + require.NoError(t, err) + require.NoError(t, repo.Provision(ctx)) + + f.repo = repo + f.provider = New(Params{ + Config: changeprovider.Config{QueueName: "q"}, + Repo: repo, + Logger: zap.NewNop().Sugar(), + MetricsScope: tally.NoopScope, + }) + return f +} + +func (f *fixture) run(dir string, args ...string) string { + f.t.Helper() + cmd := exec.Command(f.git, args...) + cmd.Dir = dir + cmd.Env = commandEnv() + out, err := cmd.CombinedOutput() + require.NoError(f.t, err, "git %s: %s", strings.Join(args, " "), out) + return strings.TrimSpace(string(out)) +} + +func (f *fixture) write(path, contents string) { + f.t.Helper() + full := filepath.Join(f.work, path) + require.NoError(f.t, os.MkdirAll(filepath.Dir(full), 0o755)) + require.NoError(f.t, os.WriteFile(full, []byte(contents), 0o644)) +} + +// push commits the given files on a branch cut from base and pushes it, +// returning the head SHA. +func (f *fixture) push(branch, base string, files map[string]string, message string) string { + f.t.Helper() + f.run(f.work, "checkout", "-B", branch, base) + for path, contents := range files { + f.write(path, contents) + } + f.run(f.work, "add", "-A") + f.run(f.work, "commit", "-m", message) + f.run(f.work, "push", "-f", "origin", branch) + return f.run(f.work, "rev-parse", "HEAD") +} + +func (f *fixture) mainSHA() string { + f.t.Helper() + return f.run(f.work, "rev-parse", "origin/main") +} + +// uri builds the change URI for a branch head, percent-encoding the ref the way +// a real caller must. +func (f *fixture) uri(branch, sha string) string { + return fmt.Sprintf("git://git.example.com/demo/%s/%s", + url.PathEscape("refs/heads/"+branch), sha) +} + +func (f *fixture) get(uris ...string) []entity.ChangeInfo { + f.t.Helper() + infos, err := f.provider.Get(context.Background(), entity.Request{ + Queue: "q", + Change: change.Change{URIs: uris}, + }) + require.NoError(f.t, err) + return infos +} + +// pathsOf reduces a result to file paths, which is what a path-keyed analyzer +// reads and the easiest thing to assert exactly. +func pathsOf(info entity.ChangeInfo) []string { + paths := make([]string, 0, len(info.Details.ChangedFiles)) + for _, f := range info.Details.ChangedFiles { + paths = append(paths, f.Path) + } + return paths +} + +func TestGet_ReportsFilesAndLineCounts(t *testing.T) { + f := newFixture(t) + head := f.push("feature/one", f.mainSHA(), map[string]string{ + "pkg/a/one.go": "one\ntwo\nthree\n", + }, "add one") + + infos := f.get(f.uri("feature/one", head)) + require.Len(t, infos, 1) + + require.Len(t, infos[0].Details.ChangedFiles, 1) + file := infos[0].Details.ChangedFiles[0] + assert.Equal(t, "pkg/a/one.go", file.Path) + assert.Equal(t, 3, file.LinesAdded) + assert.Equal(t, 0, file.LinesDeleted) + assert.Equal(t, "Test", infos[0].Details.Author.Name) + assert.Equal(t, "test@example.invalid", infos[0].Details.Author.Email) +} + +// TestGet_StackIsReportedPerChange is the assertion the whole baseline design +// exists for. A stack's branches are cut one from the next, so measuring every +// change against the target would report the second as containing the first — +// and a consumer summing line counts over a batch would count them twice. +func TestGet_StackIsReportedPerChange(t *testing.T) { + f := newFixture(t) + base := f.mainSHA() + + first := f.push("feature/s1", base, map[string]string{"pkg/a/one.go": "a\n"}, "first") + second := f.push("feature/s2", first, map[string]string{"pkg/b/two.go": "b\nb\n"}, "second") + third := f.push("feature/s3", second, map[string]string{"pkg/c/three.go": "c\nc\nc\n"}, "third") + + infos := f.get( + f.uri("feature/s1", first), + f.uri("feature/s2", second), + f.uri("feature/s3", third), + ) + require.Len(t, infos, 3) + + assert.Equal(t, []string{"pkg/a/one.go"}, pathsOf(infos[0])) + assert.Equal(t, []string{"pkg/b/two.go"}, pathsOf(infos[1]), + "the second change must not carry the first's files") + assert.Equal(t, []string{"pkg/c/three.go"}, pathsOf(infos[2]), + "nor the third the first two's") + + assert.Equal(t, 1, infos[0].Details.TotalLinesChanged()) + assert.Equal(t, 2, infos[1].Details.TotalLinesChanged()) + assert.Equal(t, 3, infos[2].Details.TotalLinesChanged()) +} + +func TestGet_MultiCommitChangeIsReportedWhole(t *testing.T) { + // A change is the whole branch against the target, not its tip commit — so a + // change built from several commits reports every file it touched. + f := newFixture(t) + base := f.mainSHA() + + f.run(f.work, "checkout", "-B", "feature/multi", base) + for _, name := range []string{"pkg/a/one.go", "pkg/a/two.go", "pkg/a/three.go"} { + f.write(name, "x\n") + f.run(f.work, "add", "-A") + f.run(f.work, "commit", "-m", "add "+name) + } + f.run(f.work, "push", "-f", "origin", "feature/multi") + head := f.run(f.work, "rev-parse", "HEAD") + + infos := f.get(f.uri("feature/multi", head)) + assert.ElementsMatch(t, + []string{"pkg/a/one.go", "pkg/a/two.go", "pkg/a/three.go"}, + pathsOf(infos[0])) +} + +func TestGet_BinaryFileIsReportedWithoutLineCounts(t *testing.T) { + f := newFixture(t) + head := f.push("feature/bin", f.mainSHA(), map[string]string{ + "assets/blob.bin": "\x00\x01\x02\x00binary\x00", + }, "add a binary") + + infos := f.get(f.uri("feature/bin", head)) + require.Len(t, infos[0].Details.ChangedFiles, 1) + + file := infos[0].Details.ChangedFiles[0] + assert.Equal(t, "assets/blob.bin", file.Path, "a binary must still be reported as touched") + assert.Equal(t, 0, file.LinesAdded) + assert.Equal(t, 0, file.LinesDeleted) +} + +// A rename is one record split across extra NUL-delimited fields, which is the +// case a naive parse silently mangles into a path of "". +func TestGet_RenameIsReportedAtItsNewPath(t *testing.T) { + f := newFixture(t) + + f.run(f.work, "checkout", "main") + f.write("pkg/a/original.go", strings.Repeat("keep\n", 20)) + f.run(f.work, "add", "-A") + f.run(f.work, "commit", "-m", "add original") + f.run(f.work, "push", "origin", "main") + + f.run(f.work, "checkout", "-B", "feature/rename", f.mainSHA()) + require.NoError(t, os.MkdirAll(filepath.Join(f.work, "pkg/b"), 0o755)) + f.run(f.work, "mv", "pkg/a/original.go", "pkg/b/moved.go") + f.run(f.work, "commit", "-m", "move it") + f.run(f.work, "push", "-f", "origin", "feature/rename") + head := f.run(f.work, "rev-parse", "HEAD") + + infos := f.get(f.uri("feature/rename", head)) + assert.Equal(t, []string{"pkg/b/moved.go"}, pathsOf(infos[0])) +} + +// The provider's copy is its own, so a change pushed after the copy was made is +// not there until it fetches — the normal case, not an edge one. +func TestGet_FetchesACommitItHasNotSeen(t *testing.T) { + f := newFixture(t) + head := f.push("feature/later", f.mainSHA(), map[string]string{"late.txt": "late\n"}, "later") + + require.False(t, f.repo.hasCommit(context.Background(), head), + "the fixture must start without the commit for this to prove anything") + + infos := f.get(f.uri("feature/later", head)) + assert.Equal(t, []string{"late.txt"}, pathsOf(infos[0])) +} + +func TestGet_UnknownCommitIsAnError(t *testing.T) { + f := newFixture(t) + missing := "0123456789abcdef0123456789abcdef01234567" + + _, err := f.provider.Get(context.Background(), entity.Request{ + Queue: "q", + Change: change.Change{URIs: []string{f.uri("feature/nope", missing)}}, + }) + require.Error(t, err) +} + +func TestGet_MalformedURIIsAnError(t *testing.T) { + f := newFixture(t) + + _, err := f.provider.Get(context.Background(), entity.Request{ + Queue: "q", + Change: change.Change{URIs: []string{"not-a-change-uri"}}, + }) + require.Error(t, err) +} + +// A change sharing no history with the target is reported as an error rather +// than as a change that happens to touch everything. +func TestGet_UnrelatedHistoryIsAnError(t *testing.T) { + f := newFixture(t) + + orphan := filepath.Join(t.TempDir(), "orphan") + f.run("", "clone", f.remote, orphan) + f.run(orphan, "config", "user.name", "Test") + f.run(orphan, "config", "user.email", "test@example.invalid") + f.run(orphan, "checkout", "--orphan", "feature/orphan") + f.run(orphan, "rm", "-rf", ".") + require.NoError(t, os.WriteFile(filepath.Join(orphan, "alone.txt"), []byte("alone\n"), 0o644)) + f.run(orphan, "add", "-A") + f.run(orphan, "commit", "-m", "unrelated") + f.run(orphan, "push", "-f", "origin", "feature/orphan") + head := f.run(orphan, "rev-parse", "HEAD") + + _, err := f.provider.Get(context.Background(), entity.Request{ + Queue: "q", + Change: change.Change{URIs: []string{f.uri("feature/orphan", head)}}, + }) + require.Error(t, err) +} + +// Two queues sharing a repository share its lock, so concurrent reads must not +// interleave git commands against one directory. +func TestGet_ConcurrentReadsAreSerialized(t *testing.T) { + f := newFixture(t) + base := f.mainSHA() + + heads := make([]string, 4) + for i := range heads { + heads[i] = f.push(fmt.Sprintf("feature/c%d", i), base, + map[string]string{fmt.Sprintf("pkg/c%d/f.go", i): "x\n"}, fmt.Sprintf("c%d", i)) + } + + var wg sync.WaitGroup + results := make([][]entity.ChangeInfo, len(heads)) + errs := make([]error, len(heads)) + for i, head := range heads { + wg.Add(1) + go func(i int, head string) { + defer wg.Done() + results[i], errs[i] = f.provider.Get(context.Background(), entity.Request{ + Queue: "q", + Change: change.Change{URIs: []string{f.uri(fmt.Sprintf("feature/c%d", i), head)}}, + }) + }(i, head) + } + wg.Wait() + + for i := range heads { + require.NoError(t, errs[i]) + assert.Equal(t, []string{fmt.Sprintf("pkg/c%d/f.go", i)}, pathsOf(results[i][0])) + } +} + +func TestProvision_IsIdempotentAndKeepsWhatItFetched(t *testing.T) { + f := newFixture(t) + ctx := context.Background() + head := f.push("feature/keep", f.mainSHA(), map[string]string{"keep.txt": "keep\n"}, "keep") + f.get(f.uri("feature/keep", head)) + + require.NoError(t, f.repo.Provision(ctx)) + assert.True(t, f.repo.hasCommit(ctx, head), + "re-provisioning must not discard objects already fetched") +} + +func TestNewRepo_RejectsAnIncompleteConfiguration(t *testing.T) { + git := gitexectest.Git(t) + for _, tt := range []struct { + name string + cfg RepoConfig + }{ + {name: "no path", cfg: RepoConfig{Git: git, RemoteURL: "u", Target: "main"}}, + {name: "no remote url", cfg: RepoConfig{Git: git, Path: "p", Target: "main"}}, + {name: "no target", cfg: RepoConfig{Git: git, Path: "p", RemoteURL: "u"}}, + } { + t.Run(tt.name, func(t *testing.T) { + _, err := NewRepo(tt.cfg) + require.Error(t, err) + }) + } +} diff --git a/submitqueue/extension/changeprovider/git/repo.go b/submitqueue/extension/changeprovider/git/repo.go new file mode 100644 index 00000000..06bbb723 --- /dev/null +++ b/submitqueue/extension/changeprovider/git/repo.go @@ -0,0 +1,295 @@ +// 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 git + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" +) + +// RepoConfig describes one local copy of a remote. +type RepoConfig struct { + // Git is the path to the git binary. Empty resolves through GIT_EXECUTABLE + // and then PATH. + Git string + // Path is where this service keeps its own copy. It belongs to this service + // alone: another service reading the same remote keeps its own. + Path string + // RemoteURL is where the copy fetches from — a URL or a local path. + RemoteURL string + // Remote is the name the copy records RemoteURL under. + Remote string + // Target is the branch a change's diff is measured against. + Target string + // Auth prepares the copy to reach RemoteURL. Nil when it needs nothing. + Auth Auth +} + +// Repo is one local copy of a remote, shared by every provider built over it. +// +// Bare, because nothing here checks anything out: the copy answers questions +// about commits and never produces one. That also means no index and no working +// tree to leave dirty between operations. +type Repo struct { + // mu serializes access. Git commands against one repository cannot safely + // interleave, and every provider sharing this copy shares this lock. + mu sync.Mutex + cfg RepoConfig +} + +// NewRepo returns a Repo for cfg, resolving the git binary. It touches no disk; +// Provision does that. +func NewRepo(cfg RepoConfig) (*Repo, error) { + if cfg.Path == "" { + return nil, fmt.Errorf("git change provider: a repository path is required") + } + if cfg.RemoteURL == "" { + return nil, fmt.Errorf("git change provider: a remote URL is required") + } + if cfg.Target == "" { + return nil, fmt.Errorf("git change provider: a target branch is required") + } + if cfg.Remote == "" { + cfg.Remote = "origin" + } + + git, err := resolveGit(cfg.Git) + if err != nil { + return nil, err + } + cfg.Git = git + return &Repo{cfg: cfg}, nil +} + +// Provision creates the copy if it is not already there and points it at the +// remote, leaving an existing copy's objects alone. +// +// Callers run this at wiring time rather than on first use: resolving a +// provider happens once per message on the validate path, so a copy created +// there would put a clone inside a retry loop and hide a bad remote behind +// queue processing rather than failing the service that owns it. +func (r *Repo) Provision(ctx context.Context) error { + r.mu.Lock() + defer r.mu.Unlock() + + if err := os.MkdirAll(r.cfg.Path, 0o755); err != nil { + return fmt.Errorf("could not create repository directory %q: %w", r.cfg.Path, err) + } + // HEAD is written by `git init` before anything else, so its presence marks + // a repository that exists. An existing one keeps whatever it has fetched. + if _, err := os.Stat(filepath.Join(r.cfg.Path, "HEAD")); err != nil { + if _, err := r.run(ctx, "init", "--bare", "-b", r.cfg.Target); err != nil { + // A half-initialized directory would be skipped as existing on the + // next run, so leave nothing rather than something unusable. + os.RemoveAll(r.cfg.Path) + return err + } + } + return r.configureRemote(ctx) +} + +// configureRemote records the remote, correcting it if the configuration +// changed since the copy was made. +func (r *Repo) configureRemote(ctx context.Context) error { + existing, err := r.run(ctx, "remote") + if err != nil { + return err + } + for _, name := range strings.Fields(existing) { + if name == r.cfg.Remote { + _, err := r.run(ctx, "remote", "set-url", r.cfg.Remote, r.cfg.RemoteURL) + return err + } + } + _, err = r.run(ctx, "remote", "add", r.cfg.Remote, r.cfg.RemoteURL) + return err +} + +// ensureCommit guarantees sha is present locally, fetching if it is not. +// +// By SHA first, which needs the server to allow a want for an object it does +// not advertise (github.com does); the change's own ref is the fallback for a +// server that does not. Neither is shallow — a merge base needs ancestry. +func (r *Repo) ensureCommit(ctx context.Context, sha, ref string) error { + if r.hasCommit(ctx, sha) { + return nil + } + if err := r.applyAuth(ctx); err != nil { + return err + } + + if _, err := r.run(ctx, "fetch", r.cfg.Remote, sha); err == nil && r.hasCommit(ctx, sha) { + return nil + } + if ref != "" { + if _, err := r.run(ctx, "fetch", r.cfg.Remote, ref); err == nil && r.hasCommit(ctx, sha) { + return nil + } + } + + // Separate "the remote cannot be reached" from "the remote does not have + // this commit". Only the first is worth trying again. + if _, err := r.run(ctx, "ls-remote", "--exit-code", r.cfg.Remote, "HEAD"); err != nil { + return fmt.Errorf("remote %s unreachable while resolving commit %s: %w", r.cfg.Remote, sha, err) + } + return fmt.Errorf("commit %s is not available from remote %s (tried by SHA and via %q)", sha, r.cfg.Remote, ref) +} + +// fetchTarget updates the target branch, which is the baseline a change's first +// commit is measured from and moves as other changes land. +func (r *Repo) fetchTarget(ctx context.Context) error { + if err := r.applyAuth(ctx); err != nil { + return err + } + _, err := r.run(ctx, "fetch", r.cfg.Remote, r.cfg.Target) + return err +} + +func (r *Repo) applyAuth(ctx context.Context) error { + if r.cfg.Auth == nil { + return nil + } + return r.cfg.Auth.Apply(ctx, r.cfg.Path, r.cfg.RemoteURL) +} + +func (r *Repo) hasCommit(ctx context.Context, sha string) bool { + _, err := r.run(ctx, "cat-file", "-e", sha+"^{commit}") + return err == nil +} + +// mergeBase returns the commit two revisions diverged from. Absence of one is +// reported as an error rather than an empty diff: a change sharing no history +// with what it claims to land on is a fact worth surfacing, not a change that +// touches nothing. +func (r *Repo) mergeBase(ctx context.Context, a, b string) (string, error) { + base, err := r.run(ctx, "merge-base", a, b) + if err != nil { + return "", fmt.Errorf("%s and %s share no history: %w", a, b, err) + } + return base, nil +} + +// run executes git inside the copy. +// +// The environment is replaced rather than inherited, for the reason the merger +// records: ambient configuration — a hooks path, a commit template, a signing +// requirement — is exactly what makes a scripted git behave differently on two +// machines. What survives is what reaching a remote needs and what cannot +// change an answer: the SSH agent, TLS roots, and proxy settings. +func (r *Repo) run(ctx context.Context, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, r.cfg.Git, args...) + cmd.Dir = r.cfg.Path + cmd.Env = commandEnv() + + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + message := strings.TrimSpace(stderr.String()) + if message == "" { + message = err.Error() + } + return "", fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + } + return strings.TrimSpace(string(out)), nil +} + +// output runs git and returns stdout untrimmed, for commands whose output is +// NUL-delimited and whose trailing separator is part of the format. +func (r *Repo) output(ctx context.Context, args ...string) (string, error) { + cmd := exec.CommandContext(ctx, r.cfg.Git, args...) + cmd.Dir = r.cfg.Path + cmd.Env = commandEnv() + + var stderr strings.Builder + cmd.Stderr = &stderr + out, err := cmd.Output() + if err != nil { + message := strings.TrimSpace(stderr.String()) + if message == "" { + message = err.Error() + } + return "", fmt.Errorf("git %s: %s", strings.Join(args, " "), message) + } + return string(out), nil +} + +// scrubbedEnv is the configuration-denying half of a git invocation. +var scrubbedEnv = []string{ + "GIT_CONFIG_NOSYSTEM=1", + "GIT_CONFIG_GLOBAL=" + os.DevNull, + "GIT_ATTR_NOSYSTEM=1", + "GIT_TERMINAL_PROMPT=0", + "GIT_PAGER=cat", + "GIT_EDITOR=:", +} + +// transportEnvNames are inherited when set. None can change what a diff says; +// all of them decide whether a remote can be reached at all. +var transportEnvNames = []string{ + "SSH_AUTH_SOCK", + "SSH_AGENT_PID", + "PATH", + "HOME", + "GIT_SSH", + "GIT_SSH_COMMAND", + "GIT_SSH_VARIANT", + "GIT_SSL_CAINFO", + "GIT_SSL_CAPATH", + "SSL_CERT_DIR", + "SSL_CERT_FILE", + "HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY", + "http_proxy", "https_proxy", "no_proxy", +} + +func commandEnv() []string { + env := make([]string, 0, len(scrubbedEnv)+len(transportEnvNames)) + env = append(env, scrubbedEnv...) + for _, name := range transportEnvNames { + if value, ok := os.LookupEnv(name); ok { + env = append(env, name+"="+value) + } + } + return env +} + +// resolveGit locates the git binary, preferring an explicit path, then +// GIT_EXECUTABLE, then PATH — the convention the rest of the repository uses. +func resolveGit(path string) (string, error) { + candidate := strings.TrimSpace(path) + if candidate == "" { + candidate = strings.TrimSpace(os.Getenv("GIT_EXECUTABLE")) + } + if candidate == "" { + found, err := exec.LookPath("git") + if err != nil { + return "", fmt.Errorf("git change provider: no git binary found: %w", err) + } + candidate = found + } + absolute, err := filepath.Abs(candidate) + if err != nil { + return "", fmt.Errorf("git change provider: %q is not a usable path: %w", candidate, err) + } + if info, err := os.Stat(absolute); err != nil || info.IsDir() { + return "", fmt.Errorf("git change provider: %q is not an executable file", absolute) + } + return absolute, nil +} From 8bdaeb038ae70018aae4767f55bc2687589013ce Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Sun, 16 Aug 2026 13:52:06 -0700 Subject: [PATCH 3/4] feat(orchestrator): let a queue select the git change provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The previous change added a change provider that reads a git repository. Nothing could select it: `profiles.yaml` had no `type: git`, and the wiring had no case for one. This connects the two. ### What? `changeProvider: {type: git, git: {…}}` in `profiles.yaml`, nested under a named block like `github` and `phabricator` are, carrying what a copy of a repository needs: `remoteUrl`, `target`, `repoPath`, and optionally `tokenEnv`/`tokenUser`. `remote` defaults to `origin`, `tokenUser` to `x-access-token`. Three values are required because none has a default that could be right: there is no public git remote to fall back on, no universal trunk name, and nowhere obvious to keep a copy. The block deliberately reads like Runway's merger block — each service says for itself where its copy fetches from, so a queue's provider and its merger can name the same remote while being configured independently. **Two queues may share a `repoPath`, and should when they are on the same repository** — that is how they come to share one copy and one lock. Sharing a path while disagreeing about the remote or target is rejected at startup, because whichever queue was built first would silently decide what the other one reads. **The default `Auth` lives here, not in the extension.** `tokenAuth` reads a named environment variable and writes git an `http.extraheader` fragment inside the repository, mode 0600, included from its config — never into the remote URL, which git echoes back into error messages and from there into logs and dead-letter payloads. The extension only knows it has an `Auth` and calls it; a deployment that mints short-lived tokens or reads a secrets manager replaces this one file's worth of behaviour and changes nothing else. It is applied before each fetch rather than once, which is what lets an expiring credential be refreshed. A remote that needs no credential gets a nil `Auth` — a local path, or SSH served by the host's own configuration and agent. **Provisioning now fetches.** It runs at wiring time, and the reason given for that was to fail the misconfigured service rather than bury the failure in a queue's retry loop. Writing the test for it showed the claim was not yet true: initializing a directory and recording a remote succeeds whether or not the remote exists, so a wrong URL would have surfaced later, per message, as a validate failure. Provisioning fetches the target branch, which is the step that actually proves the remote is reachable and the credential works — and it warms the copy, so the first request does not pay for a clone. `newProfiles` takes a context so that provisioning can be cancelled: it reaches the network during startup, and a shutdown then should stop it rather than wait it out. Still nothing selects it — no demo queue is switched over and the orchestrator image has no git binary yet. Those are the next two steps. ## Test Plan - ✅ the seam test: configuration in, a provider out that reports `pkg/a/one.go` with 2 added lines from a repository the test built — which only passes if the config block, provisioning, the injected auth and the factory case all line up - ✅ an unreachable remote fails `newProfiles`. This is the test that found the gap above; before provisioning fetched, it passed startup and would have failed per message instead - ✅ defaults applied (`remote`, `tokenUser`) and each of the three required values rejected when missing - ✅ two queues sharing a `repoPath` with different remotes are rejected; two that agree are allowed, since sharing a copy is the point - ✅ `make test`, `make lint`, `make gazelle` The config block is nested under `git:` rather than flat as the plan sketched. Flat would have matched `merge.yaml` more literally, but this file's own grammar is a named block per provider, and consistency inside the file a reader is editing seemed worth more than symmetry with a file in another service. --- .../orchestrator/server/BUILD.bazel | 10 + .../orchestrator/server/changerepo.go | 110 ++++++++++ .../submitqueue/orchestrator/server/config.go | 94 ++++++++ .../orchestrator/server/config_test.go | 207 +++++++++++++++++- .../submitqueue/orchestrator/server/main.go | 2 +- .../orchestrator/server/profiles.go | 24 ++ .../extension/changeprovider/git/repo.go | 35 ++- 7 files changed, 475 insertions(+), 7 deletions(-) create mode 100644 service/submitqueue/orchestrator/server/changerepo.go diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index c11a1f67..2f923f2a 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -8,6 +8,7 @@ exports_files( go_library( name = "orchestrator_lib", srcs = [ + "changerepo.go", "config.go", "main.go", "profiles.go", @@ -36,6 +37,7 @@ go_library( "//submitqueue/extension/buildrunner/githubactions:go_default_library", "//submitqueue/extension/changeprovider:go_default_library", "//submitqueue/extension/changeprovider/fake:go_default_library", + "//submitqueue/extension/changeprovider/git:go_default_library", "//submitqueue/extension/changeprovider/github:go_default_library", "//submitqueue/extension/changeprovider/phabricator:go_default_library", "//submitqueue/extension/changeprovider/routing:go_default_library", @@ -101,8 +103,16 @@ go_test( "config_test.go", "profiles_test.go", ], + # The pinned git, for the tests that resolve a git change provider against a + # real repository. + data = ["@git"], embed = [":orchestrator_lib"], # keep + env = { + "SUBMITQUEUE_TEST_GIT": "$(location @git//:git)", + }, deps = [ + "//platform/base/change:go_default_library", + "//platform/gitexec/gitexectest:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/buildrunner:go_default_library", "//submitqueue/extension/changeprovider:go_default_library", diff --git a/service/submitqueue/orchestrator/server/changerepo.go b/service/submitqueue/orchestrator/server/changerepo.go new file mode 100644 index 00000000..10e2d130 --- /dev/null +++ b/service/submitqueue/orchestrator/server/changerepo.go @@ -0,0 +1,110 @@ +// 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 main + +import ( + "context" + "encoding/base64" + "fmt" + "os" + "path/filepath" + "strings" + + gitprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/git" +) + +// credentialFile holds the git configuration fragment carrying a token. It is +// written inside the repository and included from its config, so the token +// never appears in a remote URL — which git echoes back into error messages, +// and from there into logs and dead-letter payloads. +const credentialFile = "submitqueue-changeprovider-credentials.config" + +// tokenAuth is the default gitprovider.Auth: a credential read from an +// environment variable and presented to git as an HTTP header. +// +// It is deliberately here rather than in the extension. The extension takes an +// Auth and calls it; what a credential is and where it comes from is a +// deployment's business, so a deployment that mints short-lived tokens or +// reads a secrets manager supplies its own implementation instead of this one +// and changes nothing else. +type tokenAuth struct { + tokenEnv string + tokenUser string +} + +// Apply writes the credential fragment, or removes it when the remote needs +// none, so a repository that stops using a token stops carrying one. +// +// Called before every fetch rather than once, which is what lets a short-lived +// credential be refreshed — reading the variable again each time is the whole +// mechanism for that. +func (a tokenAuth) Apply(ctx context.Context, repoPath, remoteURL string) error { + path := filepath.Join(repoPath, credentialFile) + + if !isHTTPRemote(remoteURL) { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("could not remove stale credential %q: %w", path, err) + } + return nil + } + + token, ok := os.LookupEnv(a.tokenEnv) + if !ok || token == "" { + return fmt.Errorf("environment variable %q named by tokenEnv is not set", a.tokenEnv) + } + + basic := base64.StdEncoding.EncodeToString([]byte(a.tokenUser + ":" + token)) + fragment := fmt.Sprintf("[http %q]\n\textraheader = Authorization: Basic %s\n", remoteURL, basic) + if err := os.WriteFile(path, []byte(fragment), 0o600); err != nil { + return fmt.Errorf("could not write credential %q: %w", path, err) + } + + // include.path resolves relative to the config file holding it, so the bare + // filename lands beside it. A bare repository's config is at its root. + return gitprovider.SetConfig(ctx, repoPath, "include.path", credentialFile) +} + +func isHTTPRemote(remoteURL string) bool { + return strings.HasPrefix(remoteURL, "http://") || strings.HasPrefix(remoteURL, "https://") +} + +// newChangeRepo builds the local copy a git change provider reads through, and +// provisions it. +// +// Provisioning happens here, at wiring time, rather than on first use: a change +// provider is resolved once per message on the validate path, so a clone +// started there would sit inside a retry loop and report an unreachable remote +// as a queue processing failure instead of a service that is misconfigured. +func newChangeRepo(ctx context.Context, cfg gitProviderConfig) (*gitprovider.Repo, error) { + var auth gitprovider.Auth + if cfg.TokenEnv != "" { + auth = tokenAuth{tokenEnv: cfg.TokenEnv, tokenUser: cfg.TokenUser} + } + + repo, err := gitprovider.NewRepo(gitprovider.RepoConfig{ + Path: cfg.RepoPath, + RemoteURL: cfg.RemoteURL, + Remote: cfg.Remote, + Target: cfg.Target, + Auth: auth, + }) + if err != nil { + return nil, err + } + if err := repo.Provision(ctx); err != nil { + return nil, fmt.Errorf("could not provision change repository %q: %w", cfg.RepoPath, err) + } + return repo, nil +} diff --git a/service/submitqueue/orchestrator/server/config.go b/service/submitqueue/orchestrator/server/config.go index d27c2a77..94537452 100644 --- a/service/submitqueue/orchestrator/server/config.go +++ b/service/submitqueue/orchestrator/server/config.go @@ -25,6 +25,7 @@ import ( // Change provider types selectable from configuration. const ( changeProviderTypeFake = "fake" + changeProviderTypeGit = "git" changeProviderTypeGitHub = "github" changeProviderTypePhabricator = "phabricator" changeProviderTypeRouting = "routing" @@ -74,6 +75,10 @@ const defaultBuildBudget = 4 // Defaults for the provider integrations, matching each vendor's convention. const ( + // defaultGitRemote and defaultGitTokenUser match git's own convention and + // the username a forge expects a token to be presented under. + defaultGitRemote = "origin" + defaultGitTokenUser = "x-access-token" defaultGitHubTokenEnv = "GITHUB_TOKEN" defaultGitHubBaseURL = "https://api.github.com" defaultPhabTokenEnv = "PHAB_API_TOKEN" @@ -121,10 +126,37 @@ type queueProfileConfig struct { // dispatches between them on the change URI's scheme. type changeProviderConfig struct { Type string `yaml:"type"` + Git *gitProviderConfig `yaml:"git"` GitHub *githubProviderConfig `yaml:"github"` Phabricator *phabProviderConfig `yaml:"phabricator"` } +// gitProviderConfig configures the git change provider, which derives change +// metadata from a repository rather than asking a service for it. +// +// It mirrors Runway's merger block deliberately: each service keeps its own +// copy of a queue's repository and says for itself where that copy fetches +// from, so the two are configured independently even when they name the same +// remote. +type gitProviderConfig struct { + // RemoteURL is where this service's copy fetches from. A URL or a local + // path; a bind-mounted bare repository and a remote host differ only here. + RemoteURL string `yaml:"remoteUrl"` + // Remote is the name the copy records RemoteURL under. + Remote string `yaml:"remote"` + // Target is the branch a change's first commit is measured against. + Target string `yaml:"target"` + // RepoPath is where this service keeps its copy. It belongs to this service + // alone — another service reading the same remote keeps its own. + RepoPath string `yaml:"repoPath"` + // TokenEnv names the environment variable holding a credential for an + // http(s) remote. Empty means the remote needs none, which is the case for a + // local path and for SSH served by the host's own configuration. + TokenEnv string `yaml:"tokenEnv"` + // TokenUser is the username the credential is presented under. + TokenUser string `yaml:"tokenUser"` +} + // githubProviderConfig configures the GitHub change provider. type githubProviderConfig struct { // TokenEnv names the environment variable holding the API token. @@ -269,6 +301,41 @@ func (c *profilesConfig) normalizeAndValidate() error { } } } + return c.validateGitRepoPaths() +} + +// validateGitRepoPaths rejects two queues keeping their copies in one directory +// while disagreeing about what that copy is. +// +// A shared path is legitimate — two queues on the same repository should share +// one copy, and sharing it is what makes them share its lock. Sharing it with a +// different remote or target is not: whichever queue was built first silently +// decides what the other one reads. +func (c profilesConfig) validateGitRepoPaths() error { + type owner struct { + queue string + cfg gitProviderConfig + } + byPath := make(map[string]owner) + + for _, q := range c.Queues { + resolved := c.resolve(q) + if resolved.ChangeProvider.Type != changeProviderTypeGit || resolved.ChangeProvider.Git == nil { + continue + } + git := *resolved.ChangeProvider.Git + previous, seen := byPath[git.RepoPath] + if !seen { + byPath[git.RepoPath] = owner{queue: q.Name, cfg: git} + continue + } + if previous.cfg.RemoteURL != git.RemoteURL || previous.cfg.Target != git.Target { + return fmt.Errorf( + "queues %q and %q share repoPath %q but read different repositories (%s@%s vs %s@%s); give each its own path", + previous.queue, q.Name, git.RepoPath, + previous.cfg.RemoteURL, previous.cfg.Target, git.RemoteURL, git.Target) + } + } return nil } @@ -317,6 +384,8 @@ func (c *changeProviderConfig) normalizeAndValidate(where string) error { switch c.Type { case changeProviderTypeFake: return nil + case changeProviderTypeGit: + return c.ensureGit(where) case changeProviderTypeGitHub: c.ensureGitHub() case changeProviderTypePhabricator: @@ -343,6 +412,31 @@ func (c *changeProviderConfig) normalizeAndValidate(where string) error { return nil } +// ensureGit defaults and checks the git block. Three values have no sensible +// default and are required: there is no public git remote to fall back on, no +// universal trunk name, and nowhere obvious to keep a copy. +func (c *changeProviderConfig) ensureGit(where string) error { + if c.Git == nil { + c.Git = &gitProviderConfig{} + } + if c.Git.Remote == "" { + c.Git.Remote = defaultGitRemote + } + if c.Git.TokenUser == "" { + c.Git.TokenUser = defaultGitTokenUser + } + if c.Git.RemoteURL == "" { + return fmt.Errorf("%s: git change provider requires remoteUrl", where) + } + if c.Git.Target == "" { + return fmt.Errorf("%s: git change provider requires target", where) + } + if c.Git.RepoPath == "" { + return fmt.Errorf("%s: git change provider requires repoPath", where) + } + return nil +} + func (c *changeProviderConfig) ensureGitHub() { if c.GitHub == nil { c.GitHub = &githubProviderConfig{} diff --git a/service/submitqueue/orchestrator/server/config_test.go b/service/submitqueue/orchestrator/server/config_test.go index 7ce27917..b15ce8e6 100644 --- a/service/submitqueue/orchestrator/server/config_test.go +++ b/service/submitqueue/orchestrator/server/config_test.go @@ -17,8 +17,11 @@ package main import ( "context" "fmt" + "net/url" "os" + "os/exec" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -26,7 +29,10 @@ import ( "github.com/uber-go/tally" "go.uber.org/zap/zaptest" + "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/platform/gitexec/gitexectest" "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/changeprovider" "github.com/uber/submitqueue/submitqueue/extension/conflict" "github.com/uber/submitqueue/submitqueue/extension/speculation/speculator" ) @@ -264,7 +270,7 @@ func TestNewProfiles_ResolvesPerQueueAnalyzers(t *testing.T) { // Asserted on behavior rather than instance identity: the analyzers are // stateless value types, so what matters is that each queue got the type it // asked for. - profiles, err := newProfiles(zaptest.NewLogger(t), tally.NoopScope, nil, nil, defaultProfilesConfig()) + profiles, err := newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, defaultProfilesConfig()) require.NoError(t, err) tests := []struct { @@ -306,7 +312,7 @@ defaults: cfg, err := loadProfilesConfig(path) require.NoError(t, err) - _, err = newProfiles(zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) + _, err = newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) require.Error(t, err) } @@ -323,7 +329,7 @@ defaults: cfg, err := loadProfilesConfig(path) require.NoError(t, err) - profiles, err := newProfiles(zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) + profiles, err := newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) require.NoError(t, err) assert.NotNil(t, profiles.For("anything").ChangeProvider) } @@ -365,7 +371,7 @@ func keysOf(m map[string]scorerConfig) []string { func TestNewProfiles_ComposesASpeculatorPerQueue(t *testing.T) { // Speculation is only "on" if every queue resolves to a real speculator — // a nil one leaves the stage inert and nothing ever gets built. - profiles, err := newProfiles(zaptest.NewLogger(t), tally.NoopScope, nil, nil, defaultProfilesConfig()) + profiles, err := newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, defaultProfilesConfig()) require.NoError(t, err) for _, queue := range []string{"test-queue", "e2e-test-queue", "file-overlap-queue", "unlisted"} { @@ -395,7 +401,7 @@ queues: `) cfg, err := loadProfilesConfig(path) require.NoError(t, err) - profiles, err := newProfiles(zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) + profiles, err := newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) require.NoError(t, err) batches := make([]entity.Batch, 0, 8) @@ -425,6 +431,197 @@ queues: } } +func TestLoadProfilesConfig_GitChangeProvider(t *testing.T) { + t.Run("defaults what it can and keeps what is given", func(t *testing.T) { + path := writeProfiles(t, ` +defaults: + changeProvider: + type: git + git: + remoteUrl: file:///srv/git/sandbox.git + target: main + repoPath: /var/submitqueue/changerepos/demo +`) + cfg, err := loadProfilesConfig(path) + require.NoError(t, err) + + git := cfg.Defaults.ChangeProvider.Git + require.NotNil(t, git) + assert.Equal(t, "file:///srv/git/sandbox.git", git.RemoteURL) + assert.Equal(t, "main", git.Target) + assert.Equal(t, defaultGitRemote, git.Remote) + assert.Equal(t, defaultGitTokenUser, git.TokenUser) + }) + + // Each of the three has no default that could be right: there is no public + // git remote, no universal trunk name, and nowhere obvious to keep a copy. + for _, tt := range []struct { + name string + omitted string + yaml string + }{ + { + name: "remoteUrl", omitted: "remoteUrl", + yaml: "git:\n target: main\n repoPath: /var/repos/x", + }, + { + name: "target", omitted: "target", + yaml: "git:\n remoteUrl: file:///srv/git/x.git\n repoPath: /var/repos/x", + }, + { + name: "repoPath", omitted: "repoPath", + yaml: "git:\n remoteUrl: file:///srv/git/x.git\n target: main", + }, + } { + t.Run("rejects a missing "+tt.name, func(t *testing.T) { + path := writeProfiles(t, ` +defaults: + changeProvider: + type: git + `+tt.yaml+"\n") + _, err := loadProfilesConfig(path) + require.Error(t, err) + }) + } +} + +// Two queues on one repository should share a copy — that is how they share its +// lock. Sharing a directory while disagreeing about what is in it is the +// mistake: whichever queue is built first silently decides what the other reads. +func TestLoadProfilesConfig_RejectsQueuesSharingARepoPathWithDifferentRepositories(t *testing.T) { + shared := ` +defaults: + changeProvider: {type: fake} +queues: + - name: a + changeProvider: + type: git + git: + remoteUrl: file:///srv/git/one.git + target: main + repoPath: /var/repos/shared + - name: b + changeProvider: + type: git + git: + remoteUrl: file:///srv/git/two.git + target: main + repoPath: /var/repos/shared +` + _, err := loadProfilesConfig(writeProfiles(t, shared)) + require.Error(t, err) + + agreeing := ` +defaults: + changeProvider: {type: fake} +queues: + - name: a + changeProvider: + type: git + git: + remoteUrl: file:///srv/git/one.git + target: main + repoPath: /var/repos/shared + - name: b + changeProvider: + type: git + git: + remoteUrl: file:///srv/git/one.git + target: main + repoPath: /var/repos/shared +` + _, err = loadProfilesConfig(writeProfiles(t, agreeing)) + require.NoError(t, err, "queues that agree about the repository may share a copy") +} + +// TestNewProfiles_GitChangeProviderReadsARealRepository is the seam test for +// the wiring: configuration in, a provider out that answers from a repository. +// Everything between — provisioning, the injected auth, the factory case — only +// works if all of it lines up. +func TestNewProfiles_GitChangeProviderReadsARealRepository(t *testing.T) { + git := gitexectest.Git(t) + root := t.TempDir() + remote := filepath.Join(root, "remote.git") + work := filepath.Join(root, "work") + + run := func(dir string, args ...string) string { + t.Helper() + cmd := exec.Command(git, args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, out) + return strings.TrimSpace(string(out)) + } + + run("", "init", "--bare", "-b", "main", remote) + run("", "clone", remote, work) + run(work, "config", "user.name", "Test") + run(work, "config", "user.email", "test@example.invalid") + require.NoError(t, os.WriteFile(filepath.Join(work, "seed.txt"), []byte("seed\n"), 0o644)) + run(work, "add", ".") + run(work, "commit", "-m", "seed") + run(work, "push", "origin", "main") + + run(work, "checkout", "-B", "feature/x", "main") + require.NoError(t, os.MkdirAll(filepath.Join(work, "pkg/a"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(work, "pkg/a/one.go"), []byte("a\nb\n"), 0o644)) + run(work, "add", "-A") + run(work, "commit", "-m", "change") + run(work, "push", "origin", "feature/x") + head := run(work, "rev-parse", "HEAD") + + path := writeProfiles(t, fmt.Sprintf(` +defaults: + changeProvider: + type: git + git: + remoteUrl: %s + target: main + repoPath: %s +`, remote, filepath.Join(root, "copy.git"))) + + cfg, err := loadProfilesConfig(path) + require.NoError(t, err) + profiles, err := newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) + require.NoError(t, err) + + provider, err := profiles.ChangeProviderFactory().For(changeprovider.Config{QueueName: "q"}) + require.NoError(t, err) + + uri := fmt.Sprintf("git://git.example.com/demo/%s/%s", url.PathEscape("refs/heads/feature/x"), head) + infos, err := provider.Get(context.Background(), entity.Request{ + Queue: "q", + Change: change.Change{URIs: []string{uri}}, + }) + require.NoError(t, err) + + require.Len(t, infos, 1) + require.Len(t, infos[0].Details.ChangedFiles, 1) + assert.Equal(t, "pkg/a/one.go", infos[0].Details.ChangedFiles[0].Path, + "the wired provider must report what the repository actually says") + assert.Equal(t, 2, infos[0].Details.ChangedFiles[0].LinesAdded) +} + +func TestNewProfiles_GitChangeProviderFailsOnAnUnreachableRemote(t *testing.T) { + // Provisioning happens at wiring time precisely so this is a startup + // failure, not something discovered inside a queue's retry loop. + path := writeProfiles(t, fmt.Sprintf(` +defaults: + changeProvider: + type: git + git: + remoteUrl: %s + target: main + repoPath: %s +`, filepath.Join(t.TempDir(), "does-not-exist.git"), filepath.Join(t.TempDir(), "copy.git"))) + + cfg, err := loadProfilesConfig(path) + require.NoError(t, err) + + _, err = newProfiles(context.Background(), zaptest.NewLogger(t), tally.NoopScope, nil, nil, cfg) + require.Error(t, err) +} + func TestLoadProfilesConfig_RejectsBudgets(t *testing.T) { // A negative budget leaves sticky with no free slots forever, so a queue // would batch and then never build — indistinguishable from a stuck queue. diff --git a/service/submitqueue/orchestrator/server/main.go b/service/submitqueue/orchestrator/server/main.go index 8c4f91ff..25a74db6 100644 --- a/service/submitqueue/orchestrator/server/main.go +++ b/service/submitqueue/orchestrator/server/main.go @@ -181,7 +181,7 @@ func run() error { if err != nil { return fmt.Errorf("failed to load extension profiles: %w", err) } - profiles, err := newProfiles(logger, scope, changeset.New(storageFty), storageFty, profilesCfg) + profiles, err := newProfiles(ctx, logger, scope, changeset.New(storageFty), storageFty, profilesCfg) if err != nil { return fmt.Errorf("failed to build profiles: %w", err) } diff --git a/service/submitqueue/orchestrator/server/profiles.go b/service/submitqueue/orchestrator/server/profiles.go index 578fa758..cfeac341 100644 --- a/service/submitqueue/orchestrator/server/profiles.go +++ b/service/submitqueue/orchestrator/server/profiles.go @@ -36,6 +36,7 @@ import ( githubactionsrunner "github.com/uber/submitqueue/submitqueue/extension/buildrunner/githubactions" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" cpfake "github.com/uber/submitqueue/submitqueue/extension/changeprovider/fake" + gitprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/git" githubprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/github" phabprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/phabricator" routingprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/routing" @@ -186,6 +187,7 @@ func (f speculatorFunc) For(c speculator.Config) (speculator.Speculator, error) // which is what lets the same wiring serve both the hermetic test stack and a // live repository. func newProfiles( + ctx context.Context, logger *zap.Logger, scope tally.Scope, resolver changeset.Resolver, @@ -193,6 +195,7 @@ func newProfiles( cfg profilesConfig, ) (Profiles, error) { b := &profileBuilder{ + ctx: ctx, logger: logger, scope: scope, resolver: resolver, @@ -234,6 +237,10 @@ func newProfiles( // scopes — is built once here and captured by the factory. Several queues // pointing at one repository therefore still share a single HTTP client. type profileBuilder struct { + // ctx bounds work done while building a profile — provisioning a change + // provider's repository reaches the network, and a shutdown during startup + // should stop it rather than wait it out. + ctx context.Context logger *zap.Logger scope tally.Scope resolver changeset.Resolver @@ -374,6 +381,23 @@ func (b *profileBuilder) newChangeProviderFactory(cfg changeProviderConfig, wher return cpfake.New(c), nil }), nil + case changeProviderTypeGit: + // One copy per distinct configuration, provisioned now. Queues whose + // blocks are identical share it — and therefore share its lock — because + // reuse hands them the same factory. + repo, err := newChangeRepo(b.ctx, *cfg.Git) + if err != nil { + return nil, fmt.Errorf("%s: %w", where, err) + } + return changeProviderFunc(func(c changeprovider.Config) (changeprovider.ChangeProvider, error) { + return gitprovider.New(gitprovider.Params{ + Config: c, + Repo: repo, + Logger: b.logger.Sugar(), + MetricsScope: b.scope, + }), nil + }), nil + case changeProviderTypeGitHub: makeProvider, err := b.newGitHubChangeProvider(*cfg.GitHub, where) if err != nil { diff --git a/submitqueue/extension/changeprovider/git/repo.go b/submitqueue/extension/changeprovider/git/repo.go index 06bbb723..6652a3cc 100644 --- a/submitqueue/extension/changeprovider/git/repo.go +++ b/submitqueue/extension/changeprovider/git/repo.go @@ -102,7 +102,15 @@ func (r *Repo) Provision(ctx context.Context) error { return err } } - return r.configureRemote(ctx) + if err := r.configureRemote(ctx); err != nil { + return err + } + + // Fetch once here, which is what makes provisioning worth doing at startup + // at all: an unreachable remote, a wrong URL, or a credential that does not + // work fails the service that is misconfigured. Initializing a directory and + // recording a remote would succeed against a remote that does not exist. + return r.fetchTarget(ctx) } // configureRemote records the remote, correcting it if the configuration @@ -270,6 +278,31 @@ func commandEnv() []string { return env } +// SetConfig writes one local configuration value into the repository at path. +// +// Exported for an Auth implementation, which configures a repository from +// outside this package and would otherwise have to find and run git itself. +func SetConfig(ctx context.Context, path, key, value string) error { + git, err := resolveGit("") + if err != nil { + return err + } + cmd := exec.CommandContext(ctx, git, "config", key, value) + cmd.Dir = path + cmd.Env = commandEnv() + + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + message := strings.TrimSpace(stderr.String()) + if message == "" { + message = err.Error() + } + return fmt.Errorf("git config %s: %s", key, message) + } + return nil +} + // resolveGit locates the git binary, preferring an explicit path, then // GIT_EXECUTABLE, then PATH — the convention the rest of the repository uses. func resolveGit(path string) (string, error) { From df1616e5c6d2ed175c6c4662c968ecf790de642e Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Sun, 16 Aug 2026 14:01:57 -0700 Subject: [PATCH 4/4] feat(demo): read the git rung's change metadata from the repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The git rung merged for real and made up everything upstream of it. Because no change provider could read a plain git remote, `demo-queue` and `e2e-git-queue` both used the fake one, and `make demo-requests` compensated by writing the paths it had just committed onto the change URI (`sq-files=…`) for the fake to read back. That worked for changes the demo created and for nothing else. A branch pushed by hand carried no marker, so as far as the conflict analyzer could tell it touched nothing and conflicted with nothing — on the rung whose entire purpose is that the repository is real. It was also what pushed change URIs into the 255-byte storage limit and forced the paths to be budgeted down to one per directory. ### What? Both queues select the git change provider added in the previous two commits, and `gitSource` stops stating anything about what it touched. The orchestrator keeps its own copy of the same bare repository Runway merges into, points its own remote at it, and reads each change out of the commits. Each queue gets its own copy, because two copies at one path with different configuration would let whichever queue was built first decide what the other reads — which the config layer now rejects outright. **The orchestrator image gains git and a writable directory**, the same two things Runway's needed for the same reasons: git because the provider shells out to it, and `/var/submitqueue/changerepos` pre-created `0777` because Docker seeds a named volume from the image and the container's user is deployment-configurable, so a root-owned directory would leave a non-root service unable to provision. **The compose overlay mounts the sandbox into the orchestrator read-only** — it only ever fetches — plus a named volume for the copies. The volume rather than a bind mount for the reason Runway's checkout already is one: this is where git writes objects, and on macOS a freshly written loose object can read back as corrupt across the host filesystem bridge. The ladder table in the quickstart gains a "Read from" column. The rung's honesty was the point of the change and was not visible in the summary a reader skims. ## Test Plan Against a live `PROVIDER=git` stack, in order: - ✅ the container first, with the orchestrator still on `fake`: `git version 2.39.5`, `/srv/git` holding `sandbox.git`, and the repos directory `drwxrwxrwx` - ✅ **`FOLDERS=1`, six changes, no marker on any URI → a full dependency chain**, every batch depending on all the later ones. With `sq-files=` gone, the only way `pathoverlap` could see a shared directory is from paths the provider read out of the repository - ✅ **`FOLDERS=50`, five changes → no dependencies at all**, and they land in 3s rather than 10-14s. So it is genuinely keying on paths, not serializing everything - ✅ **the case that was impossible before**: two branches pushed by hand, carrying no marker, were described correctly — `{"author": {"name": "Hand", …}, "changed_files": [{"path": "shared/a.txt", "lines_added": 1, …}]}` read straight out of the `change` table - ✅ `make e2e-git-test` with both queues switched - ✅ `make test`, `make lint`, `make gazelle` **The risk flagged when planning this did not materialise.** `TestLand_ResubmittedAfterLanding_IsRejectedAsStale` asserts only that a resubmission errors, and the worry was that it would now error inside the provider — the head branch has moved, so the original SHA is no longer reachable from the ref — and pass while no longer testing staleness. Reproduced by hand: it still fails on `refs/heads/hand/one now points at 5b96c363…`, the staleness check. The provider's copy keeps the object it fetched the first time, so it resolves the commit locally and never refetches. Worth knowing about what the E2E does and does not buy: `e2e-git-queue` uses `analyzer: {type: none}`, so nothing there consumes the provider's output. Switching it proves the provider runs end to end without breaking a land; it does not check the metadata. That check is the `FOLDERS` runs above, which are hand-observed rather than asserted in CI. --- doc/howto/QUICKSTART.md | 14 +++++----- service/submitqueue/demo/provider/README.md | 6 +++-- .../demo/provider/git/profiles.yaml | 27 +++++++++++++++---- service/submitqueue/demo/requests/git.go | 12 +++++---- service/submitqueue/docker-compose.git.yml | 19 +++++++++++-- .../orchestrator/server/Dockerfile | 15 +++++++++-- 6 files changed, 71 insertions(+), 22 deletions(-) diff --git a/doc/howto/QUICKSTART.md b/doc/howto/QUICKSTART.md index 19859165..da49f489 100644 --- a/doc/howto/QUICKSTART.md +++ b/doc/howto/QUICKSTART.md @@ -4,15 +4,17 @@ Start the stack, put traffic through it, and watch changes land — beginning wi The stack always runs the same way. What changes is where the changes come from and what landing them does, chosen with `PROVIDER`: -| `PROVIDER` | A change is | Building it | Landing it | Needs | -|---|---|---|---|---| -| **`fake`** (default) | a URI, and nothing else | instant fake pass | reports success without touching a repository | nothing | -| **`git`** | a branch in a bare repository on disk | instant fake pass | a real fetch, cherry-pick and push | nothing | -| **`github`** | a real pull request | a real GitHub Actions run per batch | a real push to a real repository | a repository, a token, and CI minutes | +| `PROVIDER` | A change is | Read from | Building it | Landing it | Needs | +|---|---|---|---|---|---| +| **`fake`** (default) | a URI, and nothing else | the URI itself | instant fake pass | reports success without touching a repository | nothing | +| **`git`** | a branch in a bare repository on disk | the repository | instant fake pass | a real fetch, cherry-pick and push | nothing | +| **`github`** | a real pull request | GitHub's API | a real GitHub Actions run per batch | a real push to a real repository | a repository, a token, and CI minutes | They are a ladder, not alternatives: the same commands work on each rung, so you can start with the one that needs nothing and only pay for what you want to see next. Each is a directory of configuration under [`service/submitqueue/demo/provider/`](../../service/submitqueue/demo/provider) — the difference between rungs is two YAML files, not a code path. -The queue's own logic is real on every rung; what changes is how much of the world around it is. Two things are worth knowing before reading a `landed` as more than it is. On `fake` and `git` **the build is faked**, so `landed` means the pipeline ran, not that anything was tested. And on `fake` and `git` the change provider is faked too: it cannot read a repository to see what a change touched, so `make demo-requests` states the paths on the change URI itself (`sq-files=`) for the conflict analyzer to key on. A change submitted by hand on those rungs touches nothing as far as the analyzer can tell, and conflicts with nothing. +The queue's own logic is real on every rung; what changes is how much of the world around it is. The one thing to keep in mind before reading a `landed` as more than it is: on `fake` and `git` **the build is faked**, so it means the pipeline ran, not that anything was tested. + +"Read from" is what the queue knows about a change — which files it touches, how large it is — and it is what conflict analysis and scoring are computed from. Only `fake` invents it: a change there is a URI pointing at nothing, so `make demo-requests` states the paths on the URI itself (`sq-files=`) and the fake reads them back, which means a change submitted by hand on that rung conflicts with nothing. On `git` the orchestrator keeps its own copy of the repository and reads the commits, so a change pushed by anyone is described correctly. ## Start the stack diff --git a/service/submitqueue/demo/provider/README.md b/service/submitqueue/demo/provider/README.md index 679f0122..4ee0987d 100644 --- a/service/submitqueue/demo/provider/README.md +++ b/service/submitqueue/demo/provider/README.md @@ -14,10 +14,12 @@ Pick one with `make local-submitqueue-start PROVIDER=`, which bind-mounts | Directory | What it demonstrates | Needs | |---|---|---| | [`fake/`](fake) | the queue alone: a change is a URI, nothing merges anywhere — the default, and what the quickstart runs | nothing | -| [`git/`](git) | a plain git remote with no provider at all: real fetch, cherry-pick and push against a bare repository | nothing | +| [`git/`](git) | a plain git remote with no provider at all: change metadata read out of the repository, and a real fetch, cherry-pick and push against it | nothing | | [`github/`](github) | a live provider: GitHub change metadata, a real repository, pull requests marked merged | a repository and a token | -The three are a ladder, and the rung is the only thing that changes: the same commands land against all of them. `git/` is worth reading first of the two real ones. It is proof that the merge machinery has no provider in it — the same Runway code path lands changes against a bare repository addressed by path, with no credential and no API — and it is what the hermetic git E2E (`make e2e-git-test`) runs against. +The three are a ladder, and the rung is the only thing that changes: the same commands land against all of them. `git/` is worth reading first of the two real ones. It is proof that neither half needs a provider — change metadata comes from reading the repository and the merge is the same Runway code path, both against a bare repository addressed by path, with no credential and no API — and it is what the hermetic git E2E (`make e2e-git-test`) runs against. + +Note that the orchestrator and Runway each keep their **own** copy of a queue's repository and configure their own remote for it: the change provider's copy is in `profiles.yaml`, the merger's checkout in `merge.yaml`, and they are independent even when they name the same remote. That is the same shape a deployment has when both point at a remote host. ## Adding a provider diff --git a/service/submitqueue/demo/provider/git/profiles.yaml b/service/submitqueue/demo/provider/git/profiles.yaml index 1b4bf384..e47054cc 100644 --- a/service/submitqueue/demo/provider/git/profiles.yaml +++ b/service/submitqueue/demo/provider/git/profiles.yaml @@ -17,15 +17,32 @@ queues: # The queue `make demo-requests` and `make land` use by default. Serializes # batches that touch a shared directory, matching the github mode. # - # The commits here are real, but the change provider above is not, and it - # cannot read a repository to find out what they touched. `make demo-requests` - # states the paths it committed on the change URI (`sq-files=`) for the fake - # provider to report back. A change submitted by hand carries no such marker - # and so conflicts with nothing. + # Both halves are real here. The change provider keeps its own copy of the + # same bare repository Runway merges into and reads each change out of it, so + # the analyzer keys on paths that were actually committed — including for a + # change submitted by hand, which nothing else could have described. - name: demo-queue + changeProvider: + type: git + git: + # The orchestrator's own copy, and its own remote pointing at the + # sandbox — mounted read-only, since it only ever fetches. Runway + # configures the same repository separately for itself in merge.yaml. + remoteUrl: file:///srv/git/sandbox.git + target: main + repoPath: /var/submitqueue/changerepos/demo analyzer: {type: pathoverlap, by: directory} - name: e2e-git-queue + # Its own copy, separate from demo-queue's: the two agree about the + # repository but not about everything else, and a shared path would make + # whichever was built first decide for both. + changeProvider: + type: git + git: + remoteUrl: file:///srv/git/sandbox.git + target: main + repoPath: /var/submitqueue/changerepos/sandbox # Maximum parallelism: batches never conflict, so the test controls # ordering through what it lands rather than through the analyzer. analyzer: {type: none} diff --git a/service/submitqueue/demo/requests/git.go b/service/submitqueue/demo/requests/git.go index d8f76546..dec85959 100644 --- a/service/submitqueue/demo/requests/git.go +++ b/service/submitqueue/demo/requests/git.go @@ -136,13 +136,15 @@ func (s *gitSource) open(ctx context.Context, spec changeSpec) (openedChange, er return openedChange{ headSHA: headSHA, - // The commits are real, but the fake change provider is what the - // orchestrator asks about them, and it cannot read a repository — so the - // paths just committed are stated on the URI for it to report back. - uri: withFiles(gitchange.ChangeID{ + // Nothing is stated about what this change touches. The orchestrator + // keeps its own copy of this repository and reads that out of the + // commits, which is the whole difference between this rung and the fake + // one — and what makes a change pushed by hand behave the same as one + // from here. + uri: gitchange.ChangeID{ Scheme: "git", Remote: gitRemote, Repo: s.repo, Ref: "refs/heads/" + spec.branch, CommitSHA: headSHA, - }.String(), spec.files), + }.String(), // No pull request to number, so the branch names the change. Empty URL: // a branch in a bare repository has nothing to open. cell: client.Cell{Text: spec.branch}, diff --git a/service/submitqueue/docker-compose.git.yml b/service/submitqueue/docker-compose.git.yml index 9371f01f..e117575e 100644 --- a/service/submitqueue/docker-compose.git.yml +++ b/service/submitqueue/docker-compose.git.yml @@ -19,16 +19,30 @@ # extensions (service/submitqueue/demo/provider/git) # SQ_GIT_SANDBOX_DIR the bare repository the merger fetches and pushes # SQ_RUNWAY_CHECKOUT_DIR storage for the working trees the merger owns +# +# Optional: +# SQ_ORCHESTRATOR_REPO_DIR storage for the orchestrator's own repository +# copies; a named volume when unset services: orchestrator-service: environment: # Which change provider, build runner, and conflict analyzer each queue - # resolves to. Everything at the edges stays fake here; only the merge is - # real. + # resolves to. The build runner stays fake here; the change provider and + # the merge are real. - PROFILES_CONFIG_PATH=/etc/submitqueue/profiles.yaml volumes: - ${SQ_PROVIDER_CONFIG_DIR}:/etc/submitqueue:ro + # The same bare repository Runway merges into, read-only: the orchestrator + # only ever fetches from it. Each service keeps its own copy and points + # its own remote at this one, which is the arrangement a real deployment + # has with both of them pointing at a remote host. + - ${SQ_GIT_SANDBOX_DIR}:/srv/git:ro + # Where the orchestrator keeps its copies. A named volume rather than a + # bind mount for the same reason Runway's checkout is one: this is where + # git writes objects, and on macOS a freshly written loose object can read + # back as corrupt over the host filesystem bridge. + - ${SQ_ORCHESTRATOR_REPO_DIR:-orchestrator-changerepos}:/var/submitqueue/changerepos runway-service: environment: @@ -55,3 +69,4 @@ services: volumes: runway-checkouts: + orchestrator-changerepos: diff --git a/service/submitqueue/orchestrator/server/Dockerfile b/service/submitqueue/orchestrator/server/Dockerfile index ff844e9d..fdd59363 100644 --- a/service/submitqueue/orchestrator/server/Dockerfile +++ b/service/submitqueue/orchestrator/server/Dockerfile @@ -1,7 +1,18 @@ FROM debian:bookworm-slim -RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/* \ - && mkdir -p /app && chmod 0755 /app +# git is a runtime dependency, not a build one: the git change provider shells +# out to it to read what a change touched. Without it the service still starts, +# but a queue configured with that provider fails at startup instead. +RUN apt-get update && apt-get install -y ca-certificates git && rm -rf /var/lib/apt/lists/* \ + && mkdir -p /app && chmod 0755 /app \ + # The repository copies the git change provider owns. Created here so that a + # named volume mounted over it starts with a mode the service can write: + # Docker seeds an empty volume from the image, and the container's user is + # deployment-configurable (SQ_CONTAINER_USER), so a root-owned directory + # would leave a non-root service unable to provision. Group- and + # world-writable for that reason, which costs nothing on a path that exists + # to be mounted over. + && mkdir -p /var/submitqueue/changerepos && chmod 0777 /var/submitqueue/changerepos WORKDIR /app # Copy pre-built Linux binary