feat(cli): add instance rescue and rescue-exit commands#446
Conversation
|
Failed to retrieve llama text: POST 503: {"error": "This CRN cannot host the requested workload at this time."} |
69e0cc8 to
c5f490f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #446 +/- ##
==========================================
- Coverage 58.15% 57.98% -0.17%
==========================================
Files 22 22
Lines 4368 4422 +54
Branches 661 672 +11
==========================================
+ Hits 2540 2564 +24
- Misses 1553 1579 +26
- Partials 275 279 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The rescue and rescue-exit commands are well-structured and follow existing patterns. Error handling is appropriate, and the code integrates cleanly with the existing CLI. However, there are no tests for the new commands, and there is a minor issue with the default runtime selection logic that should be addressed.
pyproject.toml (line 33): Typo in comment: 'changement' is French for 'change'. Please use 'change' for consistency with the rest of the codebase.
pyproject.toml (line 40): Same typo: 'changement' should be 'change'.
src/aleph_client/commands/instance/__init__.py (line 1319): The default_idx defaults to 0 if no runtime has default=True. This means the user will select the first runtime by default even when none is marked as default. Consider either printing a warning or defaulting to -1 (requiring explicit selection) when no default is found.
src/aleph_client/commands/instance/__init__.py (line 1310): If both choose_runtime=True and rescue_hash is provided, the choose_runtime flag is silently ignored. Consider either raising an error for this conflicting input, or documenting the precedence more clearly in the help text.
tests/unit/test_instance.py (line 1): Missing test coverage for the new rescue and rescue-exit commands. Following the existing test patterns (e.g., test_stop_instance, test_reboot_instance), tests should cover: successful rescue/rescue-exit, error status codes (409, 501), and the choose_runtime interactive flow.
- fix typo, default runtime warning if no default, add tests
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The rescue and rescue-exit commands correctly implement the rescue workflow, and the tests cover the happy path. However, both commands have a genuine bug: error responses (409 Conflict, 501 Not Available) print a message but do not exit with a non-zero exit code, causing the shell to see success (exit code 0). This is misleading for shell scripting and automation.
src/aleph_client/commands/instance/__init__.py (line 1352): The 409 Conflict branch echoes the error but falls through to implicit return None (exit code 0). Add 'raise typer.Exit(code=1)' after the echo.
src/aleph_client/commands/instance/__init__.py (line 1354): The 501 'Rescue mode not available' branch echoes the error but falls through to implicit return None (exit code 0). Add 'raise typer.Exit(code=1)' after the echo.
src/aleph_client/commands/instance/__init__.py (line 1385): The 409 Conflict branch echoes the error but falls through to implicit return None (exit code 0). Add 'raise typer.Exit(code=1)' after the echo.
tests/unit/test_instance.py (line 1096): test_rescue_instance_conflict and test_rescue_instance_not_available should assert that typer.Exit(code=1) is raised, not just that the API method was called. Similarly, test_rescue_exit_instance should cover the 409 case with an exit code assertion.
No description provided.