diff --git a/platform/extension/gitworkspace/BUILD.bazel b/platform/extension/gitworkspace/BUILD.bazel index c68827bd..50248781 100644 --- a/platform/extension/gitworkspace/BUILD.bazel +++ b/platform/extension/gitworkspace/BUILD.bazel @@ -1,8 +1,23 @@ -load("@rules_go//go:def.bzl", "go_library") +load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", - srcs = ["gitworkspace.go"], + srcs = [ + "failure.go", + "gitworkspace.go", + ], importpath = "github.com/uber/submitqueue/platform/extension/gitworkspace", visibility = ["//visibility:public"], + deps = ["//platform/git/exec:go_default_library"], +) + +go_test( + name = "go_default_test", + srcs = ["failure_test.go"], + embed = [":go_default_library"], + deps = [ + "//platform/errs/git: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/failure.go b/platform/extension/gitworkspace/failure.go new file mode 100644 index 00000000..676f1203 --- /dev/null +++ b/platform/extension/gitworkspace/failure.go @@ -0,0 +1,46 @@ +// Copyright (c) 2026 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 gitworkspace + +import ( + "context" + "fmt" + "strings" + + gitexec "github.com/uber/submitqueue/platform/git/exec" +) + +// CommandFailure builds the error for a command that exited non-zero, in the +// form the git error classifier recognises. The diagnostic keeps all of stderr +// and stdout, since a classifying fragment can sit behind advice text. An ended +// ctx takes precedence, so a command killed by cancellation reads as +// cancellation rather than as a git failure. +func CommandFailure(ctx context.Context, cmd Command, out Output) error { + return gitexec.CommandFailure(ctx, cmd.Args, diagnostic(out), nil) +} + +func diagnostic(out Output) string { + var streams []string + for _, stream := range []string{out.Stderr, out.Stdout} { + if detail := strings.TrimSpace(stream); detail != "" { + streams = append(streams, detail) + } + } + text := fmt.Sprintf("exit %d", out.ExitCode) + if len(streams) > 0 { + text += ": " + strings.Join(streams, "\n") + } + return text +} diff --git a/platform/extension/gitworkspace/failure_test.go b/platform/extension/gitworkspace/failure_test.go new file mode 100644 index 00000000..d3cb243b --- /dev/null +++ b/platform/extension/gitworkspace/failure_test.go @@ -0,0 +1,80 @@ +// Copyright (c) 2026 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 gitworkspace + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + giterrs "github.com/uber/submitqueue/platform/errs/git" +) + +func TestCommandFailure(t *testing.T) { + tests := []struct { + name string + cmd Command + out Output + wantOperation string + wantContains []string + }{ + { + name: "operation is the git subcommand", + cmd: Command{Alias: "fetch-target", Bin: "git", Args: []string{"fetch", "origin", "main"}}, + out: Output{ExitCode: 128, Stderr: "fatal: unable to access remote"}, + wantOperation: "fetch", + wantContains: []string{"exit 128", "unable to access remote"}, + }, + { + name: "diagnostic keeps stdout and stderr", + cmd: Command{Alias: "pick", Bin: "git", Args: []string{"cherry-pick", "abc"}}, + out: Output{ExitCode: 1, Stderr: "hint: resolve conflicts", Stdout: "CONFLICT (content)"}, + wantOperation: "cherry-pick", + wantContains: []string{"hint: resolve conflicts", "CONFLICT (content)"}, + }, + { + name: "no output still reports the exit code", + cmd: Command{Alias: "rev", Bin: "git", Args: []string{"rev-parse", "HEAD"}}, + out: Output{ExitCode: 1}, + wantOperation: "rev-parse", + wantContains: []string{"exit 1"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := CommandFailure(context.Background(), tt.cmd, tt.out) + + var failure giterrs.CommandFailure + require.True(t, errors.As(err, &failure)) + assert.Equal(t, tt.wantOperation, failure.Operation()) + for _, fragment := range tt.wantContains { + assert.Contains(t, failure.Diagnostic(), fragment) + } + }) + } +} + +func TestCommandFailure_CancelledContextTakesPrecedence(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := CommandFailure(ctx, Command{Args: []string{"fetch", "origin"}}, Output{ExitCode: -1, Stderr: "signal: killed"}) + + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) +} diff --git a/platform/extension/gitworkspace/gitworkspace.go b/platform/extension/gitworkspace/gitworkspace.go index c9c22dfd..de5b580d 100644 --- a/platform/extension/gitworkspace/gitworkspace.go +++ b/platform/extension/gitworkspace/gitworkspace.go @@ -24,11 +24,14 @@ import "context" // Command is one command to execute in a git workspace. type Command struct { - // Alias identifies this command's Output in the response. + // Alias identifies this command's Output in the response. It must be unique + // within one Exec batch. Alias string - // Bin is the executable name. + // Bin is the executable name, such as "git". The backend decides which + // binary it resolves to. Bin string - // Args are the command-line arguments. + // Args are the command-line arguments. For git, Args[0] is the subcommand; + // global options and identity belong to the backend, not here. Args []string // Stdin is optional standard input. Stdin string @@ -38,8 +41,11 @@ type Command struct { type Output struct { // Alias matches the Command that produced this output. Alias string - // ExitCode is the process exit code (0 = success, -1 = skipped). + // ExitCode is the process exit code. It is unspecified when Skipped is set. ExitCode int32 + // Skipped reports that the command did not run because an earlier command + // in the same batch failed. + Skipped bool // Stdout is the captured standard output. Stdout string // Stderr is the captured standard error. @@ -47,11 +53,14 @@ type Output struct { } // Workspace is a stateful git environment where command batches execute -// sequentially. Commands within one Exec call run in order; a non-zero -// exit code skips the remaining commands in that batch. State persists -// across Exec calls on the same Workspace. +// sequentially. Commands within one Exec call run in order; after the first +// non-zero exit, every remaining command is reported Skipped. State persists +// across Exec calls on the same Workspace. A Workspace is used by one +// goroutine at a time. type Workspace interface { - // Exec sends a batch of commands and returns their outputs. + // Exec runs a batch and returns one Output per command, in order. A + // command that exits non-zero is reported in its Output, not as an error; + // the error is reserved for failing to run the batch at all. Exec(commands []Command) ([]Output, error) // Close releases the workspace and its resources. Close() error @@ -59,6 +68,7 @@ type Workspace interface { // Factory creates Workspace instances bound to a repository. type Factory interface { - // For returns a Workspace for the given repository. + // For returns a Workspace for the given repository. Cancelling ctx aborts + // any running command and ends the Workspace, so ctx bounds its lifetime. For(ctx context.Context, repo string) (Workspace, error) } diff --git a/platform/extension/gitworkspace/local/local.go b/platform/extension/gitworkspace/local/local.go index 78697c96..19f48dc9 100644 --- a/platform/extension/gitworkspace/local/local.go +++ b/platform/extension/gitworkspace/local/local.go @@ -26,9 +26,9 @@ import ( "github.com/uber/submitqueue/platform/extension/gitworkspace" ) -// skippedExitCode marks a command that was not executed because a prior -// command in the same batch failed. -const skippedExitCode = -1 +// notRunExitCode is the exit code reported for a command that never ran to +// completion: one skipped after a failure, or one the OS could not start. +const notRunExitCode = -1 // Params configures a local workspace factory. type Params struct { @@ -46,11 +46,12 @@ func NewFactory(p Params) gitworkspace.Factory { return &factory{dir: p.Dir} } -func (f *factory) For(_ context.Context, _ string) (gitworkspace.Workspace, error) { - return &workspace{dir: f.dir}, nil +func (f *factory) For(ctx context.Context, _ string) (gitworkspace.Workspace, error) { + return &workspace{ctx: ctx, dir: f.dir}, nil } type workspace struct { + ctx context.Context dir string } @@ -63,7 +64,8 @@ func (w *workspace) Exec(commands []gitworkspace.Command) ([]gitworkspace.Output for _, remaining := range commands[i+1:] { outputs = append(outputs, gitworkspace.Output{ Alias: remaining.Alias, - ExitCode: skippedExitCode, + ExitCode: notRunExitCode, + Skipped: true, }) } break @@ -73,7 +75,7 @@ func (w *workspace) Exec(commands []gitworkspace.Command) ([]gitworkspace.Output } func (w *workspace) run(cmd gitworkspace.Command) gitworkspace.Output { - c := exec.Command(cmd.Bin, cmd.Args...) + c := exec.CommandContext(w.ctx, cmd.Bin, cmd.Args...) c.Dir = w.dir if cmd.Stdin != "" { c.Stdin = strings.NewReader(cmd.Stdin) @@ -95,7 +97,7 @@ func (w *workspace) run(cmd gitworkspace.Command) gitworkspace.Output { } return gitworkspace.Output{ Alias: cmd.Alias, - ExitCode: skippedExitCode, + ExitCode: notRunExitCode, Stderr: err.Error(), } } diff --git a/platform/extension/gitworkspace/local/local_test.go b/platform/extension/gitworkspace/local/local_test.go index c291ef7f..de2cf04b 100644 --- a/platform/extension/gitworkspace/local/local_test.go +++ b/platform/extension/gitworkspace/local/local_test.go @@ -69,7 +69,8 @@ func TestExec_SkipsAfterFailure(t *testing.T) { require.Len(t, outputs, 3) assert.Equal(t, int32(0), outputs[0].ExitCode) assert.NotEqual(t, int32(0), outputs[1].ExitCode) - assert.Equal(t, int32(skippedExitCode), outputs[2].ExitCode) + assert.False(t, outputs[1].Skipped) + assert.True(t, outputs[2].Skipped) assert.Equal(t, "skipped", outputs[2].Alias) } @@ -132,9 +133,10 @@ func TestExec_InvalidBinary(t *testing.T) { }) require.NoError(t, err) require.Len(t, outputs, 2) - assert.Equal(t, int32(skippedExitCode), outputs[0].ExitCode) + assert.NotEqual(t, int32(0), outputs[0].ExitCode) + assert.False(t, outputs[0].Skipped) assert.NotEmpty(t, outputs[0].Stderr) - assert.Equal(t, int32(skippedExitCode), outputs[1].ExitCode) + assert.True(t, outputs[1].Skipped) } func TestExec_EmptyBatch(t *testing.T) { @@ -146,3 +148,21 @@ func TestExec_EmptyBatch(t *testing.T) { require.NoError(t, err) assert.Empty(t, outputs) } + +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() + + cancel() + outputs, err := ws.Exec([]gitworkspace.Command{ + {Alias: "first", Bin: "echo", Args: []string{"never"}}, + {Alias: "second", Bin: "echo", Args: []string{"never"}}, + }) + require.NoError(t, err) + require.Len(t, outputs, 2) + assert.NotEqual(t, int32(0), outputs[0].ExitCode) + assert.False(t, outputs[0].Skipped) + assert.True(t, outputs[1].Skipped) +}