Support molecule login/molecule list for ansible-native scenarios - #4663
Open
jirib wants to merge 5 commits into
Open
Support molecule login/molecule list for ansible-native scenarios#4663jirib wants to merge 5 commits into
jirib wants to merge 5 commits into
Conversation
|
Label error. Requires at least 1 of: breaking, chore, feat, fix. Found: . Follow https://www.conventionalcommits.org to get auto-labeling to work correctly. |
📝 WalkthroughWalkthroughMolecule now discovers ansible-native hosts and connection details from persisted Ansible inventory data. Login handles empty host lists and missing connection fields explicitly, while delegated-driver inventory fallbacks and related documentation and tests are added. ChangesAnsible-native inventory discovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MoleculeLogin
participant DelegatedDriver
participant AnsibleInventory
MoleculeLogin->>DelegatedDriver: discover hosts when instances are empty
DelegatedDriver->>AnsibleInventory: query configured inventory sources
AnsibleInventory-->>DelegatedDriver: return JSON host data
DelegatedDriver-->>MoleculeLogin: return hosts and connection options
MoleculeLogin-->>MoleculeLogin: build login command
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ansible-native.md`:
- Line 373: Update both links in docs/ansible-native.md at lines 373-373 and
423-423 to use the fragment `#logging-into--listing-instances`, matching the
generated anchor for the “Logging Into / Listing Instances” heading.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 02a8d52c-8efa-4eb7-af94-06b4db2574a5
📒 Files selected for processing (6)
docs/ansible-native.mdsrc/molecule/command/login.pysrc/molecule/driver/base.pysrc/molecule/driver/delegated.pytests/unit/command/test_login.pytests/unit/driver/test_delegated.py
Ansible-native scenarios (no `platforms:` declared) left `molecule login` and `molecule list` with no way to discover instance names or connection details, since both previously relied solely on the static platform list. Both commands now fall back to querying the real inventory - the same `--inventory` sources `ansible-playbook` itself is invoked with - via `ansible-inventory --list`/`--host <name>`, reusing whatever the create playbook already wrote there instead of requiring a separate, molecule-specific config file to stay in sync. - driver/delegated.py: add inventory-backed host discovery and login-option resolution, used as a fallback when instance_config.yml has no entry. - driver/base.py: status() reports real inventory host names instead of a blank placeholder when no platforms are configured. - command/login.py: execute()/_get_hostname() use the same fallback; clearer error messages when no hosts can be found either way.
Add unit test coverage for the inventory-based fallback paths added for ansible-native scenarios: inventory source resolution, ansible-inventory subprocess success/failure handling (including the new debug logging), inventory-based login-option resolution, host discovery, and the corresponding fallback behavior in status()/execute()/_get_hostname().
Explain how molecule login/list resolve instance names and connection details for ansible-native scenarios by querying the real inventory. Also flag a gap in the existing container/cloud examples: dynamically assigned connection details (IP, published port) only exist as in-play facts unless explicitly persisted to a host_vars file, and login/list can't see anything that wasn't written to disk.
jirib
force-pushed
the
fix-login-ansible-native
branch
from
July 21, 2026 19:52
9653fdb to
dc70a07
Compare
Renamed the heading to drop the "/" (previously "Logging Into / Listing Instances") since GitHub's own anchor slugifier and mkdocs' default python-markdown slugifier disagree on how a removed "/" between two spaces collapses - GitHub yields a double hyphen, mkdocs collapses to a single hyphen. Neither in-page link worked for both renderers at once. Renaming to "Logging Into and Listing Instances" produces the same anchor under both, so the links now work regardless of where the page is viewed. Reported by CodeRabbit on PR ansible#4663 (markdownlint-cli2 MD051).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/molecule/driver/delegated.py`:
- Around line 181-213: Add a finite timeout to the subprocess.run call in
_run_ansible_inventory, using the project’s established timeout configuration or
constant if available, and catch subprocess.TimeoutExpired alongside
CalledProcessError. Log the timeout through the existing debug path and return
None, preserving current behavior for other failures.
- Around line 154-179: Update _ansible_inventory_args to preserve split
inventory option/value pairs from provisioner.ansible_args and ansible_args,
including both -i path and --inventory path forms. Keep inline forms intact,
ensure the associated value is included with its flag, and continue prepending
provisioner.inventory_directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5133898f-24b3-401f-a1bf-052fa8031f20
📒 Files selected for processing (6)
docs/ansible-native.mdsrc/molecule/command/login.pysrc/molecule/driver/base.pysrc/molecule/driver/delegated.pytests/unit/command/test_login.pytests/unit/driver/test_delegated.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/molecule/command/login.py
- src/molecule/driver/base.py
- tests/unit/command/test_login.py
Two issues flagged by CodeRabbit on PR ansible#4663: - _ansible_inventory_args() only kept extra --inventory/-i args when written in the single-token "--inventory=path" form. Space-separated form ("-i", "path" as two list items) had its value silently dropped, producing a broken ansible-inventory invocation. Now both forms are handled explicitly. - _run_ansible_inventory() had no timeout, so a hanging dynamic inventory source (network calls, cloud plugins) would hang molecule login/molecule list indefinitely. Add a 30s timeout and handle subprocess.TimeoutExpired the same way as other failure modes. Add test coverage for both.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/molecule/driver/delegated.py (1)
245-252: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRead both Ansible key-file variable names
_get_inventory_login_options()only readsansible_ssh_private_key_file; inventories that exposeansible_private_key_filewill missidentity_file, breaking key-based login fallback. Checkansible_private_key_filefirst and keepansible_ssh_private_key_fileas a fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/molecule/driver/delegated.py` around lines 245 - 252, Update _get_inventory_login_options() to read ansible_private_key_file first when setting identity_file, then fall back to ansible_ssh_private_key_file when the primary variable is absent. Preserve the existing address, user, and port handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/molecule/driver/delegated.py`:
- Around line 245-252: Update _get_inventory_login_options() to read
ansible_private_key_file first when setting identity_file, then fall back to
ansible_ssh_private_key_file when the primary variable is absent. Preserve the
existing address, user, and port handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 398df079-d0d1-4cfc-b89f-e91c6d33f3ca
📒 Files selected for processing (2)
src/molecule/driver/delegated.pytests/unit/driver/test_delegated.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/driver/test_delegated.py
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
1 participant
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.
Summary
Ansible-native scenarios (no
platforms:declared, instance lifecycle managed entirely by customcreate.yml/destroy.yml) leftmolecule loginandmolecule listnon-functional — both relied solely on the staticplatforms:list, which these scenarios don't have.This adds a fallback: when no platforms are configured, both commands query the real inventory via
ansible-inventory, reusing the exact same--inventorysourcesansible-playbookitself is invoked with. No new molecule-specific config file is required — connection details already present in your inventory (e.g. written bycreate.yml) are used directly.Changes
driver/delegated.py— resolve the same--inventorysources asansible-playbook; addansible-inventory-backed host discovery (get_ansible_native_hosts) and login-option resolution (_get_inventory_login_options), used as a fallback wheninstance_config.ymlhas no entry for the instance.driver/base.py—status()reports real host names from the inventory instead of a single blank placeholder when no platforms are configured.command/login.py—execute()/_get_hostname()use the same inventory fallback; clearer error messages when no hosts can be found by either mechanism.ansible-inventorysubprocess success/failure handling.docs/ansible-native.md, plus notes on the existing container/cloud examples flagging that dynamically-assigned connection details must be persisted to ahost_varsfile (not just registered as in-play facts) to be discoverable bylogin/list.Backward compatibility
instance_config.ymlis still checked first if a create playbook populates it — the inventory query only kicks in as a fallback, so existing non-native scenarios are unaffected.Testing
Verified end-to-end against a real ansible-native scenario (custom libvirt-based instance lifecycle): confirmed
molecule list/molecule logincorrectly resolve the real instance name and connect over SSH, both with and withoutinstance_config.ymlpresent. Full unit suite passes (53new/updated tests).Summary by CodeRabbit
molecule login/molecule listcan use runtime-resolved connection details.molecule loginbehavior when no hosts are available, including clearer messaging when--hostis missing and safer handling when--hostis provided.