From 3b8e93c74aa75fc4b905484b6d02f39a9de709e9 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 4 May 2026 07:30:59 -0500 Subject: [PATCH 1/2] feat(cmd): add --update-remote-user-uid-default flag to up command Adds a CLI flag that provides a default value for updateRemoteUserUID when devcontainer.json does not specify it. Accepts "on" or "off". When devcontainer.json explicitly sets updateRemoteUserUID, that value takes precedence over the CLI flag. Also fixes pre-existing build error in runusercommands.go (missing warnings parameter in WriteResultJSON call). --- cmd/runusercommands.go | 2 +- cmd/up.go | 13 +++++ cmd/up_update_remote_user_uid_test.go | 58 ++++++++++++++++++++ e2e/tests/up/update_remote_user_uid.go | 52 ++++++++++++++++++ pkg/driver/docker/docker.go | 22 ++++++-- pkg/driver/docker/docker_test.go | 75 ++++++++++++++++++++++++-- pkg/provider/workspace.go | 1 + 7 files changed, 214 insertions(+), 9 deletions(-) create mode 100644 cmd/up_update_remote_user_uid_test.go create mode 100644 e2e/tests/up/update_remote_user_uid.go diff --git a/cmd/runusercommands.go b/cmd/runusercommands.go index de1e77f08..179a6a824 100644 --- a/cmd/runusercommands.go +++ b/cmd/runusercommands.go @@ -90,7 +90,7 @@ func (cmd *RunUserCommandsCmd) Run(ctx context.Context) error { user := devcconfig.GetRemoteUser(result) log.Infof("lifecycle commands completed for container %s", params.containerID) - _ = devcconfig.WriteResultJSON(os.Stderr, params.containerID, user, params.workdir) + _ = devcconfig.WriteResultJSON(os.Stderr, params.containerID, user, params.workdir, nil) return nil } diff --git a/cmd/up.go b/cmd/up.go index bd3ce5f78..9530dc7f1 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -177,6 +177,16 @@ func (cmd *UpCmd) validate() error { ) } } + if cmd.UpdateRemoteUserUIDDefault != "" { + switch cmd.UpdateRemoteUserUIDDefault { + case "on", "off": + default: + return fmt.Errorf( + "invalid --update-remote-user-uid-default value %q: must be \"on\" or \"off\"", + cmd.UpdateRemoteUserUIDDefault, + ) + } + } return nil } @@ -252,6 +262,9 @@ func (cmd *UpCmd) registerDevContainerFlags(upCmd *cobra.Command) { upCmd.Flags(). StringVar(&cmd.GPUAvailability, "gpu-availability", "", "Override GPU availability detection (detect, true, false)") + upCmd.Flags(). + StringVar(&cmd.UpdateRemoteUserUIDDefault, "update-remote-user-uid-default", "", + "Default for updateRemoteUserUID when not set in devcontainer.json (on, off)") } func (cmd *UpCmd) registerIDEFlags(upCmd *cobra.Command) { diff --git a/cmd/up_update_remote_user_uid_test.go b/cmd/up_update_remote_user_uid_test.go new file mode 100644 index 000000000..7098a3959 --- /dev/null +++ b/cmd/up_update_remote_user_uid_test.go @@ -0,0 +1,58 @@ +package cmd + +import ( + "testing" + + "github.com/devsy-org/devsy/cmd/flags" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestUpCmd_UpdateRemoteUserUIDDefault_On(t *testing.T) { + upCmd := NewUpCmd(&flags.GlobalFlags{}) + err := upCmd.ParseFlags([]string{"--update-remote-user-uid-default", "on"}) + require.NoError(t, err) + + val, err := upCmd.Flags().GetString("update-remote-user-uid-default") + require.NoError(t, err) + assert.Equal(t, "on", val) +} + +func TestUpCmd_UpdateRemoteUserUIDDefault_Off(t *testing.T) { + upCmd := NewUpCmd(&flags.GlobalFlags{}) + err := upCmd.ParseFlags([]string{"--update-remote-user-uid-default", "off"}) + require.NoError(t, err) + + val, err := upCmd.Flags().GetString("update-remote-user-uid-default") + require.NoError(t, err) + assert.Equal(t, "off", val) +} + +func TestUpCmd_UpdateRemoteUserUIDDefault_Validate_Invalid(t *testing.T) { + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.UpdateRemoteUserUIDDefault = "invalid" + err := cmd.validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid --update-remote-user-uid-default value") +} + +func TestUpCmd_UpdateRemoteUserUIDDefault_Validate_Empty(t *testing.T) { + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.UpdateRemoteUserUIDDefault = "" + err := cmd.validate() + require.NoError(t, err) +} + +func TestUpCmd_UpdateRemoteUserUIDDefault_Validate_On(t *testing.T) { + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.UpdateRemoteUserUIDDefault = "on" + err := cmd.validate() + require.NoError(t, err) +} + +func TestUpCmd_UpdateRemoteUserUIDDefault_Validate_Off(t *testing.T) { + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.UpdateRemoteUserUIDDefault = "off" + err := cmd.validate() + require.NoError(t, err) +} diff --git a/e2e/tests/up/update_remote_user_uid.go b/e2e/tests/up/update_remote_user_uid.go new file mode 100644 index 000000000..859b0913e --- /dev/null +++ b/e2e/tests/up/update_remote_user_uid.go @@ -0,0 +1,52 @@ +package up + +import ( + "context" + "os" + "path/filepath" + "runtime" + + "github.com/devsy-org/devsy/e2e/framework" + docker "github.com/devsy-org/devsy/pkg/docker" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = ginkgo.Describe( + "testing --update-remote-user-uid-default flag", + ginkgo.Label("up-update-remote-user-uid"), + func() { + var dtc *dockerTestContext + + ginkgo.BeforeEach(func(ctx context.Context) { + if runtime.GOOS != "linux" { + ginkgo.Skip("updateRemoteUserUID only applies on Linux") + } + + var err error + dtc = &dockerTestContext{} + dtc.initialDir, err = os.Getwd() + framework.ExpectNoError(err) + + dtc.dockerHelper = &docker.DockerHelper{DockerCommand: "docker"} + dtc.f, err = setupDockerProvider(filepath.Join(dtc.initialDir, "bin"), "docker") + framework.ExpectNoError(err) + }) + + ginkgo.It("should accept --update-remote-user-uid-default=on", func(ctx context.Context) { + _, err := dtc.setupAndUp(ctx, "tests/up/testdata/docker", + "--update-remote-user-uid-default", "on") + framework.ExpectNoError(err) + }, ginkgo.SpecTimeout(framework.TimeoutShort())) + + ginkgo.It("should accept --update-remote-user-uid-default=off", func(ctx context.Context) { + tempDir, err := dtc.setupAndUp(ctx, "tests/up/testdata/docker", + "--update-remote-user-uid-default", "off") + framework.ExpectNoError(err) + + out, err := dtc.execSSH(ctx, tempDir, "id -u") + framework.ExpectNoError(err) + gomega.Expect(out).NotTo(gomega.BeEmpty()) + }, ginkgo.SpecTimeout(framework.TimeoutShort())) + }, +) diff --git a/pkg/driver/docker/docker.go b/pkg/driver/docker/docker.go index 3adbdb2f2..fb84e0893 100644 --- a/pkg/driver/docker/docker.go +++ b/pkg/driver/docker/docker.go @@ -57,14 +57,16 @@ func NewDockerDriver( ContainerID: workspaceInfo.Workspace.Source.Container, Builder: builder, }, - IDLabels: workspaceInfo.CLIOptions.IDLabels, + IDLabels: workspaceInfo.CLIOptions.IDLabels, + UpdateRemoteUserUIDDefault: workspaceInfo.CLIOptions.UpdateRemoteUserUIDDefault, }, nil } type dockerDriver struct { - Docker *docker.DockerHelper - Compose *compose.ComposeHelper - IDLabels []string + Docker *docker.DockerHelper + Compose *compose.ComposeHelper + IDLabels []string + UpdateRemoteUserUIDDefault string } func (d *dockerDriver) TargetArchitecture(ctx context.Context, workspaceId string) (string, error) { @@ -906,7 +908,17 @@ func (d *dockerDriver) getRemoteUser( func (d *dockerDriver) shouldUpdateUserUID(parsedConfig *config.DevContainerConfig) bool { isLinux := runtime.GOOS == "linux" hasUser := parsedConfig.ContainerUser != "" || parsedConfig.RemoteUser != "" - shouldUpdate := parsedConfig.UpdateRemoteUserUID == nil || *parsedConfig.UpdateRemoteUserUID + + var shouldUpdate bool + switch { + case parsedConfig.UpdateRemoteUserUID != nil: + shouldUpdate = *parsedConfig.UpdateRemoteUserUID + case d.UpdateRemoteUserUIDDefault == "off": + shouldUpdate = false + default: + shouldUpdate = true + } + return isLinux && hasUser && shouldUpdate } diff --git a/pkg/driver/docker/docker_test.go b/pkg/driver/docker/docker_test.go index d210d8597..c4b164df6 100644 --- a/pkg/driver/docker/docker_test.go +++ b/pkg/driver/docker/docker_test.go @@ -2,6 +2,7 @@ package docker import ( "os/user" + "runtime" "testing" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -10,9 +11,12 @@ import ( ) const ( - testSeccompUnconfined = "seccomp=unconfined" - testSecurityOptFlag = "--security-opt" - testBindMount = "type=bind,src=/a,dst=/b" + testSeccompUnconfined = "seccomp=unconfined" + testSecurityOptFlag = "--security-opt" + testBindMount = "type=bind,src=/a,dst=/b" + testUpdateUIDDefaultOff = "off" + testUpdateUIDDefaultOn = "on" + testOSLinux = "linux" ) type DockerDriverTestSuite struct { @@ -82,6 +86,71 @@ func (s *DockerDriverTestSuite) TestShouldSkipUpdate_RootWithDifferentGID() { s.True(result, "should skip when container user is root regardless of GID") } +func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_DefaultTrue_WhenConfigNil() { + cfg := &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + RemoteUser: "vscode", + }, + } + s.driver.UpdateRemoteUserUIDDefault = "" + result := s.driver.shouldUpdateUserUID(cfg) + if runtime.GOOS == testOSLinux { + s.True(result) + } +} + +func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_CLIDefaultOff_WhenConfigNil() { + cfg := &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + RemoteUser: "vscode", + }, + } + s.driver.UpdateRemoteUserUIDDefault = testUpdateUIDDefaultOff + result := s.driver.shouldUpdateUserUID(cfg) + s.False(result) +} + +func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_CLIDefaultOn_WhenConfigNil() { + cfg := &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + RemoteUser: "vscode", + }, + } + s.driver.UpdateRemoteUserUIDDefault = testUpdateUIDDefaultOn + result := s.driver.shouldUpdateUserUID(cfg) + if runtime.GOOS == testOSLinux { + s.True(result) + } +} + +func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_ConfigTakesPrecedence_True() { + t := true + cfg := &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + RemoteUser: "vscode", + UpdateRemoteUserUID: &t, + }, + } + s.driver.UpdateRemoteUserUIDDefault = testUpdateUIDDefaultOff + result := s.driver.shouldUpdateUserUID(cfg) + if runtime.GOOS == testOSLinux { + s.True(result, "devcontainer.json true should override CLI default off") + } +} + +func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_ConfigTakesPrecedence_False() { + f := false + cfg := &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + RemoteUser: "vscode", + UpdateRemoteUserUID: &f, + }, + } + s.driver.UpdateRemoteUserUIDDefault = testUpdateUIDDefaultOn + result := s.driver.shouldUpdateUserUID(cfg) + s.False(result, "devcontainer.json false should override CLI default on") +} + func (s *DockerDriverTestSuite) TestGetContainerUser_RemoteUserPriority() { cfg := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ diff --git a/pkg/provider/workspace.go b/pkg/provider/workspace.go index 7345c662a..0f92f1537 100644 --- a/pkg/provider/workspace.go +++ b/pkg/provider/workspace.go @@ -239,6 +239,7 @@ type CLIOptions struct { IDLabels []string `json:"idLabels,omitempty"` GPUAvailability string `json:"gpuAvailability,omitempty"` WorkspaceMountConsistency string `json:"workspaceMountConsistency,omitempty"` + UpdateRemoteUserUIDDefault string `json:"updateRemoteUserUIDDefault,omitempty"` // dotfiles options DotfilesRepo string `json:"dotfilesRepo,omitempty"` From ab8d90e71f505344d04e309e2562b02a27d7c6d9 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 4 May 2026 07:51:14 -0500 Subject: [PATCH 2/2] fix(lint): replace repeated string literals with named constants Addresses goconst lint violations introduced in PR #208: - "on"/"off" for UpdateRemoteUserUIDDefault replaced with exported constants - "docker" command string in e2e test extracted to local constant - "vscode" remote user string in docker driver tests extracted to constant --- cmd/up.go | 5 ++++- cmd/up_update_remote_user_uid_test.go | 16 ++++++++++------ e2e/tests/up/update_remote_user_uid.go | 9 +++++++-- pkg/driver/docker/docker_test.go | 11 ++++++----- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/cmd/up.go b/cmd/up.go index 9530dc7f1..4af2cda9c 100644 --- a/cmd/up.go +++ b/cmd/up.go @@ -37,6 +37,9 @@ const ( MountConsistencyConsistent = "consistent" MountConsistencyCached = "cached" MountConsistencyDelegated = "delegated" + + UpdateRemoteUserUIDDefaultOn = "on" + UpdateRemoteUserUIDDefaultOff = "off" ) // UpCmd holds the up cmd flags. @@ -179,7 +182,7 @@ func (cmd *UpCmd) validate() error { } if cmd.UpdateRemoteUserUIDDefault != "" { switch cmd.UpdateRemoteUserUIDDefault { - case "on", "off": + case UpdateRemoteUserUIDDefaultOn, UpdateRemoteUserUIDDefaultOff: default: return fmt.Errorf( "invalid --update-remote-user-uid-default value %q: must be \"on\" or \"off\"", diff --git a/cmd/up_update_remote_user_uid_test.go b/cmd/up_update_remote_user_uid_test.go index 7098a3959..5319a517c 100644 --- a/cmd/up_update_remote_user_uid_test.go +++ b/cmd/up_update_remote_user_uid_test.go @@ -10,22 +10,26 @@ import ( func TestUpCmd_UpdateRemoteUserUIDDefault_On(t *testing.T) { upCmd := NewUpCmd(&flags.GlobalFlags{}) - err := upCmd.ParseFlags([]string{"--update-remote-user-uid-default", "on"}) + err := upCmd.ParseFlags( + []string{"--update-remote-user-uid-default", UpdateRemoteUserUIDDefaultOn}, + ) require.NoError(t, err) val, err := upCmd.Flags().GetString("update-remote-user-uid-default") require.NoError(t, err) - assert.Equal(t, "on", val) + assert.Equal(t, UpdateRemoteUserUIDDefaultOn, val) } func TestUpCmd_UpdateRemoteUserUIDDefault_Off(t *testing.T) { upCmd := NewUpCmd(&flags.GlobalFlags{}) - err := upCmd.ParseFlags([]string{"--update-remote-user-uid-default", "off"}) + err := upCmd.ParseFlags( + []string{"--update-remote-user-uid-default", UpdateRemoteUserUIDDefaultOff}, + ) require.NoError(t, err) val, err := upCmd.Flags().GetString("update-remote-user-uid-default") require.NoError(t, err) - assert.Equal(t, "off", val) + assert.Equal(t, UpdateRemoteUserUIDDefaultOff, val) } func TestUpCmd_UpdateRemoteUserUIDDefault_Validate_Invalid(t *testing.T) { @@ -45,14 +49,14 @@ func TestUpCmd_UpdateRemoteUserUIDDefault_Validate_Empty(t *testing.T) { func TestUpCmd_UpdateRemoteUserUIDDefault_Validate_On(t *testing.T) { cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} - cmd.UpdateRemoteUserUIDDefault = "on" + cmd.UpdateRemoteUserUIDDefault = UpdateRemoteUserUIDDefaultOn err := cmd.validate() require.NoError(t, err) } func TestUpCmd_UpdateRemoteUserUIDDefault_Validate_Off(t *testing.T) { cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} - cmd.UpdateRemoteUserUIDDefault = "off" + cmd.UpdateRemoteUserUIDDefault = UpdateRemoteUserUIDDefaultOff err := cmd.validate() require.NoError(t, err) } diff --git a/e2e/tests/up/update_remote_user_uid.go b/e2e/tests/up/update_remote_user_uid.go index 859b0913e..7cc090c4a 100644 --- a/e2e/tests/up/update_remote_user_uid.go +++ b/e2e/tests/up/update_remote_user_uid.go @@ -12,6 +12,8 @@ import ( "github.com/onsi/gomega" ) +const testDockerCommand = "docker" + var _ = ginkgo.Describe( "testing --update-remote-user-uid-default flag", ginkgo.Label("up-update-remote-user-uid"), @@ -28,8 +30,11 @@ var _ = ginkgo.Describe( dtc.initialDir, err = os.Getwd() framework.ExpectNoError(err) - dtc.dockerHelper = &docker.DockerHelper{DockerCommand: "docker"} - dtc.f, err = setupDockerProvider(filepath.Join(dtc.initialDir, "bin"), "docker") + dtc.dockerHelper = &docker.DockerHelper{DockerCommand: testDockerCommand} + dtc.f, err = setupDockerProvider( + filepath.Join(dtc.initialDir, "bin"), + testDockerCommand, + ) framework.ExpectNoError(err) }) diff --git a/pkg/driver/docker/docker_test.go b/pkg/driver/docker/docker_test.go index c4b164df6..5c85c60fe 100644 --- a/pkg/driver/docker/docker_test.go +++ b/pkg/driver/docker/docker_test.go @@ -17,6 +17,7 @@ const ( testUpdateUIDDefaultOff = "off" testUpdateUIDDefaultOn = "on" testOSLinux = "linux" + testRemoteUser = "vscode" ) type DockerDriverTestSuite struct { @@ -89,7 +90,7 @@ func (s *DockerDriverTestSuite) TestShouldSkipUpdate_RootWithDifferentGID() { func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_DefaultTrue_WhenConfigNil() { cfg := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - RemoteUser: "vscode", + RemoteUser: testRemoteUser, }, } s.driver.UpdateRemoteUserUIDDefault = "" @@ -102,7 +103,7 @@ func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_DefaultTrue_WhenConfigNi func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_CLIDefaultOff_WhenConfigNil() { cfg := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - RemoteUser: "vscode", + RemoteUser: testRemoteUser, }, } s.driver.UpdateRemoteUserUIDDefault = testUpdateUIDDefaultOff @@ -113,7 +114,7 @@ func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_CLIDefaultOff_WhenConfig func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_CLIDefaultOn_WhenConfigNil() { cfg := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - RemoteUser: "vscode", + RemoteUser: testRemoteUser, }, } s.driver.UpdateRemoteUserUIDDefault = testUpdateUIDDefaultOn @@ -127,7 +128,7 @@ func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_ConfigTakesPrecedence_Tr t := true cfg := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - RemoteUser: "vscode", + RemoteUser: testRemoteUser, UpdateRemoteUserUID: &t, }, } @@ -142,7 +143,7 @@ func (s *DockerDriverTestSuite) TestShouldUpdateUserUID_ConfigTakesPrecedence_Fa f := false cfg := &config.DevContainerConfig{ DevContainerConfigBase: config.DevContainerConfigBase{ - RemoteUser: "vscode", + RemoteUser: testRemoteUser, UpdateRemoteUserUID: &f, }, }