Merge pull request #1421 from yuvipanda/docker-login

Switch to using CLI for everything except running the container

.github/workflows/test.yml

@@ -40,6 +40,7 @@ env:
   GIT_AUTHOR_EMAIL: ci-user@github.local
   GIT_AUTHOR_NAME: CI User
 
+
 jobs:
   test:
     # Don't run scheduled tests on forks
@@ -64,6 +65,7 @@ jobs:
           - unit
           - venv
           - contentproviders
+          - norun
           # Playwright test
           - ui
         include:
@@ -74,6 +76,7 @@ jobs:
 
     steps:
       - uses: actions/checkout@v4
+
       - uses: actions/setup-python@v5
         with:
           python-version: "${{ matrix.python_version }}"

dev-requirements.txt

@@ -5,3 +5,4 @@ pytest-cov
 pytest>=7
 pyyaml
 requests_mock
+bcrypt

docs/source/architecture.md

@@ -98,10 +98,8 @@ At the end of the assemble step, the docker image is ready to be used in various
 
 ### Push
 
-Optionally, repo2docker can **push** a built image to a [docker registry](https://docs.docker.com/registry/).
+Optionally, repo2docker can **push** a built image to a [docker registry](https://docs.docker.com/registry/),
-This is done as a convenience only (since you can do the same with a `docker push` after using repo2docker
+if you specify the `--push` flag.
-only to build), and implemented in `Repo2Docker.push` method. It is only activated if using the
-`--push` commandline flag.
 
 ### Run
 

repo2docker/app.py

@@ -37,7 +37,7 @@ from .buildpacks import (
     RBuildPack,
 )
 from .engine import BuildError, ContainerEngineException, ImageLoadError
-from .utils import ByteSpecification, R2dState, chdir, get_platform
+from .utils import ByteSpecification, R2dState, chdir, get_free_port, get_platform
 
 
 class Repo2Docker(Application):
@@ -572,56 +572,6 @@ class Repo2Docker(Application):
         if self.volumes and not self.run:
             raise ValueError("Cannot mount volumes if container is not run")
 
-    def push_image(self):
-        """Push docker image to registry"""
-        client = self.get_engine()
-        # Build a progress setup for each layer, and only emit per-layer
-        # info every 1.5s
-        progress_layers = {}
-        layers = {}
-        last_emit_time = time.time()
-        for chunk in client.push(self.output_image_spec):
-            if client.string_output:
-                self.log.info(chunk, extra=dict(phase=R2dState.PUSHING))
-                continue
-            # else this is Docker output
-
-            # each chunk can be one or more lines of json events
-            # split lines here in case multiple are delivered at once
-            for line in chunk.splitlines():
-                line = line.decode("utf-8", errors="replace")
-                try:
-                    progress = json.loads(line)
-                except Exception as e:
-                    self.log.warning("Not a JSON progress line: %r", line)
-                    continue
-                if "error" in progress:
-                    self.log.error(progress["error"], extra=dict(phase=R2dState.FAILED))
-                    raise ImageLoadError(progress["error"])
-                if "id" not in progress:
-                    continue
-                # deprecated truncated-progress data
-                if "progressDetail" in progress and progress["progressDetail"]:
-                    progress_layers[progress["id"]] = progress["progressDetail"]
-                else:
-                    progress_layers[progress["id"]] = progress["status"]
-                # include full progress data for each layer in 'layers' data
-                layers[progress["id"]] = progress
-                if time.time() - last_emit_time > 1.5:
-                    self.log.info(
-                        "Pushing image\n",
-                        extra=dict(
-                            progress=progress_layers,
-                            layers=layers,
-                            phase=R2dState.PUSHING,
-                        ),
-                    )
-                    last_emit_time = time.time()
-        self.log.info(
-            f"Successfully pushed {self.output_image_spec}",
-            extra=dict(phase=R2dState.PUSHING),
-        )
-
     def run_image(self):
         """Run docker container from built image
 
@@ -660,7 +610,7 @@ class Repo2Docker(Application):
                 container_port = int(container_port_proto.split("/", 1)[0])
             else:
                 # no port specified, pick a random one
-                container_port = host_port = str(self._get_free_port())
+                container_port = host_port = str(get_free_port())
                 self.ports = {f"{container_port}/tcp": host_port}
             self.port = host_port
             # To use the option --NotebookApp.custom_display_url
@@ -744,30 +694,14 @@ class Repo2Docker(Application):
             if exit_code:
                 sys.exit(exit_code)
 
-    def _get_free_port(self):
-        """
-        Hacky method to get a free random port on local host
-        """
-        import socket
-
-        s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
-        s.bind(("", 0))
-        port = s.getsockname()[1]
-        s.close()
-        return port
-
     def find_image(self):
         # if this is a dry run it is Ok for dockerd to be unreachable so we
         # always return False for dry runs.
         if self.dry_run:
             return False
         # check if we already have an image for this content
-        client = self.get_engine()
+        engine = self.get_engine()
-        for image in client.images():
+        return engine.inspect_image(self.output_image_spec) is not None
-            for tag in image.tags:
-                if tag == self.output_image_spec + ":latest":
-                    return True
-        return False
 
     def build(self):
         """
@@ -863,6 +797,12 @@ class Repo2Docker(Application):
                         extra=dict(phase=R2dState.BUILDING),
                     )
 
+                    extra_build_kwargs = self.extra_build_kwargs.copy()
+                    # Set "push" and "load" parameters in a backwards compat way, without
+                    # having to change the signature of every buildpack
+                    extra_build_kwargs["push"] = self.push
+                    extra_build_kwargs["load"] = self.run
+
                     for l in picked_buildpack.build(
                         docker_client,
                         self.output_image_spec,
@@ -870,7 +810,7 @@ class Repo2Docker(Application):
                         self.build_memory_limit,
                         build_args,
                         self.cache_from,
-                        self.extra_build_kwargs,
+                        extra_build_kwargs,
                         platform=self.platform,
                     ):
                         if docker_client.string_output:
@@ -902,8 +842,5 @@ class Repo2Docker(Application):
     def start(self):
         self.build()
 
-        if self.push:
-            self.push_image()
-
         if self.run:
             self.run_image()

repo2docker/buildpacks/docker.py

@@ -3,8 +3,6 @@
 
 import os
 
-import docker
-
 from .base import BuildPack
 
 

repo2docker/docker.py

@@ -2,16 +2,22 @@
 Docker container engine for repo2docker
 """
 
+import json
+import os
 import shutil
+import subprocess
 import tarfile
 import tempfile
+from argparse import ArgumentError
+from contextlib import ExitStack, contextmanager
+from pathlib import Path
 
 from iso8601 import parse_date
 from traitlets import Dict, List, Unicode
 
 import docker
 
-from .engine import Container, ContainerEngine, ContainerEngineException, Image
+from .engine import Container, ContainerEngine, Image
 from .utils import execute_cmd
 
 
@@ -58,7 +64,7 @@ class DockerEngine(ContainerEngine):
     https://docker-py.readthedocs.io/en/4.2.0/api.html#module-docker.api.build
     """
 
-    string_output = False
+    string_output = True
 
     extra_init_args = Dict(
         {},
@@ -82,19 +88,11 @@ class DockerEngine(ContainerEngine):
         config=True,
     )
 
-    def __init__(self, *, parent):
-        super().__init__(parent=parent)
-        try:
-            kwargs = docker.utils.kwargs_from_env()
-            kwargs.update(self.extra_init_args)
-            kwargs.setdefault("version", "auto")
-            self._apiclient = docker.APIClient(**kwargs)
-        except docker.errors.DockerException as e:
-            raise ContainerEngineException("Check if docker is running on the host.", e)
-
     def build(
         self,
         *,
+        push=False,
+        load=False,
         buildargs=None,
         cache_from=None,
         container_limits=None,
@@ -109,7 +107,17 @@ class DockerEngine(ContainerEngine):
     ):
         if not shutil.which("docker"):
             raise RuntimeError("The docker commandline client must be installed")
-        args = ["docker", "buildx", "build", "--progress", "plain", "--load"]
+        args = ["docker", "buildx", "build", "--progress", "plain"]
+        if load:
+            if push:
+                raise ValueError(
+                    "Setting push=True and load=True is currently not supported"
+                )
+            args.append("--load")
+
+        if push:
+            args.append("--push")
+
         if buildargs:
             for k, v in buildargs.items():
                 args += ["--build-arg", f"{k}={v}"]
@@ -134,38 +142,73 @@ class DockerEngine(ContainerEngine):
         # place extra args right *before* the path
         args += self.extra_buildx_build_args
 
-        if fileobj:
+        with ExitStack() as stack:
-            with tempfile.TemporaryDirectory() as d:
+            if self.registry_credentials:
-                tarf = tarfile.open(fileobj=fileobj)
+                stack.enter_context(self.docker_login(**self.registry_credentials))
-                tarf.extractall(d)
+            if fileobj:
+                with tempfile.TemporaryDirectory() as d:
+                    tarf = tarfile.open(fileobj=fileobj)
+                    tarf.extractall(d)
 
-                args += [d]
+                    args += [d]
 
-                for line in execute_cmd(args, True):
+                    yield from execute_cmd(args, True)
-                    # Simulate structured JSON output from buildx build, since we
+            else:
-                    # do get structured json output from pushing and running
+                # Assume 'path' is passed in
-                    yield {"stream": line}
+                args += [path]
-        else:
-            # Assume 'path' is passed in
-            args += [path]
 
-            for line in execute_cmd(args, True):
+                yield from execute_cmd(args, True)
-                # Simulate structured JSON output from buildx build, since we
-                # do get structured json output from pushing and running
-                yield {"stream": line}
-
-    def images(self):
-        images = self._apiclient.images()
-        return [Image(tags=image["RepoTags"]) for image in images]
 
     def inspect_image(self, image):
-        image = self._apiclient.inspect_image(image)
+        """
-        return Image(tags=image["RepoTags"], config=image["Config"])
+        Return image configuration if it exists, otherwise None
+        """
+        proc = subprocess.run(
+            ["docker", "image", "inspect", image], capture_output=True
+        )
 
-    def push(self, image_spec):
+        if proc.returncode != 0:
-        if self.registry_credentials:
+            return None
-            self._apiclient.login(**self.registry_credentials)
+
-        return self._apiclient.push(image_spec, stream=True)
+        config = json.loads(proc.stdout.decode())[0]
+        return Image(tags=config["RepoTags"], config=config["Config"])
+
+    @contextmanager
+    def docker_login(self, username, password, registry):
+        # Determine existing DOCKER_CONFIG
+        old_dc_path = os.environ.get("DOCKER_CONFIG")
+        if old_dc_path is None:
+            dc_path = Path("~/.docker/config.json").expanduser()
+        else:
+            dc_path = Path(old_dc_path)
+
+        with tempfile.TemporaryDirectory() as d:
+            new_dc_path = Path(d) / "config.json"
+            if dc_path.exists():
+                # If there is an existing DOCKER_CONFIG, copy it to new location so we inherit
+                # whatever configuration the user has already set
+                shutil.copy2(dc_path, new_dc_path)
+
+            os.environ["DOCKER_CONFIG"] = d
+            try:
+                subprocess.run(
+                    [
+                        "docker",
+                        "login",
+                        "--username",
+                        username,
+                        "--password-stdin",
+                        registry,
+                    ],
+                    input=password.encode(),
+                    check=True,
+                )
+                yield
+            finally:
+                if old_dc_path:
+                    os.environ["DOCKER_CONFIG"] = old_dc_path
+                else:
+                    del os.environ["DOCKER_CONFIG"]
 
     def run(
         self,

repo2docker/engine.py

@@ -6,7 +6,7 @@ import json
 import os
 from abc import ABC, abstractmethod
 
-from traitlets import Dict, default
+from traitlets import Dict, TraitError, default, validate
 from traitlets.config import LoggingConfigurable
 
 # Based on https://docker-py.readthedocs.io/en/4.2.0/containers.html
@@ -176,6 +176,17 @@ class ContainerEngine(LoggingConfigurable):
                 raise
         return {}
 
+    @validate("registry_credentials")
+    def _registry_credentials_validate(self, proposal):
+        """
+        Validate form of registry credentials
+        """
+        new = proposal["value"]
+        if len({"registry", "username", "password"} & new.keys()) != 3:
+            raise TraitError(
+                "registry_credentials must have keys 'registry', 'username' and 'password'"
+            )
+
     string_output = True
     """
     Whether progress events should be strings or an object.
@@ -202,6 +213,8 @@ class ContainerEngine(LoggingConfigurable):
     def build(
         self,
         *,
+        push=False,
+        load=False,
         buildargs={},
         cache_from=[],
         container_limits={},
@@ -219,6 +232,10 @@ class ContainerEngine(LoggingConfigurable):
 
         Parameters
         ----------
+        push: bool
+            Push the resulting image to a registry
+        load: bool
+            Load the resulting image into the container store ready to be run
         buildargs : dict
             Dictionary of build arguments
         cache_from : list[str]
@@ -254,19 +271,9 @@ class ContainerEngine(LoggingConfigurable):
         """
         raise NotImplementedError("build not implemented")
 
-    def images(self):
-        """
-        List images
-
-        Returns
-        -------
-        list[Image] : List of Image objects.
-        """
-        raise NotImplementedError("images not implemented")
-
     def inspect_image(self, image):
         """
-        Get information about an image
+        Get information about an image, or None if the image does not exist
 
         TODO: This is specific to the engine, can we convert it to a standard format?
 
@@ -281,27 +288,6 @@ class ContainerEngine(LoggingConfigurable):
         """
         raise NotImplementedError("inspect_image not implemented")
 
-    def push(self, image_spec):
-        """
-        Push image to a registry
-
-        If the registry_credentials traitlets is set it should be used to
-        authenticate with the registry before pushing.
-
-        Parameters
-        ----------
-        image_spec : str
-            The repository spec to push to
-
-        Returns
-        -------
-        A generator of strings. If an error occurs an exception must be thrown.
-
-        If `string_output=True` this should instead be whatever Docker returns:
-        https://github.com/jupyter/repo2docker/blob/0.11.0/repo2docker/app.py#L469-L495
-        """
-        raise NotImplementedError("push not implemented")
-
     # Note this is different from the Docker client which has Client.containers.run
     def run(
         self,

repo2docker/utils.py

@@ -1,6 +1,7 @@
 import os
 import platform
 import re
+import socket
 import subprocess
 import warnings
 from contextlib import contextmanager
@@ -545,3 +546,14 @@ def get_platform():
     else:
         warnings.warn(f"Unexpected platform '{m}', defaulting to linux/amd64")
         return "linux/amd64"
+
+
+def get_free_port():
+    """
+    Hacky method to get a free random port on local host
+    """
+    s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+    s.bind(("", 0))
+    port = s.getsockname()[1]
+    s.close()
+    return port

tests/norun/.gitignore

@@ -0,0 +1 @@
+tmp-certs-*

tests/norun/Dockerfile

@@ -0,0 +1,2 @@
+# Smallest possible dockerfile, used only for building images to be tested
+FROM scratch

tests/norun/test_find.py

@@ -0,0 +1,23 @@
+import secrets
+from pathlib import Path
+
+from repo2docker.__main__ import make_r2d
+
+HERE = Path(__file__).parent
+
+
+def test_find_image():
+    image_name = f"{secrets.token_hex(8)}:latest"
+    r2d = make_r2d(["--image", image_name, str(HERE)])
+
+    r2d.build()
+
+    assert r2d.find_image()
+
+
+def test_dont_find_image():
+    image_name = f"{secrets.token_hex(8)}:latest"
+    r2d = make_r2d(["--image", image_name, str(HERE)])
+
+    # Just don't actually start the build, so image won't be found
+    assert not r2d.find_image()

tests/norun/test_registry.py

@@ -0,0 +1,244 @@
+import json
+import os
+import secrets
+import shutil
+import socket
+import subprocess
+import time
+from base64 import b64encode
+from pathlib import Path
+from tempfile import TemporaryDirectory
+
+import bcrypt
+import pytest
+import requests
+
+from repo2docker.__main__ import make_r2d
+from repo2docker.utils import get_free_port
+
+HERE = Path(__file__).parent
+
+
+@pytest.fixture(scope="session")
+def dind(registry):
+    port = get_free_port()
+    registry_host, _, _ = registry
+
+    # docker daemon will generate certs here, that we can then use to connect to it.
+    # put it in current dir than in /tmp because on macos, current dir is likely to
+    # shared with docker VM so it can be mounted, unlike /tmp
+    cert_dir = HERE / f"tmp-certs-{secrets.token_hex(8)}"
+    cert_dir.mkdir()
+
+    dind_image = "docker:dind"
+    subprocess.check_call(["docker", "pull", dind_image])
+
+    cmd = [
+        "docker",
+        "run",
+        "-e",
+        "DOCKER_TLS_CERTDIR=/opt/certs",
+        "--privileged",
+        "--mount",
+        f"type=bind,src={cert_dir},dst=/opt/certs",
+        "-p",
+        f"{port}:2376",
+        dind_image,
+        "--host",
+        "0.0.0.0:2376",
+        "--insecure-registry",
+        registry_host,
+    ]
+    proc = subprocess.Popen(cmd)
+    time.sleep(5)
+
+    try:
+        yield f"tcp://127.0.0.1:{port}", cert_dir
+    finally:
+        try:
+            shutil.rmtree(cert_dir)
+        except PermissionError:
+            # Sometimes this is owned by root in CI. is ok, let's let it go
+            pass
+        proc.terminate()
+        proc.wait()
+
+
+@pytest.fixture(scope="session")
+def host_ip():
+    # Get the IP of the current machine, as we need to use the same IP
+    # for all our docker commands, *and* the dind we run needs to reach it
+    # in the same way.
+    # Thanks to https://stackoverflow.com/a/28950776
+    s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+    s.settimeout(0)
+    try:
+        # doesn't even have to be reachable
+        s.connect(("10.254.254.254", 1))
+        host_ip = s.getsockname()[0]
+    finally:
+        s.close()
+
+    return host_ip
+
+
+@pytest.fixture(scope="session")
+def registry(host_ip):
+    port = get_free_port()
+    username = "user"
+    password = secrets.token_hex(16)
+    bcrypted_pw = bcrypt.hashpw(
+        password.encode("utf-8"), bcrypt.gensalt(rounds=12)
+    ).decode("utf-8")
+
+    # We put our password here, and mount it into the container.
+    # put it in current dir than in /tmp because on macos, current dir is likely to
+    # shared with docker VM so it can be mounted, unlike /tmp
+    htpasswd_dir = HERE / f"tmp-certs-{secrets.token_hex(8)}"
+    htpasswd_dir.mkdir()
+    (htpasswd_dir / "htpasswd.conf").write_text(f"{username}:{bcrypted_pw}")
+
+    # Explicitly pull the image first so it runs on time
+    registry_image = "registry:3.0.0-rc.3"
+    subprocess.check_call(["docker", "pull", registry_image])
+
+    cmd = [
+        "docker",
+        "run",
+        "--rm",
+        "-e",
+        "REGISTRY_AUTH=htpasswd",
+        "-e",
+        "REGISTRY_AUTH_HTPASSWD_REALM=basic",
+        "-e",
+        "REGISTRY_AUTH_HTPASSWD_PATH=/opt/htpasswd/htpasswd.conf",
+        "--mount",
+        f"type=bind,src={htpasswd_dir},dst=/opt/htpasswd",
+        "-p",
+        f"{port}:5000",
+        registry_image,
+    ]
+    proc = subprocess.Popen(cmd)
+    health_url = f"http://{host_ip}:{port}/v2"
+    # Wait for the registry to actually come up
+    for i in range(10):
+        try:
+            resp = requests.get(health_url)
+            if resp.status_code in (401, 200):
+                break
+        except requests.ConnectionError:
+            # The service is not up yet
+            pass
+        time.sleep(i)
+    else:
+        raise TimeoutError("Test registry did not come up in time")
+
+    try:
+        yield f"{host_ip}:{port}", username, password
+    finally:
+        proc.terminate()
+        proc.wait()
+
+
+def test_registry_explicit_creds(registry, dind):
+    """
+    Test that we can push to registry when given explicit credentials
+    """
+    registry_host, username, password = registry
+    image_name = f"{registry_host}/{secrets.token_hex(8)}:latest"
+    r2d = make_r2d(["--image", image_name, "--push", "--no-run", str(HERE)])
+
+    docker_host, cert_dir = dind
+
+    old_environ = os.environ.copy()
+
+    try:
+        os.environ["DOCKER_HOST"] = docker_host
+        os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client")
+        os.environ["DOCKER_TLS_VERIFY"] = "1"
+        os.environ["CONTAINER_ENGINE_REGISTRY_CREDENTIALS"] = json.dumps(
+            {
+                "registry": f"http://{registry_host}",
+                "username": username,
+                "password": password,
+            }
+        )
+        r2d.start()
+
+        # CONTAINER_ENGINE_REGISTRY_CREDENTIALS unfortunately doesn't propagate to docker manifest, so
+        # let's explicitly set up a docker_config here so we can check if the image exists
+        with TemporaryDirectory() as d:
+            (Path(d) / "config.json").write_text(
+                json.dumps(
+                    {
+                        "auths": {
+                            f"http://{registry_host}": {
+                                "auth": b64encode(
+                                    f"{username}:{password}".encode()
+                                ).decode()
+                            }
+                        }
+                    }
+                )
+            )
+            env = os.environ.copy()
+            env["DOCKER_CONFIG"] = d
+            proc = subprocess.run(
+                ["docker", "manifest", "inspect", "--insecure", image_name], env=env
+            )
+            assert proc.returncode == 0
+
+        # Validate that we didn't leak our registry creds into existing docker config
+        docker_config_path = Path(
+            os.environ.get("DOCKER_CONFIG", "~/.docker/config.json")
+        ).expanduser()
+        if docker_config_path.exists():
+            # Just check that our randomly generated password is not in this file
+            # Can this cause a conflict? Sure, if there's a different randomly generated password in here
+            # that matches our own randomly generated password. But if you're that unlucky, take cover from the asteroid.
+            assert password not in docker_config_path.read_text()
+    finally:
+        os.environ.clear()
+        os.environ.update(old_environ)
+
+
+def test_registry_no_explicit_creds(registry, dind):
+    """
+    Test that we can push to registry *without* explicit credentials but reading from a DOCKER_CONFIG
+    """
+    registry_host, username, password = registry
+    image_name = f"{registry_host}/{secrets.token_hex(8)}:latest"
+    r2d = make_r2d(["--image", image_name, "--push", "--no-run", str(HERE)])
+
+    docker_host, cert_dir = dind
+
+    old_environ = os.environ.copy()
+
+    try:
+        os.environ["DOCKER_HOST"] = docker_host
+        os.environ["DOCKER_CERT_PATH"] = str(cert_dir / "client")
+        os.environ["DOCKER_TLS_VERIFY"] = "1"
+        with TemporaryDirectory() as d:
+            (Path(d) / "config.json").write_text(
+                json.dumps(
+                    {
+                        "auths": {
+                            f"http://{registry_host}": {
+                                "auth": b64encode(
+                                    f"{username}:{password}".encode()
+                                ).decode()
+                            }
+                        }
+                    }
+                )
+            )
+            os.environ["DOCKER_CONFIG"] = d
+            r2d.start()
+
+            proc = subprocess.run(
+                ["docker", "manifest", "inspect", "--insecure", image_name]
+            )
+            assert proc.returncode == 0
+    finally:
+        os.environ.clear()
+        os.environ.update(old_environ)

tests/unit/test_app.py

@@ -1,9 +1,7 @@
-import errno
 from tempfile import TemporaryDirectory
 from unittest.mock import patch
 
 import escapism
-import pytest
 
 import docker
 from repo2docker.__main__ import make_r2d
@@ -11,34 +9,6 @@ from repo2docker.app import Repo2Docker
 from repo2docker.utils import chdir
 
 
-def test_find_image():
-    images = [{"RepoTags": ["some-org/some-repo:latest"]}]
-
-    with patch("repo2docker.docker.docker.APIClient") as FakeDockerClient:
-        instance = FakeDockerClient.return_value
-        instance.images.return_value = images
-
-        r2d = Repo2Docker()
-        r2d.output_image_spec = "some-org/some-repo"
-        assert r2d.find_image()
-
-        instance.images.assert_called_with()
-
-
-def test_dont_find_image():
-    images = [{"RepoTags": ["some-org/some-image-name:latest"]}]
-
-    with patch("repo2docker.docker.docker.APIClient") as FakeDockerClient:
-        instance = FakeDockerClient.return_value
-        instance.images.return_value = images
-
-        r2d = Repo2Docker()
-        r2d.output_image_spec = "some-org/some-other-image-name"
-        assert not r2d.find_image()
-
-        instance.images.assert_called_with()
-
-
 def test_image_name_remains_unchanged():
     # if we specify an image name, it should remain unmodified
     with TemporaryDirectory() as src:
@@ -104,25 +74,3 @@ def test_run_kwargs(repo_with_content):
     args, kwargs = containers.run.call_args
     assert "somekey" in kwargs
     assert kwargs["somekey"] == "somevalue"
-
-
-def test_dryrun_works_without_docker(tmpdir, capsys):
-    with chdir(tmpdir):
-        with patch.object(docker, "APIClient") as client:
-            client.side_effect = docker.errors.DockerException("Error: no Docker")
-            app = Repo2Docker(dry_run=True)
-            app.build()
-            captured = capsys.readouterr()
-            assert "Error: no Docker" not in captured.err
-
-
-def test_error_log_without_docker(tmpdir, capsys):
-    with chdir(tmpdir):
-        with patch.object(docker, "APIClient") as client:
-            client.side_effect = docker.errors.DockerException("Error: no Docker")
-            app = Repo2Docker()
-
-            with pytest.raises(SystemExit):
-                app.build()
-                captured = capsys.readouterr()
-                assert "Error: no Docker" in captured.err

tests/unit/test_argumentvalidation.py

@@ -212,34 +212,6 @@ def test_invalid_container_port_protocol_mapping_fail(temp_cwd):
     assert not validate_arguments(builddir, args_list, "Port specification")
 
 
-def test_docker_handle_fail(temp_cwd):
-    """
-    Test to check if r2d fails with minimal error message on not being able to connect to docker daemon
-    """
-    args_list = []
-
-    assert not validate_arguments(
-        builddir,
-        args_list,
-        "Check if docker is running on the host.",
-        disable_dockerd=True,
-    )
-
-
-def test_docker_handle_debug_fail(temp_cwd):
-    """
-    Test to check if r2d fails with helpful error message on not being able to connect to docker daemon and debug enabled
-    """
-    args_list = ["--debug"]
-
-    assert not validate_arguments(
-        builddir,
-        args_list,
-        "Check if docker is running on the host.",
-        disable_dockerd=True,
-    )
-
-
 def test_docker_no_build_success(temp_cwd):
     """
     Test to check if r2d succeeds with --no-build argument with not being able to connect to docker daemon

tests/unit/test_docker.py

@@ -2,9 +2,6 @@
 
 import os
 from subprocess import check_output
-from unittest.mock import Mock, patch
-
-from repo2docker.docker import DockerEngine
 
 repo_root = os.path.abspath(
     os.path.join(os.path.dirname(__file__), os.pardir, os.pardir)
@@ -22,43 +19,3 @@ def test_git_credential_env():
         .strip()
     )
     assert out == credential_env
-
-
-class MockDockerEngine(DockerEngine):
-    def __init__(self, *args, **kwargs):
-        self._apiclient = Mock()
-
-
-def test_docker_push_no_credentials():
-    engine = MockDockerEngine()
-
-    engine.push("image")
-
-    assert len(engine._apiclient.method_calls) == 1
-    engine._apiclient.push.assert_called_once_with("image", stream=True)
-
-
-def test_docker_push_dict_credentials():
-    engine = MockDockerEngine()
-    engine.registry_credentials = {"username": "abc", "password": "def"}
-
-    engine.push("image")
-
-    assert len(engine._apiclient.method_calls) == 2
-    engine._apiclient.login.assert_called_once_with(username="abc", password="def")
-    engine._apiclient.push.assert_called_once_with("image", stream=True)
-
-
-def test_docker_push_env_credentials():
-    engine = MockDockerEngine()
-    with patch.dict(
-        "os.environ",
-        {
-            "CONTAINER_ENGINE_REGISTRY_CREDENTIALS": '{"username": "abc", "password": "def"}'
-        },
-    ):
-        engine.push("image")
-
-    assert len(engine._apiclient.method_calls) == 2
-    engine._apiclient.login.assert_called_once_with(username="abc", password="def")
-    engine._apiclient.push.assert_called_once_with("image", stream=True)

tests/unit/test_engine.py

@@ -0,0 +1,18 @@
+import pytest
+from traitlets import TraitError
+
+from repo2docker.engine import ContainerEngine
+
+
+def test_registry_credentials():
+    e = ContainerEngine(parent=None)
+
+    # This should be fine
+    e.registry_credentials = {
+        "registry": "something",
+        "username": "something",
+        "password": "something",
+    }
+
+    with pytest.raises(TraitError):
+        e.registry_credentials = {"hi": "bye"}

tests/unit/test_ports.py

@@ -11,7 +11,6 @@ from getpass import getuser
 import pytest
 import requests
 
-import docker
 from repo2docker.__main__ import make_r2d
 from repo2docker.app import Repo2Docker
 
@@ -69,10 +68,7 @@ def read_port_mapping_response(
         container.reload()
         if container.status == "running":
             container.kill()
-        try:
             container.remove()
-        except docker.errors.NotFound:
-            pass
 
     request.addfinalizer(_cleanup)