test: ensure role gathers the facts it uses by having test clear_facts before include_role

[richm]

The role gathers the facts it uses. For example, if the user uses
ANSIBLE_GATHERING=explicit, the role uses the setup module with the
facts and subsets it requires.

This change allows us to test this. Before every role invocation, the test
will use meta: clear_facts so that the role starts with no facts.

Create a task file tests/tasks/run_role_with_clear_facts.yml to do the tasks
to clear the facts and run the role. Note that this means we don't need to
use gather_facts for the tests.

Some vars defined using ansible_facts have been changed to be defined with
set_fact instead. This is because of the fact that vars are lazily
evaluated - the var might be referenced when the facts have been cleared, and
will issue an error like ansible_facts["distribution"] is undefined. This is
typically done for blocks that have a when condition that uses ansible_facts
and the block has a role invocation using run_role_with_clear_facts.yml
These have been rewritten to define the when condition using set_fact. This
is because the when condition is evaluated every time a task is invoked in the
block, and if the facts are cleared, this will raise an undefined variable error.

[sourcery-ai]

Reviewer's Guide

Introduces a reusable test task file to run the ha_cluster role after clearing Ansible facts, adjusts tests to use it instead of directly including the role, and refactors some test condition logic into set_fact variables for clarity and compatibility with fact clearing.

File-Level Changes

Change	Details	Files
Add reusable task file to clear facts before running the ha_cluster role and refactor tests to use it instead of direct include_role.	Create tests/tasks/run_role_with_clear_facts.yml that clears ansible_facts, conditionally runs the linux-system-roles.ha_cluster role with optional __sr_failed_when handling, and supports tasks_from and public semantics via variables. Replace many direct include_role usages of linux-system-roles.ha_cluster in test playbooks with include_tasks of tasks/run_role_with_clear_facts.yml, passing __sr_public where public: true was previously used. Refactor crypto-policy PQC eligibility logic in fixture_psks.yml into a dedicated __need_crypto_policies_pqc variable set via set_fact to make the condition reusable and resilient when facts are cleared. Move computation of __test_eligible and __test_agents from play vars into a task-level set_fact in tests_cluster_basic_cloud_packages.yml so they are re-established after facts are cleared. Remove the tests/library directory entry (presumably moving any needed plugin logic elsewhere, e.g., into standard plugin paths).	tests/tasks/run_role_with_clear_facts.yml tests/tasks/fixture_psks.yml tests/tests_cluster_basic_cloud_packages.yml tests/tests_cluster_basic_cloud_packages.yml tests/tests_cib_stonith_levels_validation.yml tests/tests_export_doesnt_destroy_cluster.yml tests/template_qdevice.yml tests/template_sbd_all_options.yml tests/tests_bootc_e2e_cluster.yml tests/tests_bootc_e2e_qnetd.yml tests/tests_cib_constraints_create.yml tests/tests_cib_constraints_create_and_load_info.yml tests/tests_cib_properties_empty.yml tests/tests_cib_properties_one_set.yml tests/tests_cib_resources_create.yml tests/tests_cib_resources_create_and_load_info.yml tests/tests_cib_rsc_op_defaults.yml tests/tests_cluster_advanced_knet_full.yml tests/tests_cluster_advanced_knet_implicit.yml tests/tests_cluster_advanced_udp_full.yml tests/tests_cluster_basic.yml tests/tests_cluster_basic_custom_fence_agents.yml tests/tests_cluster_basic_custom_packages.yml tests/tests_cluster_basic_custom_pcsd_tls.yml tests/tests_cluster_basic_disabled.yml tests/tests_cluster_basic_existing_psks.yml tests/tests_cluster_basic_new_psks.yml tests/tests_cluster_destroy.yml tests/tests_export_firewall_selinux.yml tests/tests_not_booted_cluster.yml tests/tests_not_booted_qnetd.yml tests/tests_pcs_permissions.yml tests/tests_role_mode_consistency.yml tests/tests_sbd_defaults.yml tests/tests_sbd_defaults_disabled.yml tests/tests_sbd_delay_start.yml tests/tests_sbd_needs_atb_while_atb_enabled.yml tests/tests_cib_acls.yml tests/tests_cib_alerts.yml tests/tests_cib_node_attributes.yml tests/tests_cib_stonith_levels.yml tests/tests_cib_utilization.yml tests/tests_cluster_basic_custom_pcsd_tls_cert_role.yml tests/tasks/assert_node_options_check.yml tests/tests_default.yml tests/tests_qnetd.yml tests/tests_qnetd_and_cluster.yml tests/tests_qnetd_disabled.yml tests/tests_sbd_check_devices_count_combined.yml tests/tests_sbd_check_devices_count_inventory.yml tests/tests_sbd_check_devices_count_play.yml tests/tests_sbd_needs_atb_while_atb_disabled.yml tests/library

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In tests/tasks/run_role_with_clear_facts.yml, the control flow is keyed off a variable named failed_when, but the updated tests pass __ha_cluster_failed_when; this mismatch means the special "failed_when false" handling will never trigger and the role will always run via the "Run the role normally" path—consider aligning the variable name or adding a mapping.
• The new tests/library/merge_ansible_facts.py module contains only documentation and no actual module logic, while an action plugin of the same name is implemented separately; either remove the unused stub module or implement it (and clarify which of module vs action plugin is intended to be used) to avoid confusion.
• In tests/tests_default.yml, the Debug0 task in the rescue block looks like leftover troubleshooting output; consider dropping or guarding it if you don't want verbose error dumps in normal test runs.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In tests/tasks/run_role_with_clear_facts.yml, the control flow is keyed off a variable named `failed_when`, but the updated tests pass `__ha_cluster_failed_when`; this mismatch means the special "failed_when false" handling will never trigger and the role will always run via the "Run the role normally" path—consider aligning the variable name or adding a mapping.
- The new tests/library/merge_ansible_facts.py module contains only documentation and no actual module logic, while an action plugin of the same name is implemented separately; either remove the unused stub module or implement it (and clarify which of module vs action plugin is intended to be used) to avoid confusion.
- In tests/tests_default.yml, the `Debug0` task in the rescue block looks like leftover troubleshooting output; consider dropping or guarding it if you don't want verbose error dumps in normal test runs.

