It's no longer necessary to pass a docker client instance

Fixes saltstack#111

Signed-off-by: Pedro Algarvio <palgarvio@vmware.com>

changelog/111.improvement.rst

@@ -0,0 +1 @@
+It's no longer necessary to pass a docker client instance as ``docker_client`` when using containers.

src/saltfactories/daemons/container.py

@@ -8,6 +8,7 @@
 import logging
 import os
 
+import _pytest._version
 import attr
 import pytest
 from pytestshellutils.exceptions import FactoryNotStarted
@@ -63,6 +64,8 @@ class PyWinTypesError(Exception):
 
 log = logging.getLogger(__name__)
 
+PYTEST_GE_7 = getattr(_pytest._version, "version_tuple", (-1, -1)) >= (7, 0)
+
 
 @attr.s(kw_only=True)
 class Container(BaseFactory):
@@ -71,16 +74,17 @@ class Container(BaseFactory):
     """
 
     image = attr.ib()
-    name = attr.ib(default=None)
+    name = attr.ib()
     display_name = attr.ib(default=None)
     check_ports = attr.ib(default=None)
-    docker_client = attr.ib(repr=False, default=None)
     container_run_kwargs = attr.ib(repr=False, default=attr.Factory(dict))
     container = attr.ib(init=False, default=None, repr=False)
     start_timeout = attr.ib(repr=False, default=30)
     max_start_attempts = attr.ib(repr=False, default=3)
     pull_before_start = attr.ib(repr=False, default=True)
     skip_on_pull_failure = attr.ib(repr=False, default=False)
+    skip_if_docker_client_not_connectable = attr.ib(repr=False, default=False)
+    docker_client = attr.ib(repr=False)
     _before_start_callbacks = attr.ib(repr=False, hash=False, default=attr.Factory(list))
     _before_terminate_callbacks = attr.ib(repr=False, hash=False, default=attr.Factory(list))
     _after_start_callbacks = attr.ib(repr=False, hash=False, default=attr.Factory(list))
@@ -91,17 +95,43 @@ def __attrs_post_init__(self):
         """
         Post attrs initialization routines.
         """
-        if self.name is None:
-            self.name = random_string("factories-")
-        if self.docker_client is None:
-            if not HAS_DOCKER:
-                raise RuntimeError("The docker python library was not found installed")
-            if not HAS_REQUESTS:
-                raise RuntimeError("The requests python library was not found installed")
-            self.docker_client = docker.from_env()
+        # Check that the docker client is connectable before starting
+        self.before_start(self._check_for_connectable_docker_client)
         if self.pull_before_start:
             self.before_start(self._pull_container)
 
+    @name.default
+    def _default_name(self):
+        return random_string("factories-")
+
+    @docker_client.default
+    def _default_docker_client(self):
+        exc_kwargs = {}
+        if PYTEST_GE_7:
+            exc_kwargs["_use_item_location"] = True
+        if not HAS_DOCKER:
+            message = "The docker python library was not found installed"
+            if self.skip_if_docker_client_not_connectable:
+                raise pytest.skip.Exception(message, **exc_kwargs)
+            else:
+                pytest.fail(message)
+        if not HAS_REQUESTS:
+            message = "The requests python library was not found installed"
+            if self.skip_if_docker_client_not_connectable:
+                raise pytest.skip.Exception(message, **exc_kwargs)
+            else:
+                pytest.fail(message)
+        try:
+            docker_client = docker.from_env()
+        except APIError as exc:
+            message = "Failed to instantiate the docker client: {}".format(exc)
+            if self.skip_if_docker_client_not_connectable:
+                raise pytest.skip.Exception(message, **exc_kwargs) from exc
+            else:
+                pytest.fail(message)
+        else:
+            return docker_client
+
     def before_start(self, callback, *args, **kwargs):
         """
         Register a function callback to run before the container starts.
@@ -169,10 +199,6 @@ def start(self, *command, max_start_attempts=None, start_timeout=None):
         if self.is_running():
             log.warning("%s is already running.", self)
             return True
-        connectable = Container.client_connectable(self.docker_client)
-        if connectable is not True:
-            self.terminate()
-            raise RuntimeError(connectable)
         self._terminate_result = None
         atexit.register(self.terminate)
         factory_started = False
@@ -205,7 +231,7 @@ def start(self, *command, max_start_attempts=None, start_timeout=None):
                 detach=True,
                 stdin_open=True,
                 command=list(command) or None,
-                **self.container_run_kwargs
+                **self.container_run_kwargs,
             )
             while time.time() <= start_running_timeout:
                 # Don't know why, but if self.container wasn't previously in a running
@@ -431,6 +457,17 @@ def run_container_start_checks(self, started_at, timeout_at):
     def _container_start_checks(self):
         return True
 
