Handle inventory files that are executable but not runnable - #10
Handle inventory files that are executable but not runnable#10gdevenyi wants to merge 2 commits into
Conversation
Two failure modes in _parse_dyn_inventory(), both of which lose data
silently or crash:
1. An inventory file with the executable bit set but which the OS cannot
run -- a plain ini inventory that was chmod +x'ed, came off a
FAT/NTFS filesystem, or has a bad shebang -- was treated as a dynamic
inventory script. The exec raised OSError ("[Errno 8] Exec format
error"), which was caught and printed, and every host in that file
was dropped:
$ ansible-cmdb -i ./hosts out/ # ./hosts is 0755 ini
Exception while executing dynamic inventory script './hosts':
[Errno 8] Exec format error
(no hosts in output)
_parse_dyn_inventory() now returns False when the file could not be
executed at all, and the caller reads it as a static inventory
instead. A script that runs but exits non-zero still returns True --
its output must not be reinterpreted as an ini inventory.
2. When a dynamic inventory script exited non-zero, its stderr was
relayed with 'for line in stderr: sys.stderr.write(line)'. On Python
3 iterating a bytes object yields ints, so this raised
TypeError: write() argument must be str, not int
and the user got that instead of the script's actual error. Decode
and write the output in one go.
Also drops the stray 'input' argument to communicate(); it passed the
builtin function, which happened to be harmless only because stdin was
not a pipe.
Adds tests for both, with an executable ini fixture (committed 0755) and
a failing dynamic inventory script. Both fail before this change.
Fixes fboender#194.
There was a problem hiding this comment.
Pull request overview
Improves inventory handling in ansiblecmdb.Ansible so that inventory files marked executable but not actually runnable fall back to static parsing, and failing dynamic inventory scripts surface their real stderr instead of raising a Python 3 TypeError. This aligns behavior with reported user failures and adds regression tests + fixtures.
Changes:
- Make
_parse_dyn_inventory()return a boolean so callers can fall back to static parsing when execution is impossible. - Fix stderr relaying for failing dynamic inventories by decoding bytes before writing to
sys.stderr. - Add fixtures and unit tests covering the executable-static fallback and failing dynamic inventory behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ansiblecmdb/ansible.py |
Adds fallback behavior for non-runnable “executable” inventories and fixes stderr handling for failing dynamic inventories. |
test/test.py |
Adds regression tests for the new inventory behaviors. |
test/f_inventory/hosts_executable |
New fixture: static INI inventory committed with executable bit to reproduce the fallback case. |
test/f_inventory/dyninv_failing.py |
New fixture: failing dynamic inventory script to verify stderr relay and non-reinterpretation as static inventory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- dyninv_failing.py was a /bin/sh script with a .py extension. Rename it to .sh so the extension matches the shebang and it isn't confused with the existing Python dyninv.py fixture. What it tests -- a dynamic inventory that exits non-zero -- is language-agnostic. Executable bit preserved (mode 100755). - Drop the duplicate 'import sys' in the test module.
|
Merge note: conflicts with #6 in Short version: this PR and #6 both restructure the same dispatch chain. Factor the non-executable path into a Clean against every other open PR. |
Fixes fboender/ansible-cmdb#194, and a second bug found while reproducing it.
Bug 1 — executable inventory silently drops every host
An inventory file with the executable bit set that the OS can't actually run — a plain ini inventory that was
chmod +x'ed, came off a FAT/NTFS filesystem, or has a bad shebang — was treated as a dynamic inventory script. The exec raisedOSError, which was caught and printed, and every host in that file was dropped:_parse_dyn_inventory()now returnsFalsewhen the file couldn't be executed at all, and the caller reads it as a static inventory instead. A script that runs but exits non-zero still returnsTrue— its output must not be reinterpreted as an ini inventory.Bug 2 — failing dynamic inventory crashes with a misleading error
When a dynamic inventory script exited non-zero, its stderr was relayed with:
On Python 3, iterating a
bytesobject yields ints, so this raised:The user got that instead of the script's actual error message. Reproduced with a script that prints to stderr and exits 1 — before the fix the real message never appears; after it, you see:
This is likely what's behind some of the "dynamic inventory fails" reports such as fboender#187, where the underlying error was never visible.
Also drops the stray
inputargument tocommunicate()— it passed the builtin function, harmless only because stdin wasn't a pipe.Testing
Two tests with fixtures: an executable ini inventory (committed mode
100755, so the bit survives cloning) and a failing dynamic inventory script. Both fail before the change — one asFAIL, one asERROR(theTypeError). Full suite 11/11 after. The existingtestDynInvandtestMixedDirstill pass, confirming genuine dynamic inventories are unaffected.