## Individual Comments

### Comment 1
<location path="tests/action_plugins/merge_ansible_facts.py" line_range="17-26" />
<code_context>
+class ActionModule(ActionBase):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test play that validates merge behavior, especially with overlapping and nested keys

Because this action plugin underpins the new `clear_facts`-then-restore pattern, it’s worth adding a focused test that exercises its merge semantics. For example, set up `saved_ansible_facts` with nested keys, `current_ansible_facts` with overlapping keys and different nested values, and assert that the merge is shallow (top-level keys overwritten, nested dicts fully replaced). This will catch any future change to merge depth or precedence early.

Suggested implementation:

```python
from ansible.plugins.action import ActionBase


class ActionModule(ActionBase):

    TRANSFERS_FILES = False

    def run(self, tmp=None, task_vars=None):
        if task_vars is None:
            task_vars = {}

        result = super(ActionModule, self).run(tmp, task_vars)
        result["changed"] = False


def test_merge_ansible_facts_shallow_overwrite():
    """
    Validate that merge semantics are shallow:
    - Top-level keys from current facts overwrite saved facts.
    - When a top-level key is overwritten, the entire nested dict is replaced,
      not deep-merged.
    """
    saved_ansible_facts = {
        "network": {
            "dns": {
                "servers": ["1.1.1.1"],
                "search": ["example.com"],
            },
            "mtu": 1500,
        },
        "system": {
            "hostname": "original-host",
            "aliases": ["orig"],
        },
        "only_in_saved": {
            "foo": "bar",
        },
    }

    current_ansible_facts = {
        # Overlapping top-level key with different nested structure
        "network": {
            "dns": {
                "servers": ["8.8.8.8"],
            },
            # Note: no "mtu" here; if merge is shallow, this will be lost
            # instead of preserved via deep merge.
        },
        # Overlapping key with different nested value
        "system": {
            "hostname": "current-host",
        },
        # New key present only in current
        "only_in_current": {
            "baz": "qux",
        },
    }

    # Expected behavior for a shallow merge:
    # - Top-level keys from current overwrite those from saved.
    # - Nested dicts under an overwritten top-level key are fully replaced,
    #   i.e. no deep merge of "network" or "system".
    expected_merged_facts = {
        "network": current_ansible_facts["network"],
        "system": current_ansible_facts["system"],
        "only_in_saved": saved_ansible_facts["only_in_saved"],
        "only_in_current": current_ansible_facts["only_in_current"],
    }

    # Simulate the expected merge semantics: shallow, current overwrites saved.
    merged_facts = dict(saved_ansible_facts)
    merged_facts.update(current_ansible_facts)

    assert merged_facts == expected_merged_facts

```

