refactor: support Ansible 2.19#794
Conversation
Reviewer's GuideThis PR refactors the existing test playbooks to fully support Ansible 2.19 by replacing deprecated import_playbook/import_role patterns with include_tasks/include_role calls, reorganizing task playbooks into a dedicated tasks directory, removing redundant set_fact usage for already-declared vars, and adding a targeted workaround for a known Ansible warnings bug. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @richm - I've reviewed your changes - here's some feedback:
- The repeated cleanup and assertion include_tasks patterns across test playbooks could be abstracted into a shared task list or role to reduce duplication.
- The bug workaround in assert_output_in_stderr_without_warnings.yml relies on the exact plugin error text; consider using a regex or more flexible match to guard against upstream message changes.
- Multiple #FIXME annotations indicate missing assertions (e.g., verifying interface down/up); please address these or file follow-up issues to avoid gaps in test coverage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated cleanup and assertion include_tasks patterns across test playbooks could be abstracted into a shared task list or role to reduce duplication.
- The bug workaround in assert_output_in_stderr_without_warnings.yml relies on the exact plugin error text; consider using a regex or more flexible match to guard against upstream message changes.
- Multiple #FIXME annotations indicate missing assertions (e.g., verifying interface down/up); please address these or file follow-up issues to avoid gaps in test coverage.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 #794 +/- ##
==========================================
+ Coverage 43.11% 43.25% +0.13%
==========================================
Files 12 12
Lines 3124 3121 -3
==========================================
+ Hits 1347 1350 +3
+ Misses 1777 1771 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
[citest] |
|
[citest] |
The big problem was trying to use `vars` with `import_playbook`. We do not need to use `import_playbook` when `include_tasks` will work. Perhaps the original author of these tests thought that the play `roles` keyword was the only way to invoke roles, so that had to be "called" using an `import_playbook`? Use `include_tasks` instead of `import_playbook`, and move some of those "tasks" playbooks to be tasks files in tests/tasks. Use `include_role` instead of `import_role`. Do not set variables using `set_fact` if they have already been set at the appropriate scope using `vars`. "Modernize" the code somewhat. Improve formatting. Work around an Ansible bug ansible/ansible#85394 Fix ansible-lint and ansible-test issues related newer versions of those tools. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
|
[citest] |
The big problem was trying to use
varswithimport_playbook.We do not need to use
import_playbookwheninclude_taskswillwork. Perhaps the original author of these tests thought that
the play
roleskeyword was the only way to invoke roles, sothat had to be "called" using an
import_playbook?Use
include_tasksinstead ofimport_playbook, and move someof those "tasks" playbooks to be tasks files in tests/tasks.
Use
include_roleinstead ofimport_role.Do not set variables using
set_factif they have already beenset at the appropriate scope using
vars."Modernize" the code somewhat.
Improve formatting.
Work around an Ansible bug ansible/ansible#85394
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Refactor the test suite to support Ansible 2.19 by replacing deprecated import_* directives with include_tasks/include_role, removing redundant set_fact usage, reorganizing task files, and filtering expected plugin warnings to work around Ansible issue #85394.
Bug Fixes:
Enhancements:
Tests: