The Container._pull_container callback now properly registers on …

…the ``SaltMinion`` and the ``SaltMaster`` classes when ``pull_before_start`` is True

Fixes #168

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

changelog/168.bugfix.rst

@@ -0,0 +1 @@
+The ``Container._pull_container`` callback now properly registers on the ``SaltMinion`` and the ``SaltMaster`` classes when ``pull_before_start`` is True

pyproject.toml

@@ -176,6 +176,7 @@ ignore = [
   "PTH",      # Convert to pathlib.Path
   "ARG001",   # Unused function argument
   "D100",     # Missing docstring in public module
+  "D102",     # Missing docstring in public method
   "D103",     # Missing docstring in public function
   "D104",     # Missing docstring in public package
   "DTZ003",   # The use of `datetime.datetime.utcnow()` is not allowed, use `datetime.datetime.now(tz=)` instead

src/saltfactories/daemons/container.py

@@ -755,6 +755,7 @@ class SaltDaemon(Container, bases.SaltDaemon):
 
     _daemon_started = attr.ib(init=False, repr=False, default=False)
     _daemon_starting = attr.ib(init=False, repr=False, default=False)
+    _in_container_post_init = attr.ib(init=False, repr=False, default=False)
 
     def __attrs_post_init__(self):
         """
@@ -764,7 +765,9 @@ def __attrs_post_init__(self):
         # and not the python_executable set by salt-factories
         self.python_executable = "python"
         bases.SaltDaemon.__attrs_post_init__(self)
+        self._in_container_post_init = True
         Container.__attrs_post_init__(self)
+        self._in_container_post_init = False
         # There are some volumes which NEED to exist on the container so
         # that configs are in the right place and also our custom salt
         # plugins along with the custom scripts to start the daemons.
@@ -870,7 +873,7 @@ def before_start(
         :keyword kwargs:
             The keyword arguments to pass to the callback
         """
-        if on_container:
+        if self._in_container_post_init or on_container:
             Container.before_start(self, callback, *args, **kwargs)
         else:
             bases.SaltDaemon.before_start(self, callback, *args, **kwargs)
@@ -890,7 +893,7 @@ def after_start(
         :keyword kwargs:
             The keyword arguments to pass to the callback
         """
-        if on_container:
+        if self._in_container_post_init or on_container:
             Container.after_start(self, callback, *args, **kwargs)
         else:
             bases.SaltDaemon.after_start(self, callback, *args, **kwargs)
@@ -910,7 +913,7 @@ def before_terminate(
         :keyword kwargs:
             The keyword arguments to pass to the callback
         """
-        if on_container:
+        if self._in_container_post_init or on_container:
             Container.before_terminate(self, callback, *args, **kwargs)
         else:
             bases.SaltDaemon.before_terminate(self, callback, *args, **kwargs)
@@ -930,7 +933,7 @@ def after_terminate(
         :keyword kwargs:
             The keyword arguments to pass to the callback
         """
-        if on_container:
+        if self._in_container_post_init or on_container:
             Container.after_terminate(self, callback, *args, **kwargs)
         else:
             bases.SaltDaemon.after_terminate(self, callback, *args, **kwargs)

tests/integration/factories/daemons/container/conftest.py

@@ -1,3 +1,4 @@
+import attr
 import pytest
 
 from saltfactories.daemons.container import Container
@@ -56,3 +57,34 @@ def salt_factories_config(salt_factories_config, host_docker_network_ip_address)
     config = salt_factories_config.copy()
     config["log_server_host"] = host_docker_network_ip_address
     return config
+
+
+@attr.s(slots=True)
+class ContainerCallbacksCounter:
+    """
+    A callbacks counter.
+    """
+
+    before_start_count = attr.ib(init=False, default=0)
+    after_start_count = attr.ib(init=False, default=0)
+    before_terminate_count = attr.ib(init=False, default=0)
+    after_terminate_count = attr.ib(init=False, default=0)
+
+    def before_start(self):
+        self.before_start_count += 1
+
+    def after_start(self):
+        self.after_start_count += 1
+
+    def before_terminate(self):
+        self.before_terminate_count += 1
+
+    def after_terminate(self):
+        self.after_terminate_count += 1
+
+
+@attr.s(slots=True)
+class SaltCallbacksCounter(ContainerCallbacksCounter):
+    """
+    The same, thing, a different class name.
+    """

tests/integration/factories/daemons/container/test_container_factory.py

@@ -5,6 +5,8 @@
 from pytestshellutils.utils import ports
 from pytestshellutils.utils import socket
 
+from .conftest import ContainerCallbacksCounter
+
 pytestmark = [
     pytest.mark.skip_on_darwin,
     pytest.mark.skip_on_windows,
@@ -130,3 +132,73 @@ def test_non_connectable_check_ports(salt_factories, docker_client, caplog):
         with pytest.raises(FactoryNotStarted):
             container.start()
     assert "Remaining ports to check: {12345: 12345}" in caplog.text
+
+
+def test_pull_before_start(salt_factories, docker_client):
+    container = salt_factories.get_container(
+        "echo-server-cbs",
+        "cjimti/go-echo",
+        docker_client=docker_client,
+        container_run_kwargs={
+            "ports": {"5000/tcp": None},
+            "environment": {"TCP_PORT": "5000", "NODE_NAME": "echo-server-cbs"},
+        },
+        pull_before_start=False,
+    )
+    assert len(container._before_start_callbacks) == 1
+    for cb in container._before_start_callbacks:
+        if cb.func == container._pull_container:
+            pytest.fail("Found the container._pull_container callback")
+            break
+
+    container = salt_factories.get_container(
+        "echo-server-cbs",
+        "cjimti/go-echo",
+        docker_client=docker_client,
+        container_run_kwargs={
+            "ports": {"5000/tcp": None},
+            "environment": {"TCP_PORT": "5000", "NODE_NAME": "echo-server-cbs"},
+        },
+        pull_before_start=True,
+    )
+    assert len(container._before_start_callbacks) == 2
+    for cb in container._before_start_callbacks:
+        if cb.func == container._pull_container:
+            break
+    else:
+        pytest.fail("Failed to find the container._pull_container callback")
+
+
+def test_callbacks(salt_factories, docker_client):
+    counter = ContainerCallbacksCounter()
+
+    container = salt_factories.get_container(
+        "echo-server-cbs",
+        "cjimti/go-echo",
+        docker_client=docker_client,
+        container_run_kwargs={
+            "ports": {"5000/tcp": None},
+            "environment": {"TCP_PORT": "5000", "NODE_NAME": "echo-server-cbs"},
+        },
+    )
+
+    container.before_start(counter.before_start)
+    container.after_start(counter.after_start)
+    container.before_terminate(counter.before_terminate)
+    container.after_terminate(counter.after_terminate)
+
+    assert counter.before_start_count == 0
+    assert counter.after_start_count == 0
+    assert counter.before_terminate_count == 0
+    assert counter.after_terminate_count == 0
+
+    with container.started():
+        assert counter.before_start_count == 1
+        assert counter.after_start_count == 1
+        assert counter.before_terminate_count == 0
+        assert counter.after_terminate_count == 0
+
+    assert counter.before_start_count == 1
+    assert counter.after_start_count == 1
+    assert counter.before_terminate_count == 1
+    assert counter.after_terminate_count == 1

tests/integration/factories/daemons/container/test_minion.py

@@ -5,6 +5,9 @@
 from saltfactories.daemons.container import SaltMinion
 from saltfactories.utils import random_string
 
+from .conftest import ContainerCallbacksCounter
+from .conftest import SaltCallbacksCounter
+
 docker = pytest.importorskip("docker")
 
 log = logging.getLogger(__name__)
@@ -92,3 +95,79 @@ def test_minion(salt_minion, salt_cli):
     ret = salt_cli.run("test.ping", minion_tgt=salt_minion.id)
     assert ret.returncode == 0, ret
     assert ret.data is True
+
+
+def test_callbacks(salt_master, docker_client, host_docker_network_ip_address):
+    minion_id = random_string("salt-cb-counter-", uppercase=False)
+    salt_counter = SaltCallbacksCounter()
+    container_counter = ContainerCallbacksCounter()
+
+    config_overrides = {
+        "master": salt_master.config["interface"],
+        "user": False,
+        "pytest-minion": {
+            "log": {"host": host_docker_network_ip_address},
+            "returner_address": {"host": host_docker_network_ip_address},
+        },
+        # We also want to scrutinize the key acceptance
+        "open_mode": False,
+    }
+    container = salt_master.salt_minion_daemon(
+        minion_id,
+        overrides=config_overrides,
+        factory_class=SaltMinion,
+        extra_cli_arguments_after_first_start_failure=["--log-level=debug"],
+        # SaltMinion kwargs
+        name=minion_id,
+        image="ghcr.io/saltstack/salt-ci-containers/salt:3005",
+        docker_client=docker_client,
+        start_timeout=120,
+        pull_before_start=True,
+    )
+
+    for cb in container._before_start_callbacks:
+        if cb.func == container._pull_container:
+            break
+    else:
+        pytest.fail("Failed to find the container._pull_container callback")
+
+    container.before_start(container_counter.before_start, on_container=True)
+    container.after_start(container_counter.after_start, on_container=True)
+    container.before_terminate(container_counter.before_terminate, on_container=True)
+    container.after_terminate(container_counter.after_terminate, on_container=True)
+
+    container.before_start(salt_counter.before_start)
+    container.after_start(salt_counter.after_start)
+    container.before_terminate(salt_counter.before_terminate)
+    container.after_terminate(salt_counter.after_terminate)
+
+    assert container_counter.before_start_count == 0
+    assert container_counter.after_start_count == 0
+    assert container_counter.before_terminate_count == 0
+    assert container_counter.after_terminate_count == 0
+
+    assert salt_counter.before_start_count == 0
+    assert salt_counter.after_start_count == 0
+    assert salt_counter.before_terminate_count == 0
+    assert salt_counter.after_terminate_count == 0
+
+    with container.started():
+        assert container_counter.before_start_count == 1
+        assert container_counter.after_start_count == 1
+        assert container_counter.before_terminate_count == 0
+        assert container_counter.after_terminate_count == 0
+
+        assert salt_counter.before_start_count == 1
+        assert salt_counter.after_start_count == 1
+        assert salt_counter.before_terminate_count == 0
+        assert salt_counter.after_terminate_count == 0
+
+    assert container_counter.before_start_count == 1
+    assert container_counter.after_start_count == 1
+    assert container_counter.before_terminate_count == 1
+    assert container_counter.after_terminate_count == 1
+
+    assert salt_counter.before_start_count == 1
+    assert salt_counter.after_start_count == 1
+    assert salt_counter.before_terminate_count == 1
+    assert salt_counter.after_terminate_count == 1