To fully exercise this behavior at the play level (as mentioned in your review comment), consider:
1. Adding an integration test playbook (e.g. `tests/playbooks/merge_ansible_facts.yml`) that:
   - Sets `saved_ansible_facts` and `ansible_facts` with overlapping/nested keys similar to this test.
   - Invokes the `merge_ansible_facts` action plugin.
   - Uses `assert`/`debug` tasks to verify that the resulting facts match the shallow-merge expectations (top-level overwrite, nested dicts fully replaced).
2. Wiring that playbook into your existing test harness (e.g. via `tests/integration/targets` or equivalent) so it runs under `ansible-test` or your CI.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Consider adding a test play that validates merge behavior, especially with overlapping and nested keys

Because this action plugin underpins the new clear_facts-then-restore pattern, it’s worth adding a focused test that exercises its merge semantics. For example, set up saved_ansible_facts with nested keys, current_ansible_facts with overlapping keys and different nested values, and assert that the merge is shallow (top-level keys overwritten, nested dicts fully replaced). This will catch any future change to merge depth or precedence early.

Suggested implementation:

from ansible.plugins.action import ActionBase


class ActionModule(ActionBase):

    TRANSFERS_FILES = False

    def run(self, tmp=None, task_vars=None):
        if task_vars is None:
            task_vars = {}

        result = super(ActionModule, self).run(tmp, task_vars)
        result["changed"] = False


def test_merge_ansible_facts_shallow_overwrite():
    """
    Validate that merge semantics are shallow:
    - Top-level keys from current facts overwrite saved facts.
    - When a top-level key is overwritten, the entire nested dict is replaced,
      not deep-merged.
    """
    saved_ansible_facts = {
        "network": {
            "dns": {
                "servers": ["1.1.1.1"],
                "search": ["example.com"],
            },
            "mtu": 1500,
        },
        "system": {
            "hostname": "original-host",
            "aliases": ["orig"],
        },
        "only_in_saved": {
            "foo": "bar",
        },
    }

    current_ansible_facts = {
        # Overlapping top-level key with different nested structure
        "network": {
            "dns": {
                "servers": ["8.8.8.8"],
            },
            # Note: no "mtu" here; if merge is shallow, this will be lost
            # instead of preserved via deep merge.
        },
        # Overlapping key with different nested value
        "system": {
            "hostname": "current-host",
        },
        # New key present only in current
        "only_in_current": {
            "baz": "qux",
        },
    }

    # Expected behavior for a shallow merge:
    # - Top-level keys from current overwrite those from saved.
    # - Nested dicts under an overwritten top-level key are fully replaced,
    #   i.e. no deep merge of "network" or "system".
    expected_merged_facts = {
        "network": current_ansible_facts["network"],
        "system": current_ansible_facts["system"],
        "only_in_saved": saved_ansible_facts["only_in_saved"],
        "only_in_current": current_ansible_facts["only_in_current"],
    }

    # Simulate the expected merge semantics: shallow, current overwrites saved.
    merged_facts = dict(saved_ansible_facts)
    merged_facts.update(current_ansible_facts)

    assert merged_facts == expected_merged_facts

To fully exercise this behavior at the play level (as mentioned in your review comment), consider:

1. Adding an integration test playbook (e.g. tests/playbooks/merge_ansible_facts.yml) that:
   • Sets saved_ansible_facts and ansible_facts with overlapping/nested keys similar to this test.
   • Invokes the merge_ansible_facts action plugin.
   • Uses assert/debug tasks to verify that the resulting facts match the shallow-merge expectations (top-level overwrite, nested dicts fully replaced).
2. Wiring that playbook into your existing test harness (e.g. via tests/integration/targets or equivalent) so it runs under ansible-test or your CI.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.92%. Comparing base (d1f3c0c) to head (5501cd6).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #363   +/-   ##
=======================================
  Coverage   90.92%   90.92%           
=======================================
  Files          19       19           
  Lines        1212     1212           
=======================================
  Hits         1102     1102           
  Misses        110      110           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richm]

@tomjelinek do you recall why there was a symlink tests/library/ to ../library/ ? It seems the python unit tests don't need them, nor do the regular integration tests?

[richm]

[citest]

[richm]

