refactor: support Ansible 2.19#261
Merged
Merged
Conversation
Reviewer's GuideThis PR refactors playbook tasks and templates to support Ansible 2.19’s immutable Jinja data structures and stricter boolean evaluations by replacing in-template mutations with set_fact/combine loops, introducing a Jinja macro for sections, and adding explicit length checks in conditionals. Entity relationship diagram for kernel settings data structureserDiagram
KERNEL_SETTINGS_PROFILE_CONTENTS ||--o{ SYSCTL : contains
KERNEL_SETTINGS_PROFILE_CONTENTS ||--o{ SYSFS : contains
KERNEL_SETTINGS_PROFILE_CONTENTS ||--o{ SYSTEMD : contains
KERNEL_SETTINGS_PROFILE_CONTENTS ||--o{ VM : contains
SYSCTL {
string name
value value
}
SYSFS {
string name
value value
}
SYSTEMD {
cpu_affinity value
}
VM {
transparent_hugepages value
transparent_hugepage_defrag value
}
Class diagram for Jinja data structure handling before and after Ansible 2.19classDiagram
class JinjaTemplateOld {
+__settings: dict
+__setitem__()
+setdefault()
+update()
+Mutates dicts/lists in-place
}
class JinjaTemplateNew {
+__kernel_settings_new_sysctl: dict
+__kernel_settings_new_sysfs: dict
+combine()
+No in-place mutation
+Uses set_fact and loops
}
JinjaTemplateOld <|-- JinjaTemplateNew : Refactored
Class diagram for kernel settings template macro and variablesclassDiagram
class KernelSettingsTemplate {
+write_section(section_name, settings)
+__sysctl_has_values: bool
+__sysfs_has_values: bool
+__systemd_new: value
+__trans_huge_new: value
+__trans_defrag_new: value
}
class Macro_write_section {
+section_name: str
+settings: dict
+Outputs INI section
}
KernelSettingsTemplate o-- Macro_write_section : uses
Flow diagram for sysctl/sysfs dict construction with set_fact and combineflowchart TD
A[Start: Load old sysctl/sysfs] --> B[Initialize new dict with old values]
B --> C[Loop over new items]
C --> D[Combine mutated item into new dict]
D --> E[Repeat for all items]
E --> F[Set new dict as final sysctl/sysfs]
F --> G[Apply kernel settings template]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @richm - I've reviewed your changes - here's some feedback:
- Consolidate the nearly identical set_fact loops for sysctl and sysfs into a shared include or iterate over both sections in a single loop to reduce duplication.
- In the Jinja2 template macro, values are rendered without quoting—consider wrapping
{{ val }}in quotes or using thequotefilter to avoid issues when values contain spaces or special characters. - Rather than checking
var | length > 0, you could usevar | boolorvar | default([]) | boolto make empty‐value tests more concise and intention-revealing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consolidate the nearly identical set_fact loops for sysctl and sysfs into a shared include or iterate over both sections in a single loop to reduce duplication.
- In the Jinja2 template macro, values are rendered without quoting—consider wrapping `{{ val }}` in quotes or using the `quote` filter to avoid issues when values contain spaces or special characters.
- Rather than checking `var | length > 0`, you could use `var | bool` or `var | default([]) | bool` to make empty‐value tests more concise and intention-revealing.
## Individual Comments
### Comment 1
<location> `templates/kernel_settings.j2:3` </location>
<code_context>
-{% if section[key] != __kernel_settings_state_absent %}
-{% if not seen_sections.__contains__(section_name) %}
-{% set _ = seen_sections.__setitem__(section_name, true) %}
+{% macro write_section(section_name, settings) %}
[{{ section_name }}]
-{% endif %}
-{{ key }} = {{ section[key] }}
+{% for key, val in settings.items() %}
+{% if val != {"state": "absent"} %}
+{{ key }} = {{ val }}
{% endif %}
{% endfor %}
-{% endfor %}
+{% endmacro %}
+[main]
+summary = kernel settings
</code_context>
<issue_to_address>
The macro write_section does not filter out keys with values that are empty strings or None.
Previously, keys with None or empty string values were omitted from the output. The current macro may now include these values. If this is unintended, please reintroduce the filtering for None and empty strings.
</issue_to_address>
### Comment 2
<location> `templates/kernel_settings.j2:6` </location>
<code_context>
-{% endif %}
-{{ key }} = {{ section[key] }}
+{% for key, val in settings.items() %}
+{% if val != {"state": "absent"} %}
+{{ key }} = {{ val }}
{% endif %}
</code_context>
<issue_to_address>
The check for val != {"state": "absent"} assumes no other dict values will be present.
If other dict values are possible, this check may behave unexpectedly. Make the check more robust or document the assumption clearly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
Author
|
[citest] |
1 similar comment
Collaborator
Author
|
[citest] |
spetrosi
reviewed
Jul 1, 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: ```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 ``` Signed-off-by: Rich Megginson <rmeggins@redhat.com>
Collaborator
Author
|
[citest] |
1 similar comment
Collaborator
Author
|
[citest] |
spetrosi
approved these changes
Jul 2, 2025
Collaborator
Author
|
[citest] |
1 similar comment
Collaborator
Author
|
[citest] |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #261 +/- ##
=========================================
- Coverage 72.66% 0 -72.67%
=========================================
Files 1 0 -1
Lines 450 0 -450
=========================================
- Hits 327 0 -327
+ Misses 123 0 -123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
2 participants
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.
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:
Similarly with
dictitems:Another big change is that a boolean expression in a
whenor similar constructmust be converted to a boolean - we cannot rely on the implicit evaluation in
a boolean context. For example, if
varis some iterable, like adict,list,or
string, you used to be able to evaluate an empty value in a boolean context:You now have to explicitly test for empty using
length:Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Refactor kernel settings role to support Ansible 2.19 by replacing in-place list/dict mutations with Jinja filters and set_fact loops, adding explicit boolean and length checks, and updating templates and tests accordingly.
Enhancements:
is not noneand| length > 0checks for boolean expressions in variable assignments and when conditionsTests:
is not noneand| length > 0in when conditions