diff --git a/platform/extension/gitworkspace/local/BUILD.bazel b/platform/extension/gitworkspace/local/BUILD.bazel index d080b538a..8fbc5d308 100644 --- a/platform/extension/gitworkspace/local/BUILD.bazel +++ b/platform/extension/gitworkspace/local/BUILD.bazel @@ -20,9 +20,19 @@ go_test( "local_test.go", "runtime_test.go", ], + data = [ + "@git", + "@git//:templates", + "@git//:templates/description", + ], embed = [":go_default_library"], + env = { + "SUBMITQUEUE_TEST_GIT": "$(location @git//:git)", + "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION": "$(location @git//:templates/description)", + }, deps = [ "//platform/extension/gitworkspace: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/platform/extension/gitworkspace/local/local.go b/platform/extension/gitworkspace/local/local.go index 19f48dc9b..f9798ab10 100644 --- a/platform/extension/gitworkspace/local/local.go +++ b/platform/extension/gitworkspace/local/local.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "errors" + "fmt" "os/exec" "strings" @@ -30,29 +31,61 @@ import ( // completion: one skipped after a failure, or one the OS could not start. const notRunExitCode = -1 +// gitBin is the Bin that the runtime and committer identity apply to. +const gitBin = "git" + // Params configures a local workspace factory. type Params struct { // Dir is the path to the local git clone. Dir string + // Runtime pins the executable, helper paths and scrubbed environment that + // git commands run on. When zero, git is resolved from PATH and inherits the + // caller's environment. + Runtime Runtime + // CommitterName and CommitterEmail identify the committer of every commit a + // git command creates. Set both or neither; when unset, the repository and + // environment configuration decide. + CommitterName string + CommitterEmail string } type factory struct { - dir string + dir string + runtime Runtime + // gitConfig is prepended to every git command's arguments. + gitConfig []string } // NewFactory creates a gitworkspace.Factory that runs commands as local -// subprocesses in the given directory. -func NewFactory(p Params) gitworkspace.Factory { - return &factory{dir: p.Dir} +// subprocesses in the given directory. It fails when a set Runtime is invalid +// or only half of the committer identity is set. +func NewFactory(p Params) (gitworkspace.Factory, error) { + if p.Runtime.Executable != "" { + if err := p.Runtime.Validate(); err != nil { + return nil, err + } + } + if (p.CommitterName == "") != (p.CommitterEmail == "") { + return nil, fmt.Errorf("committer name and email must be set together") + } + f := &factory{dir: p.Dir, runtime: p.Runtime} + if p.CommitterName != "" { + f.gitConfig = []string{ + "-c", "user.name=" + p.CommitterName, + "-c", "user.email=" + p.CommitterEmail, + "-c", "commit.gpgsign=false", + } + } + return f, nil } func (f *factory) For(ctx context.Context, _ string) (gitworkspace.Workspace, error) { - return &workspace{ctx: ctx, dir: f.dir}, nil + return &workspace{ctx: ctx, factory: f}, nil } type workspace struct { ctx context.Context - dir string + *factory } func (w *workspace) Exec(commands []gitworkspace.Command) ([]gitworkspace.Output, error) { @@ -74,9 +107,23 @@ func (w *workspace) Exec(commands []gitworkspace.Command) ([]gitworkspace.Output return outputs, nil } +func (w *workspace) command(cmd gitworkspace.Command) *exec.Cmd { + if cmd.Bin != gitBin { + c := exec.CommandContext(w.ctx, cmd.Bin, cmd.Args...) + c.Dir = w.dir + return c + } + args := append(append(make([]string, 0, len(w.gitConfig)+len(cmd.Args)), w.gitConfig...), cmd.Args...) + if w.runtime.Executable == "" { + c := exec.CommandContext(w.ctx, gitBin, args...) + c.Dir = w.dir + return c + } + return w.runtime.Command(w.ctx, w.dir, args...) +} + func (w *workspace) run(cmd gitworkspace.Command) gitworkspace.Output { - c := exec.CommandContext(w.ctx, cmd.Bin, cmd.Args...) - c.Dir = w.dir + c := w.command(cmd) if cmd.Stdin != "" { c.Stdin = strings.NewReader(cmd.Stdin) } diff --git a/platform/extension/gitworkspace/local/local_test.go b/platform/extension/gitworkspace/local/local_test.go index de2cf04b6..25116dec3 100644 --- a/platform/extension/gitworkspace/local/local_test.go +++ b/platform/extension/gitworkspace/local/local_test.go @@ -16,17 +16,86 @@ package local import ( "context" + "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber/submitqueue/platform/extension/gitworkspace" + gitexectest "github.com/uber/submitqueue/platform/git/exectest" ) -func TestExec_Success(t *testing.T) { - ws, err := NewFactory(Params{Dir: t.TempDir()}).For(context.Background(), "") +func newWorkspace(t *testing.T, ctx context.Context, p Params) gitworkspace.Workspace { + t.Helper() + f, err := NewFactory(p) + require.NoError(t, err) + ws, err := f.For(ctx, "") + require.NoError(t, err) + t.Cleanup(func() { _ = ws.Close() }) + return ws +} + +func pinnedRuntime(t *testing.T) Runtime { + t.Helper() + executable := gitexectest.Git(t) + return Runtime{ + Executable: executable, + ExecPath: filepath.Dir(executable), + TemplateDir: filepath.Dir(gitexectest.Runfile(t, "SUBMITQUEUE_TEST_GIT_TEMPLATE_DESCRIPTION")), + } +} + +func TestNewFactory(t *testing.T) { + tests := []struct { + name string + params Params + wantErr bool + }{ + {name: "directory only", params: Params{Dir: "/repo"}}, + {name: "committer identity", params: Params{Dir: "/repo", CommitterName: "Runway", CommitterEmail: "runway@example.com"}}, + {name: "committer name without email", params: Params{Dir: "/repo", CommitterName: "Runway"}, wantErr: true}, + {name: "committer email without name", params: Params{Dir: "/repo", CommitterEmail: "runway@example.com"}, wantErr: true}, + {name: "relative runtime path", params: Params{Dir: "/repo", Runtime: Runtime{Executable: "git", ExecPath: "/git-core", TemplateDir: "/templates"}}, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewFactory(tt.params) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +} + +func TestExec_GitRunsOnPinnedRuntimeAsCommitter(t *testing.T) { + // The ambient identity must not reach the commit: the pinned runtime scrubs + // the environment, so only the configured committer can be recorded. + t.Setenv("GIT_COMMITTER_NAME", "ambient") + dir := t.TempDir() + ws := newWorkspace(t, context.Background(), Params{ + Dir: dir, + Runtime: pinnedRuntime(t), + CommitterName: "Runway", + CommitterEmail: "runway@example.com", + }) + + outputs, err := ws.Exec([]gitworkspace.Command{ + {Alias: "init", Bin: "git", Args: []string{"init", "-q", "-b", "main"}}, + {Alias: "commit", Bin: "git", Args: []string{"commit", "-q", "--allow-empty", "-m", "seed", "--author", "Jane Dev "}}, + {Alias: "idents", Bin: "git", Args: []string{"log", "-1", "--format=%an <%ae>|%cn <%ce>"}}, + }) require.NoError(t, err) - defer ws.Close() + require.Len(t, outputs, 3) + for _, out := range outputs { + require.Equal(t, int32(0), out.ExitCode, "%s: %s", out.Alias, out.Stderr) + } + assert.Equal(t, "Jane Dev |Runway \n", outputs[2].Stdout) +} + +func TestExec_Success(t *testing.T) { + ws := newWorkspace(t, context.Background(), Params{Dir: t.TempDir()}) outputs, err := ws.Exec([]gitworkspace.Command{ {Alias: "echo", Bin: "echo", Args: []string{"hello"}}, @@ -39,9 +108,7 @@ func TestExec_Success(t *testing.T) { } func TestExec_MultipleCommands(t *testing.T) { - ws, err := NewFactory(Params{Dir: t.TempDir()}).For(context.Background(), "") - require.NoError(t, err) - defer ws.Close() + ws := newWorkspace(t, context.Background(), Params{Dir: t.TempDir()}) outputs, err := ws.Exec([]gitworkspace.Command{ {Alias: "first", Bin: "echo", Args: []string{"one"}}, @@ -56,9 +123,7 @@ func TestExec_MultipleCommands(t *testing.T) { } func TestExec_SkipsAfterFailure(t *testing.T) { - ws, err := NewFactory(Params{Dir: t.TempDir()}).For(context.Background(), "") - require.NoError(t, err) - defer ws.Close() + ws := newWorkspace(t, context.Background(), Params{Dir: t.TempDir()}) outputs, err := ws.Exec([]gitworkspace.Command{ {Alias: "ok", Bin: "echo", Args: []string{"fine"}}, @@ -76,11 +141,9 @@ func TestExec_SkipsAfterFailure(t *testing.T) { func TestExec_StatePersistsAcrossCalls(t *testing.T) { dir := t.TempDir() - ws, err := NewFactory(Params{Dir: dir}).For(context.Background(), "") - require.NoError(t, err) - defer ws.Close() + ws := newWorkspace(t, context.Background(), Params{Dir: dir}) - _, err = ws.Exec([]gitworkspace.Command{ + _, err := ws.Exec([]gitworkspace.Command{ {Alias: "write", Bin: "sh", Args: []string{"-c", "echo content > testfile"}}, }) require.NoError(t, err) @@ -95,9 +158,7 @@ func TestExec_StatePersistsAcrossCalls(t *testing.T) { } func TestExec_Stdin(t *testing.T) { - ws, err := NewFactory(Params{Dir: t.TempDir()}).For(context.Background(), "") - require.NoError(t, err) - defer ws.Close() + ws := newWorkspace(t, context.Background(), Params{Dir: t.TempDir()}) outputs, err := ws.Exec([]gitworkspace.Command{ {Alias: "cat", Bin: "cat", Stdin: "from stdin"}, @@ -109,9 +170,7 @@ func TestExec_Stdin(t *testing.T) { } func TestExec_Stderr(t *testing.T) { - ws, err := NewFactory(Params{Dir: t.TempDir()}).For(context.Background(), "") - require.NoError(t, err) - defer ws.Close() + ws := newWorkspace(t, context.Background(), Params{Dir: t.TempDir()}) outputs, err := ws.Exec([]gitworkspace.Command{ {Alias: "err", Bin: "sh", Args: []string{"-c", "echo oops >&2"}}, @@ -123,9 +182,7 @@ func TestExec_Stderr(t *testing.T) { } func TestExec_InvalidBinary(t *testing.T) { - ws, err := NewFactory(Params{Dir: t.TempDir()}).For(context.Background(), "") - require.NoError(t, err) - defer ws.Close() + ws := newWorkspace(t, context.Background(), Params{Dir: t.TempDir()}) outputs, err := ws.Exec([]gitworkspace.Command{ {Alias: "bad", Bin: "nonexistent-binary-xyz"}, @@ -140,9 +197,7 @@ func TestExec_InvalidBinary(t *testing.T) { } func TestExec_EmptyBatch(t *testing.T) { - ws, err := NewFactory(Params{Dir: t.TempDir()}).For(context.Background(), "") - require.NoError(t, err) - defer ws.Close() + ws := newWorkspace(t, context.Background(), Params{Dir: t.TempDir()}) outputs, err := ws.Exec(nil) require.NoError(t, err) @@ -151,9 +206,7 @@ func TestExec_EmptyBatch(t *testing.T) { func TestExec_CancelledContextStopsCommands(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) - ws, err := NewFactory(Params{Dir: t.TempDir()}).For(ctx, "") - require.NoError(t, err) - defer ws.Close() + ws := newWorkspace(t, ctx, Params{Dir: t.TempDir()}) cancel() outputs, err := ws.Exec([]gitworkspace.Command{ diff --git a/service/submitqueue/orchestrator/server/profiles.go b/service/submitqueue/orchestrator/server/profiles.go index e2ad5b718..c760bfbed 100644 --- a/service/submitqueue/orchestrator/server/profiles.go +++ b/service/submitqueue/orchestrator/server/profiles.go @@ -784,7 +784,11 @@ func (b *profileBuilder) newTangoAnalyzerFactory(key tangoAnalyzerKey, where str type cloneWorkspaces struct{} func (cloneWorkspaces) For(ctx context.Context, repo string) (gitworkspace.Workspace, error) { - return gitworkspacelocal.NewFactory(gitworkspacelocal.Params{Dir: repo}).For(ctx, repo) + f, err := gitworkspacelocal.NewFactory(gitworkspacelocal.Params{Dir: repo}) + if err != nil { + return nil, err + } + return f.For(ctx, repo) } // pathOverlapKey maps the configured granularity onto the projection the