8000 fix: write only recorded connection vars for managed instances by jeffcpullen · Pull Request #4702 · ansible/molecule · GitHub
[go: up one dir, main page]
Skip to content

fix: write only recorded connection vars for managed instances - #4702

Merged
shvenkat-rh merged 1 commit into
ansible:mainfrom
jeffcpullen:fix/connection-vars-only-when-recorded
Sep 17, 2026
Merged

fix: write only recorded connection vars for managed instances#4702
shvenkat-rh merged 1 commit into
ansible:mainfrom
jeffcpullen:fix/connection-vars-only-when-recorded

Conversation

@jeffcpullen
@jeffcpullen jeffcpullen commented Sep 17, 2026 •
edited by coderabbitai Bot
Loading
Copy link
Copy Markdown
Contributor

Fixes #4696.

Problem

On the default driver with Molecule managing the instances, every host with an instance-config entry gets ansible_connection: smart in the generated inventory unless the create playbook recorded a connection. If the create playbook recorded no user or port, the host also gets ansible_user: null and ansible_port: null.

A host variable beats the same variable set at group or all level, so a user's own ansible_connection, ansible_user or ansible_port in the scenario's inventory is silently discarded.

Scenario, with a create playbook that records only instance and address:

# molecule/default/molecule.yml
driver:
  name: default
platforms:
  - name: probe
    groups:
      - probes
provisioner:
  name: ansible
  inventory:
    group_vars:
      probes:
        ansible_connection: local
        ansible_user: groupuser
        ansible_port: 2222

On main (20b71bda), the host entry Molecule generates and the effective host vars:

# <ephemeral directory>/inventory/ansible_inventory.yml
    probe: &id001
      ansible_connection: smart
      ansible_host: 127.0.0.1
      ansible_port: null
      ansible_ssh_common_args: -o UserKnownHostsFile=/dev/null -o ControlMaster=auto
        -o ControlPersist=60s -o ForwardX11=no -o LogLevel=ERROR -o IdentitiesOnly=yes
        -o StrictHostKeyChecking=no
      ansible_user: null
// ansible-inventory --host probe
    "ansible_connection": "smart",
    "ansible_port": null,
    "ansible_user": null,

What changes

  • Molecule writes ansible_connection, ansible_user, ansible_host and ansible_port only when the create playbook recorded the matching value. No smart fallback, no null.
  • A scenario that sets no transport anywhere connects as before on every supported core.
  • A scenario that set its own value now gets it.

The same scenario on this branch (fb42ac7d). The ansible_connection: smart, ansible_port: null and ansible_user: null lines are gone from the host entry, and the group values apply:

# <ephemeral directory>/inventory/ansible_inventory.yml
    probe: &id001
      ansible_host: 127.0.0.1
      ansible_ssh_common_args: -o UserKnownHostsFile=/dev/null -o ControlMaster=auto
        -o ControlPersist=60s -o ForwardX11=no -o LogLevel=ERROR -o IdentitiesOnly=yes
        -o StrictHostKeyChecking=no
// ansible-inventory --host probe
    "ansible_connection": "local",
    "ansible_port": 2222,
    "ansible_user": "groupuser",

The reporter's scenario records no connection, so it now gets no ansible_connection and connects over ssh without the connection: ssh workaround. driver.options.ansible_connection_options still applies only with managed: False, and provisioner.connection_options is the override for the generated host vars. Both are unchanged. ansible_ssh_common_args is still written for every managed host.

Risk / Impact

A managed scenario that set ansible_connection, ansible_user or ansible_port in its own inventory and depended on Molecule's host-level smart or null winning now gets its own value.

Root cause

Delegated.ansible_connection_options() in src/molecule/driver/delegated.py builds the managed host vars from instance_config.yml. The instance_params table's last entry is ("connection", "smart") under a truthy guard, so the fallback is emitted on every host unless the instance config carries its own connection. Below the table, ansible_user, ansible_host and ansible_port are assigned unconditionally from d.get(...), so an unrecorded key lands as None and is dumped as null.

The smart fallback is a regression. #1674 (0e07804b, 2019) introduced it. #2811 (4efdb48a, 2020-09) and #2988 (11d09c81, 2020-11) put it behind a guard, so ansible_connection was written only when recorded. #3932 (5b750ff4, 2023-06) folded the keys into the instance_params table and reintroduced an unconditional write, as null. #3957 (99a95f87, 2023-07) made it smart because ansible_connection: null crashed meta: reset_connection. This PR leaves the key absent rather than null, so that crash does not return. The null writes for user, host and port also date from #1674, so dropping them is new behavior.

Ansible's own default transport has been ssh since ansible-core 2.16, and smart resolved to ssh on any controller with a ControlPersist-capable OpenSSH, so removing the host var does not change the plugin for a scenario that sets nothing.

The fix changes the table entry to ("connection", None) and drops None values from the result in one comprehension at the return.

Changes

  • The default driver writes the four host vars only when the create playbook recorded a value for them.
  • docs/configuration.md, one paragraph after the instance-config API block: the four host vars are written only when recorded, where a managed scenario sets its own values, and that driver.options.ansible_connection_options supplies host vars only with managed: False.

Tests

