[container_bench] fix: resolve ansible-lint issues #858
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Skipping CI for Draft Pull Request. |
WalkthroughRenames the public variable Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Playbook
participant pull_image as pull_image.yml
participant Registry
Playbook->>pull_image: call pull_image (tarfile_exists?)
alt tarfile_exists is true
Note right of pull_image `#E6F2FF`: Windows tar-load branch removed
pull_image->>Registry: load image from tar (Unix/macOS path)
Registry-->>pull_image: image loaded
else tarfile not present
pull_image->>Registry: pull image from registry
Registry-->>pull_image: image pulled
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/ansible-lint.yml(1 hunks)projects/container_bench/toolbox/container_bench_exec_benchmark/tasks/main.yml(1 hunks)projects/container_bench/toolbox/container_bench_helloworld_benchmark/tasks/main.yml(1 hunks)projects/container_bench/toolbox/container_bench_image_build_large_build_context_benchmark/tasks/main.yaml(1 hunks)projects/container_bench/toolbox/shared_tasks/map_variables.yml(1 hunks)projects/container_bench/toolbox/shared_tasks/pull_image.yml(0 hunks)projects/crc-timing/toolbox/crc_timing_refresh_image/tasks/main.yml(2 hunks)
💤 Files with no reviewable changes (1)
- projects/container_bench/toolbox/shared_tasks/pull_image.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/container_bench/toolbox/container_bench_helloworld_benchmark/tasks/main.yml
- projects/container_bench/toolbox/container_bench_image_build_large_build_context_benchmark/tasks/main.yaml
🔇 Additional comments (4)
projects/container_bench/toolbox/container_bench_exec_benchmark/tasks/main.yml (1)
11-14: Named task with explicit variable passing looks good.The refactored approach with a named task and explicit
benchmark_role_namevariable is cleaner and more maintainable than the previous pattern. The variable value correctly matches the role identifier.projects/container_bench/toolbox/shared_tasks/map_variables.yml (1)
6-13: Variable renaming is consistent and correctly applied.All variable references have been updated from
role_nametobenchmark_role_name. The dynamic key construction pattern (e.g.,vars[benchmark_role_name + '_binary_path']) is correct and will properly map to the caller's prefixed variables (e.g.,container_bench_exec_benchmark_binary_path)..github/workflows/ansible-lint.yml (2)
35-58: Simplified configuration is cleaner and more maintainable.The removal of redundant
with:blocks (targets, override-deps, etc.) in favor of the explicitargs: "-c ansible-config/ansible-lint.yml"approach is a good improvement. This makes the workflow more straightforward and easier to maintain.
37-37: v25.9.2 is a stable and current ansible-lint release.Version v25.9.2 is an official stable release published Oct 8, 2025, and is the current release on PyPI and in the repository as of Nov 4, 2025. The version pin to v25.9.2 provides reproducibility while ensuring a stable, tested release.
| ] | ||
| EOF | ||
| when: crc_timing_refresh_image_disk_size is not None | ||
| when: crc_timing_refresh_image_disk_size != None |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that != None comparison resolves the ansible-lint violations.
The changes replace Jinja2 test operators (is not None) with comparison operators (!= None). While both syntaxes work, Jinja2 idiomatically prefers test operators like is not none (lowercase none) over direct comparisons to None.
Confirm that this change (!= None) is indeed what ansible-lint expects and that it resolves the specific lint rule violations. If ansible-lint recommends a different approach, it may be that lowercase is not none (the Jinja2 test operator) is actually the preferred style.
Also applies to: 111-111
🌐 Web query:
What does ansible-lint recommend for checking None values in Jinja2 conditionals and when clauses?
💡 Result:
Use the Jinja2 none test (is none / is not none) rather than comparing to None/ "None" or relying on truthiness. Also prefer checking defined/undefined when appropriate to avoid broken conditionals. [1][2][3]
References:
- ansible.builtin.none test (use "var is none" / "var is not none"). [1]
- Ansible broken-conditionals guidance (avoid truthy/non-boolean conditionals; use explicit tests). [2]
- ansible-lint docs / usage (lint enforces good Jinja2 spacing/tests and flags problematic conditionals). [3]
Replace != None comparisons with Jinja2 test operator is not none (lowercase).
The changes incorrectly replace is not None with != None, but ansible-lint recommends using Jinja2 test operators (is not none with lowercase none) rather than comparing to None. The current changes move away from idiomatic Jinja2 syntax instead of toward it.
Update both line 100 and line 111 to use is not none (lowercase none) instead:
- Line 100:
when: crc_timing_refresh_image_disk_size is not none - Line 111:
{% if crc_timing_refresh_image_disk_size is not none %}
🤖 Prompt for AI Agents
In projects/crc-timing/toolbox/crc_timing_refresh_image/tasks/main.yml around
lines 100 and 111, the code uses `!= None` which is not idiomatic Jinja2;
replace those comparisons with the Jinja2 test operator using lowercase `none`.
Change the when at line 100 to use `is not none` and update the Jinja2
conditional at line 111 to `{% if crc_timing_refresh_image_disk_size is not none
%}`, ensuring both use the `is not none` form.
Signed-off-by: Jan Rodák <[email protected]>
|
thanks @Honny1 , merging this |
2 participants
Summary by CodeRabbit