Skip to content

Conversation

@richm
Copy link
Contributor

@richm richm commented Jun 17, 2025

Ansible 2.19 introduces some big changes
https://docs.ansible.com/ansible/devel/porting_guides/porting_guide_core_2.19.html

One big change is that data structures are no longer mutable by the use of python
methods such as __setitem__, setdefault, update, etc. in Jinja constructs.
Instead, items must use filters or other Jinja operations.

One common idiom is to mutate each element in a list. Since we cannot do this
"in-place" anymore, a common way to do this is:

- name: Construct a new list from an existing list and mutate each element
  set_fact:
    __new_list: "{{ __new_list | d([]) + [mutated_item] }}"
  loop: "{{ old_list }}"
  mutated_item: "{{ some value based on item from old list }}"

- name: Reset original old list
  set_fact:
    old_list: "{{ __new_list }}"

Similarly with dict items:

- name: Construct a new dict from an existing dict and mutate each element
  set_fact:
    __new_dict: "{{ __new_dict | d({}) | combine(mutated_item) }}"
  loop: "{{ old_dict | dict2items }}"
  mutated_item: "{{ {item.key: mutation of item.value} }}"

- name: Reset original old dict
  set_fact:
    old_dict: "{{ __new_dict }}"

Another big change is that a boolean expression in a when or similar construct
must be converted to a boolean - we cannot rely on the implicit evaluation in
a boolean context. For example, if var is some iterable, like a dict, list,
or string, you used to be able to evaluate an empty value in a boolean context:

when: var  # do this only if var is not empty

You now have to explicitly test for empty using length:

when: var | length > 0  # do this only if var is not empty

These are the biggest changes. See the porting guide for others.

Signed-off-by: Rich Megginson rmeggins@redhat.com

Summary by Sourcery

Refactor the VPN role’s tasks, templates, and tests to comply with Ansible 2.19 by eliminating in-place Jinja mutations and replacing them with filter-based list/dict constructions and explicit boolean checks.

Enhancements:

  • Replace Python data-structure methods in Jinja with set_fact and filters (combine, union) to build new lists and dicts (e.g. __vpn_connections_fixed, __vpn_psks, __vpn_policies).
  • Extract PSK pair generation into a separate vpn_get_psks_for_tunnel.yml include task using combinations filters.
  • Normalize mesh configuration tasks to set and use __vpn_current_ip and __vpn_current_subnet facts.
  • Convert implicit truthiness in when statements to explicit length or boolean tests.
  • Update templates to iterate over the new filtered variables (__vpn_connections_fixed, __vpn_policies) instead of mutating the original collections.

Tests:

  • Revise test playbooks to use __new_vpn_connections and fixup_hosts.yml tasks instead of inline mutations.
  • Align cleanup and defaults tests with refactored variable names and immutable patterns.

@richm richm requested a review from ueno as a code owner June 17, 2025 15:20
@sourcery-ai
Copy link

sourcery-ai bot commented Jun 17, 2025

Reviewer's Guide

This PR refactors the VPN role to comply with Ansible 2.19’s immutable data model by replacing all in-place Jinja mutations with accumulator patterns and filter-based transformations, modularizing the PSK generation into a separate task, converting implicit boolean checks into explicit length tests, and standardizing variable prefixes for clarity.

Sequence diagram for PSK generation modularization

sequenceDiagram
    participant MainTask as tasks/main.yml
    participant PSKTask as tasks/vpn_get_psks_for_tunnel.yml
    loop For each tunnel in __vpn_connections_fixed
        MainTask->>PSKTask: include_tasks: vpn_get_psks_for_tunnel.yml (with tunnel, tunnel_idx)
        PSKTask->>PSKTask: Reset __vpn_host_pairs
        PSKTask->>PSKTask: Generate host pairs and PSKs
        PSKTask->>PSKTask: Set __vpn_psks[tunnel_idx]
        PSKTask-->>MainTask: Update __vpn_psks
    end
Loading

Class diagram for VPN connection data structure refactor

classDiagram
    class VPNConnection {
        +auth_method
        +opportunistic
        +hosts
        +policies
        +shared_key_content
        +cert_name
    }
    class VPNConnectionsFixed {
        +List<VPNConnection>
    }
    class VPNPSKs {
        +List<HostPairPSK>
    }
    class HostPairPSK {
        +host_pairs: Tuple
        +pre_shared_key: String
    }
    VPNConnectionsFixed "1" -- "*" VPNConnection
    VPNPSKs "1" -- "*" HostPairPSK
    VPNConnection "1" -- "*" Policy
    class Policy {
        +policy
        +cidr
    }
Loading

File-Level Changes

Change Details Files
Replace in-place list/dict mutations in main task flow with accumulator and filter patterns
  • Initialize an empty accumulator variable
  • Accumulate mutated tunnel items using union/combine and list filters
  • Reset the original variable by assigning the accumulator
