Merge pull request #79 from forcedotcom/jo_win_docker

fix windows docker zip/deploy bug

Author: joroscoSF

src/datacustomcode/cmd.py

@@ -85,9 +85,13 @@ def _cmd_output(
     **kwargs: Any,
 ) -> tuple[int, bytes, Union[bytes, None]]:
     _setdefault_kwargs(kwargs)
+    kwargs.setdefault("shell", True)
+    # On Windows, Popen with shell=True and a sequence uses list2cmdline() which
+    # quotes the entire string, causing cmd.exe to fail. Joining to a plain string
+    # works correctly on both Unix (/bin/sh -c "...") and Windows (cmd.exe /c ...).
+    cmd_arg: Union[tuple[str, ...], str] = " ".join(cmd) if kwargs.get("shell") else cmd
     try:
-        kwargs.setdefault("shell", True)
-        proc = subprocess.Popen(cmd, **kwargs)
+        proc = subprocess.Popen(cmd_arg, **kwargs)
     except OSError as e:
         returncode, stdout_b, stderr_b = _oserror_to_output(e)
     else:

src/datacustomcode/deploy.py

@@ -272,7 +272,7 @@ def create_deployment(
         raise
 
 
-PLATFORM_ENV_VAR = "DOCKER_DEFAULT_PLATFORM=linux/amd64"
+PLATFORM_ENV = {"DOCKER_DEFAULT_PLATFORM": "linux/amd64"}
 DOCKER_IMAGE_NAME = "datacloud-custom-code-dependency-builder"
 DEPENDENCIES_ARCHIVE_NAME = "native_dependencies"
 DEPENDENCIES_ARCHIVE_FULL_NAME = f"{DEPENDENCIES_ARCHIVE_NAME}.tar.gz"
@@ -286,19 +286,25 @@ def prepare_dependency_archive(directory: str, docker_network: str) -> None:
     cmd = f"docker images -q {DOCKER_IMAGE_NAME}"
     image_exists = cmd_output(cmd)
 
+    docker_env = {**os.environ, **PLATFORM_ENV}
+
     if not image_exists:
         logger.info(f"Building docker image with docker network: {docker_network}...")
         cmd = docker_build_cmd(docker_network)
-        cmd_output(cmd)
+        cmd_output(cmd, env=docker_env)
 
-    with tempfile.TemporaryDirectory() as temp_dir:
+    # ignore_cleanup_errors=True: on Windows, Docker creates files inside the
+    # mounted volume whose permissions prevent the host from deleting them.
+    # The archive has already been copied out, so silently skipping leftover
+    # files is safe and avoids a fatal error on context-manager exit.
+    with tempfile.TemporaryDirectory(ignore_cleanup_errors=True) as temp_dir:
         logger.info(
             f"Building dependencies archive with docker network: {docker_network}"
         )
         shutil.copy("requirements.txt", temp_dir)
         shutil.copy("build_native_dependencies.sh", temp_dir)
         cmd = docker_run_cmd(docker_network, temp_dir)
-        cmd_output(cmd)
+        cmd_output(cmd, env=docker_env)
         archives_temp_path = os.path.join(temp_dir, DEPENDENCIES_ARCHIVE_FULL_NAME)
         os.makedirs(os.path.dirname(DEPENDENCIES_ARCHIVE_PATH), exist_ok=True)
         shutil.copy(archives_temp_path, DEPENDENCIES_ARCHIVE_PATH)
@@ -307,23 +313,19 @@ def prepare_dependency_archive(directory: str, docker_network: str) -> None:
 
 
 def docker_build_cmd(network: str) -> str:
-    cmd = (
-        f"{PLATFORM_ENV_VAR} docker build -t {DOCKER_IMAGE_NAME} "
-        f"--file Dockerfile.dependencies . "
-    )
+    cmd = f"docker build -t {DOCKER_IMAGE_NAME} --file Dockerfile.dependencies . "
 
     if network != "default":
         cmd = cmd + f"--network {network}"
     logger.debug(f"Docker build command: {cmd}")
     return cmd
 
 
-def docker_run_cmd(network: str, temp_dir) -> str:
-    cmd = (
-        f"{PLATFORM_ENV_VAR} docker run --rm "
-        f"-v {temp_dir}:/workspace "
-        f"{DOCKER_IMAGE_NAME} "
-    )
+def docker_run_cmd(network: str, temp_dir: str) -> str:
+    # Normalise path separators: Docker expects forward slashes even on Windows,
+    # and quoting handles paths that contain spaces.
+    docker_path = temp_dir.replace("\\", "/")
+    cmd = f'docker run --rm -v "{docker_path}:/workspace" {DOCKER_IMAGE_NAME} '
 
     if network != "default":
         cmd = cmd + f"--network {network} "

tests/test_cmd.py

@@ -168,3 +168,40 @@ def test_cmd_output_with_custom_kwargs(self, mock_cmd_output):
         _, kwargs = mock_cmd_output.call_args
         assert kwargs["env"] == {"PATH": "/usr/bin"}
         assert kwargs["cwd"] == "/tmp"
+
+    @patch("subprocess.Popen")
+    def test_cmd_output_shell_passes_string_not_tuple(self, mock_popen):
+        """When shell=True, Popen must receive a plain string, not a tuple.
+
+        On Windows, Popen(tuple, shell=True) calls list2cmdline() which quotes
+        the entire string (e.g. '"docker images -q foo"'), causing cmd.exe to
+        fail with 'not recognized as an internal or external command'. Passing
+        a joined string avoids this on both Windows and Unix.
+        """
+        process_mock = Mock()
+        process_mock.communicate.return_value = (b"", b"")
+        process_mock.returncode = 0
+        mock_popen.return_value = process_mock
+
+        _cmd_output("docker images -q my-image")
+
+        args, _ = mock_popen.call_args
+        cmd_arg = args[0]
+        assert isinstance(cmd_arg, str), (
+            f"Expected str but got {type(cmd_arg)}: {cmd_arg!r}. "
+            "Passing a tuple with shell=True causes Windows quoting issues."
+        )
+        assert cmd_arg == "docker images -q my-image"
+
+    @patch("subprocess.Popen")
+    def test_cmd_output_multi_arg_shell_joins_to_string(self, mock_popen):
+        """Multiple positional args are joined into one string for shell=True."""
+        process_mock = Mock()
+        process_mock.communicate.return_value = (b"", b"")
+        process_mock.returncode = 0
+        mock_popen.return_value = process_mock
+
+        _cmd_output("docker", "images", "-q")
+
+        args, _ = mock_popen.call_args
+        assert args[0] == "docker images -q"

tests/test_deploy.py

@@ -2,6 +2,7 @@
 
 import json
 from unittest.mock import (
+    ANY,
     MagicMock,
     mock_open,
     patch,
@@ -49,12 +50,12 @@ class TestPrepareDependencyArchive:
         "docker images -q datacloud-custom-code-dependency-builder"
     )
     EXPECTED_BUILD_CMD = (
-        "DOCKER_DEFAULT_PLATFORM=linux/amd64 docker build "
-        "-t datacloud-custom-code-dependency-builder -f Dockerfile.dependencies . "
+        "docker build "
+        "-t datacloud-custom-code-dependency-builder --file Dockerfile.dependencies . "
     )
     EXPECTED_DOCKER_RUN_CMD = (
-        "DOCKER_DEFAULT_PLATFORM=linux/amd64 docker run --rm "
-        "-v /tmp/test_dir:/workspace "
+        "docker run --rm "
+        '-v "/tmp/test_dir:/workspace" '
         "datacloud-custom-code-dependency-builder "
     )
 
@@ -106,7 +107,7 @@ def test_prepare_dependency_archive_image_exists(
 
         # Verify docker run command was called
         mock_docker_run_cmd.assert_called_once_with("default", "/tmp/test_dir")
-        mock_cmd_output.assert_any_call("mock run command")
+        mock_cmd_output.assert_any_call("mock run command", env=ANY)
 
         # Verify archives directory was created
         mock_makedirs.assert_called_once_with("payload/archives", exist_ok=True)
@@ -159,15 +160,15 @@ def test_prepare_dependency_archive_build_image(
 
         # Verify docker build command was called
         mock_docker_build_cmd.assert_called_once_with("default")
-        mock_cmd_output.assert_any_call("mock build command")
+        mock_cmd_output.assert_any_call("mock build command", env=ANY)
 
         # Verify files were copied to temp directory
         mock_copy.assert_any_call("requirements.txt", "/tmp/test_dir")
         mock_copy.assert_any_call("build_native_dependencies.sh", "/tmp/test_dir")
 
         # Verify docker run command was called
         mock_docker_run_cmd.assert_called_once_with("default", "/tmp/test_dir")
-        mock_cmd_output.assert_any_call("mock run command")
+        mock_cmd_output.assert_any_call("mock run command", env=ANY)
 
         # Verify archives directory was created
         mock_makedirs.assert_called_once_with("payload/archives", exist_ok=True)