+    def _check_for_connectable_docker_client(self):
+        connectable = Container.client_connectable(self.docker_client)
+        if connectable is not True:
+            if self.skip_if_docker_client_not_connectable:
+                exc_kwargs = {}
+                if PYTEST_GE_7:
+                    exc_kwargs["_use_item_location"] = True
+                raise pytest.skip.Exception(connectable, **exc_kwargs)
+            else:
+                pytest.fail(connectable)
+
     def _pull_container(self):
         connectable = Container.client_connectable(self.docker_client)
         if connectable is not True:
@@ -440,12 +477,16 @@ def _pull_container(self):
             self.docker_client.images.pull(self.image)
         except APIError as exc:
             if self.skip_on_pull_failure:
-                pytest.skip(
+                exc_kwargs = {}
+                if PYTEST_GE_7:
+                    exc_kwargs["_use_item_location"] = True
+                raise pytest.skip.Exception(
                     "Failed to pull docker image '{}': {}".format(
                         self.image,
                         exc,
-                    )
-                )
+                    ),
+                    **exc_kwargs,
+                ) from exc
             raise
 
     def __enter__(self):
@@ -529,7 +570,7 @@ def start(self, *extra_cli_arguments, max_start_attempts=None, start_timeout=Non
             self,
             *extra_cli_arguments,
             max_start_attempts=max_start_attempts,
-            start_timeout=start_timeout
+            start_timeout=start_timeout,
         )
         return self.daemon_started
 
@@ -647,7 +688,7 @@ def started(self, *extra_cli_arguments, max_start_attempts=None, start_timeout=N
             self,
             *extra_cli_arguments,
             max_start_attempts=max_start_attempts,
-            start_timeout=start_timeout
+            start_timeout=start_timeout,
         )
 
     def get_check_events(self):

src/saltfactories/manager.py

@@ -581,7 +581,6 @@ def get_container(
         self,
         container_name,
         image_name,
-        docker_client=None,
         display_name=None,
         factory_class=daemons.container.Container,
         max_start_attempts=3,
@@ -596,8 +595,6 @@ def get_container(
                 The name to give the container
             image_name(str):
                 The image to use
-            docker_client:
-                An instance of the docker client to use
             display_name(str):
                 Human readable name for the factory
             factory_class:
@@ -617,7 +614,6 @@ def get_container(
         return factory_class(
             name=container_name,
             image=image_name,
-            docker_client=docker_client,
             display_name=display_name or container_name,
             environ=self.environ,
             cwd=self.cwd,

tests/functional/factories/daemons/test_container_factory.py

@@ -0,0 +1,83 @@
+from unittest import mock
+
+import pytest
+
+
+@pytest.mark.parametrize("skip_on_pull_failure", [True, False])
+def test_skip_on_pull_failure(pytester, skip_on_pull_failure):
+    pytester.makepyfile(
+        """
+        import pytest
+
+        @pytest.fixture
+        def container(salt_factories):
+            factory = salt_factories.get_container(
+                "skip_on_pull_failure-{0}",
+                "non-existing/docker-container",
+                pull_before_start=True,
+                skip_on_pull_failure={0}
+            )
+            with factory.started() as factory:
+                yield factory
+
+        def test_container(container):
+            assert container.is_running()
+        """.format(
+            skip_on_pull_failure
+        )
+    )
+    res = pytester.runpytest()
+    if skip_on_pull_failure:
+        res.assert_outcomes(skipped=1)
+    else:
+        res.assert_outcomes(errors=1)
+
+
+@pytest.mark.parametrize("skip_if_docker_client_not_connectable", [True, False])
+def test_skip_if_docker_client_not_connectable(
+    pytester, subtests, skip_if_docker_client_not_connectable
+):
+    pytester.makepyfile(
+        """
+        import pytest
+
+        @pytest.fixture
+        def container(salt_factories):
+            factory = salt_factories.get_container(
+                "skip_if_docker_client_not_connectable-{0}",
+                "non-existing/docker-container",
+                skip_if_docker_client_not_connectable={0}
+            )
+            with factory.started() as factory:
+                yield factory
+
+        def test_container(container):
+            assert container.is_running()
+        """.format(
+            skip_if_docker_client_not_connectable
+        )
+    )
+    with subtests.test("dockerpy not installed"):
+        with mock.patch("saltfactories.daemons.container.HAS_DOCKER", False):
+            res = pytester.runpytest_inprocess()
+            if skip_if_docker_client_not_connectable:
+                res.assert_outcomes(skipped=1)
+            else:
+                res.assert_outcomes(errors=1)
+    with subtests.test("requests not installed"):
+        with mock.patch("saltfactories.daemons.container.HAS_REQUESTS", False):
+            res = pytester.runpytest_inprocess()
+            if skip_if_docker_client_not_connectable:
+                res.assert_outcomes(skipped=1)
+            else:
+                res.assert_outcomes(errors=1)
+    with subtests.test("Container.client_connectable() is False"):
+        with mock.patch(
+            "saltfactories.daemons.container.Container.client_connectable",
+            return_value="Not Connectable!",
+        ):
+            res = pytester.runpytest_inprocess()
+            if skip_if_docker_client_not_connectable:
+                res.assert_outcomes(skipped=1)
+            else:
+                res.assert_outcomes(errors=1)