tasks/main.yml
Convert implicit boolean context checks into explicit length-based conditions
  • Introduce count_not_opps and count_has_hosts vars via selectattr/rejectattr
  • Use length > 0 tests in when clauses instead of relying on truthiness
  • Replace string-based failure flags with explicit boolean conditions
tasks/main.yml
Modularize PSK generation into its own task file
  • Reset PSK accumulator to a simple list
  • Include vpn_get_psks_for_tunnel.yml per tunnel via include_tasks
  • Generate host_pairs and pre_shared_key entries in the new task
tasks/main.yml
tasks/vpn_get_psks_for_tunnel.yml
Streamline host-to-host tunnel list computation with filter pipelines
  • Use rejectattr, selectattr, map, flatten, unique to build the tunnels list
  • Eliminate previous nested Jinja loops
  • Apply no_log where appropriate
tasks/main.yml
tests/tasks/cleanup.yml
Refactor mesh_conf.yml to consistently use prefixed facts and explicit loops
  • Prefix all intermediate facts with _vpn
  • Define block vars (pol_default, conn_policies) instead of long Jinja templates
  • Use product to build policy/node pairs and accumulate new policies
tasks/mesh_conf.yml
Update templates to reference the new accumulators and host_pairs structure
  • Loop over __vpn_connections_fixed instead of vpn_connections
  • Adjust PSK lookup to iterate host_pairs entries
  • Loop over __vpn_policies in policy.j2
templates/libreswan-host-to-host.secrets.j2
templates/libreswan-host-to-host.conf.j2
templates/policy.j2
Refactor test setups to adopt the accumulator approach and new variable names
  • Introduce __new_vpn_connections accumulators across tests
  • Add fixup_hosts.yml to handle cert name assignments
  • Simplify add_hosts.yml using dict and map functions
tests/tests_host_to_host_cert.yml
tests/tests_mesh_cert.yml
tests/tests_host_to_host_psk_custom.yml
tests/tests_subnet_to_subnet.yml
tests/tasks/add_hosts.yml
tests/tasks/fixup_hosts.yml
tests/tests_defaults_vars.yml

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

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @richm - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@richm richm force-pushed the fix-ansible-2.19 branch from d5357e5 to b311154 Compare June 17, 2025 15:49
@richm
Copy link
Contributor Author

richm commented Jun 17, 2025

[citest]

@richm
Copy link
Contributor Author

richm commented Jun 17, 2025

hmm - installing collection dependencies from galaxy is timing out, but the script does not report that as an error

@richm
Copy link
Contributor Author

richm commented Jun 17, 2025

[citest]

Ansible 2.19 introduces some big changes
https://docs.ansible.com/ansible/devel/porting_guides/porting_guide_core_2.19.html

One big change is that data structures are no longer mutable by the use of python
methods such as `__setitem__`, `setdefault`, `update`, etc.  in Jinja constructs.
Instead, items must use filters or other Jinja operations.

One common idiom is to mutate each element in a list.  Since we cannot do this
"in-place" anymore, a common way to do this is:

```yaml
- name: Construct a new list from an existing list and mutate each element
  set_fact:
    __new_list: "{{ __new_list | d([]) + [mutated_item] }}"
  loop: "{{ old_list }}"
  mutated_item: "{{ some value based on item from old list }}"

- name: Reset original old list
  set_fact:
    old_list: "{{ __new_list }}"
```

Similarly with `dict` items:

```yaml
- name: Construct a new dict from an existing dict and mutate each element
  set_fact:
    __new_dict: "{{ __new_dict | d({}) | combine(mutated_item) }}"
  loop: "{{ old_dict | dict2items }}"
  mutated_item: "{{ {item.key: mutation of item.value} }}"

- name: Reset original old dict
  set_fact:
    old_dict: "{{ __new_dict }}"
```

Another big change is that a boolean expression in a `when` or similar construct
must be converted to a boolean - we cannot rely on the implicit evaluation in
a boolean context.  For example, if `var` is some iterable, like a `dict`, `list`,
or `string`, you used to be able to evaluate an empty value in a boolean context:

```yaml
when: var  # do this only if var is not empty
```

You now have to explicitly test for empty using `length`:

```yaml
when: var | length > 0  # do this only if var is not empty
```

These are the biggest changes.  See the porting guide for others.

Signed-off-by: Rich Megginson <rmeggins@redhat.com>
@richm richm force-pushed the fix-ansible-2.19 branch from b311154 to 42905cf Compare June 17, 2025 19:17
@richm
Copy link
Contributor Author

richm commented Jun 17, 2025

[citest]

@richm richm requested a review from spetrosi June 17, 2025 19:57
@richm
Copy link
Contributor Author

richm commented Jun 17, 2025

@spetrosi This one is pretty complicated - would appreciate it if you could take a look

@richm richm merged commit d74ad96 into linux-system-roles:main Jul 2, 2025
34 checks passed
@richm richm deleted the fix-ansible-2.19 branch July 2, 2025 20:42
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.

1 participant