docker.container: Refactor to support container recreation

This change refactors the way the `docker.container` operation manages
containers, in preparation for work to make recreation more intelligent.
This change is (mostly) a pure refactor; future changes will diff the
current container against the operation parameters to determine if a
container needs to be recreated. This will fix an issue where changing
any of the operation arguments does not result in actual container
changes upon execution.

In this refactor, container parameters are moved to a dedicated class,
which centralizes the `docker container` command-line argument
generation logic and the future diffing logic. The diff function is
roughed in, though it currently reports "no diff" (to match the
operation's current behavior). Since conditional recreation complicates
the operation's logic on which commands to execute, the decisions are
boosted into boolean variables to increase readability.

As a side benefit, supporting additional docker container parameters
should be more straightforward due to the centralization in said
dedicated class (I'm planning on adding support for container args, uid,
and other Docker params currently not supported).

The only behavioral change is that creating and starting a container is
no longer done in one exec (joined by `;`) but rather two separate
docker commands. This sidesteps questions about whether `;` is the
correct joiner (as opposed to `&&`) and reduces the amount of `kwargs`
fishing in the implementation.

Tested:
* `scripts/dev-test.sh` and `scripts/dev-test-e2e.sh` both pass, save
  for warnings also present prior to this change

Author: minor-fixes

pyinfra/operations/docker.py

@@ -8,7 +8,7 @@
 from pyinfra.api import operation
 from pyinfra.facts.docker import DockerContainer, DockerNetwork, DockerVolume
 
-from .util.docker import handle_docker
+from .util.docker import ContainerSpec, handle_docker
 
 
 @operation()
@@ -70,56 +70,60 @@ def container(
         )
     """
 
+    want_spec = ContainerSpec(
+        image,
+        ports or list(),
+        networks or list(),
+        volumes or list(),
+        env_vars or list(),
+        pull_always,
+    )
     existent_container = host.get_fact(DockerContainer, object_id=container)
 
-    if force:
-        if existent_container:
-            yield handle_docker(
-                resource="container",
-                command="remove",
-                container=container,
-            )
+    container_spec_changes = want_spec.diff_from_inspect(existent_container)
 
-    if present:
-        if not existent_container or force:
-            yield handle_docker(
-                resource="container",
-                command="create",
-                container=container,
-                image=image,
-                ports=ports,
-                networks=networks,
-                volumes=volumes,
-                env_vars=env_vars,
-                pull_always=pull_always,
-                present=present,
-                force=force,
-                start=start,
-            )
-
-    if existent_container and start:
-        if existent_container[0]["State"]["Status"] != "running":
-            yield handle_docker(
-                resource="container",
-                command="start",
-                container=container,
-            )
-
-    if existent_container and not start:
-        if existent_container[0]["State"]["Status"] == "running":
-            yield handle_docker(
-                resource="container",
-                command="stop",
-                container=container,
-            )
-
-    if existent_container and not present:
+    is_running = (
+        (existent_container[0]["State"]["Status"] == "running")
+        if existent_container and existent_container[0]
+        else False
+    )
+    recreating = existent_container and (force or container_spec_changes)
+    removing = existent_container and not present
+
+    do_remove = recreating or removing
+    do_create = (present and not existent_container) or recreating
+    do_start = start and (recreating or not is_running)
+    do_stop = not start and not removing and is_running
+
+    if do_remove:
         yield handle_docker(
             resource="container",
             command="remove",
             container=container,
         )
 
+    if do_create:
+        yield handle_docker(
+            resource="container",
+            command="create",
+            container=container,
+            spec=want_spec,
+        )
+
+    if do_start:
+        yield handle_docker(
+            resource="container",
+            command="start",
+            container=container,
+        )
+
+    if do_stop:
+        yield handle_docker(
+            resource="container",
+            command="stop",
+            container=container,
+        )
+
 
 @operation(is_idempotent=False)
 def image(image, present=True):

pyinfra/operations/util/docker.py

@@ -1,38 +1,60 @@
+import dataclasses
+from typing import Any
+
 from pyinfra.api import OperationError
 
 
-def _create_container(**kwargs):
-    command = []
+@dataclasses.dataclass
+class ContainerSpec:
+    image: str = ""
+    ports: list[str] = dataclasses.field(default_factory=list)
+    networks: list[str] = dataclasses.field(default_factory=list)
+    volumes: list[str] = dataclasses.field(default_factory=list)
+    env_vars: list[str] = dataclasses.field(default_factory=list)
+    pull_always: bool = False
+
+    def container_create_args(self):
+        args = []
+        for network in self.networks:
+            args.append("--network {0}".format(network))
 
-    networks = kwargs["networks"] if kwargs["networks"] else []
-    ports = kwargs["ports"] if kwargs["ports"] else []
-    volumes = kwargs["volumes"] if kwargs["volumes"] else []
-    env_vars = kwargs["env_vars"] if kwargs["env_vars"] else []
+        for port in self.ports:
+            args.append("-p {0}".format(port))
 
-    if kwargs["image"] == "":
-        raise OperationError("missing 1 required argument: 'image'")
+        for volume in self.volumes:
+            args.append("-v {0}".format(volume))
 
-    command.append("docker container create --name {0}".format(kwargs["container"]))
+        for env_var in self.env_vars:
+            args.append("-e {0}".format(env_var))
 
-    for network in networks:
-        command.append("--network {0}".format(network))
+        if self.pull_always:
+            args.append("--pull always")
 
-    for port in ports:
-        command.append("-p {0}".format(port))
+        args.append(self.image)
 
-    for volume in volumes:
-        command.append("-v {0}".format(volume))
+        return args
 
-    for env_var in env_vars:
-        command.append("-e {0}".format(env_var))
+    def diff_from_inspect(self, inspect_dict: dict[str, Any]) -> list[str]:
+        # TODO(@minor-fixes): Diff output of "docker inspect" against this spec
+        # to determine if the container needs to be recreated. Currently, this
+        # function will never recreate when attributes change, which is
+        # consistent with prior behavior.
+        del inspect_dict
+        return []
+
+
+def _create_container(**kwargs):
+    if "spec" not in kwargs:
+        raise OperationError("missing 1 required argument: 'spec'")
 
-    if kwargs["pull_always"]:
-        command.append("--pull always")
+    spec = kwargs["spec"]
 
-    command.append(kwargs["image"])
+    if not spec.image:
+        raise OperationError("Docker image not specified")
 
-    if kwargs["start"]:
-        command.append("; {0}".format(_start_container(container=kwargs["container"])))
+    command = [
+        "docker container create --name {0}".format(kwargs["container"])
+    ] + spec.container_create_args()
 
     return " ".join(command)
 

tests/operations/docker.container/add_and_start_no_existent_container.json

@@ -10,10 +10,11 @@
     },
     "facts": {
         "docker.DockerContainer": {
-            "object_id=nginx": [] 
+            "object_id=nginx": []
         }
     },
     "commands": [
-        "docker container create --name nginx -p 80:80 nginx:alpine ; docker container start nginx"
+        "docker container create --name nginx -p 80:80 nginx:alpine",
+        "docker container start nginx"
     ]
 }