Return non-zero exit from get on query failure#2313
Draft
ideaship wants to merge 1 commit into
Draft
Conversation
osism get hostvars and osism get hosts logged an error but then
returned None when the underlying ansible-inventory subprocess
failed. cliff turns a take_action return value of None into exit
code 0 (return_code = self.take_action(parsed_args) or 0), so these
commands exited successfully even though the inventory query could
not be run at all. That makes them unsafe to use in set -e scripts
or && chains, where a failed lookup silently looks like success.
Both commands now return 1 when subprocess.CalledProcessError is
raised, i.e. when the query itself could not be executed.
A missing host is one such failure. With the pinned ansible-core
(2.18.9, release latest/base.yml), ansible-inventory --host on a
host that is not in the inventory does not return an empty result;
it refuses the query with "ERROR! You must pass a single valid host
to --host parameter" and exits 5. osism get hostvars for an unknown
host therefore now exits 1, which matches its existing "Host not
found in inventory." error message.
Empty results are deliberately left as exit 0: a host that exists
but has no host vars (ansible-inventory prints {} and exits 0), a
requested variable or fact that is absent from an otherwise
successfully retrieved data set, or an inventory that lists no
matching hosts. These are valid answers to a query that did run.
This keeps the behaviour consistent with the convention used
elsewhere in the CLI, where a non-zero status signals an operational
failure rather than an empty result set.
Adds unit tests for both commands covering the failure path (a
raised CalledProcessError yields exit code 1) and the empty-result
path (a successful query with nothing to show yields exit code 0).
AI-assisted: Claude Code
Signed-off-by: Roger Luethi <luethi@osism.tech>
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
2 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.
What
osism get hostvarsandosism get hostslogged an error but then returnedNonewhen the underlyingansible-inventorysubprocess failed. cliff turns atake_actionreturn value ofNoneinto exit code 0 (return_code = self.take_action(parsed_args) or 0), so these commands exited successfully even when the inventory query could not be run at all — making them unsafe inset -escripts and&&chains.Both commands now
return 1onsubprocess.CalledProcessError, i.e. when the query itself could not be executed.Missing host
With the pinned
ansible-core(2.18.9 —releaserepolatest/base.yml),ansible-inventory --host <unknown>does not return an empty result. It refuses the query withERROR! You must pass a single valid host to --host parameterand exits 5. Soosism get hostvars <unknown-host>now exits 1, matching its existingHost not found in inventory.error message.What stays exit 0 (empty results)
Empty results are deliberately left as success — a query that ran fine but had nothing to show:
ansible-inventoryprints{}, exits 0)A note on the convention
There is no documented exit-code contract for the
osismCLI — the behaviour is emergent and inconsistent across commands. This PR does not attempt to define or enforce one. It only tries to makeosism getmatch what looks like the prevailing convention elsewhere in the CLI: a non-zero exit signals an operational failure (the query could not be run), while an empty result set is treated as a valid, successful answer.Tests
Adds unit tests for both commands covering the failure path (raised
CalledProcessError→ exit 1) and the empty-result path (successful query with nothing to show → exit 0).