refactor: support ansible 2.19, new ansible-lint #96
Conversation
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
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 ``` 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>
Reviewer's GuideRefactored in-place Jinja mutations to use Ansible 2.19–compliant filters and combine operations for list and dict construction, and updated test task names to satisfy the new ansible-lint requirements. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
[citest] |
![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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tasks/manage_user_info.yml:42` </location>
<code_context>
- {% set _ = rv.append(newitem) %}
- {% endfor %}
- {{ rv }}
+ __systemd_dict_list: "{{ __systemd_dict_list +
+ [newitem | combine(newstate, newuser, newgroup, newxdg_dir, newunits_dir, newmode, newdir_mode)] }}"
+ loop: "{{ __systemd_list }}"
</code_context>
<issue_to_address>
Appending to a fact in a loop can lead to race conditions or unexpected results in Ansible.
Appending to a list with set_fact inside a loop can cause lost updates or only the last value being set. Use a map or accumulate values, then set_fact once after the loop.
</issue_to_address>
### Comment 2
<location> `tasks/manage_user_info.yml:53` </location>
<code_context>
+ newuser:
+ user: "{{ item['user'] if item is mapping and 'user' in item
+ else 'root' }}"
+ newgroup:
+ group: "{{ item['group'] if item is mapping and 'group' in item
+ else __systemd_user_info[newuser['user']]['group'] }}"
+ newxdg_dir:
</code_context>
<issue_to_address>
Accessing __systemd_user_info[newuser['user']] assumes the user exists, which may not always be true.
Handle the case where newuser['user'] is not in __systemd_user_info to prevent errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
====================================
Coverage ? 0
====================================
Files ? 0
Lines ? 0
Branches ? 0
====================================
Hits ? 0
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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:Similarly for
intvalues - you cannot rely on0being evaluated as falseand 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
Adapt role tasks to comply with Ansible 2.19’s immutable data structures by replacing in-place Jinja mutations with filter-based constructions and update tests to pass new ansible-lint checks
Enhancements:
Tests: