feat: Add argument spec validation to Image_builder role - #5
Conversation
📝 WalkthroughWalkthroughThe role now defines public ChangesImage Builder validation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tasks/assert_role_vars.yml`:
- Around line 6-7: Update the ib_packages validation assertion to require every
item in list-valued ib_packages to be a string, while preserving validation of
scalar package names against __ib_package_types. Add a regression test covering
lists containing non-string values such as 123 or {} and ensure those inputs are
rejected.
In `@tests/tests_invalid_input.yml`:
- Around line 7-10: Update both ansible_version.full gates in
tests/tests_invalid_input.yml to require version 2.11 or newer, preserving the
invalid-input validation flow. Document the ansible-core 2.11 requirement in
README.md at the referenced location.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe9fbdf6-7986-483f-8a2e-004de78f143e
📒 Files selected for processing (13)
.gitignoreREADME.mddefaults/main.ymlmeta/argument_specs.ymltasks/assert_role_vars.ymltasks/main.ymltests/roles/linux-system-roles.image_builder/defaultstests/roles/linux-system-roles.image_builder/handlerstests/roles/linux-system-roles.image_builder/metatests/roles/linux-system-roles.image_builder/taskstests/roles/linux-system-roles.image_builder/varstests/tests_invalid_input.ymltox.ini
| - ib_packages is string and ib_packages in __ib_package_types | ||
| or ib_packages is not string and ib_packages | type_debug == 'list' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(tasks/assert_role_vars\.yml|meta/argument_specs\.yml|tests?/|molecule/)' | head -200
printf '%s\n' '--- assertion file ---'
if [ -f tasks/assert_role_vars.yml ]; then
nl -ba tasks/assert_role_vars.yml | sed -n '1,140p'
fi
printf '%s\n' '--- argument specifications and references ---'
rg -n -C 4 'ib_packages|__ib_package_types|assert_role_vars' . \
-g '!*.md' -g '!*.json' -g '!*.lock' | head -300Repository: linux-system-roles/image_builder
Length of output: 14209
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- assertion task ---'
awk '{printf "%5d %s\n", NR, $0}' tasks/assert_role_vars.yml
printf '%s\n' '--- argument specification ---'
awk '{printf "%5d %s\n", NR, $0}' meta/argument_specs.yml
printf '%s\n' '--- invalid-input tests ---'
awk '{printf "%5d %s\n", NR, $0}' tests/tests_invalid_input.yml
printf '%s\n' '--- package task consumers ---'
for f in tasks/setup-yum.yml tasks/setup-dnf.yml tasks/setup-default.yml tasks/main.yml; do
printf '%s\n' "--- $f ---"
awk '{printf "%5d %s\n", NR, $0}' "$f"
done
printf '%s\n' '--- available local runtimes/modules ---'
command -v ansible-playbook || true
python3 - <<'PY'
try:
import ansible
print("ansible python module:", ansible.__version__)
except Exception as exc:
print("ansible python module: unavailable:", type(exc).__name__)
PYRepository: linux-system-roles/image_builder
Length of output: 7387
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def ansible_like_type_debug(value):
if isinstance(value, bool):
return "bool"
if isinstance(value, str):
return "str"
if isinstance(value, list):
return "list"
if isinstance(value, dict):
return "dict"
if isinstance(value, int):
return "int"
if value is None:
return "NoneType"
return type(value).__name__
def accepts(value, package_types=("minimal", "default", "full")):
is_string = isinstance(value, str)
return (
(is_string and value in package_types)
or (not is_string and ansible_like_type_debug(value) == "list")
)
cases = {
"minimal": "minimal",
"unknown tier": "not_a_valid_tier",
"integer": 123,
"list of strings": ["vim", "git"],
"list with integer": [123],
"list with mapping": [{}],
"empty list": [],
}
for label, value in cases.items():
print(f"{label}: accepted={accepts(value)} type_debug={ansible_like_type_debug(value)}")
PY
printf '%s\n' '--- all ib_packages references and list-oriented tests ---'
rg -n -C 3 'ib_packages' . -g '!*.md' -g '!*.json' -g '!*.lock' | head -400Repository: linux-system-roles/image_builder
Length of output: 12593
Validate each explicit package name.
The list branch checks only the outer type, so [123] and [{}] pass. meta/argument_specs.yml uses type: raw, so it performs no element validation. Reject non-string list items and add a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tasks/assert_role_vars.yml` around lines 6 - 7, Update the ib_packages
validation assertion to require every item in list-valued ib_packages to be a
string, while preserving validation of scalar package names against
__ib_package_types. Add a regression test covering lists containing non-string
values such as 123 or {} and ensure those inputs are rejected.
Source: MCP tools
| # Role argument validation requires Ansible 2.10+; skipped on older versions. | ||
|
|
||
| - name: Argument specs reject invalid ib_daemon | ||
| when: ansible_version.full is version('2.10', '>=') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What ansible-core version introduced role argument specifications and the include_role rolespec_validate option?
💡 Result:
Role argument specifications and the include_role rolespec_validate option were both introduced in ansible-core 2.11 [1][2]. This functionality allows users to define an argument specification in a role's meta/argument_specs.yml file [1][3]. When enabled via the rolespec_validate option in an include_role task, Ansible will validate the parameters provided to the role against this specification, causing the role to fail execution if the parameters do not conform [1][2]. Documentation also notes that the ansible.builtin.validate_argument_spec module, which is used to validate these role arguments, was introduced in the same version [4][5].
Citations:
- 1: https://docs.ansible.com/projects/ansible/latest/playbook_guide/playbooks_reuse_roles.html
- 2: https://docs.ansible.com/projects/ansible-core/2.20/collections/ansible/builtin/include%5Frole%5Fmodule.html
- 3: https://docs.ansible.com/projects/ansible-core/devel/playbook_guide/playbooks_reuse_roles.html
- 4: https://docs.ansible.com/projects/ansible/latest/collections/ansible/builtin/validate_argument_spec_module.html
- 5: https://docs.ansible.com/projects/ansible-core/devel/collections/ansible/builtin/validate_argument_spec_module.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- tests/tests_invalid_input.yml ---'
cat -n tests/tests_invalid_input.yml | sed -n '1,80p'
printf '%s\n' '--- README.md ---'
cat -n README.md | sed -n '1,35p'
printf '%s\n' '--- argument-spec and validation references ---'
rg -n -C 3 'argument_specs|rolespec_validate|rescue|invalid ib_daemon|ansible_version' . \
-g '!*.pyc' -g '!node_modules' -g '!dist' -g '!build'Repository: linux-system-roles/image_builder
Length of output: 9157
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- role files ---'
git ls-files | rg '(^|/)(tasks|meta)/|tests|README.md'
printf '%s\n' '--- role task structure ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline tasks --lang yaml || true
ast-grep outline meta/argument_specs.yml --lang yaml || true
fi
printf '%s\n' '--- relevant role logic ---'
rg -n -C 5 'ib_daemon|not_a_valid_daemon|assert_role_vars|argument_specs|include_role' \
tasks meta tests README.mdRepository: linux-system-roles/image_builder
Length of output: 9272
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
test = Path("tests/tests_invalid_input.yml").read_text()
main = Path("tasks/main.yml").read_text()
args = Path("meta/argument_specs.yml").read_text()
readme = Path("README.md").read_text()
gates = re.findall(r"ansible_version\.full is version\('([^']+)', '>= '\)|ansible_version\.full is version\('([^']+)', '>='\)", test)
print("argument-spec test gates:", re.findall(r"ansible_version\.full is version\('([^']+)', '>='\)", test))
print("README validation statement:", next(
(line.strip() for line in readme.splitlines() if "meta/argument_specs.yml" in line),
"<missing>",
))
print("argument-spec declares ib_daemon choices:", bool(
re.search(r"ib_daemon:.*?choices:", args, re.S)
))
print("role task order:")
for name in ("include_tasks: assert_role_vars.yml", 'service:', 'name: "{{ ib_daemon }}"'):
print(f" {name!r}:", main.find(name))
print("invalid daemon is passed to include_role:", "ib_daemon: not_a_valid_daemon" in test)
assert re.findall(r"ansible_version\.full is version\('([^']+)', '>='\)", test) == ["2.10", "2.10"]
assert main.find("include_tasks: assert_role_vars.yml") < main.find('name: "{{ ib_daemon }}"')
PYRepository: linux-system-roles/image_builder
Length of output: 502
Require ansible-core 2.11 for role argument-spec validation.
Change both version gates in tests/tests_invalid_input.yml from 2.10 to 2.11. Otherwise, the rescue block can treat a later role failure as argument-spec validation. State the ansible-core 2.11 requirement in README.md.
📍 Affects 2 files
tests/tests_invalid_input.yml#L7-L10(this comment)README.md#L15-L15
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tests_invalid_input.yml` around lines 7 - 10, Update both
ansible_version.full gates in tests/tests_invalid_input.yml to require version
2.11 or newer, preserving the invalid-input validation flow. Document the
ansible-core 2.11 requirement in README.md at the referenced location.
1 participant
Enhancement: Added argument spec validation to image builder role. Also added separate value assertion and wrote tests for both methods.
Reason: Because it is a good addition to the linux-system-roles project.
Result: Successfully added it and prepared tests for it. I used AI during this implementation.
Issue Tracker Tickets (Jira or BZ if any): linux-system-roles/postfix#206 https://redhat.atlassian.net/browse/RHELMISC-16008
Summary by CodeRabbit
New Features
Documentation
Tests
Chores