Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cmd/up.go
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,10 @@ func (cmd *UpCmd) registerDevContainerFlags(upCmd *cobra.Command) {
upCmd.Flags().
StringVar(&cmd.AdditionalFeatures, "additional-features", "",
`Additional features to apply to the dev container (JSON as per "features" section in devcontainer.json)`)
upCmd.Flags().
StringArrayVar(&cmd.Mounts, "mount", []string{},
"Additional mount to apply when creating the dev container. "+
"Format type=<bind|volume>,source=<source>,target=<target>[,external=<true|false>]")
}

func (cmd *UpCmd) registerIDEFlags(upCmd *cobra.Command) {
Expand Down Expand Up @@ -714,6 +718,7 @@ func mergeDevPodUpOptions(baseOptions *provider2.CLIOptions) error {
} else if found {
baseOptions.WorkspaceEnv = append(oldOptions.WorkspaceEnv, baseOptions.WorkspaceEnv...)
baseOptions.InitEnv = append(oldOptions.InitEnv, baseOptions.InitEnv...)
baseOptions.Mounts = append(oldOptions.Mounts, baseOptions.Mounts...)
baseOptions.PrebuildRepositories = append(
oldOptions.PrebuildRepositories,
baseOptions.PrebuildRepositories...)
Expand Down
25 changes: 25 additions & 0 deletions cmd/up_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package cmd

import (
"testing"

"github.com/skevetter/devpod/cmd/flags"
"github.com/stretchr/testify/require"
)

func TestUpMountFlagIsRepeatableAndPreservesValues(t *testing.T) {
upCmd := NewUpCmd(&flags.GlobalFlags{})
args := []string{
"--mount", `{"type":"volume","source":"cache","target":"/cache"}`,
"--mount", `type=bind,source=/tmp/data,target=/data,readonly`,
}

require.NoError(t, upCmd.ParseFlags(args))

mounts, err := upCmd.Flags().GetStringArray("mount")
require.NoError(t, err)
require.Equal(t, []string{
`{"type":"volume","source":"cache","target":"/cache"}`,
`type=bind,source=/tmp/data,target=/data,readonly`,
}, mounts)
}
6 changes: 6 additions & 0 deletions pkg/devcontainer/compose.go
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@ func (r *runner) runDockerCompose(
if err != nil {
return nil, fmt.Errorf("merge config: %w", err)
}
if err := mergeCLIMounts(mergedConfig, substitutionContext, options.Mounts); err != nil {
return nil, err
}

// expose the compose project name inside the container
if mergedConfig.RemoteEnv == nil {
Expand Down Expand Up @@ -479,6 +482,9 @@ func (r *runner) startContainer(
if err != nil {
return nil, fmt.Errorf("merge configuration: %w", err)
}
if err := mergeCLIMounts(mergedConfig, substitutionContext, options.Mounts); err != nil {
return nil, err
}

additionalLabels := map[string]string{
metadata.ImageMetadataLabel: extendResult.metadataLabel,
Expand Down
67 changes: 67 additions & 0 deletions pkg/devcontainer/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,3 +221,70 @@ func (r *runner) substitute(
Raw: rawParsedConfig,
}, substitutionContext, nil
}

func resolveCLIMounts(
substitutionContext *config.SubstitutionContext,
mounts []string,
) ([]*config.Mount, error) {
if len(mounts) == 0 {
return nil, nil
}

resolvedMounts := make([]*config.Mount, 0, len(mounts))
for _, mount := range mounts {
parsedMount := &config.Mount{}
if err := json.Unmarshal([]byte(mount), parsedMount); err != nil {
parsed := config.ParseMount(mount)
parsedMount = &parsed
}

resolvedMount := &config.Mount{}
if err := config.Substitute(substitutionContext, parsedMount, resolvedMount); err != nil {
return nil, fmt.Errorf("substitute --mount values: %w", err)
}
Comment on lines +236 to +244
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject malformed --mount input early and require a target.

Right now, malformed JSON mount strings fall back to raw parsing, which can silently create mounts with empty Target. Those then enter dedupe/merge logic and only fail later at container startup with a less actionable error.

Proposed fix
 import (
 	"encoding/json"
 	"fmt"
 	"maps"
 	"os"
 	"path"
 	"path/filepath"
 	"slices"
+	"strings"
@@
 	for _, mount := range mounts {
+		mountInput := strings.TrimSpace(mount)
 		parsedMount := &config.Mount{}
-		if err := json.Unmarshal([]byte(mount), parsedMount); err != nil {
-			parsed := config.ParseMount(mount)
+		if err := json.Unmarshal([]byte(mountInput), parsedMount); err != nil {
+			if strings.HasPrefix(mountInput, "{") || strings.HasPrefix(mountInput, "[") {
+				return nil, fmt.Errorf("parse --mount JSON: %w", err)
+			}
+			parsed := config.ParseMount(mountInput)
 			parsedMount = &parsed
 		}
@@
 		if err := config.Substitute(substitutionContext, parsedMount, resolvedMount); err != nil {
 			return nil, fmt.Errorf("substitute --mount values: %w", err)
 		}
+		if strings.TrimSpace(resolvedMount.Target) == "" {
+			return nil, fmt.Errorf("invalid --mount %q: target is required", mount)
+		}
 
 		resolvedMounts = append(resolvedMounts, resolvedMount)
 	}

Also applies to: 265-281

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/devcontainer/config.go` around lines 236 - 244, If json.Unmarshal fails
we must not silently fallback to raw parsing because that can leave
parsedMount.Target empty; instead return a clear error for malformed --mount
input and require a non-empty Target before calling config.Substitute. Update
the block around json.Unmarshal / config.ParseMount to: on unmarshal error,
return fmt.Errorf("malformed --mount: %w", err) (or validate the ParseMount
result and return fmt.Errorf("invalid --mount: missing target") if Target ==
""), and add the same Target non-empty check before calling config.Substitute
(apply the same validation for the alternate code path referenced around the
265-281 region) so Substitute is only called for mounts with a valid Target.


resolvedMounts = append(resolvedMounts, resolvedMount)
}

return resolvedMounts, nil
}

func mergeCLIMounts(
mergedConfig *config.MergedDevContainerConfig,
substitutionContext *config.SubstitutionContext,
mounts []string,
) error {
resolvedMounts, err := resolveCLIMounts(substitutionContext, mounts)
if err != nil {
return err
}
if len(resolvedMounts) == 0 {
return nil
}

cliTargets := map[string]bool{}
dedupedCLIMounts := make([]*config.Mount, 0, len(resolvedMounts))
for i := len(resolvedMounts) - 1; i >= 0; i-- {
mount := resolvedMounts[i]
if cliTargets[mount.Target] {
continue
}

cliTargets[mount.Target] = true
dedupedCLIMounts = append(dedupedCLIMounts, mount)
}
slices.Reverse(dedupedCLIMounts)

filteredMounts := make([]*config.Mount, 0, len(mergedConfig.Mounts)+len(dedupedCLIMounts))
for _, mount := range mergedConfig.Mounts {
if cliTargets[mount.Target] {
continue
}

filteredMounts = append(filteredMounts, mount)
}

filteredMounts = append(filteredMounts, dedupedCLIMounts...)
mergedConfig.Mounts = filteredMounts
return nil
}
19 changes: 11 additions & 8 deletions pkg/devcontainer/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -413,21 +413,24 @@ func ParseMount(str string) Mount {
retMount := Mount{}
splitted := strings.SplitSeq(str, ",")
for split := range splitted {
splitted2 := strings.Split(split, "=")
key := splitted2[0]
key, value, ok := strings.Cut(split, "=")
if !ok {
retMount.Other = append(retMount.Other, split)
continue
}
switch key {
case "src", "source":
retMount.Source = splitted2[1]
retMount.Source = value
case "workspaceMount":
retMount.Source = splitted2[1]
retMount.Source = value
case "workspaceFolder":
retMount.Target = splitted2[1]
retMount.Target = value
case "dst", "destination", "target":
retMount.Target = splitted2[1]
retMount.Target = value
case "type":
retMount.Type = splitted2[1]
retMount.Type = value
case "external":
retMount.External, _ = strconv.ParseBool(splitted2[1])
retMount.External, _ = strconv.ParseBool(value)
default:
retMount.Other = append(retMount.Other, split)
}
Expand Down
25 changes: 25 additions & 0 deletions pkg/devcontainer/config/mount_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package config

import "testing"

func TestParseMount_PreservesEqualsInValue(t *testing.T) {
mount := ParseMount("type=volume,source=my-cache=1,target=/cache")

if mount.Type != "volume" {
t.Fatalf("expected type volume, got %q", mount.Type)
}
if mount.Source != "my-cache=1" {
t.Fatalf("expected source my-cache=1, got %q", mount.Source)
}
if mount.Target != "/cache" {
t.Fatalf("expected target /cache, got %q", mount.Target)
}
}

func TestParseMount_InvalidSegmentDoesNotPanic(t *testing.T) {
mount := ParseMount("type=volume,readonly,target=/cache")

if len(mount.Other) != 1 || mount.Other[0] != "readonly" {
t.Fatalf("expected readonly in other, got %#v", mount.Other)
}
}
73 changes: 73 additions & 0 deletions pkg/devcontainer/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,76 @@ func (s *SubstituteTestSuite) TestSubstitute_AdditionalFeaturesEmpty() {
s.NoError(err)
s.Nil(result.Config.Features)
}

func (s *SubstituteTestSuite) TestResolveCLIMounts_SubstitutesVariables() {
substitutionContext := &config.SubstitutionContext{
DevContainerID: "test-id",
LocalWorkspaceFolder: "/workspace",
ContainerWorkspaceFolder: "/workspaces/test-workspace",
Env: map[string]string{
"CACHE_NAME": "my-cache=1",
},
}

mounts, err := resolveCLIMounts(substitutionContext, []string{
`{"type":"volume","source":"${localEnv:CACHE_NAME}","target":"${containerWorkspaceFolder}/cache"}`,
})

s.NoError(err)
s.Len(mounts, 1)
s.Equal("volume", mounts[0].Type)
s.Equal("my-cache=1", mounts[0].Source)
s.Equal("/workspaces/test-workspace/cache", mounts[0].Target)
}

func (s *SubstituteTestSuite) TestResolveCLIMounts_AcceptsRawStringForm() {
substitutionContext := &config.SubstitutionContext{
ContainerWorkspaceFolder: "/workspaces/test-workspace",
}

mounts, err := resolveCLIMounts(substitutionContext, []string{
"type=bind,source=/tmp/data,target=${containerWorkspaceFolder}/data,readonly",
})

s.NoError(err)
s.Len(mounts, 1)
s.Equal("bind", mounts[0].Type)
s.Equal("/tmp/data", mounts[0].Source)
s.Equal("/workspaces/test-workspace/data", mounts[0].Target)
s.Equal([]string{"readonly"}, mounts[0].Other)
}

func (s *SubstituteTestSuite) TestMergeCLIMounts_OverridesByTarget() {
mergedConfig := &config.MergedDevContainerConfig{
NonComposeBase: config.NonComposeBase{
Mounts: []*config.Mount{
{
Type: "volume",
Source: "existing-cache",
Target: "/cache",
},
{
Type: "volume",
Source: "existing-tools",
Target: "/tools",
},
},
},
}
substitutionContext := &config.SubstitutionContext{
ContainerWorkspaceFolder: "/workspaces/test-workspace",
}

err := mergeCLIMounts(mergedConfig, substitutionContext, []string{
`{"type":"volume","source":"cli-cache","target":"/cache"}`,
`{"type":"volume","source":"cli-data","target":"${containerWorkspaceFolder}/data"}`,
})

s.NoError(err)
s.Len(mergedConfig.Mounts, 3)
s.Equal("existing-tools", mergedConfig.Mounts[0].Source)
s.Equal("cli-cache", mergedConfig.Mounts[1].Source)
s.Equal("/cache", mergedConfig.Mounts[1].Target)
s.Equal("cli-data", mergedConfig.Mounts[2].Source)
s.Equal("/workspaces/test-workspace/data", mergedConfig.Mounts[2].Target)
}
6 changes: 6 additions & 0 deletions pkg/devcontainer/single.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ func (r *runner) mergeExistingContainerConfig(
if err != nil {
return nil, fmt.Errorf("merge config: %w", err)
}
if err := mergeCLIMounts(mergedConfig, p.substitutionContext, p.options.Mounts); err != nil {
return nil, err
}
return mergedConfig, nil
}

Expand Down Expand Up @@ -241,6 +244,9 @@ func (r *runner) resolveNewContainer(
if err != nil {
return nil, fmt.Errorf("merge config: %w", err)
}
if err := mergeCLIMounts(mergedConfig, p.substitutionContext, p.options.Mounts); err != nil {
return nil, err
}

r.injectDaemonEntrypoint(p, mergedConfig)

Expand Down
1 change: 1 addition & 0 deletions pkg/provider/workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ type CLIOptions struct {
SSHAuthSockID string `json:"sshAuthSockID,omitempty"` // ID to use when looking for SSH_AUTH_SOCK, defaults to a new random ID if not set (only used for browser IDEs)
StrictHostKeyChecking bool `json:"strictHostKeyChecking,omitempty"`
AdditionalFeatures string `json:"additionalFeatures,omitempty"`
Mounts []string `json:"mounts,omitempty"`
ExtraDevContainerPath string `json:"extraDevContainerPath,omitempty"`
User string `json:"user,omitempty"`
Userns string `json:"userns,omitempty"`
Expand Down
Loading