From cb275a2e6f381d6eb2fd70cab9aca04c5de0be38 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Thu, 5 Jun 2025 11:00:04 +0000 Subject: [PATCH 1/2] feat(agent/agentcontainers): add Exec method to devcontainers CLI This change adds Exec to the devcontainer CLI. Updates coder/internal#621 --- agent/agentcontainers/acmock/acmock.go | 21 +- agent/agentcontainers/api_test.go | 45 ++-- agent/agentcontainers/devcontainercli.go | 92 ++++++-- agent/agentcontainers/devcontainercli_test.go | 218 +++++++++++++++--- 4 files changed, 314 insertions(+), 62 deletions(-) diff --git a/agent/agentcontainers/acmock/acmock.go b/agent/agentcontainers/acmock/acmock.go index 4be3b2cf53519..0ff3c85c0955f 100644 --- a/agent/agentcontainers/acmock/acmock.go +++ b/agent/agentcontainers/acmock/acmock.go @@ -130,8 +130,27 @@ func (m *MockDevcontainerCLI) EXPECT() *MockDevcontainerCLIMockRecorder { return m.recorder } +// Exec mocks base method. +func (m *MockDevcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath, cmd string, cmdArgs []string, opts ...agentcontainers.DevcontainerCLIOptions) error { + m.ctrl.T.Helper() + varargs := []any{ctx, workspaceFolder, configPath, cmd, cmdArgs} + for _, a := range opts { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "Exec", varargs...) + ret0, _ := ret[0].(error) + return ret0 +} + +// Exec indicates an expected call of Exec. +func (mr *MockDevcontainerCLIMockRecorder) Exec(ctx, workspaceFolder, configPath, cmd, cmdArgs any, opts ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]any{ctx, workspaceFolder, configPath, cmd, cmdArgs}, opts...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Exec", reflect.TypeOf((*MockDevcontainerCLI)(nil).Exec), varargs...) +} + // Up mocks base method. -func (m *MockDevcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...agentcontainers.DevcontainerCLIUpOptions) (string, error) { +func (m *MockDevcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...agentcontainers.DevcontainerCLIOptions) (string, error) { m.ctrl.T.Helper() varargs := []any{ctx, workspaceFolder, configPath} for _, a := range opts { diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 57c4c8f894f3e..5d3f41fe2cb6c 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -58,20 +58,39 @@ func (f *fakeContainerCLI) ExecAs(ctx context.Context, name, user string, args . // fakeDevcontainerCLI implements the agentcontainers.DevcontainerCLI // interface for testing. type fakeDevcontainerCLI struct { - id string - err error - continueUp chan struct{} + upID string + upErr error + upErrC chan error // If set, send to return err, close to return upErr. + execErr error + execErrC chan error // If set, send to return err, close to return execErr. } -func (f *fakeDevcontainerCLI) Up(ctx context.Context, _, _ string, _ ...agentcontainers.DevcontainerCLIUpOptions) (string, error) { - if f.continueUp != nil { +func (f *fakeDevcontainerCLI) Up(ctx context.Context, _, _ string, _ ...agentcontainers.DevcontainerCLIOptions) (string, error) { + if f.upErrC != nil { select { case <-ctx.Done(): - return "", xerrors.New("test timeout") - case <-f.continueUp: + return "", ctx.Err() + case err, ok := <-f.upErrC: + if ok { + return f.upID, err + } } } - return f.id, f.err + return f.upID, f.upErr +} + +func (f *fakeDevcontainerCLI) Exec(ctx context.Context, _, _ string, _ string, _ []string, _ ...agentcontainers.DevcontainerCLIOptions) error { + if f.execErrC != nil { + select { + case <-ctx.Done(): + return ctx.Err() + case err, ok := <-f.execErrC: + if ok { + return err + } + } + } + return f.execErr } // fakeWatcher implements the watcher.Watcher interface for testing. @@ -398,7 +417,7 @@ func TestAPI(t *testing.T) { }, }, devcontainerCLI: &fakeDevcontainerCLI{ - err: xerrors.New("devcontainer CLI error"), + upErr: xerrors.New("devcontainer CLI error"), }, wantStatus: []int{http.StatusAccepted, http.StatusConflict}, wantBody: []string{"Devcontainer recreation initiated", "Devcontainer recreation already in progress"}, @@ -432,7 +451,7 @@ func TestAPI(t *testing.T) { nowRecreateErrorTrap := mClock.Trap().Now("recreate", "errorTimes") nowRecreateSuccessTrap := mClock.Trap().Now("recreate", "successTimes") - tt.devcontainerCLI.continueUp = make(chan struct{}) + tt.devcontainerCLI.upErrC = make(chan error) // Setup router with the handler under test. r := chi.NewRouter() @@ -470,7 +489,7 @@ func TestAPI(t *testing.T) { // because we must check what state the devcontainer ends up in // after the recreation process is initiated and finished. if tt.wantStatus[0] != http.StatusAccepted { - close(tt.devcontainerCLI.continueUp) + close(tt.devcontainerCLI.upErrC) nowRecreateSuccessTrap.Close() nowRecreateErrorTrap.Close() return @@ -497,10 +516,10 @@ func TestAPI(t *testing.T) { assert.Equal(t, codersdk.WorkspaceAgentDevcontainerStatusStarting, resp.Devcontainers[0].Container.DevcontainerStatus, "container dc status is not starting") // Allow the devcontainer CLI to continue the up process. - close(tt.devcontainerCLI.continueUp) + close(tt.devcontainerCLI.upErrC) // Ensure the devcontainer ends up in error state if the up call fails. - if tt.devcontainerCLI.err != nil { + if tt.devcontainerCLI.upErr != nil { nowRecreateSuccessTrap.Close() // The timestamp for the error will be stored, which gives // us a good anchor point to know when to do our request. diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index 7e3122b182fdb..b77507320a0cd 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -16,37 +16,67 @@ import ( // DevcontainerCLI is an interface for the devcontainer CLI. type DevcontainerCLI interface { - Up(ctx context.Context, workspaceFolder, configPath string, opts ...DevcontainerCLIUpOptions) (id string, err error) + Up(ctx context.Context, workspaceFolder, configPath string, opts ...DevcontainerCLIOptions) (id string, err error) + Exec(ctx context.Context, workspaceFolder, configPath string, cmd string, cmdArgs []string, opts ...DevcontainerCLIOptions) error } -// DevcontainerCLIUpOptions are options for the devcontainer CLI up -// command. -type DevcontainerCLIUpOptions func(*devcontainerCLIUpConfig) +// DevcontainerCLIOptions are options for the devcontainer CLI commands. +type DevcontainerCLIOptions func(*devcontainerCLIUpConfig) // WithRemoveExistingContainer is an option to remove the existing -// container. -func WithRemoveExistingContainer() DevcontainerCLIUpOptions { +// container. Can only be used with the Up command. +func WithRemoveExistingContainer() DevcontainerCLIOptions { return func(o *devcontainerCLIUpConfig) { - o.removeExistingContainer = true + if o.command != "up" { + panic("developer error: WithRemoveExistingContainer can only be used with the Up command") + } + o.args = append(o.args, "--remove-existing-container") } } -// WithOutput sets stdout and stderr writers for Up command logs. -func WithOutput(stdout, stderr io.Writer) DevcontainerCLIUpOptions { +// WithOutput sets additional stdout and stderr writers for logs. +func WithOutput(stdout, stderr io.Writer) DevcontainerCLIOptions { return func(o *devcontainerCLIUpConfig) { o.stdout = stdout o.stderr = stderr } } +// WithContainerID sets the container ID to target a specific container. +// Can only be used with the Exec command. +func WithContainerID(id string) DevcontainerCLIOptions { + return func(o *devcontainerCLIUpConfig) { + if o.command != "exec" { + panic("developer error: WithContainerID can only be used with the Exec command") + } + o.args = append(o.args, "--container-id", id) + } +} + +// WithRemoteEnv sets environment variables for the Exec command. +// Can only be used with the Exec command. +func WithRemoteEnv(env ...string) DevcontainerCLIOptions { + return func(o *devcontainerCLIUpConfig) { + if o.command != "exec" { + panic("developer error: WithRemoteEnv can only be used with the Exec command") + } + for _, e := range env { + o.args = append(o.args, "--remote-env", e) + } + } +} + type devcontainerCLIUpConfig struct { + command string // The devcontainer CLI command to run, e.g. "up", "exec". removeExistingContainer bool + args []string // Additional arguments for the command. stdout io.Writer stderr io.Writer } -func applyDevcontainerCLIUpOptions(opts []DevcontainerCLIUpOptions) devcontainerCLIUpConfig { +func applyDevcontainerCLIOptions(command string, opts []DevcontainerCLIOptions) devcontainerCLIUpConfig { conf := devcontainerCLIUpConfig{ + command: command, removeExistingContainer: false, } for _, opt := range opts { @@ -71,8 +101,8 @@ func NewDevcontainerCLI(logger slog.Logger, execer agentexec.Execer) Devcontaine } } -func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...DevcontainerCLIUpOptions) (string, error) { - conf := applyDevcontainerCLIUpOptions(opts) +func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...DevcontainerCLIOptions) (string, error) { + conf := applyDevcontainerCLIOptions("up", opts) logger := d.logger.With(slog.F("workspace_folder", workspaceFolder), slog.F("config_path", configPath), slog.F("recreate", conf.removeExistingContainer)) args := []string{ @@ -83,9 +113,7 @@ func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath st if configPath != "" { args = append(args, "--config", configPath) } - if conf.removeExistingContainer { - args = append(args, "--remove-existing-container") - } + args = append(args, conf.args...) cmd := d.execer.CommandContext(ctx, "devcontainer", args...) // Capture stdout for parsing and stream logs for both default and provided writers. @@ -117,6 +145,40 @@ func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath st return result.ContainerID, nil } +func (d *devcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath string, cmd string, cmdArgs []string, opts ...DevcontainerCLIOptions) error { + conf := applyDevcontainerCLIOptions("exec", opts) + logger := d.logger.With(slog.F("workspace_folder", workspaceFolder), slog.F("config_path", configPath)) + + args := []string{"exec"} + if workspaceFolder != "" { + args = append(args, "--workspace-folder", workspaceFolder) + } + if configPath != "" { + args = append(args, "--config", configPath) + } + args = append(args, conf.args...) + args = append(args, cmd) + args = append(args, cmdArgs...) + c := d.execer.CommandContext(ctx, "devcontainer", args...) + + stdoutWriters := []io.Writer{&devcontainerCLILogWriter{ctx: ctx, logger: logger.With(slog.F("stdout", true))}} + if conf.stdout != nil { + stdoutWriters = append(stdoutWriters, conf.stdout) + } + c.Stdout = io.MultiWriter(stdoutWriters...) + stderrWriters := []io.Writer{&devcontainerCLILogWriter{ctx: ctx, logger: logger.With(slog.F("stderr", true))}} + if conf.stderr != nil { + stderrWriters = append(stderrWriters, conf.stderr) + } + c.Stderr = io.MultiWriter(stderrWriters...) + + if err := c.Run(); err != nil { + return xerrors.Errorf("devcontainer exec failed: %w", err) + } + + return nil +} + // parseDevcontainerCLILastLine parses the last line of the devcontainer CLI output // which is a JSON object. func parseDevcontainerCLILastLine(ctx context.Context, logger slog.Logger, p []byte) (result devcontainerCLIResult, err error) { diff --git a/agent/agentcontainers/devcontainercli_test.go b/agent/agentcontainers/devcontainercli_test.go index cdba0211ab94e..44ca68f4f5cf4 100644 --- a/agent/agentcontainers/devcontainercli_test.go +++ b/agent/agentcontainers/devcontainercli_test.go @@ -42,7 +42,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { logFile string workspace string config string - opts []agentcontainers.DevcontainerCLIUpOptions + opts []agentcontainers.DevcontainerCLIOptions wantArgs string wantError bool }{ @@ -93,7 +93,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { name: "with remove existing container", logFile: "up.log", workspace: "/test/workspace", - opts: []agentcontainers.DevcontainerCLIUpOptions{ + opts: []agentcontainers.DevcontainerCLIOptions{ agentcontainers.WithRemoveExistingContainer(), }, wantArgs: "up --log-format json --workspace-folder /test/workspace --remove-existing-container", @@ -126,6 +126,113 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { }) } }) + + t.Run("Exec", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + workspaceFolder string + configPath string + cmd string + cmdArgs []string + opts []agentcontainers.DevcontainerCLIOptions + wantArgs string + wantError bool + }{ + { + name: "simple command", + workspaceFolder: "/test/workspace", + configPath: "", + cmd: "echo", + cmdArgs: []string{"hello"}, + wantArgs: "exec --workspace-folder /test/workspace echo hello", + wantError: false, + }, + { + name: "command with multiple args", + workspaceFolder: "/test/workspace", + configPath: "/test/config.json", + cmd: "ls", + cmdArgs: []string{"-la", "/workspace"}, + wantArgs: "exec --workspace-folder /test/workspace --config /test/config.json ls -la /workspace", + wantError: false, + }, + { + name: "empty command args", + workspaceFolder: "/test/workspace", + configPath: "", + cmd: "bash", + cmdArgs: nil, + wantArgs: "exec --workspace-folder /test/workspace bash", + wantError: false, + }, + { + name: "workspace not found", + workspaceFolder: "/nonexistent/workspace", + configPath: "", + cmd: "echo", + cmdArgs: []string{"test"}, + wantArgs: "exec --workspace-folder /nonexistent/workspace echo test", + wantError: true, + }, + { + name: "with container ID", + workspaceFolder: "/test/workspace", + configPath: "", + cmd: "echo", + cmdArgs: []string{"hello"}, + opts: []agentcontainers.DevcontainerCLIOptions{agentcontainers.WithContainerID("test-container-123")}, + wantArgs: "exec --workspace-folder /test/workspace --container-id test-container-123 echo hello", + wantError: false, + }, + { + name: "with container ID and config", + workspaceFolder: "/test/workspace", + configPath: "/test/config.json", + cmd: "bash", + cmdArgs: []string{"-c", "ls -la"}, + opts: []agentcontainers.DevcontainerCLIOptions{agentcontainers.WithContainerID("my-container")}, + wantArgs: "exec --workspace-folder /test/workspace --config /test/config.json --container-id my-container bash -c ls -la", + wantError: false, + }, + { + name: "with container ID and output capture", + workspaceFolder: "/test/workspace", + configPath: "", + cmd: "cat", + cmdArgs: []string{"/etc/hostname"}, + opts: []agentcontainers.DevcontainerCLIOptions{ + agentcontainers.WithContainerID("test-container-789"), + }, + wantArgs: "exec --workspace-folder /test/workspace --container-id test-container-789 cat /etc/hostname", + wantError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitMedium) + + testExecer := &testDevcontainerExecer{ + testExePath: testExePath, + wantArgs: tt.wantArgs, + wantError: tt.wantError, + logFile: "", // Exec doesn't need log file parsing + } + + dccli := agentcontainers.NewDevcontainerCLI(logger, testExecer) + err := dccli.Exec(ctx, tt.workspaceFolder, tt.configPath, tt.cmd, tt.cmdArgs, tt.opts...) + if tt.wantError { + assert.Error(t, err, "want error") + } else { + assert.NoError(t, err, "want no error") + } + }) + } + }) } // TestDevcontainerCLI_WithOutput tests that WithOutput captures CLI @@ -136,35 +243,77 @@ func TestDevcontainerCLI_WithOutput(t *testing.T) { // Prepare test executable and logger. testExePath, err := os.Executable() require.NoError(t, err, "get test executable path") - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) - ctx := testutil.Context(t, testutil.WaitMedium) - - // Buffers to capture stdout and stderr. - outBuf := &bytes.Buffer{} - errBuf := &bytes.Buffer{} - - // Simulate CLI execution with a standard up.log file. - wantArgs := "up --log-format json --workspace-folder /test/workspace" - testExecer := &testDevcontainerExecer{ - testExePath: testExePath, - wantArgs: wantArgs, - wantError: false, - logFile: filepath.Join("testdata", "devcontainercli", "parse", "up.log"), - } - dccli := agentcontainers.NewDevcontainerCLI(logger, testExecer) - // Call Up with WithOutput to capture CLI logs. - containerID, err := dccli.Up(ctx, "/test/workspace", "", agentcontainers.WithOutput(outBuf, errBuf)) - require.NoError(t, err, "Up should succeed") - require.NotEmpty(t, containerID, "expected non-empty container ID") + t.Run("Up", func(t *testing.T) { + t.Parallel() + + // Buffers to capture stdout and stderr. + outBuf := &bytes.Buffer{} + errBuf := &bytes.Buffer{} + + // Simulate CLI execution with a standard up.log file. + wantArgs := "up --log-format json --workspace-folder /test/workspace" + testExecer := &testDevcontainerExecer{ + testExePath: testExePath, + wantArgs: wantArgs, + wantError: false, + logFile: filepath.Join("testdata", "devcontainercli", "parse", "up.log"), + } + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + dccli := agentcontainers.NewDevcontainerCLI(logger, testExecer) - // Read expected log content. - expLog, err := os.ReadFile(filepath.Join("testdata", "devcontainercli", "parse", "up.log")) - require.NoError(t, err, "reading expected log file") + // Call Up with WithOutput to capture CLI logs. + ctx := testutil.Context(t, testutil.WaitMedium) + containerID, err := dccli.Up(ctx, "/test/workspace", "", agentcontainers.WithOutput(outBuf, errBuf)) + require.NoError(t, err, "Up should succeed") + require.NotEmpty(t, containerID, "expected non-empty container ID") - // Verify stdout buffer contains the CLI logs and stderr is empty. - assert.Equal(t, string(expLog), outBuf.String(), "stdout buffer should match CLI logs") - assert.Empty(t, errBuf.String(), "stderr buffer should be empty on success") + // Read expected log content. + expLog, err := os.ReadFile(filepath.Join("testdata", "devcontainercli", "parse", "up.log")) + require.NoError(t, err, "reading expected log file") + + // Verify stdout buffer contains the CLI logs and stderr is empty. + assert.Equal(t, string(expLog), outBuf.String(), "stdout buffer should match CLI logs") + assert.Empty(t, errBuf.String(), "stderr buffer should be empty on success") + }) + + t.Run("Exec", func(t *testing.T) { + t.Parallel() + + logFile := filepath.Join(t.TempDir(), "exec.log") + f, err := os.Create(logFile) + require.NoError(t, err, "create exec log file") + _, err = f.WriteString("exec command log\n") + require.NoError(t, err, "write to exec log file") + err = f.Close() + require.NoError(t, err, "close exec log file") + + // Buffers to capture stdout and stderr. + outBuf := &bytes.Buffer{} + errBuf := &bytes.Buffer{} + + // Simulate CLI execution for exec command with container ID. + wantArgs := "exec --workspace-folder /test/workspace --container-id test-container-456 echo hello" + testExecer := &testDevcontainerExecer{ + testExePath: testExePath, + wantArgs: wantArgs, + wantError: false, + logFile: logFile, + } + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + dccli := agentcontainers.NewDevcontainerCLI(logger, testExecer) + + // Call Exec with WithOutput and WithContainerID to capture any command output. + ctx := testutil.Context(t, testutil.WaitMedium) + err = dccli.Exec(ctx, "/test/workspace", "", "echo", []string{"hello"}, + agentcontainers.WithContainerID("test-container-456"), + agentcontainers.WithOutput(outBuf, errBuf), + ) + require.NoError(t, err, "Exec should succeed") + + assert.NotEmpty(t, outBuf.String(), "stdout buffer should not be empty for exec with log file") + assert.Empty(t, errBuf.String(), "stderr buffer should be empty") + }) } // testDevcontainerExecer implements the agentexec.Execer interface for testing. @@ -243,13 +392,16 @@ func TestDevcontainerHelperProcess(t *testing.T) { } logFilePath := os.Getenv("TEST_DEVCONTAINER_LOG_FILE") - output, err := os.ReadFile(logFilePath) - if err != nil { - fmt.Fprintf(os.Stderr, "Reading log file %s failed: %v\n", logFilePath, err) - os.Exit(2) + if logFilePath != "" { + // Read and output log file for commands that need it (like "up") + output, err := os.ReadFile(logFilePath) + if err != nil { + fmt.Fprintf(os.Stderr, "Reading log file %s failed: %v\n", logFilePath, err) + os.Exit(2) + } + _, _ = io.Copy(os.Stdout, bytes.NewReader(output)) } - _, _ = io.Copy(os.Stdout, bytes.NewReader(output)) if os.Getenv("TEST_DEVCONTAINER_WANT_ERROR") == "true" { os.Exit(1) } From adbfd45c5ca8799cf3986d8d216b032ff29903ef Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 6 Jun 2025 11:20:36 +0000 Subject: [PATCH 2/2] separate options for Up and Exec --- agent/agentcontainers/acmock/acmock.go | 4 +- agent/agentcontainers/api.go | 2 +- agent/agentcontainers/api_test.go | 4 +- agent/agentcontainers/devcontainercli.go | 93 +++++++++++-------- agent/agentcontainers/devcontainercli_test.go | 22 ++--- 5 files changed, 70 insertions(+), 55 deletions(-) diff --git a/agent/agentcontainers/acmock/acmock.go b/agent/agentcontainers/acmock/acmock.go index 0ff3c85c0955f..f9723e8a15758 100644 --- a/agent/agentcontainers/acmock/acmock.go +++ b/agent/agentcontainers/acmock/acmock.go @@ -131,7 +131,7 @@ func (m *MockDevcontainerCLI) EXPECT() *MockDevcontainerCLIMockRecorder { } // Exec mocks base method. -func (m *MockDevcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath, cmd string, cmdArgs []string, opts ...agentcontainers.DevcontainerCLIOptions) error { +func (m *MockDevcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath, cmd string, cmdArgs []string, opts ...agentcontainers.DevcontainerCLIExecOptions) error { m.ctrl.T.Helper() varargs := []any{ctx, workspaceFolder, configPath, cmd, cmdArgs} for _, a := range opts { @@ -150,7 +150,7 @@ func (mr *MockDevcontainerCLIMockRecorder) Exec(ctx, workspaceFolder, configPath } // Up mocks base method. -func (m *MockDevcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...agentcontainers.DevcontainerCLIOptions) (string, error) { +func (m *MockDevcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...agentcontainers.DevcontainerCLIUpOptions) (string, error) { m.ctrl.T.Helper() varargs := []any{ctx, workspaceFolder, configPath} for _, a := range opts { diff --git a/agent/agentcontainers/api.go b/agent/agentcontainers/api.go index 4017de1931093..d826cb23cbc1f 100644 --- a/agent/agentcontainers/api.go +++ b/agent/agentcontainers/api.go @@ -684,7 +684,7 @@ func (api *API) recreateDevcontainer(dc codersdk.WorkspaceAgentDevcontainer, con logger.Debug(ctx, "starting devcontainer recreation") - _, err = api.dccli.Up(ctx, dc.WorkspaceFolder, configPath, WithOutput(infoW, errW), WithRemoveExistingContainer()) + _, err = api.dccli.Up(ctx, dc.WorkspaceFolder, configPath, WithUpOutput(infoW, errW), WithRemoveExistingContainer()) if err != nil { // No need to log if the API is closing (context canceled), as this // is expected behavior when the API is shutting down. diff --git a/agent/agentcontainers/api_test.go b/agent/agentcontainers/api_test.go index 5d3f41fe2cb6c..313da6f9f615f 100644 --- a/agent/agentcontainers/api_test.go +++ b/agent/agentcontainers/api_test.go @@ -65,7 +65,7 @@ type fakeDevcontainerCLI struct { execErrC chan error // If set, send to return err, close to return execErr. } -func (f *fakeDevcontainerCLI) Up(ctx context.Context, _, _ string, _ ...agentcontainers.DevcontainerCLIOptions) (string, error) { +func (f *fakeDevcontainerCLI) Up(ctx context.Context, _, _ string, _ ...agentcontainers.DevcontainerCLIUpOptions) (string, error) { if f.upErrC != nil { select { case <-ctx.Done(): @@ -79,7 +79,7 @@ func (f *fakeDevcontainerCLI) Up(ctx context.Context, _, _ string, _ ...agentcon return f.upID, f.upErr } -func (f *fakeDevcontainerCLI) Exec(ctx context.Context, _, _ string, _ string, _ []string, _ ...agentcontainers.DevcontainerCLIOptions) error { +func (f *fakeDevcontainerCLI) Exec(ctx context.Context, _, _ string, _ string, _ []string, _ ...agentcontainers.DevcontainerCLIExecOptions) error { if f.execErrC != nil { select { case <-ctx.Done(): diff --git a/agent/agentcontainers/devcontainercli.go b/agent/agentcontainers/devcontainercli.go index b77507320a0cd..94b4de610a93b 100644 --- a/agent/agentcontainers/devcontainercli.go +++ b/agent/agentcontainers/devcontainercli.go @@ -16,69 +16,84 @@ import ( // DevcontainerCLI is an interface for the devcontainer CLI. type DevcontainerCLI interface { - Up(ctx context.Context, workspaceFolder, configPath string, opts ...DevcontainerCLIOptions) (id string, err error) - Exec(ctx context.Context, workspaceFolder, configPath string, cmd string, cmdArgs []string, opts ...DevcontainerCLIOptions) error + Up(ctx context.Context, workspaceFolder, configPath string, opts ...DevcontainerCLIUpOptions) (id string, err error) + Exec(ctx context.Context, workspaceFolder, configPath string, cmd string, cmdArgs []string, opts ...DevcontainerCLIExecOptions) error } -// DevcontainerCLIOptions are options for the devcontainer CLI commands. -type DevcontainerCLIOptions func(*devcontainerCLIUpConfig) +// DevcontainerCLIUpOptions are options for the devcontainer CLI Up +// command. +type DevcontainerCLIUpOptions func(*devcontainerCLIUpConfig) + +type devcontainerCLIUpConfig struct { + args []string // Additional arguments for the Up command. + stdout io.Writer + stderr io.Writer +} // WithRemoveExistingContainer is an option to remove the existing -// container. Can only be used with the Up command. -func WithRemoveExistingContainer() DevcontainerCLIOptions { +// container. +func WithRemoveExistingContainer() DevcontainerCLIUpOptions { return func(o *devcontainerCLIUpConfig) { - if o.command != "up" { - panic("developer error: WithRemoveExistingContainer can only be used with the Up command") - } o.args = append(o.args, "--remove-existing-container") } } -// WithOutput sets additional stdout and stderr writers for logs. -func WithOutput(stdout, stderr io.Writer) DevcontainerCLIOptions { +// WithUpOutput sets additional stdout and stderr writers for logs +// during Up operations. +func WithUpOutput(stdout, stderr io.Writer) DevcontainerCLIUpOptions { return func(o *devcontainerCLIUpConfig) { o.stdout = stdout o.stderr = stderr } } +// DevcontainerCLIExecOptions are options for the devcontainer CLI Exec +// command. +type DevcontainerCLIExecOptions func(*devcontainerCLIExecConfig) + +type devcontainerCLIExecConfig struct { + args []string // Additional arguments for the Exec command. + stdout io.Writer + stderr io.Writer +} + +// WithExecOutput sets additional stdout and stderr writers for logs +// during Exec operations. +func WithExecOutput(stdout, stderr io.Writer) DevcontainerCLIExecOptions { + return func(o *devcontainerCLIExecConfig) { + o.stdout = stdout + o.stderr = stderr + } +} + // WithContainerID sets the container ID to target a specific container. -// Can only be used with the Exec command. -func WithContainerID(id string) DevcontainerCLIOptions { - return func(o *devcontainerCLIUpConfig) { - if o.command != "exec" { - panic("developer error: WithContainerID can only be used with the Exec command") - } +func WithContainerID(id string) DevcontainerCLIExecOptions { + return func(o *devcontainerCLIExecConfig) { o.args = append(o.args, "--container-id", id) } } // WithRemoteEnv sets environment variables for the Exec command. -// Can only be used with the Exec command. -func WithRemoteEnv(env ...string) DevcontainerCLIOptions { - return func(o *devcontainerCLIUpConfig) { - if o.command != "exec" { - panic("developer error: WithRemoteEnv can only be used with the Exec command") - } +func WithRemoteEnv(env ...string) DevcontainerCLIExecOptions { + return func(o *devcontainerCLIExecConfig) { for _, e := range env { o.args = append(o.args, "--remote-env", e) } } } -type devcontainerCLIUpConfig struct { - command string // The devcontainer CLI command to run, e.g. "up", "exec". - removeExistingContainer bool - args []string // Additional arguments for the command. - stdout io.Writer - stderr io.Writer +func applyDevcontainerCLIUpOptions(opts []DevcontainerCLIUpOptions) devcontainerCLIUpConfig { + conf := devcontainerCLIUpConfig{} + for _, opt := range opts { + if opt != nil { + opt(&conf) + } + } + return conf } -func applyDevcontainerCLIOptions(command string, opts []DevcontainerCLIOptions) devcontainerCLIUpConfig { - conf := devcontainerCLIUpConfig{ - command: command, - removeExistingContainer: false, - } +func applyDevcontainerCLIExecOptions(opts []DevcontainerCLIExecOptions) devcontainerCLIExecConfig { + conf := devcontainerCLIExecConfig{} for _, opt := range opts { if opt != nil { opt(&conf) @@ -101,9 +116,9 @@ func NewDevcontainerCLI(logger slog.Logger, execer agentexec.Execer) Devcontaine } } -func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...DevcontainerCLIOptions) (string, error) { - conf := applyDevcontainerCLIOptions("up", opts) - logger := d.logger.With(slog.F("workspace_folder", workspaceFolder), slog.F("config_path", configPath), slog.F("recreate", conf.removeExistingContainer)) +func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath string, opts ...DevcontainerCLIUpOptions) (string, error) { + conf := applyDevcontainerCLIUpOptions(opts) + logger := d.logger.With(slog.F("workspace_folder", workspaceFolder), slog.F("config_path", configPath)) args := []string{ "up", @@ -145,8 +160,8 @@ func (d *devcontainerCLI) Up(ctx context.Context, workspaceFolder, configPath st return result.ContainerID, nil } -func (d *devcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath string, cmd string, cmdArgs []string, opts ...DevcontainerCLIOptions) error { - conf := applyDevcontainerCLIOptions("exec", opts) +func (d *devcontainerCLI) Exec(ctx context.Context, workspaceFolder, configPath string, cmd string, cmdArgs []string, opts ...DevcontainerCLIExecOptions) error { + conf := applyDevcontainerCLIExecOptions(opts) logger := d.logger.With(slog.F("workspace_folder", workspaceFolder), slog.F("config_path", configPath)) args := []string{"exec"} diff --git a/agent/agentcontainers/devcontainercli_test.go b/agent/agentcontainers/devcontainercli_test.go index 44ca68f4f5cf4..48325ab83fb21 100644 --- a/agent/agentcontainers/devcontainercli_test.go +++ b/agent/agentcontainers/devcontainercli_test.go @@ -42,7 +42,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { logFile string workspace string config string - opts []agentcontainers.DevcontainerCLIOptions + opts []agentcontainers.DevcontainerCLIUpOptions wantArgs string wantError bool }{ @@ -93,7 +93,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { name: "with remove existing container", logFile: "up.log", workspace: "/test/workspace", - opts: []agentcontainers.DevcontainerCLIOptions{ + opts: []agentcontainers.DevcontainerCLIUpOptions{ agentcontainers.WithRemoveExistingContainer(), }, wantArgs: "up --log-format json --workspace-folder /test/workspace --remove-existing-container", @@ -136,7 +136,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { configPath string cmd string cmdArgs []string - opts []agentcontainers.DevcontainerCLIOptions + opts []agentcontainers.DevcontainerCLIExecOptions wantArgs string wantError bool }{ @@ -182,7 +182,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { configPath: "", cmd: "echo", cmdArgs: []string{"hello"}, - opts: []agentcontainers.DevcontainerCLIOptions{agentcontainers.WithContainerID("test-container-123")}, + opts: []agentcontainers.DevcontainerCLIExecOptions{agentcontainers.WithContainerID("test-container-123")}, wantArgs: "exec --workspace-folder /test/workspace --container-id test-container-123 echo hello", wantError: false, }, @@ -192,7 +192,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { configPath: "/test/config.json", cmd: "bash", cmdArgs: []string{"-c", "ls -la"}, - opts: []agentcontainers.DevcontainerCLIOptions{agentcontainers.WithContainerID("my-container")}, + opts: []agentcontainers.DevcontainerCLIExecOptions{agentcontainers.WithContainerID("my-container")}, wantArgs: "exec --workspace-folder /test/workspace --config /test/config.json --container-id my-container bash -c ls -la", wantError: false, }, @@ -202,7 +202,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { configPath: "", cmd: "cat", cmdArgs: []string{"/etc/hostname"}, - opts: []agentcontainers.DevcontainerCLIOptions{ + opts: []agentcontainers.DevcontainerCLIExecOptions{ agentcontainers.WithContainerID("test-container-789"), }, wantArgs: "exec --workspace-folder /test/workspace --container-id test-container-789 cat /etc/hostname", @@ -235,7 +235,7 @@ func TestDevcontainerCLI_ArgsAndParsing(t *testing.T) { }) } -// TestDevcontainerCLI_WithOutput tests that WithOutput captures CLI +// TestDevcontainerCLI_WithOutput tests that WithUpOutput and WithExecOutput capture CLI // logs to provided writers. func TestDevcontainerCLI_WithOutput(t *testing.T) { t.Parallel() @@ -262,9 +262,9 @@ func TestDevcontainerCLI_WithOutput(t *testing.T) { logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) dccli := agentcontainers.NewDevcontainerCLI(logger, testExecer) - // Call Up with WithOutput to capture CLI logs. + // Call Up with WithUpOutput to capture CLI logs. ctx := testutil.Context(t, testutil.WaitMedium) - containerID, err := dccli.Up(ctx, "/test/workspace", "", agentcontainers.WithOutput(outBuf, errBuf)) + containerID, err := dccli.Up(ctx, "/test/workspace", "", agentcontainers.WithUpOutput(outBuf, errBuf)) require.NoError(t, err, "Up should succeed") require.NotEmpty(t, containerID, "expected non-empty container ID") @@ -303,11 +303,11 @@ func TestDevcontainerCLI_WithOutput(t *testing.T) { logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) dccli := agentcontainers.NewDevcontainerCLI(logger, testExecer) - // Call Exec with WithOutput and WithContainerID to capture any command output. + // Call Exec with WithExecOutput and WithContainerID to capture any command output. ctx := testutil.Context(t, testutil.WaitMedium) err = dccli.Exec(ctx, "/test/workspace", "", "echo", []string{"hello"}, agentcontainers.WithContainerID("test-container-456"), - agentcontainers.WithOutput(outBuf, errBuf), + agentcontainers.WithExecOutput(outBuf, errBuf), ) require.NoError(t, err, "Exec should succeed")