From f7dcb8cd77af3fcdb7dab3b5b3ff25e28c196c67 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 6 Jul 2026 16:04:01 -0500 Subject: [PATCH 1/3] feat(devcontainer): volume workspaces, managed-volume labels, and config refactor Adds isolated named-volume workspace support and the surrounding infrastructure, and refactors the devcontainer config code it touches. Volume workspaces (docker): - Seed a named workspace volume from the local source folder on first up, as a faithful working-tree copy. --reset re-seeds a fresh snapshot. External (user-provided, unlabeled) volumes are never seeded or removed. - Label every devsy-managed volume with a shared vocabulary and clean them up on workspace delete, gated on the managed label so foreign volumes are left untouched. Consolidated cleanup behind a single Cleaner interface. Kubernetes: - Apply the same managed-volume labels to the workspace PVC. (Seeding is not needed: the PVC is already an isolated, persistently-populated workspace.) Labels: - Centralize all label/annotation keys in pkg/config, using each platform's idiomatic convention for the devsy.sh domain (docker: sh.devsy.*; kubernetes: devsy.sh/*). Reference external-contract keys in one place. Config fixes and refactor: - Honor --devcontainer-id over the root-config short-circuit. - Warn when workspaceMount/workspaceFolder is set without its partner. - Stop emitting the non-spec external flag into docker --mount. - Restructure pkg/devcontainer/config.go and pkg/devcontainer/config/parse.go for idiomatic Go: split by responsibility, replace the selector/bool params with a ParseOptions struct, thread context.Context, and decompose oversized methods to satisfy the project's lint gates. --- cmd/config/apply.go | 7 +- cmd/config/read.go | 3 +- cmd/feature/outdated.go | 8 +- cmd/feature/resolvedeps.go | 5 +- cmd/feature/upgrade.go | 2 +- cmd/internal/agent.go | 4 +- cmd/internal/runusercommands.go | 6 +- cmd/pro/check_update.go | 3 +- cmd/pro/daemon/netcheck.go | 3 +- cmd/pro/daemon/status.go | 4 +- cmd/pro/health.go | 3 +- cmd/pro/provider/list/workspaces.go | 3 +- cmd/pro/provider/provider.go | 6 +- e2e/tests/up-docker-compose/build.go | 17 +- e2e/tests/up-docker-compose/helper.go | 9 +- .../up-docker-compose/up_docker_compose.go | 13 +- e2e/tests/up/docker_wsl.go | 4 +- e2e/tests/up/dockerfile_build.go | 10 +- e2e/tests/up/helper.go | 4 +- e2e/tests/up/up.go | 4 +- pkg/agent/delivery/delivery.go | 8 +- pkg/agent/delivery/local_docker.go | 69 +++- pkg/agent/delivery/local_docker_test.go | 29 ++ pkg/agent/delivery/workspace_seed.go | 188 +++++++++ .../clientimplementation/daemonclient/form.go | 4 +- pkg/compose/helper.go | 10 +- pkg/config/labels.go | 68 ++++ pkg/config/labels_test.go | 86 ++++ pkg/daemon/platform/local_server.go | 4 +- pkg/devcontainer/build.go | 2 +- pkg/devcontainer/compose.go | 8 +- pkg/devcontainer/compose_test.go | 9 +- pkg/devcontainer/compose_up.go | 3 +- pkg/devcontainer/config.go | 369 +++++++++++------- pkg/devcontainer/config/build.go | 6 +- pkg/devcontainer/config/config.go | 9 +- pkg/devcontainer/config/discover.go | 167 ++++++++ pkg/devcontainer/config/envfile.go | 61 +++ pkg/devcontainer/config/extends_test.go | 56 +-- pkg/devcontainer/config/jsonutil.go | 13 + pkg/devcontainer/config/legacy.go | 59 +++ pkg/devcontainer/config/merge.go | 7 +- pkg/devcontainer/config/merge_test.go | 3 +- pkg/devcontainer/config/mount_test.go | 31 ++ pkg/devcontainer/config/parse.go | 347 +++++----------- pkg/devcontainer/config/parse_test.go | 147 +++++-- pkg/devcontainer/config/result.go | 8 +- pkg/devcontainer/config/substitute.go | 8 +- pkg/devcontainer/config/substitute_test.go | 16 +- pkg/devcontainer/config_test.go | 61 +++ pkg/devcontainer/delete.go | 7 +- pkg/devcontainer/feature/features.go | 2 +- pkg/devcontainer/metadata/metadata.go | 3 +- pkg/devcontainer/setup.go | 2 +- pkg/devcontainer/single.go | 41 +- pkg/driver/kubernetes/pvc.go | 3 +- pkg/driver/kubernetes/pvc_test.go | 43 ++ pkg/driver/kubernetes/run.go | 16 +- pkg/platform/form/form.go | 4 +- pkg/platform/labels/labels.go | 4 - pkg/workspace/list.go | 4 +- 61 files changed, 1508 insertions(+), 595 deletions(-) create mode 100644 pkg/agent/delivery/workspace_seed.go create mode 100644 pkg/config/labels.go create mode 100644 pkg/config/labels_test.go create mode 100644 pkg/devcontainer/config/discover.go create mode 100644 pkg/devcontainer/config/envfile.go create mode 100644 pkg/devcontainer/config/jsonutil.go create mode 100644 pkg/devcontainer/config/legacy.go create mode 100644 pkg/driver/kubernetes/pvc_test.go delete mode 100644 pkg/platform/labels/labels.go diff --git a/cmd/config/apply.go b/cmd/config/apply.go index b59316881..bbb7dfcff 100644 --- a/cmd/config/apply.go +++ b/cmd/config/apply.go @@ -177,13 +177,16 @@ func (cmd *ApplyCmd) loadConfig( var err error if cmd.Config != "" { - devContainerConfig, err = devcconfig.ParseDevContainerJSONFile(cmd.Config) + devContainerConfig, err = devcconfig.ParseDevContainerJSONFile( + context.Background(), + cmd.Config, + ) } else { cwd, cwdErr := os.Getwd() if cwdErr != nil { return nil, fmt.Errorf("get working directory: %w", cwdErr) } - devContainerConfig, err = devcconfig.ParseDevContainerJSON(cwd, "") + devContainerConfig, err = devcconfig.ParseDevContainerJSON(context.Background(), cwd, "") } if err != nil { return nil, fmt.Errorf("parse devcontainer config: %w", err) diff --git a/cmd/config/read.go b/cmd/config/read.go index 60d8ab343..3a06756fa 100644 --- a/cmd/config/read.go +++ b/cmd/config/read.go @@ -191,9 +191,10 @@ func (cmd *ReadCmd) resolveConfig() ( var parsedConfig *devcconfig.DevContainerConfig if cmd.Config != "" { - parsedConfig, err = devcconfig.ParseDevContainerJSONFile(cmd.Config) + parsedConfig, err = devcconfig.ParseDevContainerJSONFile(context.Background(), cmd.Config) } else { parsedConfig, err = devcconfig.ParseDevContainerJSON( + context.Background(), workspaceFolder, "", ) diff --git a/cmd/feature/outdated.go b/cmd/feature/outdated.go index d635fb61e..2f5aa09ee 100644 --- a/cmd/feature/outdated.go +++ b/cmd/feature/outdated.go @@ -92,9 +92,13 @@ func (cmd *OutdatedCmd) loadConfig() (*devconfig.DevContainerConfig, error) { var parsedConfig *devconfig.DevContainerConfig if cmd.Config != "" { - parsedConfig, err = devconfig.ParseDevContainerJSONFile(cmd.Config) + parsedConfig, err = devconfig.ParseDevContainerJSONFile(context.Background(), cmd.Config) } else { - parsedConfig, err = devconfig.ParseDevContainerJSON(workspaceFolder, "") + parsedConfig, err = devconfig.ParseDevContainerJSON( + context.Background(), + workspaceFolder, + "", + ) } if err != nil { return nil, fmt.Errorf("parse devcontainer config: %w", err) diff --git a/cmd/feature/resolvedeps.go b/cmd/feature/resolvedeps.go index 590edb974..079b6e7ff 100644 --- a/cmd/feature/resolvedeps.go +++ b/cmd/feature/resolvedeps.go @@ -1,6 +1,7 @@ package feature import ( + "context" "fmt" "os" "path/filepath" @@ -122,7 +123,7 @@ func buildResolvedList(sorted []*config.FeatureSet) []resolvedFeature { func (cmd *ResolveDepsCmd) loadConfig() (*config.DevContainerConfig, error) { if cmd.Config != "" { - return config.ParseDevContainerJSONFile(cmd.Config) + return config.ParseDevContainerJSONFile(context.Background(), cmd.Config) } absPath, err := filepath.Abs(cmd.WorkspaceFolder) @@ -130,7 +131,7 @@ func (cmd *ResolveDepsCmd) loadConfig() (*config.DevContainerConfig, error) { return nil, err } - return config.ParseDevContainerJSON(absPath, "") + return config.ParseDevContainerJSON(context.Background(), absPath, "") } func (cmd *ResolveDepsCmd) printText(resolved []resolvedFeature) error { diff --git a/cmd/feature/upgrade.go b/cmd/feature/upgrade.go index aa86ebdc9..2d9b84360 100644 --- a/cmd/feature/upgrade.go +++ b/cmd/feature/upgrade.go @@ -129,7 +129,7 @@ func (cmd *UpgradeCmd) applyUpgrades(configPath string, outdated []outdatedEntry old := entry.repo + ":" + entry.current updated := entry.repo + ":" + entry.latest content = strings.ReplaceAll(content, old, updated) - log.Infof("Upgraded %s: %s → %s", entry.repo, entry.current, entry.latest) + log.Infof("Upgraded %s: %s to %s", entry.repo, entry.current, entry.latest) } //nolint:gosec // G306 -- matching existing file permissions in the codebase diff --git a/cmd/internal/agent.go b/cmd/internal/agent.go index 28a805424..c2a7a9108 100644 --- a/cmd/internal/agent.go +++ b/cmd/internal/agent.go @@ -12,8 +12,6 @@ import ( "github.com/spf13/cobra" ) -var AgentExecutedAnnotation = "devsy.sh/agent-executed" - // NewAgentCmd is the hidden parent for commands that run inside a workspace or // container, invoked by the daemon over the agent tunnel. func NewAgentCmd(globalFlags *flags.GlobalFlags) *cobra.Command { @@ -48,7 +46,7 @@ func agentPreRunE(globalFlags *flags.GlobalFlags) func(*cobra.Command, []string) if root.Annotations == nil { root.Annotations = map[string]string{} } - root.Annotations[AgentExecutedAnnotation] = "true" + root.Annotations[config.AgentExecutedAnnotation] = "true" log.Init(log.Config{ Quiet: globalFlags.Quiet, diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index efbdfe464..1eee05cc7 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -272,7 +272,11 @@ func (cmd *RunUserCommandsCmd) loadContainerIDConfig( configFolder = "." } - devContainerConfig, err := devcconfig.ParseDevContainerJSON(configFolder, cmd.Config) + devContainerConfig, err := devcconfig.ParseDevContainerJSON( + context.Background(), + configFolder, + cmd.Config, + ) if err != nil { _ = devcconfig.WriteErrorJSON(os.Stderr, err.Error()) return nil, fmt.Errorf("parse devcontainer config: %w", err) diff --git a/cmd/pro/check_update.go b/cmd/pro/check_update.go index 848dda7c1..248f586c0 100644 --- a/cmd/pro/check_update.go +++ b/cmd/pro/check_update.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" - cmdinternal "github.com/devsy-org/devsy/cmd/internal" "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/cmd/pro/proutil" "github.com/devsy-org/devsy/pkg/config" @@ -55,7 +54,7 @@ func NewCheckUpdateCmd(globalFlags *flags.GlobalFlags) *cobra.Command { root.Annotations = map[string]string{} } // Don't print debug message - root.Annotations[cmdinternal.AgentExecutedAnnotation] = "true" //nolint:goconst + root.Annotations[config.AgentExecutedAnnotation] = "true" //nolint:goconst }, } diff --git a/cmd/pro/daemon/netcheck.go b/cmd/pro/daemon/netcheck.go index 6e308a8ec..6300bc001 100644 --- a/cmd/pro/daemon/netcheck.go +++ b/cmd/pro/daemon/netcheck.go @@ -5,7 +5,6 @@ import ( "fmt" "strconv" - cmdinternal "github.com/devsy-org/devsy/cmd/internal" "github.com/devsy-org/devsy/cmd/pro/completion" proflags "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/cmd/pro/proutil" @@ -54,7 +53,7 @@ func NewNetcheckCmd(flags *proflags.GlobalFlags) *cobra.Command { root.Annotations = map[string]string{} } // Don't print debug message - root.Annotations[cmdinternal.AgentExecutedAnnotation] = "true" + root.Annotations[config.AgentExecutedAnnotation] = "true" }, } diff --git a/cmd/pro/daemon/status.go b/cmd/pro/daemon/status.go index 11641da13..62dd5d4b2 100644 --- a/cmd/pro/daemon/status.go +++ b/cmd/pro/daemon/status.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" - cmdinternal "github.com/devsy-org/devsy/cmd/internal" "github.com/devsy-org/devsy/cmd/pro/completion" proflags "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/cmd/pro/proutil" @@ -51,8 +50,7 @@ func NewStatusCmd(flags *proflags.GlobalFlags) *cobra.Command { if root.Annotations == nil { root.Annotations = map[string]string{} } - // Don't print debug message - root.Annotations[cmdinternal.AgentExecutedAnnotation] = "true" + root.Annotations[config.AgentExecutedAnnotation] = "true" }, } diff --git a/cmd/pro/health.go b/cmd/pro/health.go index 7a1899aa0..2bf0083e6 100644 --- a/cmd/pro/health.go +++ b/cmd/pro/health.go @@ -5,7 +5,6 @@ import ( "context" "fmt" - cmdinternal "github.com/devsy-org/devsy/cmd/internal" "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/cmd/pro/proutil" "github.com/devsy-org/devsy/pkg/client/clientimplementation" @@ -57,7 +56,7 @@ func NewHealthCmd(globalFlags *flags.GlobalFlags) *cobra.Command { root.Annotations = map[string]string{} } // Don't print debug message - root.Annotations[cmdinternal.AgentExecutedAnnotation] = "true" //nolint:goconst + root.Annotations[config.AgentExecutedAnnotation] = "true" //nolint:goconst }, } diff --git a/cmd/pro/provider/list/workspaces.go b/cmd/pro/provider/list/workspaces.go index 7655b363b..f47df9a5d 100644 --- a/cmd/pro/provider/list/workspaces.go +++ b/cmd/pro/provider/list/workspaces.go @@ -12,7 +12,6 @@ import ( "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/platform" "github.com/devsy-org/devsy/pkg/platform/client" - "github.com/devsy-org/devsy/pkg/platform/labels" "github.com/devsy-org/devsy/pkg/platform/project" "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -85,7 +84,7 @@ func (cmd *WorkspacesCmd) Run(ctx context.Context) error { if instance.GetLabels() == nil { instance.Labels = map[string]string{} } - instance.Labels[labels.ProjectLabel] = p.GetName() + instance.Labels[config.K8sProjectLabel] = p.GetName() workspaces = append(workspaces, instance) } diff --git a/cmd/pro/provider/provider.go b/cmd/pro/provider/provider.go index d20241c33..4bb1fc138 100644 --- a/cmd/pro/provider/provider.go +++ b/cmd/pro/provider/provider.go @@ -3,7 +3,6 @@ package provider import ( "os" - cmdinternal "github.com/devsy-org/devsy/cmd/internal" "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/cmd/pro/provider/create" "github.com/devsy-org/devsy/cmd/pro/provider/get" @@ -33,16 +32,13 @@ func NewProProviderCmd(globalFlags *flags.GlobalFlags) *cobra.Command { globalFlags.Debug = true } - // Disable debug hints if we execute pro commands from Devsy Desktop - // We're reusing the cmdinternal.AgentExecutedAnnotation for simplicity, could rename in the future if os.Getenv(config.EnvUI) == config.BoolTrue { cmd.VisitParents(func(c *cobra.Command) { - // find the root command if c.Name() == config.BinaryName { if c.Annotations == nil { c.Annotations = map[string]string{} } - c.Annotations[cmdinternal.AgentExecutedAnnotation] = config.BoolTrue + c.Annotations[config.AgentExecutedAnnotation] = config.BoolTrue } }) } diff --git a/e2e/tests/up-docker-compose/build.go b/e2e/tests/up-docker-compose/build.go index 3e4ea007c..c7b4a3f37 100644 --- a/e2e/tests/up-docker-compose/build.go +++ b/e2e/tests/up-docker-compose/build.go @@ -12,6 +12,7 @@ import ( "github.com/devsy-org/devsy/e2e/framework" "github.com/devsy-org/devsy/pkg/compose" + pkgconfig "github.com/devsy-org/devsy/pkg/config" docker "github.com/devsy-org/devsy/pkg/docker" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" @@ -73,10 +74,10 @@ var _ = ginkgo.Describe( ids, err = dockerHelper.FindContainer(ctx, []string{ fmt.Sprintf( "%s=%s", - compose.ProjectLabel, + pkgconfig.ComposeProjectLabel, composeHelper.GetProjectName(workspace.UID), ), - fmt.Sprintf("%s=%s", compose.ServiceLabel, "app"), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, "app"), }) if err != nil { return 0 @@ -116,10 +117,10 @@ var _ = ginkgo.Describe( ids2, err := dockerHelper.FindContainer(ctx, []string{ fmt.Sprintf( "%s=%s", - compose.ProjectLabel, + pkgconfig.ComposeProjectLabel, composeHelper.GetProjectName(workspace.UID), ), - fmt.Sprintf("%s=%s", compose.ServiceLabel, "app"), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, "app"), }) framework.ExpectNoError(err) gomega.Expect(ids2[0]).To(gomega.Equal(ids[0]), "Should use original container") @@ -146,10 +147,10 @@ var _ = ginkgo.Describe( ids, err = dockerHelper.FindContainer(ctx, []string{ fmt.Sprintf( "%s=%s", - compose.ProjectLabel, + pkgconfig.ComposeProjectLabel, composeHelper.GetProjectName(workspace.UID), ), - fmt.Sprintf("%s=%s", compose.ServiceLabel, "app"), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, "app"), }) if err != nil { return 0 @@ -168,10 +169,10 @@ var _ = ginkgo.Describe( ids2, err := dockerHelper.FindContainer(ctx, []string{ fmt.Sprintf( "%s=%s", - compose.ProjectLabel, + pkgconfig.ComposeProjectLabel, composeHelper.GetProjectName(workspace.UID), ), - fmt.Sprintf("%s=%s", compose.ServiceLabel, "app"), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, "app"), }) framework.ExpectNoError(err) gomega.Expect(ids2[0]).NotTo(gomega.Equal(ids[0]), "Should restart container") diff --git a/e2e/tests/up-docker-compose/helper.go b/e2e/tests/up-docker-compose/helper.go index 257ddb4d6..2f793fed0 100644 --- a/e2e/tests/up-docker-compose/helper.go +++ b/e2e/tests/up-docker-compose/helper.go @@ -14,6 +14,7 @@ import ( "github.com/devsy-org/devsy/e2e/framework" "github.com/devsy-org/devsy/pkg/compose" + pkgconfig "github.com/devsy-org/devsy/pkg/config" docker "github.com/devsy-org/devsy/pkg/docker" provider2 "github.com/devsy-org/devsy/pkg/provider" "github.com/docker/docker/api/types/container" @@ -197,8 +198,12 @@ func findComposeContainer( workspaceUID, serviceName string, ) ([]string, error) { return dockerHelper.FindContainer(ctx, []string{ - fmt.Sprintf("%s=%s", compose.ProjectLabel, composeHelper.GetProjectName(workspaceUID)), - fmt.Sprintf("%s=%s", compose.ServiceLabel, serviceName), + fmt.Sprintf( + "%s=%s", + pkgconfig.ComposeProjectLabel, + composeHelper.GetProjectName(workspaceUID), + ), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, serviceName), }) } diff --git a/e2e/tests/up-docker-compose/up_docker_compose.go b/e2e/tests/up-docker-compose/up_docker_compose.go index 03136c68c..321f2713f 100644 --- a/e2e/tests/up-docker-compose/up_docker_compose.go +++ b/e2e/tests/up-docker-compose/up_docker_compose.go @@ -17,6 +17,7 @@ import ( "github.com/devsy-org/devsy/e2e/framework" "github.com/devsy-org/devsy/pkg/compose" + pkgconfig "github.com/devsy-org/devsy/pkg/config" docker "github.com/devsy-org/devsy/pkg/docker" "github.com/docker/docker/api/types/container" "github.com/onsi/ginkgo/v2" @@ -498,10 +499,10 @@ var _ = ginkgo.Describe( ids, err := tc.dockerHelper.FindContainer(ctx, []string{ fmt.Sprintf( "%s=%s", - compose.ProjectLabel, + pkgconfig.ComposeProjectLabel, tc.composeHelper.GetProjectName(workspace.UID), ), - fmt.Sprintf("%s=%s", compose.ServiceLabel, "app"), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, "app"), }) framework.ExpectNoError(err) gomega.Expect(ids).To(gomega.HaveLen(1), "1 compose container to be created") @@ -552,10 +553,10 @@ var _ = ginkgo.Describe( ids, err := tc.dockerHelper.FindContainer(ctx, []string{ fmt.Sprintf( "%s=%s", - compose.ProjectLabel, + pkgconfig.ComposeProjectLabel, tc.composeHelper.GetProjectName(workspace.UID), ), - fmt.Sprintf("%s=%s", compose.ServiceLabel, "app"), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, "app"), }) framework.ExpectNoError(err) gomega.Expect(ids).To(gomega.HaveLen(1), "1 compose container to be created") @@ -586,10 +587,10 @@ var _ = ginkgo.Describe( ids, err := tc.dockerHelper.FindContainer(ctx, []string{ fmt.Sprintf( "%s=%s", - compose.ProjectLabel, + pkgconfig.ComposeProjectLabel, tc.composeHelper.GetProjectName(workspace.UID), ), - fmt.Sprintf("%s=%s", compose.ServiceLabel, "app"), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, "app"), }) framework.ExpectNoError(err) gomega.Expect(ids).To(gomega.HaveLen(1), "1 compose container to be created") diff --git a/e2e/tests/up/docker_wsl.go b/e2e/tests/up/docker_wsl.go index acfc28323..d442dd661 100644 --- a/e2e/tests/up/docker_wsl.go +++ b/e2e/tests/up/docker_wsl.go @@ -8,7 +8,7 @@ import ( "strings" "github.com/devsy-org/devsy/e2e/framework" - "github.com/devsy-org/devsy/pkg/devcontainer/config" + pkgconfig "github.com/devsy-org/devsy/pkg/config" docker "github.com/devsy-org/devsy/pkg/docker" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" @@ -48,7 +48,7 @@ var _ = ginkgo.Describe("testing up command for windows", ginkgo.Label("up-docke projectName := workspace.ID ids, err := dockerHelper.FindContainer(ctx, []string{ - fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, workspace.UID), }) framework.ExpectNoError(err) gomega.Expect(ids).To(gomega.HaveLen(1), "1 compose container to be created") diff --git a/e2e/tests/up/dockerfile_build.go b/e2e/tests/up/dockerfile_build.go index 6f47475ba..985628eda 100644 --- a/e2e/tests/up/dockerfile_build.go +++ b/e2e/tests/up/dockerfile_build.go @@ -7,7 +7,7 @@ import ( "path/filepath" "github.com/devsy-org/devsy/e2e/framework" - "github.com/devsy-org/devsy/pkg/devcontainer/config" + pkgconfig "github.com/devsy-org/devsy/pkg/config" docker "github.com/devsy-org/devsy/pkg/docker" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" @@ -78,7 +78,7 @@ var _ = ginkgo.Describe( framework.ExpectNoError(err) container, err := dockerHelper.FindDevContainer(ctx, []string{ - fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, workspace.UID), }) framework.ExpectNoError(err) @@ -95,7 +95,7 @@ var _ = ginkgo.Describe( framework.ExpectNoError(err) container, err = dockerHelper.FindDevContainer(ctx, []string{ - fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, workspace.UID), }) framework.ExpectNoError(err) @@ -124,7 +124,7 @@ var _ = ginkgo.Describe( framework.ExpectNoError(err) container, err := dockerHelper.FindDevContainer(ctx, []string{ - fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, workspace.UID), }) framework.ExpectNoError(err) @@ -142,7 +142,7 @@ var _ = ginkgo.Describe( framework.ExpectNoError(err) container, err = dockerHelper.FindDevContainer(ctx, []string{ - fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, workspace.UID), }) framework.ExpectNoError(err) diff --git a/e2e/tests/up/helper.go b/e2e/tests/up/helper.go index 75a1ba5ab..2fbb84a38 100644 --- a/e2e/tests/up/helper.go +++ b/e2e/tests/up/helper.go @@ -8,7 +8,7 @@ import ( "strings" "github.com/devsy-org/devsy/e2e/framework" - "github.com/devsy-org/devsy/pkg/devcontainer/config" + pkgconfig "github.com/devsy-org/devsy/pkg/config" docker "github.com/devsy-org/devsy/pkg/docker" provider2 "github.com/devsy-org/devsy/pkg/provider" "github.com/devsy-org/devsy/pkg/scanner" @@ -61,7 +61,7 @@ func (dtc *dockerTestContext) findWorkspaceContainer( ) ([]string, error) { return dtc.dockerHelper.FindContainer( ctx, - []string{fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID)}, + []string{fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, workspace.UID)}, ) } diff --git a/e2e/tests/up/up.go b/e2e/tests/up/up.go index 08276c104..34c43449e 100644 --- a/e2e/tests/up/up.go +++ b/e2e/tests/up/up.go @@ -8,7 +8,7 @@ import ( "path/filepath" "github.com/devsy-org/devsy/e2e/framework" - "github.com/devsy-org/devsy/pkg/devcontainer/config" + pkgconfig "github.com/devsy-org/devsy/pkg/config" docker "github.com/devsy-org/devsy/pkg/docker" "github.com/devsy-org/devsy/pkg/language" "github.com/onsi/ginkgo/v2" @@ -168,7 +168,7 @@ var _ = ginkgo.Describe("testing up command", ginkgo.Label("up-workspaces"), fun projectName := workspace.ID ids, err := dockerHelper.FindContainer(ctx, []string{ - fmt.Sprintf("%s=%s", config.DockerIDLabel, workspace.UID), + fmt.Sprintf("%s=%s", pkgconfig.DevcontainerIDLabel, workspace.UID), }) framework.ExpectNoError(err) gomega.Expect(ids).To(gomega.HaveLen(1), "1 compose container to be created") diff --git a/pkg/agent/delivery/delivery.go b/pkg/agent/delivery/delivery.go index 0c3e1968b..5540aef3e 100644 --- a/pkg/agent/delivery/delivery.go +++ b/pkg/agent/delivery/delivery.go @@ -43,9 +43,15 @@ type PostStartOptions struct { Arch string } +// Cleaner removes the resources a delivery created for a workspace. Cleanup is +// best-effort and safe to call when nothing was created. +type Cleaner interface { + Cleanup(ctx context.Context, workspaceID string) error +} + type AgentDelivery interface { + Cleaner Phase() DeliveryPhase DeliverPreStart(ctx context.Context, opts PreStartOptions) error DeliverPostStart(ctx context.Context, opts PostStartOptions) error - Cleanup(ctx context.Context, workspaceID string) error } diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index f22607fe3..22eca6b11 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -24,6 +24,10 @@ const ( volumePrefix = "devsy-agent-" volumeMountPath = "/opt/devsy" defaultHelperImage = "busybox:latest" + + // cmdRun / flagRM build throwaway helper-container invocations. + cmdRun = "run" + flagRM = "--rm" ) type LocalDockerDelivery struct { @@ -44,7 +48,8 @@ func (d *LocalDockerDelivery) DeliverPreStart(ctx context.Context, opts PreStart volumeName := volumePrefix + opts.WorkspaceID - if err := d.createVolume(ctx, volumeName); err != nil { + labels := pkgconfig.DockerVolumeLabels(opts.WorkspaceID, pkgconfig.VolumeRoleAgent) + if err := d.createVolume(ctx, volumeName, labels); err != nil { return fmt.Errorf("create agent volume: %w", err) } @@ -70,8 +75,21 @@ func (d *LocalDockerDelivery) DeliverPostStart(_ context.Context, _ PostStartOpt return fmt.Errorf("LocalDockerDelivery does not support post-start delivery") } +// Cleanup removes every devsy-managed volume owned by the workspace (the agent +// volume and any seeded workspace volume), identified by labels. Only labeled +// volumes are removed, so foreign/external volumes are left untouched. func (d *LocalDockerDelivery) Cleanup(ctx context.Context, workspaceID string) error { - return d.removeVolume(ctx, workspaceID) + volumes, err := d.listManagedVolumes(ctx, workspaceID) + if err != nil { + return err + } + for _, name := range volumes { + if err := d.removeVolume(ctx, name); err != nil { + return err + } + log.Infof("removed devsy-managed volume: %s", name) + } + return nil } func (d *LocalDockerDelivery) ensureCurrentBinary( @@ -92,7 +110,7 @@ func (d *LocalDockerDelivery) ensureCurrentBinary( } if actual != "" { - log.Infof("upgraded remote agent from %s → %s", actual, expected) + log.Infof("upgraded remote agent from %s to %s", actual, expected) } if err := d.populateVolume(ctx, volumeName, binarySource, arch); err != nil { @@ -104,8 +122,14 @@ func (d *LocalDockerDelivery) ensureCurrentBinary( return nil } -func (d *LocalDockerDelivery) createVolume(ctx context.Context, name string) error { - out, err := d.cmd(ctx, "volume", "create", name).CombinedOutput() +func (d *LocalDockerDelivery) createVolume( + ctx context.Context, + name string, + labels map[string]string, +) error { + args := append([]string{"volume", "create"}, pkgconfig.LabelArgs(labels)...) + args = append(args, name) + out, err := d.cmd(ctx, args...).CombinedOutput() if err != nil { return fmt.Errorf("%s: %w", string(out), err) } @@ -133,7 +157,7 @@ func (d *LocalDockerDelivery) detectVolumeVersion(ctx context.Context, volumeNam binaryPath, binaryPath, ) args := []string{ - "run", "--rm", + cmdRun, flagRM, "-v", volumeName + ":" + volumeMountPath, d.helperImageName(), "sh", "-c", script, @@ -184,7 +208,7 @@ func (d *LocalDockerDelivery) populateVolumeWithHelper( volumeMountPath, binaryName(), volumeMountPath, binaryName(), ) args := []string{ - "run", "--rm", + cmdRun, flagRM, "--name", containerName, "-v", volumeName + ":" + volumeMountPath, "-i", @@ -263,15 +287,40 @@ func (d *LocalDockerDelivery) volumeMountpoint( return strings.TrimSpace(string(out)), nil } -func (d *LocalDockerDelivery) removeVolume(ctx context.Context, workspaceID string) error { - volumeName := volumePrefix + workspaceID - out, err := d.cmd(ctx, "volume", "rm", "-f", volumeName).CombinedOutput() +// removeVolume force-removes a single named volume. It is safe to call for a +// non-existent volume since removal is forced. +func (d *LocalDockerDelivery) removeVolume(ctx context.Context, name string) error { + out, err := d.cmd(ctx, "volume", "rm", "-f", name).CombinedOutput() if err != nil { return fmt.Errorf("%s: %w", string(out), err) } return nil } +// listManagedVolumes returns the names of devsy-managed volumes owned by the +// given workspace, identified by labels. Only labeled volumes are returned, so +// foreign (user-created) volumes are never included. +func (d *LocalDockerDelivery) listManagedVolumes( + ctx context.Context, + workspaceID string, +) ([]string, error) { + out, err := d.cmd(ctx, + "volume", "ls", "--quiet", + "--filter", "label="+pkgconfig.DockerManagedLabel+"=true", + "--filter", "label="+pkgconfig.DockerWorkspaceIDLabel+"="+workspaceID, + ).CombinedOutput() + if err != nil { + return nil, fmt.Errorf("%s: %w", string(out), err) + } + var names []string + for line := range strings.SplitSeq(strings.TrimSpace(string(out)), "\n") { + if name := strings.TrimSpace(line); name != "" { + names = append(names, name) + } + } + return names, nil +} + func (d *LocalDockerDelivery) cmd(ctx context.Context, args ...string) *exec.Cmd { // #nosec G204 -- args are constructed internally, not from user input cmd := exec.CommandContext(ctx, d.dockerCommand(), args...) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 8bf425200..2e72bd013 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -386,3 +386,32 @@ func TestExpectedVersion_FallsBackToGetVersion(t *testing.T) { d := &LocalDockerDelivery{} assert.NotEmpty(t, d.expectedVersion()) } + +func TestLocalDockerDelivery_Cleanup_RemovesManagedVolumes(t *testing.T) { + tmpDir := t.TempDir() + logPath := filepath.Join(tmpDir, "calls.log") + + // Fake docker: `volume ls` lists two managed volumes; `volume rm` records + // the removed name so the test can assert both were cleaned up. + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"ls\" ]; then\n" + + " printf 'devsy-agent-ws1\\nws1-workspace\\n'; exit 0\n" + + "fi\n" + + "if [ \"$1\" = \"volume\" ] && [ \"$2\" = \"rm\" ]; then\n" + + " echo \"$@\" >> \"" + logPath + "\"; exit 0\n" + + "fi\n" + + "exit 0\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + d := &LocalDockerDelivery{DockerCommand: scriptPath} + require.NoError(t, d.Cleanup(context.Background(), "ws1")) + + logged, err := os.ReadFile(logPath) //nolint:gosec // test reads a temp file we control + require.NoError(t, err) + removed := string(logged) + assert.Contains(t, removed, "devsy-agent-ws1") + assert.Contains(t, removed, "ws1-workspace") +} diff --git a/pkg/agent/delivery/workspace_seed.go b/pkg/agent/delivery/workspace_seed.go new file mode 100644 index 000000000..9d50fcf4c --- /dev/null +++ b/pkg/agent/delivery/workspace_seed.go @@ -0,0 +1,188 @@ +package delivery + +import ( + "context" + "fmt" + "strings" + + pkgconfig "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/log" +) + +// WorkspaceSeedOptions describes a request to seed a named workspace volume +// from a local source directory. +type WorkspaceSeedOptions struct { + // WorkspaceID owns the volume (used for labels). + WorkspaceID string + // VolumeName is the named volume backing the workspace mount. + VolumeName string + // SourceDir is the host/remote directory whose contents are copied into + // the volume (a faithful working-tree copy). + SourceDir string + // Reset removes an existing managed volume first so it is re-seeded. + Reset bool +} + +// WorkspaceVolumeSeeder is implemented by deliveries that can populate a named +// workspace volume from a local directory. +type WorkspaceVolumeSeeder interface { + SeedWorkspaceVolume(ctx context.Context, opts WorkspaceSeedOptions) error +} + +var _ WorkspaceVolumeSeeder = (*LocalDockerDelivery)(nil) + +// SeedWorkspaceVolume ensures the workspace volume exists and, unless it was +// already seeded, copies SourceDir into it as a faithful working-tree copy. +// Seeding is idempotent: an already-seeded volume is left untouched unless +// Reset is set. +func (d *LocalDockerDelivery) SeedWorkspaceVolume( + ctx context.Context, + opts WorkspaceSeedOptions, +) error { + if opts.VolumeName == "" || opts.SourceDir == "" { + return fmt.Errorf("volume name and source dir are required") + } + + proceed, err := d.prepareSeedTarget(ctx, opts) + if err != nil { + return err + } + if !proceed { + return nil + } + + labels := pkgconfig.DockerVolumeLabels(opts.WorkspaceID, pkgconfig.VolumeRoleWorkspace) + if err := d.createVolume(ctx, opts.VolumeName, labels); err != nil { + return fmt.Errorf("create workspace volume: %w", err) + } + + if err := d.copyDirIntoVolume(ctx, opts.SourceDir, opts.VolumeName); err != nil { + // Leave the volume in place but unseeded so a retry re-copies. + return fmt.Errorf("seed workspace volume: %w", err) + } + + if err := d.markVolumeSeeded(ctx, opts.VolumeName); err != nil { + log.Debugf("failed to mark volume %s seeded: %v", opts.VolumeName, err) + } + log.Infof("seeded workspace volume %s from %s", opts.VolumeName, opts.SourceDir) + return nil +} + +// prepareSeedTarget decides whether seeding should proceed and performs any +// required reset. It returns false (without error) when seeding should be +// skipped: the volume is external (present but not devsy-managed) or already +// seeded and no reset was requested. +func (d *LocalDockerDelivery) prepareSeedTarget( + ctx context.Context, + opts WorkspaceSeedOptions, +) (proceed bool, err error) { + managed, seeded, err := d.volumeSeedState(ctx, opts.VolumeName) + if err != nil { + return false, err + } + + // A pre-existing volume that devsy does not manage is treated as external: + // never overwrite user-provided content. + if !managed && d.volumeExists(ctx, opts.VolumeName) { + log.Debugf("workspace volume %s is not devsy-managed; skipping seed", opts.VolumeName) + return false, nil + } + + if opts.Reset && managed { + log.Warnf( + "--reset: removing workspace volume %s and re-seeding from source; "+ + "any changes made inside the volume will be lost", + opts.VolumeName, + ) + if err := d.removeVolume(ctx, opts.VolumeName); err != nil { + return false, fmt.Errorf("reset workspace volume: %w", err) + } + return true, nil + } + + if seeded { + log.Debugf("workspace volume %s already seeded; skipping", opts.VolumeName) + return false, nil + } + return true, nil +} + +// volumeSeedState reports whether the volume is devsy-managed (from its label) +// and whether it has already been seeded (from a sentinel file written inside +// the volume after a successful copy). Docker cannot add labels to an existing +// volume, so seeded state is tracked with the sentinel rather than a label. +func (d *LocalDockerDelivery) volumeSeedState( + ctx context.Context, + name string, +) (managed, seeded bool, err error) { + if !d.volumeExists(ctx, name) { + return false, false, nil + } + + out, err := d.cmd(ctx, + "volume", "inspect", + "--format", "{{index .Labels \""+pkgconfig.DockerManagedLabel+"\"}}", + name, + ).CombinedOutput() + if err != nil { + return false, false, fmt.Errorf("inspect volume %s: %s: %w", name, string(out), err) + } + managed = strings.TrimSpace(string(out)) == "true" + + return managed, d.volumeSeeded(ctx, name), nil +} + +// volumeSeeded reports whether the seeded sentinel file exists in the volume. +func (d *LocalDockerDelivery) volumeSeeded(ctx context.Context, name string) bool { + args := []string{ + cmdRun, flagRM, + "-v", name + ":/target:ro", + d.helperImageName(), + "sh", "-c", "[ -e /target/" + seededSentinel + " ]", + } + return d.cmd(ctx, args...).Run() == nil +} + +func (d *LocalDockerDelivery) volumeExists(ctx context.Context, name string) bool { + err := d.cmd(ctx, "volume", "inspect", name).Run() + return err == nil +} + +// copyDirIntoVolume copies the contents of sourceDir into the volume root using +// a throwaway helper container. The source is bind-mounted read-only. +func (d *LocalDockerDelivery) copyDirIntoVolume( + ctx context.Context, + sourceDir, volumeName string, +) error { + args := []string{ + cmdRun, flagRM, + "-v", sourceDir + ":/source:ro", + "-v", volumeName + ":/target", + d.helperImageName(), + "sh", "-c", "cp -a /source/. /target/", + } + out, err := d.cmd(ctx, args...).CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + return nil +} + +// markVolumeSeeded records that the volume has been populated by writing a +// sentinel file inside it. Docker cannot add labels to an existing volume, so +// the sentinel is the source of truth for seeded state. +func (d *LocalDockerDelivery) markVolumeSeeded(ctx context.Context, volumeName string) error { + args := []string{ + cmdRun, flagRM, + "-v", volumeName + ":/target", + d.helperImageName(), + "sh", "-c", "touch /target/" + seededSentinel, + } + out, err := d.cmd(ctx, args...).CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + return nil +} + +const seededSentinel = ".devsy-seeded" diff --git a/pkg/client/clientimplementation/daemonclient/form.go b/pkg/client/clientimplementation/daemonclient/form.go index 640678d12..4edd73915 100644 --- a/pkg/client/clientimplementation/daemonclient/form.go +++ b/pkg/client/clientimplementation/daemonclient/form.go @@ -11,11 +11,11 @@ import ( managementv1 "github.com/devsy-org/api/pkg/apis/management/v1" storagev1 "github.com/devsy-org/api/pkg/apis/storage/v1" "github.com/devsy-org/devsy/cmd/pro/provider/list" + "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/encoding" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/platform" platformclient "github.com/devsy-org/devsy/pkg/platform/client" - "github.com/devsy-org/devsy/pkg/platform/labels" "github.com/devsy-org/devsy/pkg/platform/project" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/yaml" @@ -100,7 +100,7 @@ func createInstanceInteractive( Labels: map[string]string{ storagev1.DevsyWorkspaceIDLabel: id, storagev1.DevsyWorkspaceUIDLabel: uid, - labels.ProjectLabel: selectedProject.GetName(), + config.K8sProjectLabel: selectedProject.GetName(), }, Annotations: map[string]string{ storagev1.DevsyWorkspacePictureAnnotation: picture, diff --git a/pkg/compose/helper.go b/pkg/compose/helper.go index 9d53d71ba..4820b4e6f 100644 --- a/pkg/compose/helper.go +++ b/pkg/compose/helper.go @@ -14,16 +14,12 @@ import ( "github.com/blang/semver/v4" composecli "github.com/compose-spec/compose-go/v2/cli" composetypes "github.com/compose-spec/compose-go/v2/types" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/docker" "github.com/devsy-org/devsy/pkg/log" ) -const ( - ProjectLabel = "com.docker.compose.project" - ServiceLabel = "com.docker.compose.service" -) - func LoadDockerComposeProject( ctx context.Context, paths []string, @@ -207,8 +203,8 @@ func (h *ComposeHelper) FindDevContainer( projectName, serviceName string, ) (*config.ContainerDetails, error) { containerIDs, err := h.Docker.FindContainer(ctx, []string{ - fmt.Sprintf("%s=%s", ProjectLabel, projectName), - fmt.Sprintf("%s=%s", ServiceLabel, serviceName), + fmt.Sprintf("%s=%s", pkgconfig.ComposeProjectLabel, projectName), + fmt.Sprintf("%s=%s", pkgconfig.ComposeServiceLabel, serviceName), }) if err != nil { return nil, err diff --git a/pkg/config/labels.go b/pkg/config/labels.go new file mode 100644 index 000000000..867b9b68e --- /dev/null +++ b/pkg/config/labels.go @@ -0,0 +1,68 @@ +package config + +const ( + Domain = BinaryName + ".sh" + ReverseDomain = "sh." + BinaryName + + DockerManagedLabel = ReverseDomain + ".managed" + DockerWorkspaceIDLabel = ReverseDomain + ".workspace-id" + DockerResourceLabel = ReverseDomain + ".resource" + DockerVolumeRoleLabel = ReverseDomain + ".volume-role" + DockerSeededLabel = ReverseDomain + ".seeded" + DockerUserLabel = BinaryName + ".user" + + K8sCreatedLabel = Domain + "/created" + K8sWorkspaceLabel = Domain + "/workspace" + K8sWorkspaceUIDLabel = Domain + "/workspace-uid" + K8sProjectLabel = Domain + "/project" + K8sManagedLabel = Domain + "/managed" + K8sResourceLabel = Domain + "/resource" + K8sVolumeRoleLabel = Domain + "/volume-role" + K8sInfoAnnotation = Domain + "/info" + K8sLastAppliedAnnotation = Domain + "/last-applied-configuration" + + AgentExecutedAnnotation = Domain + "/agent-executed" + + LabelValueTrue = "true" + ResourceVolume = "volume" + ResourceContainer = "container" + + VolumeRoleWorkspace = "workspace" + VolumeRoleAgent = "agent" + VolumeRoleFeature = "feature" + + DevcontainerIDLabel = "dev.containers.id" + DevcontainerMetadataLabel = "devcontainer.metadata" + + ComposeProjectLabel = "com.docker.compose.project" + ComposeServiceLabel = "com.docker.compose.service" + ComposeConfigFilesLabel = "com.docker.compose.project.config_files" + + ClusterAutoscalerSafeToEvictAnnotation = "cluster-autoscaler.kubernetes.io/safe-to-evict" +) + +func DockerVolumeLabels(workspaceID, role string) map[string]string { + return map[string]string{ + DockerManagedLabel: LabelValueTrue, + DockerResourceLabel: ResourceVolume, + DockerWorkspaceIDLabel: workspaceID, + DockerVolumeRoleLabel: role, + } +} + +func K8sVolumeLabels(workspaceID, role string) map[string]string { + return map[string]string{ + K8sManagedLabel: LabelValueTrue, + K8sResourceLabel: ResourceVolume, + K8sWorkspaceUIDLabel: workspaceID, + K8sVolumeRoleLabel: role, + } +} + +func LabelArgs(labels map[string]string) []string { + args := make([]string, 0, len(labels)*2) + for k, v := range labels { + args = append(args, "--label", k+"="+v) + } + return args +} diff --git a/pkg/config/labels_test.go b/pkg/config/labels_test.go new file mode 100644 index 000000000..82063df4d --- /dev/null +++ b/pkg/config/labels_test.go @@ -0,0 +1,86 @@ +package config + +import ( + "sort" + "strings" + "testing" +) + +func TestLabelKeyConventions(t *testing.T) { + // Docker keys use the reverse-DNS dotted form; k8s keys use prefix/name. + docker := []string{ + DockerManagedLabel, DockerWorkspaceIDLabel, DockerResourceLabel, + DockerVolumeRoleLabel, DockerSeededLabel, + } + for _, k := range docker { + if !strings.HasPrefix(k, ReverseDomain+".") { + t.Errorf("docker label %q should start with %q.", k, ReverseDomain) + } + if strings.Contains(k, "/") { + t.Errorf("docker label %q should not contain '/'", k) + } + } + + k8s := []string{ + K8sCreatedLabel, K8sWorkspaceUIDLabel, K8sProjectLabel, + K8sManagedLabel, K8sResourceLabel, K8sVolumeRoleLabel, + } + for _, k := range k8s { + if !strings.HasPrefix(k, Domain+"/") { + t.Errorf("k8s label %q should start with %q/", k, Domain) + } + } +} + +func TestDockerVolumeLabels(t *testing.T) { + labels := DockerVolumeLabels("ws-123", VolumeRoleWorkspace) + want := map[string]string{ + DockerManagedLabel: LabelValueTrue, + DockerResourceLabel: ResourceVolume, + DockerWorkspaceIDLabel: "ws-123", + DockerVolumeRoleLabel: VolumeRoleWorkspace, + } + for k, v := range want { + if labels[k] != v { + t.Errorf("label %s = %q, want %q", k, labels[k], v) + } + } +} + +func TestK8sVolumeLabels(t *testing.T) { + labels := K8sVolumeLabels("ws-123", VolumeRoleWorkspace) + want := map[string]string{ + K8sManagedLabel: LabelValueTrue, + K8sResourceLabel: ResourceVolume, + K8sWorkspaceUIDLabel: "ws-123", + K8sVolumeRoleLabel: VolumeRoleWorkspace, + } + for k, v := range want { + if labels[k] != v { + t.Errorf("label %s = %q, want %q", k, labels[k], v) + } + } +} + +func TestLabelArgs(t *testing.T) { + args := LabelArgs(map[string]string{ + DockerManagedLabel: LabelValueTrue, + DockerResourceLabel: ResourceVolume, + }) + if len(args) != 4 { + t.Fatalf("expected 4 args, got %d: %v", len(args), args) + } + var pairs []string + for i := 0; i < len(args); i += 2 { + if args[i] != "--label" { + t.Errorf("arg %d = %q, want --label", i, args[i]) + } + pairs = append(pairs, args[i+1]) + } + sort.Strings(pairs) + got := strings.Join(pairs, ",") + want := DockerManagedLabel + "=true," + DockerResourceLabel + "=" + ResourceVolume + if got != want { + t.Errorf("pairs = %q, want %q", got, want) + } +} diff --git a/pkg/daemon/platform/local_server.go b/pkg/daemon/platform/local_server.go index 5e14f279f..c0b68cfa2 100644 --- a/pkg/daemon/platform/local_server.go +++ b/pkg/daemon/platform/local_server.go @@ -10,13 +10,13 @@ import ( "time" managementv1 "github.com/devsy-org/api/pkg/apis/management/v1" + "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/dockercredentials" "github.com/devsy-org/devsy/pkg/gitcredentials" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/platform" platformclient "github.com/devsy-org/devsy/pkg/platform/client" "github.com/devsy-org/devsy/pkg/platform/kube" - "github.com/devsy-org/devsy/pkg/platform/labels" "github.com/devsy-org/devsy/pkg/platform/project" "github.com/gorilla/handlers" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -465,7 +465,7 @@ func collectProjectWorkspaces( if instance.GetLabels() == nil { instance.Labels = map[string]string{} } - instance.Labels[labels.ProjectLabel] = p.GetName() + instance.Labels[config.K8sProjectLabel] = p.GetName() instances = append(instances, instance) } return instances, nil diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index 6ed36784c..cb3b03e50 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -48,7 +48,7 @@ func (r *runner) build( if buildInfo.ImageMetadata == nil { buildInfo.ImageMetadata = &config.ImageMetadataConfig{} } - extraConfig, err := config.ParseDevContainerJSONFile(options.ExtraDevContainerPath) + extraConfig, err := config.ParseDevContainerJSONFile(ctx, options.ExtraDevContainerPath) if err != nil { return nil, err } diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index cbcae5484..962a55407 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -10,6 +10,7 @@ import ( composetypes "github.com/compose-spec/compose-go/v2/types" "github.com/devsy-org/devsy/pkg/compose" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/devcontainer/metadata" "github.com/devsy-org/devsy/pkg/driver" @@ -18,7 +19,7 @@ import ( ) const ( - ConfigFilesLabel = "com.docker.compose.project.config_files" + ConfigFilesLabel = pkgconfig.ComposeConfigFilesLabel FeaturesBuildOverrideFilePrefix = "docker-compose.devcontainer.build" FeaturesStartOverrideFilePrefix = "docker-compose.devcontainer.containerFeatures" @@ -839,7 +840,10 @@ func mergeImageMetadataConfig( if imageMetadata == nil { imageMetadata = &config.ImageMetadataConfig{} } - extraConfig, err := config.ParseDevContainerJSONFile(extraDevContainerPath) + extraConfig, err := config.ParseDevContainerJSONFile( + context.Background(), + extraDevContainerPath, + ) if err != nil { return nil, err } diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index ea0c08516..845e08994 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -8,6 +8,7 @@ import ( composetypes "github.com/compose-spec/compose-go/v2/types" "github.com/devsy-org/devsy/pkg/compose" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/devcontainer/feature" "github.com/stretchr/testify/suite" @@ -555,8 +556,12 @@ func TestBuildServiceLabels(t *testing.T) { labels := r.buildServiceLabels(nil) - if labels[config.DockerIDLabel] != "workspace-id" { - t.Errorf("default ID label = %q, want %q", labels[config.DockerIDLabel], "workspace-id") + if labels[pkgconfig.DevcontainerIDLabel] != "workspace-id" { + t.Errorf( + "default ID label = %q, want %q", + labels[pkgconfig.DevcontainerIDLabel], + "workspace-id", + ) } }) diff --git a/pkg/devcontainer/compose_up.go b/pkg/devcontainer/compose_up.go index 885607aef..c6747351d 100644 --- a/pkg/devcontainer/compose_up.go +++ b/pkg/devcontainer/compose_up.go @@ -6,6 +6,7 @@ import ( composetypes "github.com/compose-spec/compose-go/v2/types" "github.com/devsy-org/devsy/pkg/compose" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/log" "gopkg.in/yaml.v3" @@ -157,7 +158,7 @@ func (r *runner) buildServiceLabels(additionalLabels map[string]string) composet labels[k] = escapeComposeLabelValue(v) } } else { - labels[config.DockerIDLabel] = r.ID + labels[pkgconfig.DevcontainerIDLabel] = r.ID } for k, v := range additionalLabels { labels.Add(k, escapeComposeLabelValue(v)) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 84211c659..9c3f602f5 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -1,6 +1,7 @@ package devcontainer import ( + "context" "encoding/json" "fmt" "maps" @@ -14,212 +15,278 @@ import ( "github.com/devsy-org/devsy/pkg/devcontainer/crane" "github.com/devsy-org/devsy/pkg/language" "github.com/devsy-org/devsy/pkg/log" - provider2 "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/provider" ) -func (r *runner) getRawConfig(options provider2.CLIOptions) (*config.DevContainerConfig, error) { - if r.WorkspaceConfig.Workspace.DevContainerConfig != nil { - rawParsedConfig := config.CloneDevContainerConfig( - r.WorkspaceConfig.Workspace.DevContainerConfig, - ) - if r.WorkspaceConfig.Workspace.DevContainerPath != "" { - rawParsedConfig.Origin = path.Join( - filepath.ToSlash(r.LocalWorkspaceFolder), - r.WorkspaceConfig.Workspace.DevContainerPath, - ) - } else { - rawParsedConfig.Origin = path.Join( - filepath.ToSlash(r.LocalWorkspaceFolder), - ".devcontainer."+pkgconfig.BinaryName+".json", - ) - } - return rawParsedConfig, nil - } else if r.WorkspaceConfig.Workspace.Source.Container != "" { - return &config.DevContainerConfig{ - DevContainerConfigBase: config.DevContainerConfigBase{ - // Default workspace directory for containers - // Upon inspecting the container, this would be updated to the correct folder, if found set - WorkspaceFolder: "/", - }, - RunningContainer: config.RunningContainer{ - ContainerID: r.WorkspaceConfig.Workspace.Source.Container, - }, - Origin: "", - }, nil - } else if crane.ShouldUse(&options) { - localWorkspaceFolder, err := crane.PullConfigFromSource(r.WorkspaceConfig, &options) - if err != nil { - return nil, err - } +// getRawConfig resolves the raw devcontainer config for the workspace, trying +// each supported source in order and falling back to an auto-detected default +// when none applies. +func (r *runner) getRawConfig(options provider.CLIOptions) (*config.DevContainerConfig, error) { + if conf := r.rawConfigFromWorkspace(); conf != nil { + return conf, nil + } + if conf := r.rawConfigFromContainer(); conf != nil { + return conf, nil + } + if crane.ShouldUse(&options) { + return r.rawConfigFromCrane(options) + } + return r.rawConfigFromFilesystem(options) +} + +// rawConfigFromWorkspace returns the config embedded in the workspace metadata, +// or nil when none is present. +func (r *runner) rawConfigFromWorkspace() *config.DevContainerConfig { + if r.WorkspaceConfig.Workspace.DevContainerConfig == nil { + return nil + } - return config.ParseDevContainerJSON( - localWorkspaceFolder, - r.WorkspaceConfig.Workspace.DevContainerPath, + rawConfig := config.CloneDevContainerConfig(r.WorkspaceConfig.Workspace.DevContainerConfig) + if devContainerPath := r.WorkspaceConfig.Workspace.DevContainerPath; devContainerPath != "" { + rawConfig.Origin = path.Join(filepath.ToSlash(r.LocalWorkspaceFolder), devContainerPath) + } else { + rawConfig.Origin = path.Join( + filepath.ToSlash(r.LocalWorkspaceFolder), + ".devcontainer."+pkgconfig.BinaryName+".json", ) } + return rawConfig +} - localWorkspaceFolder := r.LocalWorkspaceFolder - // if a subpath is specified, let's move to it +// rawConfigFromContainer returns a synthetic config for a running-container +// source, or nil when the source is not a container. +func (r *runner) rawConfigFromContainer() *config.DevContainerConfig { + containerID := r.WorkspaceConfig.Workspace.Source.Container + if containerID == "" { + return nil + } - if r.WorkspaceConfig.Workspace.Source.GitSubPath != "" { - localWorkspaceFolder = filepath.Join( - r.LocalWorkspaceFolder, - r.WorkspaceConfig.Workspace.Source.GitSubPath, - ) + return &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + // Default workspace directory for containers. Once the container is + // inspected this is updated to the discovered folder, if any. + WorkspaceFolder: "/", + }, + RunningContainer: config.RunningContainer{ContainerID: containerID}, + } +} + +// rawConfigFromCrane pulls the config from the image source via crane. +func (r *runner) rawConfigFromCrane( + options provider.CLIOptions, +) (*config.DevContainerConfig, error) { + localWorkspaceFolder, err := crane.PullConfigFromSource(r.WorkspaceConfig, &options) + if err != nil { + return nil, err } + return config.ParseDevContainerJSON( + context.Background(), + localWorkspaceFolder, + r.WorkspaceConfig.Workspace.DevContainerPath, + ) +} - // parse the devcontainer json - var rawParsedConfig *config.DevContainerConfig - var err error +// rawConfigFromFilesystem discovers and parses the devcontainer.json under the +// workspace folder, falling back to an auto-detected default when none is found. +func (r *runner) rawConfigFromFilesystem( + options provider.CLIOptions, +) (*config.DevContainerConfig, error) { + localWorkspaceFolder := r.LocalWorkspaceFolder + if subPath := r.WorkspaceConfig.Workspace.Source.GitSubPath; subPath != "" { + localWorkspaceFolder = filepath.Join(localWorkspaceFolder, subPath) + } + opts := config.ParseOptions{Selector: config.SelectSingle(localWorkspaceFolder)} if options.DevContainerID != "" { - // Use selector to find specific devcontainer by ID - rawParsedConfig, err = config.ParseDevContainerJSONWithSelector( - localWorkspaceFolder, - r.WorkspaceConfig.Workspace.DevContainerPath, - func(matches []string) (string, error) { - for _, match := range matches { - if filepath.Base(filepath.Dir(match)) == options.DevContainerID { - return match, nil - } - } - return "", fmt.Errorf("devcontainer with ID %q not found", options.DevContainerID) - }, - ) - } else { - rawParsedConfig, err = config.ParseDevContainerJSONWithSelector( - localWorkspaceFolder, - r.WorkspaceConfig.Workspace.DevContainerPath, - func(matches []string) (string, error) { - if len(matches) > 1 { - ids, _ := config.ListDevContainerIDs(localWorkspaceFolder) - return "", fmt.Errorf( - "multiple devcontainer configurations found. Use --devcontainer-id to select one: %v", - ids, - ) - } - return matches[0], nil - }, - ) + // An explicit id must not be shadowed by a root config, and a mismatch + // must error rather than silently fall back. + opts = config.ParseOptions{ + Selector: config.SelectByID(options.DevContainerID), + ForceSelect: true, + } } - // We want to fail only in case of real errors, non-existing devcontainer.jon - // will be gracefully handled by the auto-detection mechanism + rawConfig, err := config.ParseDevContainerJSONWithOptions( + context.Background(), + localWorkspaceFolder, + r.WorkspaceConfig.Workspace.DevContainerPath, + opts, + ) + // A missing devcontainer.json is not an error: fall back to auto-detection. if err != nil && !os.IsNotExist(err) { return nil, fmt.Errorf("parsing devcontainer.json: %w", err) - } else if rawParsedConfig == nil { + } + if rawConfig == nil { log.Infof("Couldn't find a devcontainer.json") return r.getDefaultConfig(options) } - return rawParsedConfig, nil + + if msg := workspaceMountFolderWarning(rawConfig); msg != "" { + log.Warnf("%s", msg) + } + return rawConfig, nil +} + +// workspaceMountFolderWarning returns a warning when only one of +// workspaceMount / workspaceFolder is set, and "" when the pairing is fine. +// Per the devcontainer spec both must be set together +// (https://containers.dev/implementors/json_reference/). devsy still applies a +// sensible default for the missing one, so this is a warning, not an error. +func workspaceMountFolderWarning(conf *config.DevContainerConfig) string { + if conf == nil { + return "" + } + // A present workspaceMount satisfies the pairing even when it is the empty + // string, which is devsy's documented "suppress the default mount" signal. + hasMount := conf.WorkspaceMount != nil + hasFolder := conf.WorkspaceFolder != "" + switch { + case hasMount && !hasFolder: + return "devcontainer.json sets workspaceMount without workspaceFolder; " + + "the spec requires both. Falling back to the default workspace folder." + case hasFolder && !hasMount: + return "devcontainer.json sets workspaceFolder without workspaceMount; " + + "the spec requires both. Falling back to the default workspace mount." + } + return "" } func (r *runner) getDefaultConfig( - options provider2.CLIOptions, + options provider.CLIOptions, ) (*config.DevContainerConfig, error) { defaultConfig := &config.DevContainerConfig{} if options.FallbackImage != "" { log.Infof("Using fallback image %s", options.FallbackImage) - defaultConfig.ImageContainer = config.ImageContainer{ - Image: options.FallbackImage, - } + defaultConfig.ImageContainer = config.ImageContainer{Image: options.FallbackImage} } else { log.Infof("Try detecting project programming language") defaultConfig = language.DefaultConfig(r.LocalWorkspaceFolder) } defaultConfig.Origin = path.Join(filepath.ToSlash(r.LocalWorkspaceFolder), ".devcontainer.json") - err := config.SaveDevContainerJSON(defaultConfig) - if err != nil { + if err := config.SaveDevContainerJSON(defaultConfig); err != nil { return nil, fmt.Errorf("write default devcontainer.json: %w", err) } return defaultConfig, nil } func (r *runner) getSubstitutedConfig( - options provider2.CLIOptions, + options provider.CLIOptions, ) (*config.SubstitutedConfig, *config.SubstitutionContext, error) { rawConfig, err := r.getRawConfig(options) if err != nil { return nil, nil, err } - return r.substitute(options, rawConfig) } +// substitute resolves devcontainer.json variables and applies CLI overrides, +// returning the substituted config alongside the substitution context used. func (r *runner) substitute( - options provider2.CLIOptions, + options provider.CLIOptions, rawParsedConfig *config.DevContainerConfig, ) (*config.SubstitutedConfig, *config.SubstitutionContext, error) { - configFile := rawParsedConfig.Origin + substitutionContext := r.buildSubstitutionContext(options, rawParsedConfig) - // get workspace folder within container + parsedConfig, err := applySubstitution(substitutionContext, rawParsedConfig) + if err != nil { + return nil, nil, err + } + + applyMountContext(substitutionContext, parsedConfig, options) + + if err := applyCLIOverrides(parsedConfig, options); err != nil { + return nil, nil, err + } + + parsedConfig.Origin = rawParsedConfig.Origin + return &config.SubstitutedConfig{ + Config: parsedConfig, + Raw: rawParsedConfig, + }, substitutionContext, nil +} + +// buildSubstitutionContext assembles the context used for variable substitution: +// the derived container id, workspace folders, and the merged environment. +func (r *runner) buildSubstitutionContext( + options provider.CLIOptions, + rawParsedConfig *config.DevContainerConfig, +) *config.SubstitutionContext { + configFile := rawParsedConfig.Origin workspaceMount, containerWorkspaceFolder := getWorkspace( r.LocalWorkspaceFolder, r.WorkspaceConfig.Workspace.ID, rawParsedConfig, ) - // merge InitEnv into environment for variable substitution env := config.ListToObject(os.Environ()) if len(options.InitEnv) > 0 { - initEnv := config.ListToObject(options.InitEnv) - maps.Copy(env, initEnv) + maps.Copy(env, config.ListToObject(options.InitEnv)) } - substitutionContext := &config.SubstitutionContext{ + return &config.SubstitutionContext{ DevContainerID: config.DeriveDevContainerID(r.LocalWorkspaceFolder, configFile), LocalWorkspaceFolder: r.LocalWorkspaceFolder, ContainerWorkspaceFolder: containerWorkspaceFolder, Env: env, - - WorkspaceMount: workspaceMount, + WorkspaceMount: workspaceMount, } +} - // Substitute applies phase-aware variable scoping per the devcontainer spec - // (https://containers.dev/implementors/reference/ — "Variables in devcontainer.json"): - // - All workspace-folder variables (local* and container*) are resolved - // host-side, including in containerEnv values, because the host knows - // ContainerWorkspaceFolder (it is computed by getWorkspace). - // - Post-container fields (remoteEnv, lifecycle commands, etc.) likewise - // resolve containerWorkspaceFolder and containerWorkspaceFolderBasename. - // - Only containerEnv references (${containerEnv:VAR}) remain literal here; - // they are resolved host-side from the image's inspected env (see - // ResolveContainerEnvFromImage) before passing to `docker run -e`, and - // for remoteEnv inside the container via SubstituteContainerEnv. +// applySubstitution runs variable substitution over the raw config. When the +// config overrides the container workspace folder, substitution is re-run so +// dependent values pick up the override. +// +// Substitution is phase-aware per the devcontainer spec +// (https://containers.dev/implementors/reference/): workspace-folder variables +// are resolved host-side, while container-env references are resolved later +// once the image environment is known. +func applySubstitution( + substitutionContext *config.SubstitutionContext, + rawParsedConfig *config.DevContainerConfig, +) (*config.DevContainerConfig, error) { parsedConfig := &config.DevContainerConfig{} - err := config.Substitute(substitutionContext, rawParsedConfig, parsedConfig) - if err != nil { - return nil, nil, err + if err := config.Substitute(substitutionContext, rawParsedConfig, parsedConfig); err != nil { + return nil, err } - if parsedConfig.WorkspaceFolder != "" && - parsedConfig.WorkspaceFolder != substitutionContext.ContainerWorkspaceFolder { - // WorkspaceFolder was overridden via devcontainer.json. Re-run - // substitution so containerEnv/remoteEnv values that reference - // ${containerWorkspaceFolder} pick up the override. - substitutionContext.ContainerWorkspaceFolder = parsedConfig.WorkspaceFolder - reSubstituted := &config.DevContainerConfig{} - if err := config.Substitute( - substitutionContext, - rawParsedConfig, - reSubstituted, - ); err != nil { - return nil, nil, err - } - parsedConfig = reSubstituted + + overridesWorkspaceFolder := parsedConfig.WorkspaceFolder != "" && + parsedConfig.WorkspaceFolder != substitutionContext.ContainerWorkspaceFolder + if !overridesWorkspaceFolder { + return parsedConfig, nil + } + + substitutionContext.ContainerWorkspaceFolder = parsedConfig.WorkspaceFolder + reSubstituted := &config.DevContainerConfig{} + if err := config.Substitute(substitutionContext, rawParsedConfig, reSubstituted); err != nil { + return nil, err } + return reSubstituted, nil +} + +// applyMountContext finalizes the workspace mount on the substitution context +// from the parsed config and the CLI consistency flag. +func applyMountContext( + substitutionContext *config.SubstitutionContext, + parsedConfig *config.DevContainerConfig, + options provider.CLIOptions, +) { if parsedConfig.WorkspaceMount != nil { substitutionContext.WorkspaceMount = *parsedConfig.WorkspaceMount } - if options.WorkspaceMountConsistency != "" { substitutionContext.WorkspaceMount = mountSetConsistency( substitutionContext.WorkspaceMount, options.WorkspaceMountConsistency, ) } +} - // merge additional mounts from CLI --mount flags +// applyCLIOverrides applies CLI-provided overrides onto the parsed config. +func applyCLIOverrides( + parsedConfig *config.DevContainerConfig, + options provider.CLIOptions, +) error { for _, mountStr := range options.Mounts { m := config.ParseMount(mountStr) parsedConfig.Mounts = append(parsedConfig.Mounts, &m) @@ -232,29 +299,29 @@ func (r *runner) substitute( parsedConfig.ImageContainer = config.ImageContainer{Image: options.DevContainerImage} } - // merge additional features from CLI flag - if options.AdditionalFeatures != "" { - additionalFeatures := make(map[string]any) - if err := json.Unmarshal( - []byte(options.AdditionalFeatures), - &additionalFeatures, - ); err != nil { - return nil, nil, fmt.Errorf("parse --additional-features JSON: %w", err) - } - if parsedConfig.Features == nil { - parsedConfig.Features = make(map[string]any) - } - maps.Copy(parsedConfig.Features, additionalFeatures) - log.Infof( - "Merged %d additional feature(s): %v", - len(additionalFeatures), - slices.Collect(maps.Keys(additionalFeatures)), - ) + return mergeAdditionalFeatures(parsedConfig, options.AdditionalFeatures) +} + +// mergeAdditionalFeatures merges extra features (a JSON object) into the parsed +// config. It is a no-op when raw is empty. +func mergeAdditionalFeatures(parsedConfig *config.DevContainerConfig, raw string) error { + if raw == "" { + return nil } - parsedConfig.Origin = configFile - return &config.SubstitutedConfig{ - Config: parsedConfig, - Raw: rawParsedConfig, - }, substitutionContext, nil + additionalFeatures := make(map[string]any) + if err := json.Unmarshal([]byte(raw), &additionalFeatures); err != nil { + return fmt.Errorf("parse --additional-features JSON: %w", err) + } + + if parsedConfig.Features == nil { + parsedConfig.Features = make(map[string]any) + } + maps.Copy(parsedConfig.Features, additionalFeatures) + log.Infof( + "Merged %d additional feature(s): %v", + len(additionalFeatures), + slices.Collect(maps.Keys(additionalFeatures)), + ) + return nil } diff --git a/pkg/devcontainer/config/build.go b/pkg/devcontainer/config/build.go index e7f1b3c4e..b5f2897ee 100644 --- a/pkg/devcontainer/config/build.go +++ b/pkg/devcontainer/config/build.go @@ -9,15 +9,13 @@ import ( ) const ( - DockerIDLabel = "dev.containers.id" - DockerfileDefaultTarget = "dev_container_auto_added_stage_label" - + DockerfileDefaultTarget = "dev_container_auto_added_stage_label" DevsyContextFeatureFolder = pkgconfig.ConfigDirName + "-internal" DevsyDockerlessBuildInfoFolder = "/workspaces/.dockerless" ) func GetDockerLabelForID(id string) []string { - return []string{DockerIDLabel + "=" + id} + return []string{pkgconfig.DevcontainerIDLabel + "=" + id} } func GetIDLabels(id string, idLabels []string) []string { diff --git a/pkg/devcontainer/config/config.go b/pkg/devcontainer/config/config.go index 92daa285a..067d1a864 100644 --- a/pkg/devcontainer/config/config.go +++ b/pkg/devcontainer/config/config.go @@ -39,7 +39,7 @@ type DevContainerConfig struct { func CloneDevContainerConfig(config *DevContainerConfig) *DevContainerConfig { out := &DevContainerConfig{} - _ = Convert(config, out) + _ = convert(config, out) out.Origin = config.Origin return out } @@ -502,6 +502,10 @@ type Mount struct { Other []string `json:"other,omitempty"` } +// String renders the mount as a docker/podman --mount argument. The external +// flag is intentionally omitted: it is a Docker Compose volume-level directive +// (handled separately in the compose path) and is not a valid --mount option, +// so emitting it here would produce an invalid argument. func (m *Mount) String() string { components := []string{} if m.Type != "" { @@ -513,9 +517,6 @@ func (m *Mount) String() string { if m.Target != "" { components = append(components, "dst="+m.Target) } - if m.External { - components = append(components, "external="+strconv.FormatBool(m.External)) - } components = append(components, m.Other...) return strings.Join(components, ",") } diff --git a/pkg/devcontainer/config/discover.go b/pkg/devcontainer/config/discover.go new file mode 100644 index 000000000..d4fbef25d --- /dev/null +++ b/pkg/devcontainer/config/discover.go @@ -0,0 +1,167 @@ +package config + +import ( + "fmt" + "os" + "path" + "path/filepath" + + doublestar "github.com/bmatcuk/doublestar/v4" +) + +// devcontainerDirName is the conventional directory that holds devcontainer +// configuration, per https://containers.dev/implementors/spec/#devcontainerjson. +const devcontainerDirName = ".devcontainer" + +// ConfigSelector chooses a single devcontainer.json path from the candidates +// discovered under a workspace folder. It is only consulted when a folder +// contains more than one configuration (or when selection is forced). +type ConfigSelector func(candidates []string) (string, error) + +// SelectByID returns a selector that picks the config whose parent directory +// name matches id (the devcontainer id, e.g. ".devcontainer//devcontainer.json"). +func SelectByID(id string) ConfigSelector { + return func(candidates []string) (string, error) { + for _, candidate := range candidates { + if filepath.Base(filepath.Dir(candidate)) == id { + return candidate, nil + } + } + return "", fmt.Errorf("devcontainer with ID %q not found", id) + } +} + +// SelectSingle returns a selector that rejects ambiguity: it errors when more +// than one config exists, listing the available ids so the caller can choose. +func SelectSingle(folder string) ConfigSelector { + return func(candidates []string) (string, error) { + if len(candidates) > 1 { + ids, _ := ListDevContainerIDs(folder) + return "", fmt.Errorf( + "multiple devcontainer configurations found. Detected: %v", + ids, + ) + } + return candidates[0], nil + } +} + +// resolveDevContainerPath locates the devcontainer.json to use for a folder. +// +// Resolution order: +// 1. an explicit relative path, when provided; +// 2. unless forceSelect is set, the conventional root configs +// (.devcontainer/devcontainer.json then .devcontainer.json); +// 3. discovered nested configs, passed to the selector. +// +// forceSelect skips the root-config short-circuits so an explicitly requested +// config cannot be shadowed by a root config, and so a mismatched request +// errors even when a single config exists. It returns an empty path (nil error) +// when no configuration is found. +func resolveDevContainerPath( + folder, relativePath string, + selector ConfigSelector, + forceSelect bool, +) (string, error) { + if relativePath != "" { + configPath := path.Join(filepath.ToSlash(folder), relativePath) + if _, err := os.Stat(configPath); err != nil { + return "", fmt.Errorf("devcontainer path %s does not exist: %w", configPath, err) + } + return configPath, nil + } + + if !forceSelect { + if rootPath, ok := findRootConfig(folder); ok { + return rootPath, nil + } + } + + matches, err := findDevContainerConfigs(folder) + if err != nil { + return "", err + } + return selectMatch(matches, selector, forceSelect) +} + +// selectMatch resolves the discovered candidates to a single path. It returns +// "" when there are no matches. A selector runs when forced, or when the choice +// is ambiguous; otherwise the sole match is returned. +func selectMatch(matches []string, selector ConfigSelector, forceSelect bool) (string, error) { + switch { + case len(matches) == 0: + return "", nil + case forceSelect && selector != nil: + return selector(matches) + case len(matches) == 1 || selector == nil: + return matches[0], nil + default: + return selector(matches) + } +} + +// findRootConfig returns the conventional root config path for a folder, if one +// exists: .devcontainer/devcontainer.json takes precedence over .devcontainer.json. +func findRootConfig(folder string) (string, bool) { + candidates := []string{ + filepath.Join(folder, devcontainerDirName, "devcontainer.json"), + filepath.Join(folder, devcontainerDirName+".json"), + } + for _, candidate := range candidates { + if _, err := os.Stat(candidate); err == nil { + return candidate, true + } + } + return "", false +} + +// findDevContainerConfigs discovers nested devcontainer configs one level deep +// under .devcontainer//devcontainer.json, falling back to a recursive glob +// for deeper layouts. +func findDevContainerConfigs(folder string) ([]string, error) { + var configs []string + + devcontainerDir := filepath.Join(folder, devcontainerDirName) + entries, err := os.ReadDir(devcontainerDir) + if err == nil { + for _, entry := range entries { + if !entry.IsDir() { + continue + } + configPath := filepath.Join(devcontainerDir, entry.Name(), "devcontainer.json") + if _, err := os.Stat(configPath); err == nil { + configs = append(configs, configPath) + } + } + } + + if len(configs) == 0 { + matches, err := doublestar.FilepathGlob( + filepath.ToSlash(filepath.Clean(folder)) + "/.devcontainer/**/devcontainer.json", + ) + if err != nil { + return nil, err + } + configs = matches + } + + return configs, nil +} + +// ListDevContainerIDs returns the available devcontainer ids in a folder (the +// nested config directory names), excluding the conventional root directory. +func ListDevContainerIDs(folder string) ([]string, error) { + configs, err := findDevContainerConfigs(folder) + if err != nil { + return nil, err + } + + var ids []string + for _, configPath := range configs { + id := filepath.Base(filepath.Dir(configPath)) + if id != devcontainerDirName { + ids = append(ids, id) + } + } + return ids, nil +} diff --git a/pkg/devcontainer/config/envfile.go b/pkg/devcontainer/config/envfile.go new file mode 100644 index 000000000..1c1a88743 --- /dev/null +++ b/pkg/devcontainer/config/envfile.go @@ -0,0 +1,61 @@ +package config + +import ( + "bufio" + "fmt" + "os" + "strings" + "unicode/utf8" +) + +// ParseKeyValueFile reads an env-style file and returns its "KEY=VALUE" lines. +// Blank lines and lines beginning with '#' are skipped. It errors on invalid +// UTF-8, keys that are empty or contain spaces, and empty values. +func ParseKeyValueFile(filename string) ([]string, error) { + f, err := os.Open(filename) // #nosec G304 -- caller-provided env file path + if err != nil { + return nil, err + } + defer func() { _ = f.Close() }() + + var keyValuePairs []string + scanner := bufio.NewScanner(f) + for lineNum := 1; scanner.Scan(); lineNum++ { + line, keep, err := parseEnvLine(filename, lineNum, scanner.Bytes()) + if err != nil { + return nil, err + } + if keep { + keyValuePairs = append(keyValuePairs, line) + } + } + return keyValuePairs, nil +} + +// parseEnvLine validates a single env-file line. keep reports whether the line +// is a real "KEY=VALUE" entry (blank and comment lines are skipped with keep=false). +func parseEnvLine(filename string, lineNum int, raw []byte) (line string, keep bool, err error) { + if !utf8.Valid(raw) { + return "", false, fmt.Errorf( + "env file %s contains invalid utf8 bytes in line %d", filename, lineNum, + ) + } + + line = string(raw) + if len(line) == 0 || strings.HasPrefix(line, "#") { + return "", false, nil + } + + key, value, found := strings.Cut(line, "=") + if len(key) == 0 || strings.Contains(key, " ") { + return "", false, fmt.Errorf( + "env file %s contains invalid variable key in line %d: %s", filename, lineNum, line, + ) + } + if len(value) == 0 { + return "", false, fmt.Errorf( + "env file %s contains invalid variable value in line %d: %s", filename, lineNum, line, + ) + } + return line, found, nil +} diff --git a/pkg/devcontainer/config/extends_test.go b/pkg/devcontainer/config/extends_test.go index f3f8af3be..f36a7efa4 100644 --- a/pkg/devcontainer/config/extends_test.go +++ b/pkg/devcontainer/config/extends_test.go @@ -1,6 +1,7 @@ package config import ( + "context" "os" "path/filepath" "strings" @@ -42,7 +43,7 @@ func TestExtends_BasicScalarOverride(t *testing.T) { "remoteUser": "vscode" }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -78,7 +79,7 @@ func TestExtends_MapDeepMerge_ContainerEnv(t *testing.T) { } }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -111,7 +112,7 @@ func TestExtends_MapDeepMerge_Features(t *testing.T) { } }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -149,7 +150,7 @@ func TestExtends_ArrayReplacement(t *testing.T) { "capAdd": ["NET_ADMIN", "SYS_ADMIN"] }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -180,7 +181,7 @@ func TestExtends_LifecycleHookReplacement(t *testing.T) { "postCreateCommand": "echo child" }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -215,7 +216,7 @@ func TestExtends_CycleDetection(t *testing.T) { "name": "b" }`) - _, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "a.json")) + _, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "a.json")) if err == nil { t.Fatal("expected cycle detection error") } @@ -231,7 +232,7 @@ func TestExtends_MissingFile(t *testing.T) { "name": "child" }`) - _, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + _, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err == nil { t.Fatal("expected error for missing extends file") } @@ -260,7 +261,7 @@ func TestExtends_MultiLevel(t *testing.T) { "containerEnv": {"LEVEL": "child"} }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -292,7 +293,10 @@ func TestExtends_NoExtends(t *testing.T) { "image": "node:18" }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "standalone.json")) + cfg, err := ParseDevContainerJSONFile( + context.Background(), + filepath.Join(tmpDir, "standalone.json"), + ) if err != nil { t.Fatal(err) } @@ -309,7 +313,7 @@ func TestExtends_OriginPreserved(t *testing.T) { writeJSON(t, tmpDir, "parent.json", `{"name": "parent", "image": "ubuntu:20.04"}`) childPath := writeJSON(t, tmpDir, "child.json", `{"extends": "parent.json", "name": "child"}`) - cfg, err := ParseDevContainerJSONFile(childPath) + cfg, err := ParseDevContainerJSONFile(context.Background(), childPath) if err != nil { t.Fatal(err) } @@ -337,7 +341,7 @@ func TestExtends_NestedStructBuildMerge(t *testing.T) { } }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -430,7 +434,7 @@ func TestMergeExtendsConfigs_Maps(t *testing.T) { parent := &DevContainerConfig{ DevContainerConfigBase: DevContainerConfigBase{ Features: map[string]any{"feat-a": map[string]any{}}, - RemoteEnv: map[string]*string{"A": strPtr("1")}, + RemoteEnv: map[string]*string{"A": new("1")}, }, NonComposeBase: NonComposeBase{ ContainerEnv: map[string]string{"X": "parent"}, @@ -441,7 +445,7 @@ func TestMergeExtendsConfigs_Maps(t *testing.T) { child := &DevContainerConfig{ DevContainerConfigBase: DevContainerConfigBase{ Features: map[string]any{"feat-b": map[string]any{}}, - RemoteEnv: map[string]*string{"B": strPtr("2")}, + RemoteEnv: map[string]*string{"B": new("2")}, }, NonComposeBase: NonComposeBase{ ContainerEnv: map[string]string{"X": testNameChild, "Y": testNameChild}, @@ -513,7 +517,7 @@ func TestExtends_ArraySingleRef(t *testing.T) { "name": "child" }`) - cfg, err := ParseDevContainerJSONFile(childPath) + cfg, err := ParseDevContainerJSONFile(context.Background(), childPath) if err != nil { t.Fatal(err) } @@ -541,7 +545,7 @@ func TestExtends_ArrayMultipleRefs_Scalars(t *testing.T) { "containerEnv": {"FROM_CHILD": "child-val"} }`) - cfg, err := ParseDevContainerJSONFile(childPath) + cfg, err := ParseDevContainerJSONFile(context.Background(), childPath) if err != nil { t.Fatal(err) } @@ -572,7 +576,7 @@ func TestExtends_ArrayMultipleRefs_EnvMerge(t *testing.T) { "containerEnv": {"FROM_CHILD": "child-val"} }`) - cfg, err := ParseDevContainerJSONFile(childPath) + cfg, err := ParseDevContainerJSONFile(context.Background(), childPath) if err != nil { t.Fatal(err) } @@ -599,7 +603,7 @@ func TestExtends_ArrayOrderMatters(t *testing.T) { "name": "child" }`) - cfg, err := ParseDevContainerJSONFile(childPath) + cfg, err := ParseDevContainerJSONFile(context.Background(), childPath) if err != nil { t.Fatal(err) } @@ -619,7 +623,7 @@ func TestExtends_ArrayCycleDetection(t *testing.T) { "extends": ["a.json"] }`) - _, err := ParseDevContainerJSONFile(childPath) + _, err := ParseDevContainerJSONFile(context.Background(), childPath) if err == nil { t.Fatal("expected cycle error") } @@ -697,7 +701,10 @@ func TestExtends_LocalEnvInPath(t *testing.T) { "name": "child" }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(childDir, "child.json")) + cfg, err := ParseDevContainerJSONFile( + context.Background(), + filepath.Join(childDir, "child.json"), + ) if err != nil { t.Fatal(err) } @@ -727,7 +734,7 @@ func TestExtends_LocalWorkspaceFolderInPath(t *testing.T) { "name": "child" }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -746,7 +753,7 @@ func TestExtends_MissingEnvResolvesToEmpty(t *testing.T) { "name": "child" }`) - _, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + _, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err == nil { t.Fatal("expected error due to invalid path from empty env var") } @@ -781,7 +788,7 @@ func TestExtends_LocalEnvDefaultValue(t *testing.T) { "name": "child" }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -812,7 +819,7 @@ func TestExtends_LocalWorkspaceFolderBasenameInPath(t *testing.T) { "name": "child" }`) - cfg, err := ParseDevContainerJSONFile(filepath.Join(tmpDir, "child.json")) + cfg, err := ParseDevContainerJSONFile(context.Background(), filepath.Join(tmpDir, "child.json")) if err != nil { t.Fatal(err) } @@ -824,6 +831,7 @@ func TestExtends_LocalWorkspaceFolderBasenameInPath(t *testing.T) { } } +//go:fix inline func strPtr(s string) *string { - return &s + return new(s) } diff --git a/pkg/devcontainer/config/jsonutil.go b/pkg/devcontainer/config/jsonutil.go new file mode 100644 index 000000000..88761c154 --- /dev/null +++ b/pkg/devcontainer/config/jsonutil.go @@ -0,0 +1,13 @@ +package config + +import "encoding/json" + +// convert round-trips a value through JSON to reshape it into another type, +// e.g. from a map[string]any into a concrete struct. +func convert(from, to any) error { + out, err := json.Marshal(from) + if err != nil { + return err + } + return json.Unmarshal(out, to) +} diff --git a/pkg/devcontainer/config/legacy.go b/pkg/devcontainer/config/legacy.go new file mode 100644 index 000000000..ee50b13c2 --- /dev/null +++ b/pkg/devcontainer/config/legacy.go @@ -0,0 +1,59 @@ +package config + +// replaceLegacy migrates deprecated top-level fields (Extensions, Settings, +// DevPort) into customizations.vscode, where current devcontainer configs +// expect them. It is a no-op when none of the legacy fields are set. +func replaceLegacy(config *DevContainerConfig) (*DevContainerConfig, error) { + if len(config.Extensions) == 0 && len(config.Settings) == 0 && config.DevPort == 0 { + return config, nil + } + + if config.Customizations == nil { + config.Customizations = map[string]any{} + } + + vsCodeConfig := &VSCodeCustomizations{} + if vscode, ok := config.Customizations["vscode"]; ok { + if err := convert(vscode, &vsCodeConfig); err != nil { + return nil, err + } + } + + migrateLegacyExtensions(config, vsCodeConfig) + migrateLegacySettings(config, vsCodeConfig) + migrateLegacyDevPort(config, vsCodeConfig) + + config.Customizations["vscode"] = vsCodeConfig + return config, nil +} + +func migrateLegacyExtensions(config *DevContainerConfig, vsCode *VSCodeCustomizations) { + if len(config.Extensions) == 0 { + return + } + vsCode.Extensions = config.Extensions + config.Extensions = nil +} + +func migrateLegacySettings(config *DevContainerConfig, vsCode *VSCodeCustomizations) { + if len(config.Settings) == 0 { + return + } + if vsCode.Settings == nil { + vsCode.Settings = map[string]any{} + } + for k, v := range config.Settings { + if _, exists := vsCode.Settings[k]; !exists { + vsCode.Settings[k] = v + } + } + config.Settings = nil +} + +func migrateLegacyDevPort(config *DevContainerConfig, vsCode *VSCodeCustomizations) { + if vsCode.DevPort != 0 { + return + } + vsCode.DevPort = config.DevPort + config.DevPort = 0 +} diff --git a/pkg/devcontainer/config/merge.go b/pkg/devcontainer/config/merge.go index 29baa72c5..8fc8930c3 100644 --- a/pkg/devcontainer/config/merge.go +++ b/pkg/devcontainer/config/merge.go @@ -1,6 +1,7 @@ package config import ( + "context" "fmt" "maps" "slices" @@ -21,7 +22,7 @@ func MergeExtraRemoteEnv(mergedConfig *MergedDevContainerConfig, extraConfigPath if extraConfigPath == "" { return nil } - extraConfig, err := ParseDevContainerJSONFile(extraConfigPath) + extraConfig, err := ParseDevContainerJSONFile(context.Background(), extraConfigPath) if err != nil { return err } @@ -444,8 +445,8 @@ func some[T any](entries []T, m func(entry T) *bool) *bool { func ReverseSlice[T comparable](s []T) []T { var r []T - for i := len(s) - 1; i >= 0; i-- { - r = append(r, s[i]) + for _, v := range slices.Backward(s) { + r = append(r, v) } return r } diff --git a/pkg/devcontainer/config/merge_test.go b/pkg/devcontainer/config/merge_test.go index 5fefae0a1..f81438c9e 100644 --- a/pkg/devcontainer/config/merge_test.go +++ b/pkg/devcontainer/config/merge_test.go @@ -1,6 +1,7 @@ package config import ( + "context" "fmt" "os" "slices" @@ -603,7 +604,7 @@ func TestMergeConfiguration_NilMetadata_ParsedFromJSONFile(t *testing.T) { t.Fatalf("write fixture: %v", err) } - cfg, err := ParseDevContainerJSONFile(path) + cfg, err := ParseDevContainerJSONFile(context.Background(), path) if err != nil { t.Fatalf("parse: %v", err) } diff --git a/pkg/devcontainer/config/mount_test.go b/pkg/devcontainer/config/mount_test.go index 88bcd692a..d8971238c 100644 --- a/pkg/devcontainer/config/mount_test.go +++ b/pkg/devcontainer/config/mount_test.go @@ -172,3 +172,34 @@ func TestMountTmpfsAccessors(t *testing.T) { t.Errorf("TmpfsMode() empty = %q, want \"\"", got) } } + +func TestMountString(t *testing.T) { + tests := []struct { + name string + m *Mount + want string + }{ + { + "volume with external is omitted from --mount arg", + &Mount{Type: "volume", Source: "vol", Target: "/workspace", External: true}, + "type=volume,src=vol,dst=/workspace", + }, + { + "bind mount", + &Mount{Type: "bind", Source: "/host", Target: "/container"}, + "type=bind,src=/host,dst=/container", + }, + { + "other options preserved", + &Mount{Type: "volume", Source: "vol", Target: "/w", Other: []string{"volume-nocopy"}}, + "type=volume,src=vol,dst=/w,volume-nocopy", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.m.String(); got != tt.want { + t.Errorf("String() = %q, want %q", got, tt.want) + } + }) + } +} diff --git a/pkg/devcontainer/config/parse.go b/pkg/devcontainer/config/parse.go index aa7816007..612340c46 100644 --- a/pkg/devcontainer/config/parse.go +++ b/pkg/devcontainer/config/parse.go @@ -1,46 +1,53 @@ package config import ( - "bufio" "context" "encoding/json" "fmt" "os" - path2 "path" "path/filepath" "strings" - "unicode/utf8" - doublestar "github.com/bmatcuk/doublestar/v4" "github.com/tailscale/hujson" ) -const DEVCONTAINER_FEATURE_FILE_NAME = "devcontainer-feature.json" +// DevContainerFeatureFileName is the manifest file name for a devcontainer feature. +const DevContainerFeatureFileName = "devcontainer-feature.json" -func ParseDevContainerFeature(folder string) (*FeatureConfig, error) { - path := filepath.Join(folder, DEVCONTAINER_FEATURE_FILE_NAME) - _, err := os.Stat(path) - if err != nil { - return nil, fmt.Errorf("%s is missing in feature folder", DEVCONTAINER_FEATURE_FILE_NAME) - } +// ParseOptions configures devcontainer.json discovery and selection. +type ParseOptions struct { + // Selector chooses among multiple discovered configs. When nil, the first + // match is used (and ambiguity is not rejected). + Selector ConfigSelector - path, err = filepath.Abs(path) + // ForceSelect skips the conventional root-config short-circuits so the + // selector always runs against discovered configs. Used when a specific + // config was explicitly requested, so a root config cannot shadow it. + ForceSelect bool +} + +// ParseDevContainerFeature reads and parses the devcontainer-feature.json in folder. +func ParseDevContainerFeature(folder string) (*FeatureConfig, error) { + path, err := filepath.Abs(filepath.Join(folder, DevContainerFeatureFileName)) if err != nil { return nil, fmt.Errorf("make path absolute: %w", err) } + if _, err := os.Stat(path); err != nil { + return nil, fmt.Errorf("%s is missing in feature folder", DevContainerFeatureFileName) + } - data, err := os.ReadFile(path) + data, err := os.ReadFile(path) //nolint:gosec // caller-provided feature folder if err != nil { return nil, err } - featureConfig := &FeatureConfig{} normalized, err := hujson.Standardize(data) if err != nil { return nil, fmt.Errorf("parse jsonc: %w", err) } - err = json.Unmarshal(normalized, featureConfig) - if err != nil { + + featureConfig := &FeatureConfig{} + if err := json.Unmarshal(normalized, featureConfig); err != nil { return nil, err } @@ -48,14 +55,14 @@ func ParseDevContainerFeature(folder string) (*FeatureConfig, error) { return featureConfig, nil } +// SaveDevContainerJSON writes config to disk at its Origin. func SaveDevContainerJSON(config *DevContainerConfig) error { if config.Origin == "" { return fmt.Errorf("no origin in config") } // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. - err := os.MkdirAll(filepath.Dir(config.Origin), 0o755) - if err != nil { + if err := os.MkdirAll(filepath.Dir(config.Origin), 0o755); err != nil { return err } @@ -65,248 +72,87 @@ func SaveDevContainerJSON(config *DevContainerConfig) error { } // #nosec G306 -- TODO Consider using a more secure permission setting and ownership if needed. - err = os.WriteFile(config.Origin, out, 0o644) - if err != nil { - return err - } - - return nil + return os.WriteFile(config.Origin, out, 0o644) } -// ParseDevContainerJSONFile parse the given a devcontainer.json file. -func ParseDevContainerJSONFile(jsonFilePath string) (*DevContainerConfig, error) { - var err error +// ParseDevContainerJSONFile parses the devcontainer.json at jsonFilePath, +// resolving `extends` and migrating legacy fields. +func ParseDevContainerJSONFile( + ctx context.Context, + jsonFilePath string, +) (*DevContainerConfig, error) { path, err := filepath.Abs(jsonFilePath) if err != nil { return nil, fmt.Errorf("make path absolute: %w", err) } - bytes, err := os.ReadFile(path) + data, err := os.ReadFile(path) //nolint:gosec // caller-provided workspace folder if err != nil { return nil, err } - devContainer := &DevContainerConfig{} - normalized, err := hujson.Standardize(bytes) + normalized, err := hujson.Standardize(data) if err != nil { return nil, fmt.Errorf("parse jsonc: %w", err) } - err = json.Unmarshal(normalized, devContainer) - if err != nil { + + devContainer := &DevContainerConfig{} + if err := json.Unmarshal(normalized, devContainer); err != nil { return nil, err } devContainer.Origin = path - // Resolve extends before applying legacy transforms - if !devContainer.Extends.IsEmpty() { - visited := map[string]bool{path: true} - declaringDir := filepath.Dir(path) - - replacer := extendsVarReplacer(declaringDir) - for i, ref := range devContainer.Extends { - devContainer.Extends[i] = ResolveString(ref, replacer) - } - - parent, err := resolveExtendsArray( - context.TODO(), - devContainer.Extends, - declaringDir, - visited, - ) - if err != nil { - return nil, err - } - devContainer = mergeExtendsConfigs(parent, devContainer) + devContainer, err = resolveExtends(ctx, devContainer, path) + if err != nil { + return nil, err } return replaceLegacy(devContainer) } -// ParseDevContainerJSON check if a file named devcontainer.json exists in the given directory and parse it if it does. -func ParseDevContainerJSON(folder, relativePath string) (*DevContainerConfig, error) { - return ParseDevContainerJSONWithSelector(folder, relativePath, nil) -} - -// ParseDevContainerJSONWithSelector allows custom selection when multiple devcontainer configs exist. -func ParseDevContainerJSONWithSelector( - folder, relativePath string, - selector func([]string) (string, error), +// resolveExtends resolves any `extends` references, merging parent configs into +// the given config. It is a no-op when the config has no `extends`. +func resolveExtends( + ctx context.Context, + devContainer *DevContainerConfig, + path string, ) (*DevContainerConfig, error) { - path, err := resolveDevContainerPath(folder, relativePath, selector) - if err != nil { - return nil, err - } - if path == "" { - return nil, nil - } - return ParseDevContainerJSONFile(path) -} - -func resolveDevContainerPath( - folder, relativePath string, - selector func([]string) (string, error), -) (string, error) { - // Explicit path provided - if relativePath != "" { - path := path2.Join(filepath.ToSlash(folder), relativePath) - if _, err := os.Stat(path); err != nil { - return "", fmt.Errorf("devcontainer path %s doesn't exist: %w", path, err) - } - return path, nil + if devContainer.Extends.IsEmpty() { + return devContainer, nil } - // Try .devcontainer/devcontainer.json - path := filepath.Join(folder, ".devcontainer", "devcontainer.json") - if _, err := os.Stat(path); err == nil { - return path, nil - } + visited := map[string]bool{path: true} + declaringDir := filepath.Dir(path) - // Try .devcontainer.json - path = filepath.Join(folder, ".devcontainer.json") - if _, err := os.Stat(path); err == nil { - return path, nil + replacer := extendsVarReplacer(declaringDir) + for i, ref := range devContainer.Extends { + devContainer.Extends[i] = ResolveString(ref, replacer) } - // Find multiple configs - matches, err := findDevContainerConfigs(folder) - if err != nil { - return "", err - } - if len(matches) == 0 { - return "", nil - } - if len(matches) == 1 { - return matches[0], nil - } - if selector == nil { - return matches[0], nil - } - return selector(matches) -} - -func findDevContainerConfigs(folder string) ([]string, error) { - var configs []string - - // Check .devcontainer/FOLDER/devcontainer.json (one level deep) - // https://containers.dev/implementors/spec/#devcontainerjson - devcontainerDir := filepath.Join(folder, ".devcontainer") - entries, err := os.ReadDir(devcontainerDir) - if err == nil { - for _, entry := range entries { - if entry.IsDir() { - configPath := filepath.Join(devcontainerDir, entry.Name(), "devcontainer.json") - if _, err := os.Stat(configPath); err == nil { - configs = append(configs, configPath) - } - } - } - } - - // Fallback to glob for deeper structures - if len(configs) == 0 { - matches, err := doublestar.FilepathGlob( - filepath.ToSlash(filepath.Clean(folder)) + "/.devcontainer/**/devcontainer.json", - ) - if err != nil { - return nil, err - } - configs = matches - } - - return configs, nil -} - -// ListDevContainerIDs returns available devcontainer IDs in the folder. -func ListDevContainerIDs(folder string) ([]string, error) { - configs, err := findDevContainerConfigs(folder) + parent, err := resolveExtendsArray(ctx, devContainer.Extends, declaringDir, visited) if err != nil { return nil, err } - - var ids []string - for _, config := range configs { - id := filepath.Base(filepath.Dir(config)) - if id != ".devcontainer" { - ids = append(ids, id) - } - } - return ids, nil -} - -func replaceLegacy(config *DevContainerConfig) (*DevContainerConfig, error) { - if len(config.Extensions) == 0 && len(config.Settings) == 0 && config.DevPort == 0 { - return config, nil - } - - // make sure customizations exist - if config.Customizations == nil { - config.Customizations = map[string]any{} - } - - vsCodeConfig := &VSCodeCustomizations{} - vscode, ok := config.Customizations["vscode"] - if ok { - err := Convert(vscode, &vsCodeConfig) - if err != nil { - return nil, err - } - } - - if len(config.Extensions) > 0 { - vsCodeConfig.Extensions = config.Extensions - config.Extensions = nil - } - - if len(config.Settings) > 0 { - if vsCodeConfig.Settings == nil { - vsCodeConfig.Settings = map[string]any{} - } - - for k, v := range config.Settings { - _, exists := vsCodeConfig.Settings[k] - if !exists { - vsCodeConfig.Settings[k] = v - } - } - - config.Settings = nil - } - - if vsCodeConfig.DevPort == 0 { - vsCodeConfig.DevPort = config.DevPort - config.DevPort = 0 - } - - config.Customizations["vscode"] = vsCodeConfig - return config, nil -} - -func Convert(from any, to any) error { - out, err := json.Marshal(from) - if err != nil { - return err - } - - return json.Unmarshal(out, to) + return mergeExtendsConfigs(parent, devContainer), nil } // extendsVarReplacer returns a ReplaceFunction that resolves only local-scope -// variables suitable for use before the container exists. +// variables, suitable for use before the container exists (during `extends` +// path resolution). func extendsVarReplacer(localWorkspaceFolder string) ReplaceFunction { return func(match, variable string, args []string) string { switch variable { case varLocalEnv: - if len(args) > 0 { - val, ok := os.LookupEnv(args[0]) - if ok { - return val - } - if len(args) > 1 { - return strings.Join(args[1:], ":") - } - return "" + if len(args) == 0 { + return match } - return match + if val, ok := os.LookupEnv(args[0]); ok { + return val + } + if len(args) > 1 { + return strings.Join(args[1:], ":") + } + return "" case varLocalWorkspaceFolder: return localWorkspaceFolder case "localWorkspaceFolderBasename": @@ -317,48 +163,29 @@ func extendsVarReplacer(localWorkspaceFolder string) ReplaceFunction { } } -func ParseKeyValueFile(filename string) ([]string, error) { - f, err := os.Open(filename) +// ParseDevContainerJSON discovers and parses the devcontainer.json for a folder, +// optionally at relativePath. It returns (nil, nil) when no config is found. +func ParseDevContainerJSON( + ctx context.Context, + folder, relativePath string, +) (*DevContainerConfig, error) { + return ParseDevContainerJSONWithOptions(ctx, folder, relativePath, ParseOptions{}) +} + +// ParseDevContainerJSONWithOptions discovers and parses the devcontainer.json for +// a folder using the given options to control selection. It returns (nil, nil) +// when no config is found. +func ParseDevContainerJSONWithOptions( + ctx context.Context, + folder, relativePath string, + opts ParseOptions, +) (*DevContainerConfig, error) { + path, err := resolveDevContainerPath(folder, relativePath, opts.Selector, opts.ForceSelect) if err != nil { return nil, err } - defer func() { _ = f.Close() }() - keyValuePairs := []string{} - scanner := bufio.NewScanner(f) - lineNum := 1 - for scanner.Scan() { - scannedBytes := scanner.Bytes() - if !utf8.Valid(scannedBytes) { - return nil, fmt.Errorf( - "env file %s contains invalid utf8 bytes in line %d", - filename, - lineNum, - ) - } - line := string(scannedBytes) - // skip commented or empty lines - if len(line) > 0 && !strings.HasPrefix(line, "#") { - key, value, found := strings.Cut(line, "=") - if len(key) == 0 || strings.Contains(key, " ") { - return nil, fmt.Errorf( - "env file %s contains invalid variable key in line %d: %s", - filename, - lineNum, - line, - ) - } else if len(value) == 0 { - return nil, fmt.Errorf( - "env file %s contains invalid variable value in line %d: %s", - filename, - lineNum, - line, - ) - } - if found { - keyValuePairs = append(keyValuePairs, line) - } - } - lineNum++ + if path == "" { + return nil, nil } - return keyValuePairs, nil + return ParseDevContainerJSONFile(ctx, path) } diff --git a/pkg/devcontainer/config/parse_test.go b/pkg/devcontainer/config/parse_test.go index f4279a9a4..fdce8c7a3 100644 --- a/pkg/devcontainer/config/parse_test.go +++ b/pkg/devcontainer/config/parse_test.go @@ -1,6 +1,7 @@ package config import ( + "context" "errors" "os" "path/filepath" @@ -26,7 +27,7 @@ func TestParseSecrets(t *testing.T) { t.Fatal(err) } - cfg, err := ParseDevContainerJSONFile(configPath) + cfg, err := ParseDevContainerJSONFile(context.Background(), configPath) if err != nil { t.Fatal(err) } @@ -72,7 +73,7 @@ func TestSecretsRoundTrip(t *testing.T) { t.Fatal(err) } - loaded, err := ParseDevContainerJSONFile(cfg.Origin) + loaded, err := ParseDevContainerJSONFile(context.Background(), cfg.Origin) if err != nil { t.Fatal(err) } @@ -209,7 +210,8 @@ func TestListDevContainerIDs(t *testing.T) { } } -func TestParseDevContainerJSONWithSelector(t *testing.T) { +//nolint:cyclop,funlen // table-style test with many subtests +func TestParseDevContainerJSONDiscovery(t *testing.T) { t.Run("explicit path", func(t *testing.T) { tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "custom.json") @@ -218,7 +220,7 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Fatal(err) } - config, err := ParseDevContainerJSONWithSelector(tmpDir, "custom.json", nil) + config, err := ParseDevContainerJSON(context.Background(), tmpDir, "custom.json") if err != nil { t.Fatal(err) } @@ -229,7 +231,7 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Run("explicit path not found", func(t *testing.T) { tmpDir := t.TempDir() - _, err := ParseDevContainerJSONWithSelector(tmpDir, "missing.json", nil) + _, err := ParseDevContainerJSON(context.Background(), tmpDir, "missing.json") if err == nil { t.Error("expected error for missing file") } @@ -247,7 +249,7 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Fatal(err) } - config, err := ParseDevContainerJSONWithSelector(tmpDir, "", nil) + config, err := ParseDevContainerJSON(context.Background(), tmpDir, "") if err != nil { t.Fatal(err) } @@ -264,7 +266,7 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Fatal(err) } - config, err := ParseDevContainerJSONWithSelector(tmpDir, "", nil) + config, err := ParseDevContainerJSON(context.Background(), tmpDir, "") if err != nil { t.Fatal(err) } @@ -285,7 +287,7 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Fatal(err) } - config, err := ParseDevContainerJSONWithSelector(tmpDir, "", nil) + config, err := ParseDevContainerJSON(context.Background(), tmpDir, "") if err != nil { t.Fatal(err) } @@ -316,17 +318,11 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Fatal(err) } - config, err := ParseDevContainerJSONWithSelector( + config, err := ParseDevContainerJSONWithOptions( + context.Background(), tmpDir, "", - func(matches []string) (string, error) { - for _, match := range matches { - if filepath.Base(filepath.Dir(match)) == "python" { - return match, nil - } - } - return "", errors.New("not found") - }, + ParseOptions{Selector: SelectByID("python")}, ) if err != nil { t.Fatal(err) @@ -358,7 +354,7 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Fatal(err) } - config, err := ParseDevContainerJSONWithSelector(tmpDir, "", nil) + config, err := ParseDevContainerJSON(context.Background(), tmpDir, "") if err != nil { t.Fatal(err) } @@ -388,11 +384,14 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Fatal(err) } - _, err := ParseDevContainerJSONWithSelector( + _, err := ParseDevContainerJSONWithOptions( + context.Background(), tmpDir, "", - func(matches []string) (string, error) { - return "", errors.New("selector failed") + ParseOptions{ + Selector: func([]string) (string, error) { + return "", errors.New("selector failed") + }, }, ) if err == nil || err.Error() != "selector failed" { @@ -402,7 +401,7 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { t.Run("no config found", func(t *testing.T) { tmpDir := t.TempDir() - config, err := ParseDevContainerJSONWithSelector(tmpDir, "", nil) + config, err := ParseDevContainerJSON(context.Background(), tmpDir, "") if err != nil { t.Fatal(err) } @@ -411,3 +410,107 @@ func TestParseDevContainerJSONWithSelector(t *testing.T) { } }) } + +//nolint:cyclop,funlen // table-style test with many subtests +func TestParseDevContainerJSONWithOptions_ExplicitSelection(t *testing.T) { + // writeConfig creates .devcontainer//devcontainer.json (or the root + // .devcontainer/devcontainer.json when sub is empty) with the given name. + writeConfig := func(t *testing.T, dir, sub, name string) { + t.Helper() + var configPath string + if sub == "" { + configPath = filepath.Join(dir, ".devcontainer", "devcontainer.json") + } else { + configPath = filepath.Join(dir, ".devcontainer", sub, "devcontainer.json") + } + // #nosec G301 -- test fixture + if err := os.MkdirAll(filepath.Dir(configPath), 0o755); err != nil { + t.Fatal(err) + } + // #nosec G306 -- test fixture + if err := os.WriteFile(configPath, []byte(`{"name":"`+name+`"}`), 0o644); err != nil { + t.Fatal(err) + } + } + + t.Run("nested id is selected despite root config present", func(t *testing.T) { + tmpDir := t.TempDir() + writeConfig(t, tmpDir, "", "Root") + writeConfig(t, tmpDir, "skevetter", "Skevetter") + + config, err := ParseDevContainerJSONWithOptions( + context.Background(), + tmpDir, + "", + ParseOptions{Selector: SelectByID("skevetter"), ForceSelect: true}, + ) + if err != nil { + t.Fatal(err) + } + if config.Name != "Skevetter" { + t.Errorf("expected Skevetter, got %s", config.Name) + } + }) + + t.Run("root config shadows nested id without explicit selection", func(t *testing.T) { + // Without ForceSelect the root config short-circuits, so the selector + // never runs and the nested id is not chosen. + tmpDir := t.TempDir() + writeConfig(t, tmpDir, "", "Root") + writeConfig(t, tmpDir, "skevetter", "Skevetter") + + config, err := ParseDevContainerJSONWithOptions( + context.Background(), + tmpDir, + "", + ParseOptions{Selector: SelectByID("skevetter")}, + ) + if err != nil { + t.Fatal(err) + } + if config.Name != "Root" { + t.Errorf("expected Root (short-circuit), got %s", config.Name) + } + }) + + t.Run("mismatched id errors even with single config", func(t *testing.T) { + tmpDir := t.TempDir() + writeConfig(t, tmpDir, "skevetter", "Skevetter") + + _, err := ParseDevContainerJSONWithOptions( + context.Background(), + tmpDir, + "", + ParseOptions{Selector: SelectByID("does-not-exist"), ForceSelect: true}, + ) + if err == nil { + t.Error("expected error for mismatched --devcontainer-id, got nil") + } + }) + + t.Run("explicit path still wins over explicit selection", func(t *testing.T) { + tmpDir := t.TempDir() + writeConfig(t, tmpDir, "skevetter", "Skevetter") + // #nosec G306 -- test fixture + if err := os.WriteFile( + filepath.Join(tmpDir, "custom.json"), + []byte(`{"name":"Custom"}`), + 0o644, + ); err != nil { + t.Fatal(err) + } + + config, err := ParseDevContainerJSONWithOptions( + context.Background(), + tmpDir, + "custom.json", + ParseOptions{Selector: SelectByID("skevetter"), ForceSelect: true}, + ) + if err != nil { + t.Fatal(err) + } + if config.Name != "Custom" { + t.Errorf("expected Custom, got %s", config.Name) + } + }) +} diff --git a/pkg/devcontainer/config/result.go b/pkg/devcontainer/config/result.go index ffa4b9d17..e54f3fb4a 100644 --- a/pkg/devcontainer/config/result.go +++ b/pkg/devcontainer/config/result.go @@ -7,7 +7,7 @@ import ( pkgconfig "github.com/devsy-org/devsy/pkg/config" ) -const UserLabel = pkgconfig.BinaryName + ".user" +const UserLabel = pkgconfig.DockerUserLabel type Result struct { DevContainerConfigWithPath *DevContainerConfigWithPath `json:"DevContainerConfigWithPath"` @@ -85,7 +85,7 @@ func GetDevsyCustomizations(parsedConfig *DevContainerConfig) *DevsyCustomizatio } devsy := &DevsyCustomizations{} - err := Convert(parsedConfig.Customizations[pkgconfig.BinaryName], devsy) + err := convert(parsedConfig.Customizations[pkgconfig.BinaryName], devsy) if err != nil { return &DevsyCustomizations{} } @@ -104,7 +104,7 @@ func GetVSCodeConfiguration(mergedConfig *MergedDevContainerConfig) *VSCodeCusto } for _, customization := range mergedConfig.Customizations["vscode"] { vsCode := &VSCodeCustomizations{} - err := Convert(customization, vsCode) + err := convert(customization, vsCode) if err != nil { continue } @@ -136,7 +136,7 @@ func GetJetBrainsConfiguration(mergedConfig *MergedDevContainerConfig) *JetBrain } for _, customization := range mergedConfig.Customizations["jetbrains"] { jetBrains := &JetBrainsCustomizations{} - err := Convert(customization, jetBrains) + err := convert(customization, jetBrains) if err != nil { continue } diff --git a/pkg/devcontainer/config/substitute.go b/pkg/devcontainer/config/substitute.go index fb2448f77..9ceed5367 100644 --- a/pkg/devcontainer/config/substitute.go +++ b/pkg/devcontainer/config/substitute.go @@ -64,7 +64,7 @@ var preContainerFields = []string{containerEnvField, remoteEnvField} func Substitute(substitutionCtx *SubstitutionContext, config any, out any) error { newVal := map[string]any{} - err := Convert(config, &newVal) + err := convert(config, &newVal) if err != nil { return err } @@ -109,7 +109,7 @@ func Substitute(substitutionCtx *SubstitutionContext, config any, out any) error maps.Copy(retMap, preFieldValues) } - err = Convert(retVal, out) + err = convert(retVal, out) if err != nil { return err } @@ -119,7 +119,7 @@ func Substitute(substitutionCtx *SubstitutionContext, config any, out any) error func SubstituteContainerEnv(containerEnv map[string]string, config any, out any) error { newVal := map[string]any{} - err := Convert(config, &newVal) + err := convert(config, &newVal) if err != nil { return err } @@ -129,7 +129,7 @@ func SubstituteContainerEnv(containerEnv map[string]string, config any, out any) return replaceWithContainerEnv(containerEnv, match, variable, args) }) - err = Convert(retVal, out) + err = convert(retVal, out) if err != nil { return err } diff --git a/pkg/devcontainer/config/substitute_test.go b/pkg/devcontainer/config/substitute_test.go index b9bc5b538..b6a4adb50 100644 --- a/pkg/devcontainer/config/substitute_test.go +++ b/pkg/devcontainer/config/substitute_test.go @@ -351,13 +351,13 @@ func TestSubstituteRemoteEnvScoping(t *testing.T) { }{ { name: testCaseResolvesCWF, - input: map[string]*string{"V": strPtr("/prefix${containerWorkspaceFolder}")}, - want: map[string]*string{"V": strPtr("/prefix/workspaces/project")}, + input: map[string]*string{"V": new("/prefix${containerWorkspaceFolder}")}, + want: map[string]*string{"V": new("/prefix/workspaces/project")}, }, { name: testCaseResolvesCWFBasename, - input: map[string]*string{"V": strPtr(testContainerWorkspaceFolderBasenameVar)}, - want: map[string]*string{"V": strPtr(testProjectName)}, + input: map[string]*string{"V": new(testContainerWorkspaceFolderBasenameVar)}, + want: map[string]*string{"V": new(testProjectName)}, }, } for _, tt := range tests { @@ -380,8 +380,8 @@ func TestSubstituteMixedScoping(t *testing.T) { testLOCKey: testLocalWorkspaceFolderVar, }, RemoteEnv: map[string]*string{ - testCWFKey: strPtr(testContainerWorkspaceFolderVar), - testLOCKey: strPtr(testLocalWorkspaceFolderVar), + testCWFKey: new(testContainerWorkspaceFolderVar), + testLOCKey: new(testLocalWorkspaceFolderVar), }, } var out scopeTestConfig @@ -394,8 +394,8 @@ func TestSubstituteMixedScoping(t *testing.T) { testLOCKey: testWorkspaceFolder, }) assertRemoteEnv(t, out.RemoteEnv, map[string]*string{ - testCWFKey: strPtr(testContainerWorkspaceFolder), - testLOCKey: strPtr(testWorkspaceFolder), + testCWFKey: new(testContainerWorkspaceFolder), + testLOCKey: new(testWorkspaceFolder), }) } diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index ac0511611..3b4c7bfce 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -359,4 +359,65 @@ func (s *SubstituteTestSuite) TestSubstitute_CLIMountsEmpty() { s.Equal("/existing-target", substitutedConfig.Config.Mounts[0].Target) } +//go:fix inline func ptr(s string) *string { return &s } + +func TestWorkspaceMountFolderWarning(t *testing.T) { + tests := []struct { + name string + conf *config.DevContainerConfig + wantMsg bool + }{ + {name: "nil config", conf: nil, wantMsg: false}, + { + name: "neither set", + conf: &config.DevContainerConfig{}, + wantMsg: false, + }, + { + name: "both set", + conf: &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + WorkspaceFolder: "/workspace", + }, + NonComposeBase: config.NonComposeBase{WorkspaceMount: new("source=v")}, + }, + wantMsg: false, + }, + { + name: "mount without folder", + conf: &config.DevContainerConfig{ + NonComposeBase: config.NonComposeBase{WorkspaceMount: new("source=v")}, + }, + wantMsg: true, + }, + { + name: "folder without mount", + conf: &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + WorkspaceFolder: "/workspace", + }, + }, + wantMsg: true, + }, + { + name: "empty-string mount satisfies the pairing", + conf: &config.DevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{ + WorkspaceFolder: "/workspace", + }, + NonComposeBase: config.NonComposeBase{WorkspaceMount: new("")}, + }, + wantMsg: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := workspaceMountFolderWarning(tt.conf) + if (got != "") != tt.wantMsg { + t.Errorf("workspaceMountFolderWarning() = %q, wantMsg=%v", got, tt.wantMsg) + } + }) + } +} diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index e01a92c61..eef09c4d0 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -42,10 +42,11 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { return nil } +// cleanupDeliveryVolume removes the devsy-managed volumes created for this +// workspace. Best-effort: failures are logged, not returned. func (r *runner) cleanupDeliveryVolume(ctx context.Context) { - strategy := r.newAgentDelivery() - if err := strategy.Cleanup(ctx, r.ID); err != nil { - log.Debugf("best-effort agent delivery volume cleanup: %v", err) + if err := r.newAgentDelivery().Cleanup(ctx, r.ID); err != nil { + log.Debugf("best-effort delivery volume cleanup: %v", err) } } diff --git a/pkg/devcontainer/feature/features.go b/pkg/devcontainer/feature/features.go index 8565cae0b..92c6fec71 100644 --- a/pkg/devcontainer/feature/features.go +++ b/pkg/devcontainer/feature/features.go @@ -162,7 +162,7 @@ func checkFeatureCache(id string) (string, bool) { if err == nil { // make sure feature.json is there as well _, err = os.Stat( - filepath.Join(featureExtractedFolder, config.DEVCONTAINER_FEATURE_FILE_NAME), + filepath.Join(featureExtractedFolder, config.DevContainerFeatureFileName), ) if err == nil { log.Debugf("feature already cached: folder=%s", featureExtractedFolder) diff --git a/pkg/devcontainer/metadata/metadata.go b/pkg/devcontainer/metadata/metadata.go index d329c60be..57878b537 100644 --- a/pkg/devcontainer/metadata/metadata.go +++ b/pkg/devcontainer/metadata/metadata.go @@ -3,11 +3,12 @@ package metadata import ( "encoding/json" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/log" ) -const ImageMetadataLabel = "devcontainer.metadata" +const ImageMetadataLabel = pkgconfig.DevcontainerMetadataLabel const metadataLabelSizeWarningThreshold = 100 * 1024 diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index 974a097a0..28f666cb5 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -211,7 +211,7 @@ func (r *runner) legacyInject(ctx context.Context, timeout time.Duration) error IsLocal: false, RemoteAgentPath: pkgconfig.ContainerDevsyHelperLocation, DownloadURL: pkgconfig.DefaultAgentDownloadURL(), - PreferDownloadFromRemoteUrl: agent.Bool(false), + PreferDownloadFromRemoteUrl: new(false), Timeout: timeout, }) if err != nil { diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index 4dfe350f5..d2e3b399e 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -209,7 +209,10 @@ func (r *runner) mergeExistingContainerConfig( if imageMetadataConfig == nil { imageMetadataConfig = &config.ImageMetadataConfig{} } - extraConfig, parseErr := config.ParseDevContainerJSONFile(p.options.ExtraDevContainerPath) + extraConfig, parseErr := config.ParseDevContainerJSONFile( + context.Background(), + p.options.ExtraDevContainerPath, + ) if parseErr != nil { return nil, parseErr } @@ -276,6 +279,10 @@ func (r *runner) resolveNewContainer( } } + if seedErr := r.seedWorkspaceVolume(ctx, p); seedErr != nil { + return nil, fmt.Errorf("seed workspace volume: %w", seedErr) + } + err = r.runContainer(ctx, p, mergedConfig, buildInfo) if err != nil { return nil, fmt.Errorf("runner run container: %w", err) @@ -462,6 +469,38 @@ func (r *runner) deliverPreStart(ctx context.Context, runOptions *driver.RunOpti }) } +// seedWorkspaceVolume populates a named workspace volume from the local source +// folder when the workspace source is a local folder and workspaceMount is a +// named volume. This gives an isolated, disposable snapshot of the working +// tree. It is skipped for git/image sources, bind mounts, and volumes devsy +// does not manage. A reset removes the managed volume so it is re-seeded. +func (r *runner) seedWorkspaceVolume(ctx context.Context, p *resolveParams) error { + if r.WorkspaceConfig == nil || r.WorkspaceConfig.Workspace == nil { + return nil + } + if r.WorkspaceConfig.Workspace.Source.LocalFolder == "" { + return nil + } + + mount := parseWorkspaceMount(p.substitutionContext) + if mount == nil || mount.Type != "volume" || mount.Source == "" { + return nil + } + + seeder, ok := r.newAgentDelivery().(delivery.WorkspaceVolumeSeeder) + if !ok { + log.Debugf("delivery strategy cannot seed workspace volumes; skipping") + return nil + } + + return seeder.SeedWorkspaceVolume(ctx, delivery.WorkspaceSeedOptions{ + WorkspaceID: r.ID, + VolumeName: mount.Source, + SourceDir: r.LocalWorkspaceFolder, + Reset: r.WorkspaceConfig.CLIOptions.Reset, + }) +} + func (r *runner) runContainer( ctx context.Context, p *resolveParams, diff --git a/pkg/driver/kubernetes/pvc.go b/pkg/driver/kubernetes/pvc.go index e0bf7ee8f..c81de09f9 100644 --- a/pkg/driver/kubernetes/pvc.go +++ b/pkg/driver/kubernetes/pvc.go @@ -6,6 +6,7 @@ import ( "fmt" "maps" + pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/driver" "github.com/devsy-org/devsy/pkg/log" corev1 "k8s.io/api/core/v1" @@ -71,8 +72,8 @@ func (k *KubernetesDriver) buildPersistentVolumeClaim( } labels := map[string]string{} - labels[DevsyWorkspaceUIDLabel] = options.UID maps.Copy(labels, ExtraDevsyLabels) + maps.Copy(labels, pkgconfig.K8sVolumeLabels(options.UID, pkgconfig.VolumeRoleWorkspace)) annotations := map[string]string{} annotations[DevsyInfoAnnotation] = containerInfo diff --git a/pkg/driver/kubernetes/pvc_test.go b/pkg/driver/kubernetes/pvc_test.go new file mode 100644 index 000000000..ecdb4a426 --- /dev/null +++ b/pkg/driver/kubernetes/pvc_test.go @@ -0,0 +1,43 @@ +package kubernetes + +import ( + "testing" + + pkgconfig "github.com/devsy-org/devsy/pkg/config" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + provider2 "github.com/devsy-org/devsy/pkg/provider" +) + +func TestBuildPersistentVolumeClaimLabels(t *testing.T) { + k := &KubernetesDriver{ + options: &provider2.ProviderKubernetesDriverConfig{ + DiskSize: "10Gi", + }, + } + + pvc, err := k.buildPersistentVolumeClaim("devsy-ws-123", &driver.RunOptions{ + UID: "ws-123", + WorkspaceMount: &config.Mount{ + Type: pkgconfig.ResourceVolume, + Target: "/workspace", + }, + }) + if err != nil { + t.Fatalf("buildPersistentVolumeClaim: %v", err) + } + + labels := pvc.Labels + want := map[string]string{ + pkgconfig.K8sManagedLabel: pkgconfig.LabelValueTrue, + pkgconfig.K8sResourceLabel: pkgconfig.ResourceVolume, + pkgconfig.K8sWorkspaceUIDLabel: "ws-123", + pkgconfig.K8sVolumeRoleLabel: pkgconfig.VolumeRoleWorkspace, + DevsyCreatedLabel: pkgconfig.LabelValueTrue, + } + for k, v := range want { + if labels[k] != v { + t.Errorf("label %s = %q, want %q", k, labels[k], v) + } + } +} diff --git a/pkg/driver/kubernetes/run.go b/pkg/driver/kubernetes/run.go index 79aba5df4..944344770 100644 --- a/pkg/driver/kubernetes/run.go +++ b/pkg/driver/kubernetes/run.go @@ -23,16 +23,12 @@ import ( const ( DevContainerName = pkgconfig.BinaryName InitContainerName = pkgconfig.BinaryName + "-init" -) - -const ( - DevsyCreatedLabel = pkgconfig.BinaryName + ".sh/created" - DevsyWorkspaceLabel = pkgconfig.BinaryName + ".sh/workspace" - DevsyWorkspaceUIDLabel = pkgconfig.BinaryName + ".sh/workspace-uid" - DevsyInfoAnnotation = pkgconfig.BinaryName + ".sh/info" - DevsyLastAppliedAnnotation = pkgconfig.BinaryName + ".sh/last-applied-configuration" - ClusterAutoscalerSaveToEvictAnnotation = "cluster-autoscaler.kubernetes.io/safe-to-evict" + DevsyCreatedLabel = pkgconfig.K8sCreatedLabel + DevsyWorkspaceLabel = pkgconfig.K8sWorkspaceLabel + DevsyWorkspaceUIDLabel = pkgconfig.K8sWorkspaceUIDLabel + DevsyInfoAnnotation = pkgconfig.K8sInfoAnnotation + DevsyLastAppliedAnnotation = pkgconfig.K8sLastAppliedAnnotation ) var ExtraDevsyLabels = map[string]string{ @@ -329,7 +325,7 @@ func (k *KubernetesDriver) runPod(ctx context.Context, id string, pod *corev1.Po pod.Annotations = map[string]string{} } pod.Annotations[DevsyLastAppliedAnnotation] = string(lastAppliedConfigRaw) - pod.Annotations[ClusterAutoscalerSaveToEvictAnnotation] = "false" + pod.Annotations[pkgconfig.ClusterAutoscalerSafeToEvictAnnotation] = "false" // marshal the pod podRaw, err := json.Marshal(pod) diff --git a/pkg/platform/form/form.go b/pkg/platform/form/form.go index 6b6796d65..03bc51916 100644 --- a/pkg/platform/form/form.go +++ b/pkg/platform/form/form.go @@ -11,11 +11,11 @@ import ( managementv1 "github.com/devsy-org/api/pkg/apis/management/v1" storagev1 "github.com/devsy-org/api/pkg/apis/storage/v1" "github.com/devsy-org/devsy/cmd/pro/provider/list" + "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/encoding" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/platform" "github.com/devsy-org/devsy/pkg/platform/client" - "github.com/devsy-org/devsy/pkg/platform/labels" "github.com/devsy-org/devsy/pkg/platform/project" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/yaml" @@ -100,7 +100,7 @@ func CreateInstance( Labels: map[string]string{ storagev1.DevsyWorkspaceIDLabel: id, storagev1.DevsyWorkspaceUIDLabel: uid, - labels.ProjectLabel: selectedProject.GetName(), + config.K8sProjectLabel: selectedProject.GetName(), }, Annotations: map[string]string{ storagev1.DevsyWorkspacePictureAnnotation: picture, diff --git a/pkg/platform/labels/labels.go b/pkg/platform/labels/labels.go deleted file mode 100644 index 0a275dcc6..000000000 --- a/pkg/platform/labels/labels.go +++ /dev/null @@ -1,4 +0,0 @@ -package labels - -// ProjectLabel holds the project name. -const ProjectLabel = "devsy.sh/project" diff --git a/pkg/workspace/list.go b/pkg/workspace/list.go index eee403055..5dadb3cbe 100644 --- a/pkg/workspace/list.go +++ b/pkg/workspace/list.go @@ -22,8 +22,6 @@ import ( "github.com/devsy-org/devsy/pkg/types" ) -const ProjectLabel = "devsy.sh/project" - func List( ctx context.Context, devsyConfig *config.Config, @@ -253,7 +251,7 @@ func listProWorkspacesForProvider( } // project - projectName := instance.GetLabels()[ProjectLabel] + projectName := instance.GetLabels()[config.K8sProjectLabel] // source source := providerpkg.WorkspaceSource{} From 4f8d7bef4d964931e1d48e49bb812bb2a1599d3e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 6 Jul 2026 18:05:11 -0500 Subject: [PATCH 2/3] fix: address PR review findings - delivery: Cleanup now continues past a failed volume removal and aggregates errors instead of aborting on the first, so a transient failure no longer orphans the remaining managed volumes. - delivery: use the LabelValueTrue constant in the managed-volume filter instead of a hardcoded "true". - config/legacy: migrateLegacyExtensions merges legacy extensions with any new-style customizations.vscode.extensions (de-duplicated) instead of overwriting them; migrateLegacyDevPort always clears the deprecated field so it is not re-emitted on save. Add legacy migration tests. - config/discover: SelectSingle guards against an empty candidate slice. - config/read, config/apply: thread the caller's context through config resolution instead of using context.Background(). - cmd/pro: drop stale 'Don't print debug message' comments. - config test: align ptr helper with new(expr). --- cmd/config/apply.go | 10 ++- cmd/config/read.go | 12 ++-- cmd/pro/check_update.go | 1 - cmd/pro/daemon/netcheck.go | 1 - cmd/pro/health.go | 1 - pkg/agent/delivery/local_docker.go | 10 ++- pkg/devcontainer/config/discover.go | 8 ++- pkg/devcontainer/config/legacy.go | 21 +++++- pkg/devcontainer/config/legacy_test.go | 90 ++++++++++++++++++++++++++ pkg/devcontainer/config_test.go | 3 +- 10 files changed, 130 insertions(+), 27 deletions(-) create mode 100644 pkg/devcontainer/config/legacy_test.go diff --git a/cmd/config/apply.go b/cmd/config/apply.go index bbb7dfcff..dac8f5bc3 100644 --- a/cmd/config/apply.go +++ b/cmd/config/apply.go @@ -126,7 +126,7 @@ func (cmd *ApplyCmd) prepareContainer( return nil, nil, err } - result, err := cmd.loadConfig(containerDetails) + result, err := cmd.loadConfig(ctx, containerDetails) if err != nil { emitErr(emitJSON, err) return nil, nil, err @@ -171,22 +171,20 @@ func (cmd *ApplyCmd) inspectRunningContainer( } func (cmd *ApplyCmd) loadConfig( + ctx context.Context, containerDetails *devcconfig.ContainerDetails, ) (*devcconfig.Result, error) { var devContainerConfig *devcconfig.DevContainerConfig var err error if cmd.Config != "" { - devContainerConfig, err = devcconfig.ParseDevContainerJSONFile( - context.Background(), - cmd.Config, - ) + devContainerConfig, err = devcconfig.ParseDevContainerJSONFile(ctx, cmd.Config) } else { cwd, cwdErr := os.Getwd() if cwdErr != nil { return nil, fmt.Errorf("get working directory: %w", cwdErr) } - devContainerConfig, err = devcconfig.ParseDevContainerJSON(context.Background(), cwd, "") + devContainerConfig, err = devcconfig.ParseDevContainerJSON(ctx, cwd, "") } if err != nil { return nil, fmt.Errorf("parse devcontainer config: %w", err) diff --git a/cmd/config/read.go b/cmd/config/read.go index 3a06756fa..139880602 100644 --- a/cmd/config/read.go +++ b/cmd/config/read.go @@ -161,10 +161,10 @@ func (cmd *ReadCmd) resolve(ctx context.Context) ( if len(cmd.IDLabels) > 0 { return cmd.resolveConfigFromIDLabels(ctx) } - return cmd.resolveConfig() + return cmd.resolveConfig(ctx) } -func (cmd *ReadCmd) resolveConfig() ( +func (cmd *ReadCmd) resolveConfig(ctx context.Context) ( *devcconfig.DevContainerConfig, string, error, @@ -191,13 +191,9 @@ func (cmd *ReadCmd) resolveConfig() ( var parsedConfig *devcconfig.DevContainerConfig if cmd.Config != "" { - parsedConfig, err = devcconfig.ParseDevContainerJSONFile(context.Background(), cmd.Config) + parsedConfig, err = devcconfig.ParseDevContainerJSONFile(ctx, cmd.Config) } else { - parsedConfig, err = devcconfig.ParseDevContainerJSON( - context.Background(), - workspaceFolder, - "", - ) + parsedConfig, err = devcconfig.ParseDevContainerJSON(ctx, workspaceFolder, "") } if err != nil { return nil, "", fmt.Errorf("parse devcontainer config: %w", err) diff --git a/cmd/pro/check_update.go b/cmd/pro/check_update.go index 248f586c0..27953762e 100644 --- a/cmd/pro/check_update.go +++ b/cmd/pro/check_update.go @@ -53,7 +53,6 @@ func NewCheckUpdateCmd(globalFlags *flags.GlobalFlags) *cobra.Command { if root.Annotations == nil { root.Annotations = map[string]string{} } - // Don't print debug message root.Annotations[config.AgentExecutedAnnotation] = "true" //nolint:goconst }, } diff --git a/cmd/pro/daemon/netcheck.go b/cmd/pro/daemon/netcheck.go index 6300bc001..7b874eee8 100644 --- a/cmd/pro/daemon/netcheck.go +++ b/cmd/pro/daemon/netcheck.go @@ -52,7 +52,6 @@ func NewNetcheckCmd(flags *proflags.GlobalFlags) *cobra.Command { if root.Annotations == nil { root.Annotations = map[string]string{} } - // Don't print debug message root.Annotations[config.AgentExecutedAnnotation] = "true" }, } diff --git a/cmd/pro/health.go b/cmd/pro/health.go index 2bf0083e6..c7527960a 100644 --- a/cmd/pro/health.go +++ b/cmd/pro/health.go @@ -55,7 +55,6 @@ func NewHealthCmd(globalFlags *flags.GlobalFlags) *cobra.Command { if root.Annotations == nil { root.Annotations = map[string]string{} } - // Don't print debug message root.Annotations[config.AgentExecutedAnnotation] = "true" //nolint:goconst }, } diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index 22eca6b11..c61545a22 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -3,6 +3,7 @@ package delivery import ( "bytes" "context" + "errors" "fmt" "io" "os" @@ -83,13 +84,16 @@ func (d *LocalDockerDelivery) Cleanup(ctx context.Context, workspaceID string) e if err != nil { return err } + // Attempt every volume so one transient failure does not orphan the rest. + var errs []error for _, name := range volumes { if err := d.removeVolume(ctx, name); err != nil { - return err + errs = append(errs, err) + continue } log.Infof("removed devsy-managed volume: %s", name) } - return nil + return errors.Join(errs...) } func (d *LocalDockerDelivery) ensureCurrentBinary( @@ -306,7 +310,7 @@ func (d *LocalDockerDelivery) listManagedVolumes( ) ([]string, error) { out, err := d.cmd(ctx, "volume", "ls", "--quiet", - "--filter", "label="+pkgconfig.DockerManagedLabel+"=true", + "--filter", "label="+pkgconfig.DockerManagedLabel+"="+pkgconfig.LabelValueTrue, "--filter", "label="+pkgconfig.DockerWorkspaceIDLabel+"="+workspaceID, ).CombinedOutput() if err != nil { diff --git a/pkg/devcontainer/config/discover.go b/pkg/devcontainer/config/discover.go index d4fbef25d..558d6025a 100644 --- a/pkg/devcontainer/config/discover.go +++ b/pkg/devcontainer/config/discover.go @@ -35,14 +35,18 @@ func SelectByID(id string) ConfigSelector { // than one config exists, listing the available ids so the caller can choose. func SelectSingle(folder string) ConfigSelector { return func(candidates []string) (string, error) { - if len(candidates) > 1 { + switch { + case len(candidates) == 0: + return "", fmt.Errorf("no devcontainer configuration found") + case len(candidates) > 1: ids, _ := ListDevContainerIDs(folder) return "", fmt.Errorf( "multiple devcontainer configurations found. Detected: %v", ids, ) + default: + return candidates[0], nil } - return candidates[0], nil } } diff --git a/pkg/devcontainer/config/legacy.go b/pkg/devcontainer/config/legacy.go index ee50b13c2..e52198c99 100644 --- a/pkg/devcontainer/config/legacy.go +++ b/pkg/devcontainer/config/legacy.go @@ -31,7 +31,18 @@ func migrateLegacyExtensions(config *DevContainerConfig, vsCode *VSCodeCustomiza if len(config.Extensions) == 0 { return } - vsCode.Extensions = config.Extensions + // Append legacy extensions to any new-style ones rather than replacing, and + // de-duplicate so a config that sets both does not lose entries. + seen := make(map[string]bool, len(vsCode.Extensions)) + for _, ext := range vsCode.Extensions { + seen[ext] = true + } + for _, ext := range config.Extensions { + if !seen[ext] { + vsCode.Extensions = append(vsCode.Extensions, ext) + seen[ext] = true + } + } config.Extensions = nil } @@ -51,9 +62,13 @@ func migrateLegacySettings(config *DevContainerConfig, vsCode *VSCodeCustomizati } func migrateLegacyDevPort(config *DevContainerConfig, vsCode *VSCodeCustomizations) { - if vsCode.DevPort != 0 { + if config.DevPort == 0 { return } - vsCode.DevPort = config.DevPort + // Only backfill when the new-style value is unset, but always clear the + // deprecated field so it is not re-emitted on save. + if vsCode.DevPort == 0 { + vsCode.DevPort = config.DevPort + } config.DevPort = 0 } diff --git a/pkg/devcontainer/config/legacy_test.go b/pkg/devcontainer/config/legacy_test.go new file mode 100644 index 000000000..4d26f60f8 --- /dev/null +++ b/pkg/devcontainer/config/legacy_test.go @@ -0,0 +1,90 @@ +package config + +import ( + "slices" + "testing" +) + +const sharedExt = "shared.ext" + +func vscodeOf(t *testing.T, config *DevContainerConfig) *VSCodeCustomizations { + t.Helper() + vscode := &VSCodeCustomizations{} + if err := convert(config.Customizations[testUserVscode], vscode); err != nil { + t.Fatalf("convert vscode customizations: %v", err) + } + return vscode +} + +func TestReplaceLegacyExtensionsMergeWithNewStyle(t *testing.T) { + config := &DevContainerConfig{ + DevContainerConfigBase: DevContainerConfigBase{ + Extensions: []string{"legacy.ext", sharedExt}, + }, + DevContainerActions: DevContainerActions{ + Customizations: map[string]any{ + testUserVscode: map[string]any{ + "extensions": []string{"new.ext", sharedExt}, + }, + }, + }, + } + + out, err := replaceLegacy(config) + if err != nil { + t.Fatal(err) + } + if out.Extensions != nil { + t.Errorf("legacy Extensions should be cleared, got %v", out.Extensions) + } + + got := vscodeOf(t, out).Extensions + // New-style entries are preserved and legacy ones appended without dupes. + for _, want := range []string{"new.ext", sharedExt, "legacy.ext"} { + if !slices.Contains(got, want) { + t.Errorf("expected %q in merged extensions, got %v", want, got) + } + } + if len(got) != 3 { + t.Errorf("expected 3 unique extensions, got %d: %v", len(got), got) + } +} + +func TestReplaceLegacyDevPortClearedWhenNewStyleWins(t *testing.T) { + config := &DevContainerConfig{ + DevContainerConfigBase: DevContainerConfigBase{DevPort: 8080}, + DevContainerActions: DevContainerActions{ + Customizations: map[string]any{ + testUserVscode: map[string]any{"devPort": float64(9090)}, + }, + }, + } + + out, err := replaceLegacy(config) + if err != nil { + t.Fatal(err) + } + if out.DevPort != 0 { + t.Errorf("legacy DevPort should be cleared, got %d", out.DevPort) + } + if got := vscodeOf(t, out).DevPort; got != 9090 { + t.Errorf("new-style DevPort should win, got %d", got) + } +} + +func TestReplaceLegacyDevPortBackfilled(t *testing.T) { + config := &DevContainerConfig{ + DevContainerConfigBase: DevContainerConfigBase{DevPort: 8080}, + } + + out, err := replaceLegacy(config) + if err != nil { + t.Fatal(err) + } + if out.DevPort != 0 { + t.Errorf("legacy DevPort should be cleared, got %d", out.DevPort) + } + if got := vscodeOf(t, out).DevPort; got != 8080 { + t.Errorf("DevPort should be backfilled, got %d", got) + } +} diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 3b4c7bfce..24bfe72fb 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -359,8 +359,7 @@ func (s *SubstituteTestSuite) TestSubstitute_CLIMountsEmpty() { s.Equal("/existing-target", substitutedConfig.Config.Mounts[0].Target) } -//go:fix inline -func ptr(s string) *string { return &s } +func ptr(s string) *string { return new(s) } func TestWorkspaceMountFolderWarning(t *testing.T) { tests := []struct { From 159da39d336f4ff8639e6950b37e8a3ffa17d071 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 6 Jul 2026 18:14:35 -0500 Subject: [PATCH 3/3] fix: thread context through remaining config parse call sites Address the deferred review findings by propagating the caller's context instead of context.Background() through the config resolution paths that reach ParseDevContainerJSON(File): - config.MergeExtraRemoteEnv now takes a context.Context and passes it to the parser; all callers updated. - devcontainer runner: mergeExistingContainerConfig and the compose mergeImageMetadataConfig / generateComposeUpOverride helpers thread ctx from their (already context-aware) callers. - cmd/internal run-user-commands: loadContainerIDConfig accepts and forwards ctx. This lets extends resolution and file parsing honor caller cancellation and timeouts in these paths. --- cmd/internal/runusercommands.go | 12 +++++++++--- pkg/devcontainer/compose.go | 12 +++++++----- pkg/devcontainer/config/merge.go | 8 ++++++-- pkg/devcontainer/single.go | 8 +++++--- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index 1eee05cc7..0421e4546 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -193,7 +193,7 @@ func (cmd *RunUserCommandsCmd) runWithContainerID(ctx context.Context) error { return err } - result, err := cmd.loadContainerIDConfig(containerDetails) + result, err := cmd.loadContainerIDConfig(ctx, containerDetails) if err != nil { return err } @@ -265,6 +265,7 @@ func (cmd *RunUserCommandsCmd) inspectRunningContainer( } func (cmd *RunUserCommandsCmd) loadContainerIDConfig( + ctx context.Context, containerDetails *devcconfig.ContainerDetails, ) (*devcconfig.Result, error) { configFolder := cmd.WorkspaceFolder @@ -273,7 +274,7 @@ func (cmd *RunUserCommandsCmd) loadContainerIDConfig( } devContainerConfig, err := devcconfig.ParseDevContainerJSON( - context.Background(), + ctx, configFolder, cmd.Config, ) @@ -294,7 +295,11 @@ func (cmd *RunUserCommandsCmd) loadContainerIDConfig( } if cmd.OverrideConfig != "" { - if err := devcconfig.MergeExtraRemoteEnv(mergedConfig, cmd.OverrideConfig); err != nil { + if err := devcconfig.MergeExtraRemoteEnv( + ctx, + mergedConfig, + cmd.OverrideConfig, + ); err != nil { _ = devcconfig.WriteErrorJSON(os.Stderr, err.Error()) return nil, fmt.Errorf("apply override config: %w", err) } @@ -357,6 +362,7 @@ func (cmd *RunUserCommandsCmd) resolveContainer( if cmd.OverrideConfig != "" { if err := devcconfig.MergeExtraRemoteEnv( + ctx, result.MergedConfig, cmd.OverrideConfig, ); err != nil { diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index 962a55407..7d6f5eac3 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -336,6 +336,7 @@ func (r *runner) finalizeComposeContainer( } mergedConfig, err := mergeImageMetadataConfig( + ctx, parsedConfig, imageMetadataConfig, options.ExtraDevContainerPath, @@ -774,6 +775,7 @@ func (r *runner) buildComposeOverrideArgs( } overrideComposeUpFilePath, err := r.generateComposeUpOverride( + ctx, params, extendResult, imageDetails, @@ -792,6 +794,7 @@ func (r *runner) buildComposeOverrideArgs( // generateComposeUpOverride merges the image metadata into the devcontainer // config and writes the compose "up" override file, returning its path. func (r *runner) generateComposeUpOverride( + ctx context.Context, params *composeOverrideParams, extendResult composeExtendResult, imageDetails *config.ImageDetails, @@ -799,6 +802,7 @@ func (r *runner) generateComposeUpOverride( start := params.startParams mergedConfig, err := mergeImageMetadataConfig( + ctx, start.parsedConfig, extendResult.imageMetadata, start.options.ExtraDevContainerPath, @@ -832,6 +836,7 @@ func (r *runner) generateComposeUpOverride( // metadata, merges it with the parsed config, and applies extra remote env, // returning the resulting merged devcontainer config. func mergeImageMetadataConfig( + ctx context.Context, parsedConfig *config.SubstitutedConfig, imageMetadata *config.ImageMetadataConfig, extraDevContainerPath string, @@ -840,10 +845,7 @@ func mergeImageMetadataConfig( if imageMetadata == nil { imageMetadata = &config.ImageMetadataConfig{} } - extraConfig, err := config.ParseDevContainerJSONFile( - context.Background(), - extraDevContainerPath, - ) + extraConfig, err := config.ParseDevContainerJSONFile(ctx, extraDevContainerPath) if err != nil { return nil, err } @@ -855,7 +857,7 @@ func mergeImageMetadataConfig( return nil, fmt.Errorf("merge configuration: %w", err) } - if err := config.MergeExtraRemoteEnv(mergedConfig, extraDevContainerPath); err != nil { + if err := config.MergeExtraRemoteEnv(ctx, mergedConfig, extraDevContainerPath); err != nil { return nil, err } diff --git a/pkg/devcontainer/config/merge.go b/pkg/devcontainer/config/merge.go index 8fc8930c3..d633f3c19 100644 --- a/pkg/devcontainer/config/merge.go +++ b/pkg/devcontainer/config/merge.go @@ -18,11 +18,15 @@ const ( gpuOptional = "optional" ) -func MergeExtraRemoteEnv(mergedConfig *MergedDevContainerConfig, extraConfigPath string) error { +func MergeExtraRemoteEnv( + ctx context.Context, + mergedConfig *MergedDevContainerConfig, + extraConfigPath string, +) error { if extraConfigPath == "" { return nil } - extraConfig, err := ParseDevContainerJSONFile(context.Background(), extraConfigPath) + extraConfig, err := ParseDevContainerJSONFile(ctx, extraConfigPath) if err != nil { return err } diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index d2e3b399e..d8350147d 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -159,7 +159,7 @@ func (r *runner) resolveExistingContainer( p.substitutionContext.ContainerWorkspaceFolder = containerDetails.Config.WorkingDir } - mergedConfig, err := r.mergeExistingContainerConfig(containerDetails, p) + mergedConfig, err := r.mergeExistingContainerConfig(ctx, containerDetails, p) if err != nil { return nil, err } @@ -194,6 +194,7 @@ func (r *runner) ensureRunning( // mergeExistingContainerConfig extracts image metadata from the running // container and merges it with the parsed devcontainer configuration. func (r *runner) mergeExistingContainerConfig( + ctx context.Context, containerDetails *config.ContainerDetails, p *resolveParams, ) (*config.MergedDevContainerConfig, error) { @@ -210,7 +211,7 @@ func (r *runner) mergeExistingContainerConfig( imageMetadataConfig = &config.ImageMetadataConfig{} } extraConfig, parseErr := config.ParseDevContainerJSONFile( - context.Background(), + ctx, p.options.ExtraDevContainerPath, ) if parseErr != nil { @@ -228,7 +229,7 @@ func (r *runner) mergeExistingContainerConfig( } if err := config.MergeExtraRemoteEnv( - mergedConfig, p.options.ExtraDevContainerPath, + ctx, mergedConfig, p.options.ExtraDevContainerPath, ); err != nil { return nil, err } @@ -332,6 +333,7 @@ func (r *runner) buildNewContainerConfig( } if err := config.MergeExtraRemoteEnv( + ctx, mergedConfig, p.options.ExtraDevContainerPath, ); err != nil {