Cache image prior to running Docker user mapping checks

beeware · Jul 28, 2023 · d5c2cd5 · d5c2cd5
1 parent 5483fd9
commit d5c2cd5
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 18 deletions.
diff --git a/changes/1368.bugfix.rst b/changes/1368.bugfix.rst
@@ -0,0 +1 @@
+When the target Docker image name is not valid when building Linux System packages, the error message now mentions the invalid image name instead of an error for Docker user mapping.
diff --git a/src/briefcase/integrations/docker.py b/src/briefcase/integrations/docker.py
@@ -271,13 +271,18 @@ def _is_user_mapping_enabled(self, image_tag: str | None = None) -> bool:
             "/host_write_test", host_write_test_path.name
         )
 
+        image_tag = "alpine" if image_tag is None else image_tag
+        # Cache the image first so the attempts below to run the image don't
+        # log irrelevant errors when the image may just have a simple typo
+        self.cache_image(image_tag)
+
         docker_run_cmd = [
             "docker",
             "run",
             "--rm",
             "--volume",
             f"{host_write_test_path.parent}:{container_write_test_path.parent}:z",
-            "alpine" if image_tag is None else image_tag,
+            image_tag,
         ]
 
         host_write_test_path.parent.mkdir(exist_ok=True)
@@ -300,7 +305,6 @@ def _is_user_mapping_enabled(self, image_tag: str | None = None) -> bool:
             self.tools.subprocess.run(
                 docker_run_cmd + ["touch", container_write_test_path],
                 check=True,
-                stream_output=False,
             )
         except subprocess.CalledProcessError as e:
             raise BriefcaseCommandError(
@@ -314,7 +318,6 @@ def _is_user_mapping_enabled(self, image_tag: str | None = None) -> bool:
             self.tools.subprocess.run(
                 docker_run_cmd + ["rm", "-f", container_write_test_path],
                 check=True,
-                stream_output=False,
             )
         except subprocess.CalledProcessError as e:
             raise BriefcaseCommandError(
@@ -342,7 +345,12 @@ def cache_image(self, image_tag: str):
 
         if not image_id:
             try:
-                self.tools.subprocess.run(["docker", "pull", image_tag], check=True)
+                # disable streaming so image download progress bar is shown
+                self.tools.subprocess.run(
+                    ["docker", "pull", image_tag],
+                    check=True,
+                    stream_output=False,
+                )
             except subprocess.CalledProcessError as e:
                 raise BriefcaseCommandError(
                     f"Unable to obtain the Docker image for {image_tag}. "
@@ -366,13 +374,7 @@ def check_output(self, args: list[SubprocessArgT], image_tag: str) -> str:
         # This ensures that "docker.check_output()" behaves as closely to
         # "subprocess.check_output()" as possible.
         return self.tools.subprocess.check_output(
-            [
-                "docker",
-                "run",
-                "--rm",
-                image_tag,
-            ]
-            + args,
+            ["docker", "run", "--rm", image_tag] + args
         )
 
 

diff --git a/tests/integrations/docker/conftest.py b/tests/integrations/docker/conftest.py
@@ -91,7 +91,6 @@ def user_mapping_run_calls(tmp_path, monkeypatch) -> list:
                 PurePosixPath("/host_write_test/mock_write_test"),
             ],
             check=True,
-            stream_output=False,
         ),
         call(
             [
@@ -106,6 +105,5 @@ def user_mapping_run_calls(tmp_path, monkeypatch) -> list:
                 PurePosixPath("/host_write_test/mock_write_test"),
             ],
             check=True,
-            stream_output=False,
         ),
     ]
diff --git a/tests/integrations/docker/test_DockerAppContext__verify.py b/tests/integrations/docker/test_DockerAppContext__verify.py
@@ -50,6 +50,7 @@ def test_success(mock_tools, first_app_config, verify_kwargs):
         "Docker version 19.03.8, build afacb8b\n",
         "docker info return value",
         "github.com/docker/buildx v0.10.2 00ed17d\n",
+        "1ed313b0551f",  # cached image check
     ]
 
     DockerAppContext.verify(mock_tools, first_app_config, **verify_kwargs)
@@ -97,6 +98,7 @@ def test_docker_image_build_fail(mock_tools, first_app_config, verify_kwargs):
         "Docker version 19.03.8, build afacb8b\n",
         "docker info return value",
         "github.com/docker/buildx v0.10.2 00ed17d\n",
