separate options for Up and Exec

coder · mafredri · Jun 6, 2025 · Jun 5, 2025 · Jun 6, 2025 · Jun 6, 2025
commit adbfd45c5ca8799cf3986d8d216b032ff29903ef
diff --git a/agent/agentcontainers/acmock/acmock.go b/agent/agentcontainers/acmock/acmock.go
diff --git 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
@@ -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
@@ -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
@@ -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")