Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="cli.py" line_range="11-14" />
<code_context>
+import sys
+
+from core.config import CoreConfig
+from core.executor import execute
+from core.metadata import InstanceMetadata
+from core.provisioner import Provisioner
+from core.results import get_exit_code, merge_results
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Importing `cli.py` raises `ModuleNotFoundError` because the repository contains no `core.provisioner` or `core.executor` modules, so the new CLI cannot start in the checked-in tree.
**Triggers:** When CIV-011 and CIV-012 have not already been merged into the same checkout.
**Suggested fix:** Add the dependency modules in this change or ensure they are present before exposing `cli.py`.
</issue_to_address>
### Comment 2
<location path="cli.py" line_range="227-253" />
<code_context>
+ p_run.add_argument("-r", "--resources-file",
</code_context>
<issue_to_address>
**issue (bug_risk):** The `run` subcommand does not define the existing `-a`/`--attach` flag from `cloud-image-val.py`, and `cmd_run` always calls `provision()` instead of attaching to the previously provisioned instances. Users migrating the full CLI cannot run the documented attach/recovery workflow.
**Triggers:** When a previous run left instances alive and the caller needs to resume them with `--attach`.
**Suggested fix:** Add the attach option and route it to the provisioner’s attach/reuse flow instead of unconditionally provisioning.
</issue_to_address>
### Comment 3
<location path="cli.py" line_range="163-164" />
<code_context>
+ print(f"Error: {exc}")
+ return 100
+ finally:
+ if not config.stop_cleanup:
+ provisioner.cleanup()
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** An exception raised by `provisioner.cleanup()` escapes the `finally` block and overrides the handled pipeline result, so `cmd_run` raises instead of returning its documented integer exit code when cleanup fails.
**Triggers:** When provisioning, execution, or result collection completes and infrastructure cleanup itself raises.
**Suggested fix:** Wrap cleanup in its own exception handler and return or report a cleanup-specific failure without allowing the `finally` block to replace the command result unexpectedly.
```suggestion
if not config.stop_cleanup:
try:
provisioner.cleanup()
except Exception as exc:
print(f"Cleanup error: {exc}")
```
</issue_to_address>| from core.executor import execute | ||
| from core.metadata import InstanceMetadata | ||
| from core.provisioner import Provisioner | ||
| from core.results import get_exit_code, merge_results |
There was a problem hiding this comment.
issue (bug_risk): Importing cli.py raises ModuleNotFoundError because the repository contains no core.provisioner or core.executor modules, so the new CLI cannot start in the checked-in tree.
Triggers: When CIV-011 and CIV-012 have not already been merged into the same checkout.
Suggested fix: Add the dependency modules in this change or ensure they are present before exposing cli.py.
| if not config.stop_cleanup: | ||
| provisioner.cleanup() |
There was a problem hiding this comment.
issue (bug_risk): An exception raised by provisioner.cleanup() escapes the finally block and overrides the handled pipeline result, so cmd_run raises instead of returning its documented integer exit code when cleanup fails.
Triggers: When provisioning, execution, or result collection completes and infrastructure cleanup itself raises.
Suggested fix: Wrap cleanup in its own exception handler and return or report a cleanup-specific failure without allowing the finally block to replace the command result unexpectedly.
| if not config.stop_cleanup: | |
| provisioner.cleanup() | |
| if not config.stop_cleanup: | |
| try: | |
| provisioner.cleanup() | |
| except Exception as exc: | |
| print(f"Cleanup error: {exc}") |
F-X64
left a comment
F-X64
left a comment
There was a problem hiding this comment.
Already looking good! There are some issues but we need to merge CIV-011 and CIV-012 first.
| help="Output path for merged JUnit XML") | ||
| p_coll.set_defaults(func=cmd_collect) | ||
|
|
||
| # --- run --- |
There was a problem hiding this comment.
Hm, this is missing the "--attach" flag that exists in the legacy cloud-image-val.py.
In the refactor, "civ execute --instances-json" partially covers this (if pointed at existing instances), but the behavior is a bit different "--attach" also reuses .tf.json and .tfstate files. Is the attach flag in use at the moment or should we skip it?
| return get_exit_code(merge_result) | ||
|
|
||
|
|
||
| def cmd_run(args: argparse.Namespace) -> int: |
There was a problem hiding this comment.
cmd_run (lines 120-161) reimplements the pipeline inline rather than calling cmd_provision, cmd_execute, and cmd_collect. This creates divergence risks.
E.g. the result collection logic diverges from cmd_collect: cmd_run uses !exec_result.instance_results[].result_file" (see lines 144 > ~146) while cmd_collect uses "glob.glob("*.xml") (see line 109)."
This does fulfill the task requirements but it might be solved more elegantly.
| } | ||
|
|
||
| for attr in ("debug", "parallel", "stop_cleanup"): | ||
| val = getattr(args, attr, None) |
There was a problem hiding this comment.
The if val: check skips False values, which currenbtly works because CoreConfig defaults these to False. If a future flag has default=True and the user doesn't pass it in it'll break. Same could happen if someone adds a boolean config key that defaults to True or the CoreConfig defaults change.
You can fix that by using "if val is not None:" instead of "if val:".
| sys.modules.setdefault("core.provisioner", mock_provisioner_module) | ||
| sys.modules.setdefault("core.executor", mock_executor_module) |
There was a problem hiding this comment.
A heads-up: setdefault only inserts if the key is absent. Once core/provisioner.py (CIV-011) and core/executor.py (CIV-012) are merged, Python's import system will load the real modules, setdefault won't override them, and the cli import will try to import actual Provisioner/execute symbols. If those modules have unresolvable imports of their own (OpenTofu, paramiko, etc.), every test in this file will fail at import time. Or in other owrds, lets get the other tasks merged and see what needs fixing-
Add cli.py with 5 subcommands (provision, execute, collect, run, cleanup) mapping to the phase-based execution flow. Uses argparse with subparsers, wires into core modules (provisioner, executor, results). The run subcommand ties the full pipeline together. 39 unit tests at 98% coverage.
- Use 'if val is not None' instead of 'if val' for boolean flags to handle future default=True cases correctly - Refactor cmd_run to delegate to cmd_execute and cmd_collect instead of reimplementing pipeline logic inline - Add --attach flag to civ run for reusing existing infrastructure
bb89261 to
47808b6
Compare
2 participants
Summary
cli.pyat repo root with 5 subcommands:provision,execute,collect,run,cleanupcore/moduleciv runties the full pipeline (provision → execute → collect → cleanup) and accepts all flags fromcloud-image-val.pycloud-image-val.pyis not modifiedDependencies
core.provisionerandcore.executorTest plan
pytest test/test_cli.py -v)cloud-image-val.pyunchanged