+        "1ed313b0551f",  # cached image check
     ]
 
     mock_tools.subprocess.run.side_effect = [

diff --git a/tests/integrations/docker/test_Docker__cache_image.py b/tests/integrations/docker/test_Docker__cache_image.py
@@ -25,13 +25,14 @@ def test_cache_image(mock_tools, user_mapping_run_calls):
     mock_tools.docker.cache_image("ubuntu:jammy")
 
     # Confirms that image is not available
-    mock_tools.subprocess.check_output.assert_called_once_with(
+    mock_tools.subprocess.check_output.assert_called_with(
         ["docker", "images", "-q", "ubuntu:jammy"]
     )
 
     # Image is pulled and cached
     mock_tools.subprocess.run.assert_has_calls(
-        user_mapping_run_calls + [call(["docker", "pull", "ubuntu:jammy"], check=True)]
+        user_mapping_run_calls
+        + [call(["docker", "pull", "ubuntu:jammy"], check=True, stream_output=False)]
     )
 
 
@@ -44,7 +45,7 @@ def test_cache_image_already_cached(mock_tools, user_mapping_run_calls):
     mock_tools.docker.cache_image("ubuntu:jammy")
 
     # Confirms that image is not available
-    mock_tools.subprocess.check_output.assert_called_once_with(
+    mock_tools.subprocess.check_output.assert_called_with(
         ["docker", "images", "-q", "ubuntu:jammy"]
     )
 

diff --git a/tests/integrations/docker/test_Docker__verify.py b/tests/integrations/docker/test_Docker__verify.py
@@ -13,6 +13,7 @@
 VALID_DOCKER_VERSION = "Docker version 19.03.8, build afacb8b\n"
 VALID_DOCKER_INFO = "docker info printout"
 VALID_BUILDX_VERSION = "github.com/docker/buildx v0.10.2 00ed17d\n"
+VALID_USER_MAPPING_IMAGE_CACHE = "1ed313b0551f"
 DOCKER_VERIFICATION_CALLS = [
     call(["docker", "--version"]),
     call(["docker", "info"]),
@@ -70,6 +71,7 @@ def test_docker_exists(mock_tools, user_mapping_run_calls, capsys, tmp_path):
         VALID_DOCKER_VERSION,
         VALID_DOCKER_INFO,
         VALID_BUILDX_VERSION,
+        VALID_USER_MAPPING_IMAGE_CACHE,
     ]
 
     # Invoke docker verify
@@ -114,6 +116,7 @@ def test_docker_failure(mock_tools, user_mapping_run_calls, capsys):
         ),
         "Success!",
         VALID_BUILDX_VERSION,
+        VALID_USER_MAPPING_IMAGE_CACHE,
     ]
 
     # Invoke Docker verify
@@ -280,6 +283,7 @@ def test_docker_image_hint(mock_tools):
         VALID_DOCKER_VERSION,
         VALID_DOCKER_INFO,
         VALID_BUILDX_VERSION,
+        VALID_USER_MAPPING_IMAGE_CACHE,
     ]
 
     Docker.verify(mock_tools, image_tag="myimage:tagtorulethemall")
@@ -298,7 +302,6 @@ def test_docker_image_hint(mock_tools):
                     PurePosixPath("/host_write_test/container_write_test"),
                 ],
                 check=True,
-                stream_output=False,
             ),
             call(
                 [
@@ -313,7 +316,6 @@ def test_docker_image_hint(mock_tools):
                     PurePosixPath("/host_write_test/container_write_test"),
                 ],
                 check=True,
-                stream_output=False,
             ),
         ]
     )
@@ -333,6 +335,7 @@ def test_user_mapping_write_file_exists(mock_tools, mock_write_test_path):
         VALID_DOCKER_VERSION,
         VALID_DOCKER_INFO,
         VALID_BUILDX_VERSION,
+        VALID_USER_MAPPING_IMAGE_CACHE,
     ]
 
     # Mock failure for deleting an existing write test file
@@ -353,6 +356,7 @@ def test_user_mapping_write_test_file_creation_fails(mock_tools, mock_write_test
         VALID_DOCKER_VERSION,
         VALID_DOCKER_INFO,
         VALID_BUILDX_VERSION,
+        VALID_USER_MAPPING_IMAGE_CACHE,
     ]
 
     # Mock failure for deleting an existing write test file
@@ -376,6 +380,7 @@ def test_user_mapping_write_test_file_cleanup_fails(mock_tools, mock_write_test_
         VALID_DOCKER_VERSION,
         VALID_DOCKER_INFO,
         VALID_BUILDX_VERSION,
+        VALID_USER_MAPPING_IMAGE_CACHE,
     ]
 
     # Mock failure for deleting an existing write test file
@@ -406,6 +411,7 @@ def test_user_mapping_setting(
         VALID_DOCKER_VERSION,
         VALID_DOCKER_INFO,
         VALID_BUILDX_VERSION,
+        VALID_USER_MAPPING_IMAGE_CACHE,
     ]
 
     stat_result = namedtuple("stat_result", "st_uid")