tests/unit/driver/test_delegated.py:

  • test_ansible_connection_options_when_managed, ssh case: the "ansible_connection": "smart" expectation is removed and assert "ansible_connection" not in ssh_result is added. The winrm case, which records connection: winrm, is unchanged and still expects it.
  • New test_ansible_connection_options_omits_unrecorded_keys_when_managed: the instance config records instance, address and user: null. Asserts ansible_host is present, the other three are absent, and no value is None, which is the shape Fix connection plugin name setting #3957 fixed.

Test plan

  • tox -e lint passes
  • tox -e pkg passes
  • tox -e py passes, 910 passed, 7 skipped, same skips as main
  • Coverage unchanged, 90% total, delegated.py 93%
  • New test fails on main (2 failed, 44 passed), passes on this branch (46 passed)
  • Scenario above run on main and this branch, ansible_inventory.yml and ansible-inventory --host captured, outputs are the blocks in this body
  • Scenario recording address, user, port and no connection, only the ansible_connection: smart line differs
  • Scenario with no instance_config.yml, host entry {} on both sides
  • Docs updated

Summary by CodeRabbit

  • Bug Fixes

    • Delegated driver inventory no longer forces the smart connection type when no connection is recorded.
    • Connection settings from inventory, configuration files, environment variables, and other supported options now apply correctly.
    • Unrecorded connection values no longer override existing host or inventory settings.
    • SSH common arguments continue to be applied for both managed and unmanaged instances.

  • Documentation

    • Updated delegated driver documentation to clarify connection and inventory behavior.

@jeffcpullen
fix: write only recorded connection vars for managed instances
fb42ac7 
The managed path of the default driver wrote ansible_connection: smart
whenever the create playbook recorded no connection, and wrote
ansible_user, ansible_host and ansible_port as null when those keys
were not recorded. A host var shadows the same variable set at group
or all level, so a user's own ansible_connection, ansible_user or
ansible_port in the inventory was silently discarded.

Write each of those host vars only when the instance config records
it. With no ansible_connection written, Ansible resolves the transport
itself, from the inventory, ansible.cfg or the environment, otherwise
its own default. That default is ssh since ansible-core 2.16 and smart
on 2.15, so a scenario that sets no transport anywhere connects as it
did before.

The unconditional smart write is a regression. It was removed in
4efdb48 (ansible#2811) and 11d09c8 (ansible#2988). 5b750ff (ansible#3932) reintroduced
an unconditional write, as null, and 99a95f8 (ansible#3957) made it smart
because ansible_connection: null crashed meta: reset_connection. This
change leaves the key absent rather than null, so that crash does not
return. The null user, host and port vars have been written since the
keys were first mapped, so dropping them is new behavior.

Also document that these four host vars are written only when recorded.

Fixes: ansible#4696
Assisted-by: Claude (Anthropic)
Signed-off-by: Jeff Pullen <9343691+jeffcpullen@users.noreply.github.com>
@jeffcpullen
jeffcpullen requested a review from a team as a code owner September 17, 2026 04:35
@github-actions github-actions Bot added the fix label Sep 17, 2026
@coderabbitai
coderabbitai Bot commented Sep 17, 2026 •
edited
Loading
Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: a86c694a-1363-4920-837c-22f806299060

📥 Commits

Reviewing files that changed from the base of the PR and between 20b71bd and fb42ac7.

📒 Files selected for processing (3)
  • docs/configuration.md
  • src/molecule/driver/delegated.py
  • tests/unit/driver/test_delegated.py

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The delegated driver no longer defaults ansible_connection to "smart" when the instance configuration omits connection. Managed host variables now exclude None values. Documentation and unit tests describe and verify the updated behavior.

Changes

Delegated connection option handling

Layer / File(s) Summary
Filter unrecorded connection options
src/molecule/driver/delegated.py, docs/configuration.md		 The driver defaults connection to None and excludes None-valued managed options. The documentation describes inventory population and connection option behavior.
Validate generated host variables
tests/unit/driver/test_delegated.py		 Unit tests remove the default "smart" expectation and verify that unrecorded options are omitted.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: ssbarnea

Fixed issue severity: <fixed_issue_severity>Medium</fixed_issue_severity>

Merge Risk: ⚪ Minimal · up to fb42a

The generated inventory now omits unrecorded managed connection values, allowing configured inventory values and Ansible defaults to apply. No current merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: managed instances now write only recorded connection variables.
Linked Issues check ✅ Passed The changes address issue #4696. The delegated driver changes the managed-instance connection default from "smart" to None and removes unrecorded None values from generated host variables. The g…
Out of Scope Changes check ✅ Passed The changes stay within issue #4696. The source change fixes generated managed-instance host variables. The unit-test changes verify this behavior. The documentation change explains the behavior and c…
Full details: Docstring Coverage

Explanation

Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@shvenkat-rh
shvenkat-rh merged commit bf3c7d7 into ansible:main Sep 17, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shvenkat-rh shvenkat-rh shvenkat-rh approved these changes

Assignees

No one assigned

Labels

fix

Projects

🧰 devtools project board
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Default driver doesn't use options.ansible_connection_options.ansible_connection in the generated inventory

2 participants

@jeffcpullen @shvenkat-rh
0