fix(cli): move docker check off the --dry path#126
Conversation
Add test_dry_run_succeeds_without_docker to test_cli_create_dev_smoke.py: monkeypatch check_docker_available (as bound in cli_main) to False and assert `create --dry` exits 0 without the "Docker is not available" message. Confirmed against the current implementation (with a fake docker on PATH to clear the module skip): the check at cli_main.py:141 runs before the --dry early return, so the test fails with exit_code=1 and the ClickException in the output. Marked xfail(strict=True) so the red phase is visible without failing the gate; task 2.1/2.2 relocates the check after the dry-run summary and removes the marker. Ralph-Task: 1.2 Run `python -m pytest tests/unit/test_cli_create_dev_smoke.py -v` in a shell with a runtime present and confirm the new test FAILS on the `ClickException`. Record the failure output. Do not write implementation before seeing red.
Relocate the Docker-availability preflight in create() to run only after the --dry early return, so `archi create --dry` no longer requires a container runtime. Non-dry create still raises the identical ClickException, and --podman still short-circuits the check. Pin both halves of the contract with new regression tests, and narrow the module-level pytest skip in test_cli_create_dev_smoke.py so the dry-run smoke tests actually execute in runtime-less environments (loop container, CI) instead of silently skipping. Also repoints the stale tasks.md symlink (still pointing at the already-merged fix-issue-84-strip-orphan-think change) at this change's tasks.md. Ralph-Task: 2.1 In `src/cli/cli_main.py`, delete the Docker-availability block at lines 140-146
Ralph-Task: 6.3 Open the PR: gh pr create --repo fasrc/archi --base dev
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 612732e308
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Check if Docker is available when --podman is not specified | ||
| if not other_flags.get("podman", False) and not check_docker_available(): |
There was a problem hiding this comment.
For archi create --force with an existing deployment and no Docker, this relocated preflight now runs after handle_existing_deployment() has already executed. In the non-dry force branch that helper constructs DeploymentManager and calls delete_deployment(..., remove_files=True), which removes the deployment directory even if the compose stop fails because Docker is unavailable; the old placement failed before any cleanup. Please keep the Docker check off the --dry path but run it before forced cleanup on the real deployment path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — fixed in 7f4c59b.
Verified the path you describe: with --force and no Docker, handle_existing_deployment() (cli_main.py:148) takes the non-dry force branch and calls delete_deployment(..., remove_files=True). In deployment_manager.py:174-177 the failed stop_deployment() is swallowed with a warning, and shutil.rmtree(deployment_dir) at :201 then runs unconditionally. So the deployment directory is destroyed on exactly the host that cannot bring it back up — a strictly worse outcome than the pre-existing behavior.
Worth noting this also violated the spec delta already in this PR, which says the preflight must fail "before any deployment work is performed" — a forced cleanup is deployment work.
Fix: the preflight now sits before the try block, guarded by not dry (cli_main.py:140-155). That keeps it off the --dry path while restoring it ahead of the forced cleanup. New regression test test_force_create_without_docker_keeps_existing_deployment asserts delete_deployment is never called and the existing directory survives; it failed before the fix and passes now.
| result = runner.invoke( | ||
| cli_main.create, |
There was a problem hiding this comment.
In environments where Podman is installed, this non-dry --podman test does not stop after proving the Docker check was skipped: it continues into the real create path, which writes deployment files, creates volumes, stages local files, and may run podman compose up. That makes a unit test depend on and mutate the host container runtime; monkeypatch the later deployment/volume operations or force a controlled failure immediately after the preflight instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and it was worse than "may" — fixed in 7f4c59b.
I ran that single test on a Podman-equipped host and diffed podman volume ls before/after. It created three real volumes:
archi-data-smoke
archi-pg-smoke
archi-smoke
So the unit gate was mutating host container state on every run. (I removed the strays.)
While confirming, I found a second reason this was unsafe. ARCHI_DIR is resolved into a module-level constant at import time (cli_main.py:38), so the per-test monkeypatch.setenv("ARCHI_DIR", ...) only takes effect for whichever test imports cli_main first. Every later test inherited a stale path — meaning the volume/mkdir work above was not reliably sandboxed to tmp_path and could land in the operator real ~/.archi. It also made tests order-dependent: my new --force regression test passed in a full-file run and failed in isolation, because the polluted ARCHI_DIR meant the deployment directory it created was never the one the CLI looked at.
Two fixes:
- The
--podmantest now patchescli_main.TemplateManagerto raise a sentinel, halting the run at the first post-preflight step, and assertscheck_docker_availableis never called — a tighter statement of the actual contract than "the message is absent". Nothing reaches VolumeManager/DeploymentManager. - New
archi_homefixture sets the env var and patches the module-levelARCHI_DIR, so each test gets a genuinely isolated deployment root. Applied to all non-dry tests in the file.
Verified: the full tests/unit/ suite (908 passed) now leaves podman volume ls byte-identical.
| return | ||
|
|
||
| # Check if Docker is available when --podman is not specified | ||
| if not other_flags.get("podman", False) and not check_docker_available(): |
There was a problem hiding this comment.
When non-dry create is run with --verbosity 4 and Docker is unavailable, this moved ClickException is now caught by the broad except below; that verbose branch only prints the traceback and then returns, so Click treats the command as successful. Before this change the Docker check was outside the try, so the same preflight exited non-zero regardless of verbosity; keep it outside the swallowing handler or re-raise after printing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — fixed in 7f4c59b.
Reproduced before fixing: archi create --verbosity 4 with no Docker exited 0 while printing the ClickException as a traceback. The except Exception handler at cli_main.py:268-272 only re-raises when verbosity < 4; at 4 it prints and falls through, so Click sees a clean return.
Fix is the one you suggested — the preflight is back outside the swallowing handler (cli_main.py:140-155), which also resolves the P1 above, since both regressions came from the same relocation into the try. Regression test test_non_dry_create_requires_docker_under_verbose_logging pins the exit code at --verbosity 4.
I deliberately left the broad handler itself alone: swallowing at verbosity 4 is pre-existing behavior across other commands too, and widening this PR to change it would exceed the scope of the --dry fix. Happy to file a follow-up issue if you think that handler is worth revisiting on its own.
…wallowing handler the preflight moved inside the try block, which introduced two regressions: --force tore down an existing deployment before discovering docker was missing, and --verbosity 4 swallowed the ClickException and exited 0. also make the create smoke tests hermetic: the --podman test was reaching VolumeManager and creating real volumes on the host, and ARCHI_DIR is a module-level constant so setenv alone left tests leaking into each other.
1 participant
Summary
archi create --dryno longer requires a container runtime — the Docker-availability preflight now runs only after the--dryearly return, immediately before the first real container operation.createstill raises the identicalClickExceptionwhen Docker is unavailable and--podmanwas not passed; the--podmanshort-circuit is unchanged.pytest.mark.skipifintests/unit/test_cli_create_dev_smoke.py(it blanket-skipped the whole file without docker/podman on PATH) — all five smoke tests now run and pass in a runtime-less shell.test_dry_run_succeeds_without_docker,test_non_dry_create_requires_docker,test_non_dry_create_with_podman_skips_docker_check.closes #112
This fixes the environmental flake seen on PR #111 (head
eeef7f7a, a docs/shell-only diff), where two gate runs on the same commit disagreed:Test plan
python -m pytest tests/unit/test_cli_create_dev_smoke.py -v— 5 passed, verified in a shell with nodocker/podmanonPATHpython -m pytest tests/unit/— 906 passed, 1 unrelated pre-existing xfailbash scripts/gate.sh— exit 0, 100% diff coverage onsrc/cli/cli_main.pyrestartandevaluateexpose no--dryoption and are untouched