diff --git a/changelog/111.improvement.rst b/changelog/111.improvement.rst new file mode 100644 index 00000000..77773b17 --- /dev/null +++ b/changelog/111.improvement.rst @@ -0,0 +1 @@ +It's no longer necessary to pass a docker client instance as ``docker_client`` when using containers. diff --git a/src/saltfactories/daemons/container.py b/src/saltfactories/daemons/container.py index bc41f628..ac5ea50f 100644 --- a/src/saltfactories/daemons/container.py +++ b/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): diff --git a/src/saltfactories/manager.py b/src/saltfactories/manager.py index 82335f5a..887fc529 100644 --- a/src/saltfactories/manager.py +++ b/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, diff --git a/tests/functional/factories/daemons/test_container_factory.py b/tests/functional/factories/daemons/test_container_factory.py new file mode 100644 index 00000000..1642d57c --- /dev/null +++ b/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) diff --git a/tests/unit/factories/daemons/test_container.py b/tests/unit/factories/daemons/test_container.py index cf957ceb..588fcee4 100644 --- a/tests/unit/factories/daemons/test_container.py +++ b/tests/unit/factories/daemons/test_container.py @@ -10,7 +10,7 @@ def test_missing_docker_library(): "saltfactories.daemons.container.HAS_DOCKER", new_callable=mock.PropertyMock(return_value=False), ): - with pytest.raises(RuntimeError) as exc: + with pytest.raises(pytest.fail.Exception) as exc: Container(name="foo", image="bar") assert str(exc.value) == "The docker python library was not found installed" @@ -24,7 +24,7 @@ def test_missing_requests_library(): "saltfactories.daemons.container.HAS_REQUESTS", new_callable=mock.PropertyMock(return_value=False), ): - with pytest.raises(RuntimeError) as exc: + with pytest.raises(pytest.fail.Exception) as exc: Container(name="foo", image="bar") assert str(exc.value) == "The requests python library was not found installed"