From fd75c070c13a0243846abc3e4c548a75a0cd2ea4 Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 17 Aug 2026 00:21:48 -0700 Subject: [PATCH] refactor(changeprovider): make the git provider pure logic over a contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the git transport plumbing — the bare local copy, fetch, commit resolution, merge base, the git command environment — and the Auth contract out of the change provider extension into platform/git/repo, built on platform/git/exec. The provider now depends on a small Repository interface it defines and holds no os/exec and no credential handling: it parses the change URI, picks the stack baseline, reads the diff and author, and shapes the result, nothing more. Auth moves with the copy it configures, resolving the review point that authentication did not belong in the change provider. The wiring's tokenAuth now implements gitrepo.Auth and SetConfig is gitrepo.SetConfig. Repository-plumbing tests move to platform/git/repo; the provider's behavior tests stay and drive a real gitrepo.Repo through the interface. --- platform/git/repo/BUILD.bazel | 30 +++ .../git => platform/git/repo}/auth.go | 4 +- .../git => platform/git/repo}/repo.go | 199 +++++++----------- platform/git/repo/repo_test.go | 116 ++++++++++ .../orchestrator/server/BUILD.bazel | 1 + .../orchestrator/server/changerepo.go | 16 +- .../extension/changeprovider/git/BUILD.bazel | 4 +- .../extension/changeprovider/git/README.md | 16 +- .../extension/changeprovider/git/provider.go | 43 ++-- .../changeprovider/git/provider_test.go | 41 +--- 10 files changed, 270 insertions(+), 200 deletions(-) create mode 100644 platform/git/repo/BUILD.bazel rename {submitqueue/extension/changeprovider/git => platform/git/repo}/auth.go (93%) rename {submitqueue/extension/changeprovider/git => platform/git/repo}/repo.go (56%) create mode 100644 platform/git/repo/repo_test.go diff --git a/platform/git/repo/BUILD.bazel b/platform/git/repo/BUILD.bazel new file mode 100644 index 00000000..e5478812 --- /dev/null +++ b/platform/git/repo/BUILD.bazel @@ -0,0 +1,30 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "auth.go", + "repo.go", + ], + importpath = "github.com/uber/submitqueue/platform/git/repo", + visibility = ["//visibility:public"], + deps = ["//platform/git/exec:go_default_library"], +) + +go_test( + name = "go_default_test", + srcs = ["repo_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/git/exec:go_default_library", + "//platform/git/exectest:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], +) diff --git a/submitqueue/extension/changeprovider/git/auth.go b/platform/git/repo/auth.go similarity index 93% rename from submitqueue/extension/changeprovider/git/auth.go rename to platform/git/repo/auth.go index bb98f116..d622de10 100644 --- a/submitqueue/extension/changeprovider/git/auth.go +++ b/platform/git/repo/auth.go @@ -12,13 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -package git +package gitrepo 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 +// This package 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. diff --git a/submitqueue/extension/changeprovider/git/repo.go b/platform/git/repo/repo.go similarity index 56% rename from submitqueue/extension/changeprovider/git/repo.go rename to platform/git/repo/repo.go index 6652a3cc..f0aa88f0 100644 --- a/submitqueue/extension/changeprovider/git/repo.go +++ b/platform/git/repo/repo.go @@ -12,7 +12,21 @@ // See the License for the specific language governing permissions and // limitations under the License. -package git +// Package gitrepo keeps a local, bare copy of a git remote and answers +// questions about its commits. +// +// It is transport plumbing, not domain logic: it fetches, resolves commits, +// and computes merge bases, but never decides what those facts mean. A reader +// that derives change metadata — files, line counts, author — drives a copy +// through this package and interprets the raw git output itself. +// +// The copy is bare because nothing here checks anything out, so there is no +// working tree to leave dirty and no index to corrupt. Git commands against one +// copy cannot safely interleave, so a Repo carries a lock (it embeds +// sync.Mutex) that every reader sharing the copy holds across a sequence of +// commands. Command environment and git-binary resolution come from +// platform/git/exec, the one source of truth every git caller shares. +package gitrepo import ( "context" @@ -22,6 +36,8 @@ import ( "path/filepath" "strings" "sync" + + gitexec "github.com/uber/submitqueue/platform/git/exec" ) // RepoConfig describes one local copy of a remote. @@ -29,8 +45,8 @@ 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 is where this copy lives on disk. It belongs to one owner: another + // reader of the same remote keeps its own. Path string // RemoteURL is where the copy fetches from — a URL or a local path. RemoteURL string @@ -42,15 +58,11 @@ type RepoConfig struct { 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. +// Repo is one local, bare copy of a remote, shared by every reader built over +// it. The embedded mutex serializes git commands against the copy; a reader +// holds it across any sequence that must see a consistent object set. 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 + sync.Mutex cfg RepoConfig } @@ -58,19 +70,19 @@ type Repo struct { // Provision does that. func NewRepo(cfg RepoConfig) (*Repo, error) { if cfg.Path == "" { - return nil, fmt.Errorf("git change provider: a repository path is required") + return nil, fmt.Errorf("gitrepo: a repository path is required") } if cfg.RemoteURL == "" { - return nil, fmt.Errorf("git change provider: a remote URL is required") + return nil, fmt.Errorf("gitrepo: a remote URL is required") } if cfg.Target == "" { - return nil, fmt.Errorf("git change provider: a target branch is required") + return nil, fmt.Errorf("gitrepo: a target branch is required") } if cfg.Remote == "" { cfg.Remote = "origin" } - git, err := resolveGit(cfg.Git) + git, err := gitexec.Resolve(cfg.Git) if err != nil { return nil, err } @@ -78,16 +90,22 @@ func NewRepo(cfg RepoConfig) (*Repo, error) { return &Repo{cfg: cfg}, nil } +// Remote is the name the copy records its remote URL under. +func (r *Repo) Remote() string { return r.cfg.Remote } + +// Target is the branch a change's diff is measured against. +func (r *Repo) Target() string { return r.cfg.Target } + // 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. +// Callers run this at wiring time rather than on first use: a reader is often +// resolved once per message on a retry-driven 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() + r.Lock() + defer r.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) @@ -110,7 +128,7 @@ func (r *Repo) Provision(ctx context.Context) error { // 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) + return r.FetchTarget(ctx) } // configureRemote records the remote, correcting it if the configuration @@ -130,24 +148,24 @@ func (r *Repo) configureRemote(ctx context.Context) error { return err } -// ensureCommit guarantees sha is present locally, fetching if it is not. +// 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) { +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) { + 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) { + if _, err := r.run(ctx, "fetch", r.cfg.Remote, ref); err == nil && r.HasCommit(ctx, sha) { return nil } } @@ -160,9 +178,9 @@ func (r *Repo) ensureCommit(ctx context.Context, sha, ref string) error { 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 +// 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 { +func (r *Repo) FetchTarget(ctx context.Context) error { if err := r.applyAuth(ctx); err != nil { return err } @@ -177,16 +195,17 @@ func (r *Repo) applyAuth(ctx context.Context) error { return r.cfg.Auth.Apply(ctx, r.cfg.Path, r.cfg.RemoteURL) } -func (r *Repo) hasCommit(ctx context.Context, sha string) bool { +// HasCommit reports whether sha is present in the copy. +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 +// 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) { +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) @@ -194,38 +213,32 @@ func (r *Repo) mergeBase(ctx context.Context, a, b string) (string, error) { 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) { +// command builds a git invocation inside the copy, carrying the shared scrub set +// plus the transport variables a fetch needs. HOME is passed through so git can +// find the user's SSH known_hosts and credential store; it is not in the shared +// transport set, so this package asks for it explicitly. +func (r *Repo) command(ctx context.Context, args ...string) *exec.Cmd { cmd := exec.CommandContext(ctx, r.cfg.Git, args...) cmd.Dir = r.cfg.Path - cmd.Env = commandEnv() + cmd.Env = gitexec.Env(gitexec.EnvOptions{Transport: true, Passthrough: []string{"HOME"}}) + return cmd +} - 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 +// run executes git inside the copy and returns trimmed stdout. +func (r *Repo) run(ctx context.Context, args ...string) (string, error) { + out, err := r.outputOf(ctx, args...) + return strings.TrimSpace(out), err } -// 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() +// RunRaw executes git inside the copy and returns stdout untrimmed, for commands +// whose output is NUL-delimited and whose trailing separator is part of the +// format. +func (r *Repo) RunRaw(ctx context.Context, args ...string) (string, error) { + return r.outputOf(ctx, args...) +} +func (r *Repo) outputOf(ctx context.Context, args ...string) (string, error) { + cmd := r.command(ctx, args...) var stderr strings.Builder cmd.Stderr = &stderr out, err := cmd.Output() @@ -239,58 +252,16 @@ func (r *Repo) output(ctx context.Context, args ...string) (string, error) { 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 -} - // 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("") + git, err := gitexec.Resolve("") if err != nil { return err } - cmd := exec.CommandContext(ctx, git, "config", key, value) - cmd.Dir = path - cmd.Env = commandEnv() - + cmd := gitexec.Command(ctx, git, path, "config", key, value) var stderr strings.Builder cmd.Stderr = &stderr if err := cmd.Run(); err != nil { @@ -302,27 +273,3 @@ func SetConfig(ctx context.Context, path, key, value string) error { } 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) { - 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 -} diff --git a/platform/git/repo/repo_test.go b/platform/git/repo/repo_test.go new file mode 100644 index 00000000..abc5287c --- /dev/null +++ b/platform/git/repo/repo_test.go @@ -0,0 +1,116 @@ +// 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 gitrepo + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + gitexec "github.com/uber/submitqueue/platform/git/exec" + gitexectest "github.com/uber/submitqueue/platform/git/exectest" +) + +// remote is a bare "remote" plus a working clone the test commits into, driving +// the Bazel-pinned git so the assertions describe the build the services run. +type remote struct { + t *testing.T + git string + url string + work string +} + +func newRemote(t *testing.T) *remote { + t.Helper() + root := t.TempDir() + r := &remote{ + t: t, + git: gitexectest.Git(t), + url: filepath.Join(root, "remote.git"), + work: filepath.Join(root, "work"), + } + r.run("", "init", "--bare", "-b", "main", r.url) + r.run("", "clone", r.url, r.work) + r.run(r.work, "config", "user.name", "Test") + r.run(r.work, "config", "user.email", "test@example.invalid") + require.NoError(t, os.WriteFile(filepath.Join(r.work, "seed.txt"), []byte("seed\n"), 0o644)) + r.run(r.work, "add", ".") + r.run(r.work, "commit", "-m", "seed") + r.run(r.work, "push", "origin", "main") + return r +} + +func (r *remote) run(dir string, args ...string) string { + r.t.Helper() + out, err := gitexec.Command(context.Background(), r.git, dir, args...).CombinedOutput() + require.NoError(r.t, err, "git %s: %s", strings.Join(args, " "), out) + return strings.TrimSpace(string(out)) +} + +// pushFeature commits a file on a new branch and returns its head SHA and ref. +func (r *remote) pushFeature(branch, contents string) (sha, ref string) { + r.t.Helper() + r.run(r.work, "checkout", "-B", branch, "main") + require.NoError(r.t, os.WriteFile(filepath.Join(r.work, "feature.txt"), []byte(contents), 0o644)) + r.run(r.work, "add", "-A") + r.run(r.work, "commit", "-m", "feature") + r.run(r.work, "push", "-f", "origin", branch) + return r.run(r.work, "rev-parse", "HEAD"), "refs/heads/" + branch +} + +func TestProvision_IsIdempotentAndKeepsWhatItFetched(t *testing.T) { + ctx := context.Background() + rem := newRemote(t) + head, ref := rem.pushFeature("feature/keep", "keep\n") + + repo, err := NewRepo(RepoConfig{ + Git: rem.git, + Path: filepath.Join(t.TempDir(), "copy.git"), + RemoteURL: rem.url, + Target: "main", + }) + require.NoError(t, err) + require.NoError(t, repo.Provision(ctx)) + + require.False(t, repo.HasCommit(ctx, head), "provisioning fetches only the target") + require.NoError(t, repo.EnsureCommit(ctx, head, ref)) + require.True(t, repo.HasCommit(ctx, head)) + + require.NoError(t, repo.Provision(ctx)) + assert.True(t, 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/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index 4f7ec214..53e39a23 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -26,6 +26,7 @@ go_library( "//platform/extension/counter:go_default_library", "//platform/extension/counter/mysql:go_default_library", "//platform/extension/messagequeue/mysql:go_default_library", + "//platform/git/repo:go_default_library", "//platform/githubactions:go_default_library", "//platform/http:go_default_library", "//platform/pipeline:go_default_library", diff --git a/service/submitqueue/orchestrator/server/changerepo.go b/service/submitqueue/orchestrator/server/changerepo.go index 10e2d130..d8d47b4b 100644 --- a/service/submitqueue/orchestrator/server/changerepo.go +++ b/service/submitqueue/orchestrator/server/changerepo.go @@ -22,7 +22,7 @@ import ( "path/filepath" "strings" - gitprovider "github.com/uber/submitqueue/submitqueue/extension/changeprovider/git" + gitrepo "github.com/uber/submitqueue/platform/git/repo" ) // credentialFile holds the git configuration fragment carrying a token. It is @@ -31,11 +31,11 @@ import ( // 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 +// tokenAuth is the default gitrepo.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 +// It is deliberately here in the wiring rather than in gitrepo. gitrepo 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. @@ -73,7 +73,7 @@ func (a tokenAuth) Apply(ctx context.Context, repoPath, remoteURL string) error // 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) + return gitrepo.SetConfig(ctx, repoPath, "include.path", credentialFile) } func isHTTPRemote(remoteURL string) bool { @@ -87,13 +87,13 @@ func isHTTPRemote(remoteURL string) bool { // 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 +func newChangeRepo(ctx context.Context, cfg gitProviderConfig) (*gitrepo.Repo, error) { + var auth gitrepo.Auth if cfg.TokenEnv != "" { auth = tokenAuth{tokenEnv: cfg.TokenEnv, tokenUser: cfg.TokenUser} } - repo, err := gitprovider.NewRepo(gitprovider.RepoConfig{ + repo, err := gitrepo.NewRepo(gitrepo.RepoConfig{ Path: cfg.RepoPath, RemoteURL: cfg.RemoteURL, Remote: cfg.Remote, diff --git a/submitqueue/extension/changeprovider/git/BUILD.bazel b/submitqueue/extension/changeprovider/git/BUILD.bazel index 11bc776b..eb0eab9a 100644 --- a/submitqueue/extension/changeprovider/git/BUILD.bazel +++ b/submitqueue/extension/changeprovider/git/BUILD.bazel @@ -3,10 +3,8 @@ 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"], @@ -32,7 +30,9 @@ go_test( }, deps = [ "//platform/base/change:go_default_library", + "//platform/git/exec:go_default_library", "//platform/git/exectest:go_default_library", + "//platform/git/repo:go_default_library", "//submitqueue/entity:go_default_library", "//submitqueue/extension/changeprovider:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", diff --git a/submitqueue/extension/changeprovider/git/README.md b/submitqueue/extension/changeprovider/git/README.md index e91baf1d..7f9cbec1 100644 --- a/submitqueue/extension/changeprovider/git/README.md +++ b/submitqueue/extension/changeprovider/git/README.md @@ -12,21 +12,9 @@ A stack's changes are cut one from the next. Measuring every change against the 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 +## What this package is, and isn't -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. +This package is only the derivation: parse the URI, pick the baseline, read the diff and author, shape the result. Everything about *reaching* the repository — the local bare copy, fetching, merge bases, authentication, the git command environment — is transport plumbing that lives in [`platform/git/repo`](../../../../platform/git/repo) (built on [`platform/git/exec`](../../../../platform/git/exec)), which the merger and any other git caller share. The provider depends on a small `Repository` interface it defines, so it holds no `os/exec` and no credential handling of its own. ## Tests diff --git a/submitqueue/extension/changeprovider/git/provider.go b/submitqueue/extension/changeprovider/git/provider.go index be07be37..9ae620d9 100644 --- a/submitqueue/extension/changeprovider/git/provider.go +++ b/submitqueue/extension/changeprovider/git/provider.go @@ -37,6 +37,7 @@ package git import ( "context" "fmt" + "sync" "github.com/uber-go/tally" "go.uber.org/zap" @@ -49,11 +50,27 @@ import ( const opName = "git_changeprovider" -// Params carries what a provider needs. The Repo is built once per repository -// and shared by every queue reading it. +// Repository is the local copy this provider reads through. It is the git +// plumbing the provider needs and nothing more: fetching, commit resolution, +// merge bases, and a lock the provider holds across a read so a shared copy's +// object set stays consistent. platform/git/repo.Repo satisfies it. +type Repository interface { + sync.Locker + FetchTarget(ctx context.Context) error + EnsureCommit(ctx context.Context, sha, ref string) error + MergeBase(ctx context.Context, a, b string) (string, error) + // RunRaw returns git's stdout untrimmed, since the diff and author formats + // this provider reads are NUL-delimited with a meaningful trailing byte. + RunRaw(ctx context.Context, args ...string) (string, error) + Remote() string + Target() string +} + +// Params carries what a provider needs. The Repository is built once per +// repository and shared by every queue reading it. type Params struct { Config changeprovider.Config - Repo *Repo + Repo Repository Logger *zap.SugaredLogger MetricsScope tally.Scope } @@ -61,7 +78,7 @@ type Params struct { // provider reads change metadata from a local copy of a git remote. type provider struct { cfg changeprovider.Config - repo *Repo + repo Repository logger *zap.SugaredLogger metricsScope tally.Scope } @@ -87,12 +104,12 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity. uris := request.Change.URIs infos := make([]entity.ChangeInfo, 0, len(uris)) - p.repo.mu.Lock() - defer p.repo.mu.Unlock() + p.repo.Lock() + defer p.repo.Unlock() - if err := p.repo.fetchTarget(ctx); err != nil { + 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) + return nil, fmt.Errorf("failed to update target branch %s: %w", p.repo.Target(), err) } previous := "" @@ -101,14 +118,14 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity. 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 { + 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 + against := p.repo.Remote() + "/" + p.repo.Target() if previous != "" { against = previous } @@ -126,14 +143,14 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity. // 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) + 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) + raw, err := p.repo.RunRaw(ctx, "diff", "--numstat", "-M", "-z", base, sha) if err != nil { return entity.ChangeDetails{}, err } @@ -152,7 +169,7 @@ func (p *provider) describe(ctx context.Context, against, sha string) (entity.Ch // 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) + out, err := p.repo.RunRaw(ctx, "show", "--no-patch", "--format=%an%x00%ae", sha) if err != nil { return entity.Author{}, err } diff --git a/submitqueue/extension/changeprovider/git/provider_test.go b/submitqueue/extension/changeprovider/git/provider_test.go index d6dbed8c..02122286 100644 --- a/submitqueue/extension/changeprovider/git/provider_test.go +++ b/submitqueue/extension/changeprovider/git/provider_test.go @@ -19,7 +19,6 @@ import ( "fmt" "net/url" "os" - "os/exec" "path/filepath" "strings" "sync" @@ -31,7 +30,9 @@ import ( "go.uber.org/zap" "github.com/uber/submitqueue/platform/base/change" + gitexec "github.com/uber/submitqueue/platform/git/exec" gitexectest "github.com/uber/submitqueue/platform/git/exectest" + gitrepo "github.com/uber/submitqueue/platform/git/repo" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" ) @@ -45,7 +46,7 @@ type fixture struct { remote string work string provider changeprovider.ChangeProvider - repo *Repo + repo *gitrepo.Repo } func newFixture(t *testing.T) *fixture { @@ -70,7 +71,7 @@ func newFixture(t *testing.T) *fixture { f.run(f.work, "commit", "-m", "seed") f.run(f.work, "push", "origin", "main") - repo, err := NewRepo(RepoConfig{ + repo, err := gitrepo.NewRepo(gitrepo.RepoConfig{ Git: git, Path: filepath.Join(root, "copy.git"), RemoteURL: f.remote, @@ -91,9 +92,7 @@ func newFixture(t *testing.T) *fixture { func (f *fixture) run(dir string, args ...string) string { f.t.Helper() - cmd := exec.Command(f.git, args...) - cmd.Dir = dir - cmd.Env = commandEnv() + cmd := gitexec.Command(context.Background(), f.git, dir, args...) out, err := cmd.CombinedOutput() require.NoError(f.t, err, "git %s: %s", strings.Join(args, " "), out) return strings.TrimSpace(string(out)) @@ -264,7 +263,7 @@ 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), + 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)) @@ -348,31 +347,3 @@ func TestGet_ConcurrentReadsAreSerialized(t *testing.T) { 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) - }) - } -}