Skip to content

Parse YAML inventory files with ansible-inventory - #6

Open
gdevenyi wants to merge 2 commits into
chenri2006:masterfrom
DouglasNeuroInformatics:feat/yaml-inventory
Open

Parse YAML inventory files with ansible-inventory#6
gdevenyi wants to merge 2 commits into
chenri2006:masterfrom
DouglasNeuroInformatics:feat/yaml-inventory

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 5, 2026

Copy link
Copy Markdown

Based on the stalled fboender/ansible-cmdb#204 by arnisoph (refs fboender#154), reworked to fix a control-flow bug in the original.

The bug

YAML is Ansible's inventory format of choice these days, but a .yml/.yaml inventory was handed to the ini parser. It doesn't fail — it silently invents hosts out of the YAML structure.

Given an ordinary YAML inventory, the parsed host list was:

'all'  'children'  'comment'  'dev'  'dtap'  'hosts'  'prod'  'vars'
'debian.dev.local'  'eek.electricmonk.nl'  'jib.electricmonk.nl'  ...

Eight fabricated hosts from YAML keywords, with host variables attached to the wrong entries. After this change:

debian.dev.local    -> {'dtap': 'dev'}
jib.electricmonk.nl -> {'comment': 'Workstation', 'dtap': 'prod'}

The fix

Detect .yml/.yaml and shell out to ansible-inventory -i <file> --list, which emits the same JSON structure as a dynamic inventory script — so the existing DynInvParser consumes it unchanged.

Error handling is a little firmer than the original: if ansible-inventory isn't on PATH we say Ansible needs installing rather than surfacing a bare OSError, and a non-zero exit relays its stderr and skips the inventory instead of feeding the parser empty output.

Reworked from the original

The upstream patch inserted a bare if ahead of the existing elif os.path.isfile(...):

if os.path.isfile(p) and util.is_executable(p):
    self._parse_dyn_inventory(p)
if os.path.isfile(p) and p.endswith(('.yml', '.yaml')):   # <-- new bare if
    self._parse_ansible_inventory(p)
elif os.path.isfile(p):
    self._parse_hosts_inventory(p)

That detaches the elif from the executable check, so an executable non-YAML inventory matches the dynamic-script branch and then the static-file branch, and gets parsed twice. This version uses elif so the chain stays exclusive.

I also dropped the original's proc.communicate(input), which passed the builtin input function.

Testing

Adds testYamlInventory with a YAML fixture, asserting both that real hosts and their vars are picked up and that YAML keywords don't leak in as hosts. It skips cleanly when ansible-inventory is unavailable, so it won't break environments without Ansible.

Full suite 10/10 pass. Regression-checked ini, directory, and executable dynamic inventories to confirm the dispatch chain still routes each correctly.

YAML is Ansible's inventory format of choice these days, but a '.yml'
or '.yaml' inventory was handed to the ini parser, which does not fail
-- it silently invents hosts out of the YAML structure. Given a normal
YAML inventory it produced hosts named 'all', 'children', 'hosts',
'vars', 'dtap' and 'comment' alongside the real ones, with host
variables attached to the wrong entries.

Detect '.yml'/'.yaml' inventory files and shell out to
'ansible-inventory -i <file> --list', which emits the same JSON
structure as a dynamic inventory script, so the existing DynInvParser
consumes it unchanged.

If 'ansible-inventory' is not on PATH, report that Ansible needs to be
installed rather than surfacing a bare OSError. A non-zero exit relays
ansible-inventory's stderr and skips the inventory instead of feeding
the parser empty output.

Adds a test with a YAML inventory fixture, asserting both that real
hosts and their vars are picked up and that YAML keywords do not leak
in as hosts. It skips when ansible-inventory is unavailable.

Based on fboender#204 by arnisoph. Reworked: the original
added a bare 'if' ahead of the existing 'elif os.path.isfile(...)'
branch, which made executable inventories match both the dynamic-script
branch and the static-file branch and be parsed twice. This uses an
elif so the chain stays exclusive.
Copilot AI lite review requested due to automatic review settings August 5, 2026 18:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds first-class support for YAML (.yml/.yaml) Ansible inventory files by dispatching them to ansible-inventory --list and then reusing the existing dynamic-inventory JSON parsing path, preventing YAML keywords from being misinterpreted as hosts by the INI parser.

Changes:

  • Route .yml/.yaml inventory files through ansible-inventory --list and ingest its JSON output.
  • Add a YAML inventory fixture and a unit test to ensure real hosts/vars are parsed and YAML structural keywords do not appear as hosts.
  • Improve error handling around invoking ansible-inventory (missing executable / non-zero exit).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/ansiblecmdb/ansible.py Adds YAML inventory detection and a parser that shells out to ansible-inventory and feeds its JSON into DynInvParser.
test/test.py Adds a skip-guarded unit test validating YAML inventory parsing behavior.
test/f_inventory/inventory.yml Adds a YAML inventory fixture used by the new test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/test.py
Comment thread src/ansiblecmdb/ansible.py Outdated
Comment thread src/ansiblecmdb/ansible.py
- DynInvParser ignored the 'children' key, so a host listed only in a
  leaf group reported just that group. A host under all -> infra ->
  dbservers came out with groups {'dbservers'} instead of also including
  'infra'. Collect the hierarchy while parsing and resolve it once all
  groups are known, with cycle protection for malformed inventories.

  Note group *vars* were already correct: 'ansible-inventory --list'
  resolves inherited vars into _meta.hostvars, so they never depended on
  this traversal.

- Match '.yml'/'.yaml' case-insensitively. Ansible's own inventory
  plugins only match lowercase, so an 'INVENTORY.YML' still won't parse
  -- but it now fails with an explicit ansible-inventory error instead
  of being handed to the ini parser, which fabricated hosts out of the
  YAML structure.

- Drop the duplicate 'import sys' in the test module.

Adds tests for nested group membership and the uppercase extension, and
extends the YAML fixture with a host nested two levels deep.
@gdevenyi

gdevenyi commented Aug 5, 2026

Copy link
Copy Markdown
Author

Merge note: conflicts with #10 in ansible.py and test.py, whichever lands second.

test.py is trivial (keep both sides). ansible.py is not: this PR adds a YAML branch to the inventory dispatch chain and #10 makes the executable branch fall back to static parsing. Taking either side loses the other, and combining them naively sends a chmod +x'ed .yml to the ini parser — the exact bug this PR fixes.

The resolution that works is to factor the shared path into a _parse_static_inventory() helper called from both branches, which also fixes the executable-YAML case neither PR handles alone. Code in the guide: #1 (comment)

Clean against every other open PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants