Return non-zero exit codes on CLI failure paths - #2331
Merged
Conversation
Several commands wait for a Celery task's output, hit TimeoutError, log the timeout, and then fall through to the end of take_action with an implicit None return. cliff turns that into exit code 0 (return_code = self.take_action(parsed_args) or 0), so a command that gave up waiting for its task still reports success. That makes the affected commands unsafe in set -e scripts and && chains, where a timed-out sync silently looks like it completed. This is the same class of bug PR #2313 fixed for osism get, but on the timeout path rather than the inventory-query path. The affected sites: - reconciler.Sync - validate.Run._handle_task (the value take_action returns) - netbox.Ironic - netbox.Sync Each now returns 1 from the TimeoutError handler, so a timeout yields a non-zero exit. Adds unit tests asserting exit 1 when fetch_task_output raises TimeoutError, mirroring PR #2313's failure-path tests. Related-Bug: #2314 AI-assisted: Claude Code Signed-off-by: Roger Luethi <luethi@osism.tech>
osism wait returned an exit code only from a single mid-loop return, guarded by len(task_ids) == 1 and evaluated after task_ids.pop(). That return was reached wrongly, or not at all, in every case: - --live, single task: after the pop len(task_ids) == 0, so the guard never fired. Whether the stream finished or timed out, control fell off the end of take_action and cliff turned the implicit None into exit 0. A timeout was logged but ignored. - --live, two tasks: the first iteration left len(task_ids) == 1 and hit the guard after processing only the first task, skipping the second. On a timeout in that iteration rc was never assigned, raising UnboundLocalError. - --live, three or more tasks: the guard fired after processing the second-to-last task, skipping the last task. If that fetch timed out, it returned an earlier task's rc, or raised UnboundLocalError when rc had not yet been assigned. - non-live polling: the loop drained and fell off the end, relying on cliff to convert the implicit None return into exit 0. Initialise rc = 0 before the loop, preserve a non-zero result once it is seen (a later successful fetch does not overwrite it), set rc = 1 on TimeoutError, drop the broken len(task_ids) == 1 short-circuit, and return rc once all tasks have been processed. A timed-out or failed live task now yields a non-zero exit, every task is processed, and a successful wait exits 0. Adds unit tests covering the --live timeout (single and multiple tasks), a non-zero task rc, and a successful task. Part of issue #2314 (Category A, wait.py). Related-Bug: #2314 AI-assisted: Claude Code Signed-off-by: Roger Luethi <luethi@osism.tech>
osism report memory, lldp, bgp and status each load the Ansible
inventory by running ansible-inventory via subprocess.run. On a
non-zero return code ("Error loading inventory.") and on
subprocess.TimeoutExpired ("Timeout loading inventory.") they logged
the error and then bare-returned None. cliff turns that into exit
code 0 (return_code = self.take_action(parsed_args) or 0), so a report
whose inventory could not be loaded at all still exited successfully.
That makes these commands unsafe in set -e scripts and && chains.
These are the direct twins of the bug PR #2313 fixed for osism get.
All eight inventory-load failure paths now return 1.
The neighbouring "No hosts found in inventory." branch is deliberately
left at exit 0: the query ran fine and simply returned no hosts, which
is a valid empty result rather than an operational failure, matching
the convention established in PR #2313.
Adds unit tests for all four commands covering the failure path (a
non-zero ansible-inventory return code yields exit 1), the timeout
path (TimeoutExpired yields exit 1) and the empty-result path (an empty
inventory yields exit 0).
Part of issue #2314 (Category B).
Related-Bug: #2314
AI-assisted: Claude Code
Signed-off-by: Roger Luethi <luethi@osism.tech>
Several commands look up a resource, log an error or warning when it cannot be found, and then bare-return None. cliff turns that into exit code 0 (return_code = self.take_action(parsed_args) or 0), so a failed lookup silently looks like success and the commands are unsafe in set -e scripts and && chains. All the affected not-found / unresolved-lookup paths now return 1: - compute.ComputeMigrationList: no user domain, no user, no project domain, no project, multiple servers matched, no server found. - server.ServerList: no user domain, no user, domain not found, project domain not found, project not found. - volume.VolumeList: domain not found, project domain not found, project not found. - netbox.Console / netbox.Dump: NetBox integration not configured (the operation cannot run without it), and Dump's device not found. - baremetal Deploy, Undeploy, BurnIn, Clean, Provide, MaintenanceSet, MaintenanceUnset, PowerOn, PowerOff and Delete: the single named node could not be found (conn.baremetal.find_node returned nothing). Adds unit tests for each command driving the not-found path and asserting exit 1, mirroring PR #2313. Part of issue #2314 (Category C and Category D not-found lookups). Related-Bug: #2314 AI-assisted: Claude Code Signed-off-by: Roger Luethi <luethi@osism.tech>
Several commands hit a runtime failure -- an unmet precondition, a required integration being absent, a resource missing from a backend, or an exception they catch -- log it, and then bare-return None. cliff turns that into exit code 0 (return_code = self.take_action(parsed_args) or 0), so the failed operation looks like success and the commands are unsafe in set -e scripts and && chains. The affected paths now return 1: - server.ServerMigrate: the server is not in ACTIVE or PAUSED state and therefore cannot be live migrated. The requested migration cannot run, so this precondition failure is no longer reported as success (it was logged at info level, which is why it was easy to miss). - compute.ComputeEnable: clearing force-down is rejected with BadRequestException because `done` evacuation records still remain on the host. The except handler bare-returned; it now returns 1. - baremetal.BaremetalDump: the node could not be found in Ironic, the NetBox integration is not available, or the device could not be found in NetBox. - baremetal.BaremetalPing: the NetBox integration is not available, the named device was not found in NetBox, or the ping operation raised an exception caught by the outer handler. Adds unit tests driving each failure path and asserting exit 1. Part of issue #2314 (Category C precondition/exception sites and the remaining baremetal operational/not-found failures). Related-Bug: #2314 AI-assisted: Claude Code Signed-off-by: Roger Luethi <luethi@osism.tech>
Several commands reject invalid input -- a missing required argument, an unknown resource type, or mutually inconsistent options -- by logging an error and then bare-returning None. cliff turns that into exit code 0 (return_code = self.take_action(parsed_args) or 0), so the command refuses to do anything yet still reports success. Returning a non-zero status on bad input is the conventional CLI behaviour and makes the commands safe to use in set -e scripts and && chains. Unlike a failed lookup, these paths never attempt the operation; they reject the invocation up front. This is therefore a deliberate behaviour change for the argument-validation paths, distinct from the correctness fixes for operations that actually ran and failed. The affected paths now return 1: - baremetal Deploy, Undeploy, BurnIn, Clean, Provide and Delete: no node name was given and --all was not set; and BurnIn additionally when no burn-in stressor was selected. - baremetal PowerOn and PowerOff: no node name was given (these commands do not support --all). - status.Run: an unknown resource type was requested. - compute.ComputeMigrationList: --changes-since is later than --changes-before. Adds unit tests for each validation path asserting exit 1. Part of issue #2314 (Category C argument-validation paths). Related-Bug: #2314 AI-assisted: Claude Code Signed-off-by: Roger Luethi <luethi@osism.tech>
The destructive baremetal commands let an operator target every node at once with --all, guarded by a required --yes-i-really-really-mean-it confirmation. When --all was given without that confirmation, the command logged the "Please confirm ..." message and bare-returned None. cliff turns that into exit code 0 (return_code = self.take_action(parsed_args) or 0), so a destructive command that refused to run for want of confirmation still reported success. Returning a non-zero status when the command declines to act is the conventional CLI behaviour and lets wrappers and set -e scripts tell a refused invocation apart from a completed one. Like the argument-validation change, these paths never attempt the operation, so this is a deliberate behaviour change. The confirmation guards in BaremetalDeploy (rebuild), BaremetalUndeploy, BaremetalClean and BaremetalDelete now return 1. The BaremetalBurnIn active-node confirmation is intentionally not changed: it `continue`s to the next node inside a per-node loop rather than refusing the whole command, so it is a different case. Adds unit tests asserting exit 1 when --all is given without confirmation. Part of issue #2314 (Category C unconfirmed-yes paths). Related-Bug: #2314 AI-assisted: Claude Code Signed-off-by: Roger Luethi <luethi@osism.tech>
![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 left some high level feedback:
- There’s quite a bit of duplicated helper logic in the new tests (e.g. repeated patterns to construct commands, parse args, and patch
get_cloud_helpers/connections); consider extracting common helpers or pytest fixtures to reduce repetition and make future CLI behaviour tests easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s quite a bit of duplicated helper logic in the new tests (e.g. repeated patterns to construct commands, parse args, and patch `get_cloud_helpers`/connections); consider extracting common helpers or pytest fixtures to reduce repetition and make future CLI behaviour tests easier to maintain.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the exit-code-0-on-failure pattern audited in #2314: several
osismsubcommands logged an error on a failure path and then returned control
without a non-zero exit code, so a failed operation looked like success in
set -escripts and&&chains. See #2314 for the full audit and rationale.Why multiple commits
The work is split by type of fix, not by file, so each commit is one
reviewable decision and the riskier behaviour changes are isolated at the end
of the stack:
Return non-zero on task-wait timeouts— Category A timeout handlers.Fix wait exit code and timeout handling for --live— the wait.pystructural fix (init
rc, drop the brokenlen(task_ids) == 1guard).Return non-zero on inventory query failures in report— Category B.Return non-zero on failed lookups in CLI commands— not-found / lookupfailures (Category C + D).
Return non-zero on precondition and operational failures— unmetpreconditions, missing integrations, swallowed exceptions.
Return non-zero on invalid command arguments— behaviour change.Return non-zero on unconfirmed destructive --all— behaviour change.Commits 1–5 are correctness fixes: an operation actually ran and failed, so
exit 0 was simply wrong. Commits 6–7 are deliberate behaviour changes — those
paths reject the invocation up front without attempting anything — so they sit
last on the stack and can be reviewed (or dropped) independently of the
correctness fixes.
Empty-result and status paths that legitimately exit 0 (e.g. the netbox
--list"No NetBox instances configured" report, report's "No hosts found")are deliberately left untouched, following the convention established in #2313.
Each commit adds unit tests mirroring #2313 (failure path → exit 1,
empty-result path → exit 0).
Related-Bug: #2314