docker.container: Refactor to support container recreation
#1272
Conversation
| "DNSNames": "None" | ||
| } | ||
| } | ||
| "docker.DockerContainer": { |
There was a problem hiding this comment.
Apologies for the diff here; the diffs in this fact object should be purely whitespace (I think formatting this JSON does help readability, so I'd prefer to leave it in if possible)
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
minor-fixes
force-pushed
the
docker_container_spec
branch
from
e96323e to
e360626
Compare
|
Just some food for thought: Would supporting
|
Yes, I think that'd make a ton more sense - otherwise the routines here will essentially replicate the functionality of |
Not that I'm aware of 🙂 |
This change refactors the way the
docker.containeroperation 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 containercommand-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 ofkwargsfishing in the implementation.Tested:
scripts/dev-test.shandscripts/dev-test-e2e.shboth pass, save for warnings also present prior to this change