refactor: Ansible 2.19 support

[richm]

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

Similarly for int values - you cannot rely on 0 being evaluated as false
and non-zero true - you must explicitly compare the values with == or !=

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

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

Summary by Sourcery

Refactor SSH role templates and task definitions to comply with Ansible 2.19’s porting requirements by removing mutable operations, enforcing explicit boolean evaluations, and converting multi-line conditionals into inline expressions.

Enhancements:

• Convert multi-line conditional blocks in tasks/main.yml to inline Jinja expressions using ternary operators and filters
• Replace mutable data structure manipulations with filter-based constructions for list and dict variables
• Enforce explicit "is not none" checks and boolean context conversions in ssh_config.j2 macros
• Simplify conditional rendering in Jinja templates by removing undefined variables and collapsing branches into concise expressions

[sourcery-ai]

Reviewer's Guide

Refactor SSH role tasks and templates to support Ansible 2.19’s stricter immutability and boolean evaluation by replacing multi-line Jinja conditionals with inline filters and ternary expressions, adding explicit "is not none" checks, and eliminating mutable constructs.

Class diagram for updated SSH role variable resolution

classDiagram
    class SSHRoleVars {
        +ssh_skip_defaults
        +ssh_user
        +ssh_drop_in_name
        +__ssh_supports_drop_in
        +ssh_config_file
        +ansible_facts
        +ssh_config_owner
        +ssh_config_group
        +ssh_config_mode
        +__ssh_skip_defaults
        +__ssh_config_file
        +__ssh_config_owner
        +__ssh_config_group
        +__ssh_config_mode
    }
    SSHRoleVars : __ssh_skip_defaults = ssh_skip_defaults if ssh_skip_defaults != 'auto' else (ssh_user is not none) or (ssh_drop_in_name is not none and __ssh_supports_drop_in)
    SSHRoleVars : __ssh_config_file = ssh_config_file if ssh_config_file is not none else ansible_facts['getent_passwd'][ssh_user][4] + '/.ssh/config' if ssh_user is not none and ansible_facts['getent_passwd'] is defined else __ssh_drop_in_template | replace('{name}', ssh_drop_in_name) if ssh_drop_in_name is not none and __ssh_supports_drop_in else '/etc/ssh/ssh_config'
    SSHRoleVars : __ssh_config_owner = ssh_config_owner if ssh_config_owner is not none else ssh_user if ssh_user is not none else 'root'
    SSHRoleVars : __ssh_config_group = ssh_config_group if ssh_config_group is not none else ssh_user if ssh_user is not none and ansible_facts['getent_passwd'] is defined else 'root'
    SSHRoleVars : __ssh_config_mode = ssh_config_mode if ssh_config_mode is not none else '0600' if ssh_user is not none else '0644'

Loading

Class diagram for updated ssh_config.j2 macros

classDiagram
    class ssh_config_j2 {
        +macro render_option(key, value, indent=false)
        +macro body_option(key, override)
    }
    ssh_config_j2 : render_option checks value is defined and not none
    ssh_config_j2 : render_option handles true, false, list, and string values
    ssh_config_j2 : body_option checks override, ssh[key], __ssh_defaults[key] for not none
    ssh_config_j2 : body_option calls render_option directly
    ssh_config_j2 --> ssh_config_j2 : render_option used by body_option

Loading

File-Level Changes

Change	Details	Files
Convert multi-line set_fact conditionals to inline Jinja2 expressions in tasks/main.yml	Replace the validate block with a single-line ternary expression Simplify __ssh_skip_defaults using nested inline condition and boolean filter Refactor __ssh_config_file to a chained inline if/else with string concatenation Condense owner, group, and mode vars into nested inline expressions	tasks/main.yml
Update ssh_config.j2 macros to use explicit truthiness and inline rendering	Add "value is not none" checks in render_option and skip none entries in loops Remove intermediate variable assignments in body_option and invoke render_option directly Streamline macro logic with inline expressions and early empty-string returns	templates/ssh_config.j2

---

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.

[richm]

@martinpitt this supersedes #190

The key was realizing that the the macro must return a value - the implicit return value is none which is then converted into the string None - the way to return an empty string is {{- '' -}} which looks like some sort of demented emoji

[sourcery-ai]

Hey @richm - I've reviewed your changes - here's some feedback:

• The inline nested ternary chains for __ssh_skip_defaults, __ssh_config_file, etc., are becoming hard to parse—consider breaking them into separate set_fact tasks or using multi-line Jinja if/elif blocks for better readability.
• In the render_option macro, you now guard against value is none, but you might also want to explicitly handle empty strings or zero values so they don’t get inadvertently skipped.
• You’ve quoted the file mode strings as '0600' and '0644'—please verify that Ansible interprets these as octal permissions and not as plain string literals.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- The inline nested ternary chains for __ssh_skip_defaults, __ssh_config_file, etc., are becoming hard to parse—consider breaking them into separate set_fact tasks or using multi-line Jinja if/elif blocks for better readability.
- In the render_option macro, you now guard against value is none, but you might also want to explicitly handle empty strings or zero values so they don’t get inadvertently skipped.
- You’ve quoted the file mode strings as '0600' and '0644'—please verify that Ansible interprets these as octal permissions and not as plain string literals.

## Individual Comments

### Comment 1
<location> `tasks/main.yml:79` </location>
<code_context>
+    __ssh_config_group: "{{ ssh_config_group if ssh_config_group is not none
+      else ssh_user if ssh_user is not none and ansible_facts['getent_passwd'] is defined
+      else 'root' }}"
+    __ssh_config_mode: "{{ ssh_config_mode if ssh_config_mode is not none
+      else '0600' if ssh_user is not none
+      else '0644' }}"
</code_context>

<issue_to_address>
The default mode values are now quoted as strings, which may differ from previous behavior.

Previously, mode values were integers (e.g., 600/644); now they are strings ('0600'/'0644'). Ensure downstream code handles the type change to avoid permission issues.
</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]

issue (bug_risk): The default mode values are now quoted as strings, which may differ from previous behavior.

Previously, mode values were integers (e.g., 600/644); now they are strings ('0600'/'0644'). Ensure downstream code handles the type change to avoid permission issues.

[richm]

[citest]

[martinpitt]

Thanks @richm for figuring this uot! The "return empty string" escaped me in my attempts, nor did the various LLMs know about it. Now the template change looks very sensible.

BTW, the {{- '' -}} emoji reminds me of a toad or a lobster looking at you!

I'm not the primary reviewer, just some comment from the peanut gallery.

[martinpitt]

LGTM now, cheers! However, leaving final review to @Jakuje

[Jakuje]

And we will likely need similar changes in the server role in https://github.com/willshersystems/ansible-sshd

[richm]

And we will likely need similar changes in the server role in https://github.com/willshersystems/ansible-sshd

Yes - there have already been ansible 2.19 issues reported there

[richm]

[citest]

[richm]

[citest]

[richm]

[citest]