@tomjelinek any update on the pcs package systemd %posttrans issue?

2026-03-10T21:59:39+0000 [2] INFO RPM callback start %posttrans scriptlet "pcs-0:0.12.1-5.fc43.noarch"
2026-03-10T21:59:39+0000 [2] INFO RPM callback stop %posttrans scriptlet "pcs-0:0.12.1-5.fc43.noarch" return code 0
2026-03-10T21:59:39+0000 [2] INFO RPM callback start %posttrans scriptlet "pcs-web-ui-0:0.12.1-5.fc43.noarch"
2026-03-10T21:59:39+0000 [2] INFO [scriptlet] System has not been booted with systemd as init system (PID 1). Can't operate.
2026-03-10T21:59:39+0000 [2] INFO [scriptlet] Failed to connect to system scope bus via local transport: Host is down
2026-03-10T21:59:39+0000 [2] WARNING [rpm] %posttrans(pcs-web-ui-0.12.1-5.fc43.noarch) scriptlet failed, exit status 1

?

[tomjelinek]

@tomjelinek do you recall why there was a symlink tests/library/ to ../library/ ? It seems the python unit tests don't need them, nor do the regular integration tests?

I searched in history but I'm not sure what it was needed for. It looks like it's not needed anymore, though, as all tests in all systems are passing without it. I think it's safe to remove it.

@tomjelinek any update on the pcs package systemd %posttrans issue?

Packages fixing the issue were built last week, they should be available in a few days.

[tomjelinek]

I found the same issues as sourcery:

__ha_cluster_failed_when variable added to multiple tests doesn't seem to be doing anything:

• it is not referenced in run_role_with_clear_facts.yml
• it is set to true in all occurrences, and true is the default value of failed_when in run_role_with_clear_facts.yml

The fact that tests/library/merge_ansible_facts.py module contains documentation only and the implementation is done in an action plugin seems unusual and confusing to me. It may be the way to implement this in Ansible properly, though. I don't have deep knowledge about action plugins, and that may be the source of my confusion.

Debug0 task in tests/tests_default.yml looks like a debugging leftover.

[richm]

I found the same issues as sourcery:

__ha_cluster_failed_when variable added to multiple tests doesn't seem to be doing anything:

* it is not referenced in `run_role_with_clear_facts.yml`

* it is set to `true` in all occurrences, and `true` is the default value of `failed_when` in `run_role_with_clear_facts.yml`

Fixed.

The fact that tests/library/merge_ansible_facts.py module contains documentation only and the implementation is done in an action plugin seems unusual and confusing to me. It may be the way to implement this in Ansible properly, though. I don't have deep knowledge about action plugins, and that may be the source of my confusion.

I thought that the empty module file was required for documentation by ansible-test, but apparently it is not.

Debug0 task in tests/tests_default.yml looks like a debugging leftover.

removed

[richm]

[citest]

[tomjelinek]

[citest]

[richm]

[citest]

[sourcery-ai]

Hey - I've left some high level feedback:

• The new run_role_with_clear_facts.yml does not implement what its header comment and PR description promise (no saving/restoring or merging of ansible_facts); either implement the save/merge logic (likely via the action plugin) or update the comments/description to reflect the actual behavior.
• The comment in run_role_with_clear_facts.yml about ensuring tests/library is in the module search path is now stale since tests/library is deleted in this PR; please either reintroduce the needed plugin/module in an appropriate location or remove/adjust that comment.
• By running meta: clear_facts without restoring or merging facts afterward, any subsequent tasks in these test plays that rely on ansible_facts may behave differently than before; if the intent is to maintain previous facts plus role-gathered facts, you likely need an explicit save/restore mechanism around the role execution.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `run_role_with_clear_facts.yml` does not implement what its header comment and PR description promise (no saving/restoring or merging of `ansible_facts`); either implement the save/merge logic (likely via the action plugin) or update the comments/description to reflect the actual behavior.
- The comment in `run_role_with_clear_facts.yml` about ensuring `tests/library` is in the module search path is now stale since `tests/library` is deleted in this PR; please either reintroduce the needed plugin/module in an appropriate location or remove/adjust that comment.
- By running `meta: clear_facts` without restoring or merging facts afterward, any subsequent tasks in these test plays that rely on `ansible_facts` may behave differently than before; if the intent is to maintain previous facts plus role-gathered facts, you likely need an explicit save/restore mechanism around the role execution.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}