fix(env): Remove Windows-incompatible processing logic for -v

We just pass the arguments straight to `docker run`, with no extra validation, instead. This means we also have to duplicate the test to test separately for `-v` and `-m` options.

Author: Ishirui

src/dda/env/dev/types/linux_container.py

@@ -89,7 +89,6 @@ class LinuxContainerConfig(DeveloperEnvironmentConfig):
 > To mount a volume or use other options, use the more explicit `-m/--mount` option instead.
 """
                 ),
-                "callback": __validate_extra_volume_specs,
             }
         ),
     ] = msgspec.field(default_factory=list)
@@ -209,6 +208,9 @@ def start(self) -> None:
             for volume in self.extra_mounts:
                 command.extend(("--mount", volume.as_csv()))
 
+            for volume_spec in self.config.extra_volume_specs:
+                command.extend(("--volume", volume_spec))
+
             command.append(self.config.image)
 
             env = EnvVars()
@@ -429,13 +431,6 @@ def extra_mounts(self) -> list[Mount]:
         from dda.utils.container.model import Mount
 
         mounts = []
-        for spec in self.config.extra_volume_specs:
-            src, dst, *extra = spec.split(":", 2)
-            read_only = "ro" in extra
-
-            # Volume specs are always bind mounts.
-            mounts.append(Mount(type="bind", path=dst, source=src, read_only=read_only))
-
         for spec in self.config.extra_mount_specs:
             type_spec, src_spec, dst_spec, *flag_specs = spec.split(",")
             mount_type: Literal["bind", "volume"] = "bind" if type_spec == "type=bind" else "volume"
@@ -497,28 +492,6 @@ def __validate_mount_src_dst(mount_type: Literal["bind", "volume"], src: str, ds
         raise BadParameter(msg)
 
 
-def __validate_extra_volume_specs(_ctx: Context, _param: Option, value: list[str]) -> list[str]:
-    from click import BadParameter
-
-    if not value:
-        return value
-
-    for spec in value:
-        try:
-            src, dst, *extra = spec.split(":", 2)
-        except ValueError as e:
-            msg = f"Invalid volume spec: {spec}. Expected format: <source>:<destination>[:<flag>]"
-            raise BadParameter(msg) from e
-        flag = extra[0] if extra else None
-        if flag not in {None, "readonly", "ro"}:
-            msg = f"Invalid volume flag: {flag}. Only the `ro` flag is supported."
-            raise BadParameter(msg)
-        # Volume specs are always bind mounts.
-        __validate_mount_src_dst("bind", src, dst)
-
-    return value
-
-
 def __validate_extra_mount_specs(_ctx: Context, _param: Option, value: list[str]) -> list[str]:
     from click import BadParameter
 

tests/env/dev/types/test_linux_container.py

@@ -619,54 +619,117 @@ def test_multiple_clones(self, dda, helpers, mocker, temp_dir, host_user_args):
         ]
 
     @pytest.mark.parametrize(
-        ("volume_mount_specs", "result_mounts"),
+        ("volume_specs"),
         [
-            # Case 1: -v, Single absolute path
-            pytest.param(
-                ["-v", "/tmp/mounted:/tmp/mounted_abs"],
-                [Mount(type="bind", path="/tmp/mounted_abs", source="/tmp/mounted", read_only=False)],
-                id="-v, single_absolute_path",
-            ),
-            # Case 2: -v, Single relative path
-            pytest.param(
-                ["-v", "./mounted:/tmp/mounted_rel"],
-                [Mount(type="bind", path="/tmp/mounted_rel", source="./mounted", read_only=False)],
-                id="-v, single_relative_path",
-            ),
-            # Case 3: -v, --volume, Multiple mounts
-            pytest.param(
-                ["-v", "/tmp/mounted:/tmp/mounted_abs", "--volume", "./mounted:/tmp/mounted_rel"],
-                [
-                    Mount(type="bind", path="/tmp/mounted_abs", source="/tmp/mounted", read_only=False),
-                    Mount(type="bind", path="/tmp/mounted_rel", source="./mounted", read_only=False),
-                ],
-                id="-v, --volume, multiple_mounts",
+            # Case 1: Single -v
+            ["-v", "/tmp/mounted:/tmp/mounted_abs"],
+            # Case 2: Single --volume
+            ["--volume", "/tmp/mounted:/tmp/mounted_abs"],
+            # Case 3: -v and --volume
+            ["-v", "/tmp/mounted:/tmp/mounted_abs", "--volume", "./mounted:/tmp/mounted_rel"],
+        ],
+    )
+    def test_extra_volume_specs(self, dda, helpers, mocker, temp_dir, host_user_args, volume_specs):
+        mocker.patch("dda.utils.ssh.write_server_config")
+
+        # Disable source and destination validation for the extra mount specs
+        mocker.patch(
+            "dda.env.dev.types.linux_container.__validate_mount_src_dst",
+            return_value=None,
+        )
+        shared_dir = temp_dir / "data" / "env" / "dev" / "linux-container" / ".shared"
+        starship_mount = get_starship_mount(shared_dir)
+        cache_volumes = get_cache_volumes()
+
+        with (
+            temp_dir.as_cwd(),
+            helpers.hybrid_patch(
+                "subprocess.run",
+                return_values={
+                    # Start command checks the status
+                    1: CompletedProcess([], returncode=0, stdout="{}"),
+                    # Start method checks the status
+                    2: CompletedProcess([], returncode=0, stdout="{}"),
+                    # Capture container run
+                    # Readiness check
+                    4: CompletedProcess([], returncode=0, stdout="Server listening on :: port 22"),
+                    # Repo cloning
+                    5: CompletedProcess([], returncode=0, stdout="{}"),
+                },
+            ) as calls,
+        ):
+            result = dda("env", "dev", "start", "--no-pull", "--clone", *volume_specs)
+
+        result.check(
+            exit_code=0,
+            output=helpers.dedent(
+                """
+                Creating and starting container: dda-linux-container-default
+                Waiting for container: dda-linux-container-default
+                Cloning repository: datadog-agent
+                """
             ),
-            # Case 4: -v, mount with ro flag
-            pytest.param(
-                ["-v", "/tmp/mounted:/tmp/mounted_abs:ro"],
-                [
-                    Mount(type="bind", path="/tmp/mounted_abs", source="/tmp/mounted", read_only=True),
-                ],
-                id="-v, mount_with_ro",
+        )
+        assert calls == [
+            (
+                (
+                    [
+                        helpers.locate("docker"),
+                        "run",
+                        "--pull",
+                        "never",
+                        "-d",
+                        "--name",
+                        "dda-linux-container-default",
+                        "-p",
+                        "61938:22",
+                        "-p",
+                        "50069:9000",
+                        "-v",
+                        "/var/run/docker.sock:/var/run/docker.sock",
+                        *host_user_args,
+                        "-e",
+                        "DD_SHELL",
+                        "-e",
+                        AppEnvVars.TELEMETRY_API_KEY,
+                        "-e",
+                        AppEnvVars.TELEMETRY_USER_MACHINE_ID,
+                        "-e",
+                        GitEnvVars.AUTHOR_NAME,
+                        "-e",
+                        GitEnvVars.AUTHOR_EMAIL,
+                        *starship_mount,
+                        "-v",
+                        f"{shared_dir / 'shell' / 'zsh' / '.zsh_history'}:/root/.shared/shell/zsh/.zsh_history",
+                        *cache_volumes,
+                        *[(x if x != "-v" else "--volume") for x in volume_specs],
+                        "datadog/agent-dev-env-linux",
+                    ],
+                ),
+                {"encoding": "utf-8", "stdout": subprocess.PIPE, "stderr": subprocess.PIPE, "env": mocker.ANY},
             ),
-            # Case 5: -m, single bind mount
+        ]
+
+    @pytest.mark.parametrize(
+        ("mount_specs", "result_mounts"),
+        [
+            # Case 1: -m, single bind mount
             pytest.param(
                 ["-m", "type=bind,src=/tmp/mounted,dst=/tmp/mounted_abs"],
                 [
                     Mount(type="bind", path="/tmp/mounted_abs", source="/tmp/mounted", read_only=False),
                 ],
                 id="-m, single_bind_mount",
             ),
-            # Case 6: -m, single volume mount
+            # Case 2: -m, single volume mount
             pytest.param(
                 ["-m", "type=volume,src=some-volume,dst=/tmp/mounted_abs"],
                 [
                     Mount(type="volume", path="/tmp/mounted_abs", source="some-volume", read_only=False),
                 ],
                 id="-m, single_volume_mount",
             ),
-            # Case 7: -m, mounts with flags
+            # Case 3: -m, mounts with flags
             pytest.param(
                 [
                     "-m",
@@ -692,7 +755,7 @@ def test_multiple_clones(self, dda, helpers, mocker, temp_dir, host_user_args):
                 ],
                 id="-m, mounts_with_flags",
             ),
-            # Case 8: -m, --mount, multiple mounts with different syntax
+            # Case 4: -m, --mount, multiple mounts with different syntax
             pytest.param(
                 [
                     "-m",
@@ -711,7 +774,7 @@ def test_multiple_clones(self, dda, helpers, mocker, temp_dir, host_user_args):
             ),
         ],
     )
-    def test_extra_mounts(self, dda, helpers, mocker, temp_dir, host_user_args, volume_mount_specs, result_mounts):
+    def test_extra_mounts(self, dda, helpers, mocker, temp_dir, host_user_args, mount_specs, result_mounts):
         mocker.patch("dda.utils.ssh.write_server_config")
 
         # Disable source and destination validation for the extra mount specs
@@ -740,14 +803,7 @@ def test_extra_mounts(self, dda, helpers, mocker, temp_dir, host_user_args, volu
                 },
             ) as calls,
         ):
-            result = dda(
-                "env",
-                "dev",
-                "start",
-                "--no-pull",
-                "--clone",
-                *volume_mount_specs,
-            )
+            result = dda("env", "dev", "start", "--no-pull", "--clone", *mount_specs)
 
         result.check(
             exit_code=0,
@@ -803,45 +859,6 @@ def test_extra_mounts(self, dda, helpers, mocker, temp_dir, host_user_args, volu
             ),
         ]
 
-    @pytest.mark.parametrize(
-        ("volume_spec", "error_message"),
-        [
-            pytest.param(
-                "/i/dont/exist:/valid/path",
-                "Source must be an existing path on the host.",
-                id="absolute_src_does_not_exist",
-            ),
-            pytest.param(
-                "./i/dont/exist:/valid/path",
-                "Source must be an existing path on the host.",
-                id="relative_src_does_not_exist",
-            ),
-            pytest.param(
-                "./:dir",
-                "Destination must be an absolute path.",
-                id="dst_is_not_absolute",
-            ),
-            pytest.param(
-                "/tmp:/container:foobar",
-                "Invalid volume flag: foobar",
-                id="invalid_arbitrary_flag",
-            ),
-            pytest.param(
-                "/tmpcontainer",
-                "Expected format:",
-                id="no_colon",
-            ),
-        ],
-    )
-    def test_invalid_volume_specs(self, dda, temp_dir, mocker, volume_spec, error_message):
-        mocker.patch("subprocess.run", return_value=CompletedProcess([], returncode=0, stdout="{}"))
-
-        with temp_dir.as_cwd():
-            result = dda("env", "dev", "start", "-v", volume_spec)
-
-        result.check_exit_code(2)
-        assert error_message in result.output
-
     @pytest.mark.requires_unix  # This fails with Windows-style paths
     @pytest.mark.parametrize(
         ("mount_spec", "error_message"),