feat: Add argument spec validation to template role - #217
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesThe role now defines additional defaults and an argument specification covering scalar, path, state, list, raw, and nested service options. README variable documentation and example snippets were expanded to reflect the new inputs and validation source. Role argument validation
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
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: 1
🤖 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 `@meta/argument_specs.yml`:
- Around line 7-8: Update the template_foo argument definition in the argument
specifications to explicitly set required: true, ensuring validation rejects
calls that omit it while preserving its existing string type.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 768eb4de-3413-41f9-aa47-a4a05d4decaa
📒 Files selected for processing (2)
README.mdmeta/argument_specs.yml
| template_foo: | ||
| type: str |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,120p'
echo "== argument_specs snippet =="
if [ -f meta/argument_specs.yml ]; then
cat -n meta/argument_specs.yml | sed -n '1,120p'
fi
echo "== README template_foo occurrences =="
if [ -f README.md ]; then
rg -n -C 3 'template_foo|required|role options|argument' README.md || true
fi
echo "== argument_specs occurrences all files =="
rg -n 'template_foo|required:|argument_specs|argspec|ARG_SPEC|ARGSPEC' . --glob '!*.pyc' --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' || trueRepository: linux-system-roles/template
Length of output: 4374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== defaults/main.yml =="
cat -n defaults/main.yml | sed -n '1,80p'
echo "== library/sr_fingerprint.py relevant =="
cat -n library/sr_fingerprint.py | sed -n '1,120p'
echo "== ansible arg validation behavior probe from docs if available =="
python3 - <<'PY'
# Inspect whether repository has local Ansible validation plugin/source; do not execute repo code.
import pathlib
deps = [pathlib.Path(p) for p in ['.ansible-test', 'requirements.txt', 'pyproject.toml', 'tox.ini']]
for p in deps:
print(f"{p}: {p.exists()}")
PYRepository: linux-system-roles/template
Length of output: 3572
🌐 Web query:
Ansible role argument_specs required: true omitted required validation default option
💡 Result:
When defining argument specifications in Ansible (for roles or playbooks), the settings required and default have specific behaviors and constraints: Mutually Exclusive In an argument specification, required: true and default are considered mutually exclusive [1][2]. You should not define a default value for an argument that is marked as required: true [1][3]. If an argument is required, it must be provided by the user, rendering a default value logically unnecessary [1]. Behavior of Required When required is omitted, the argument is treated as optional by default [4][5]. If required: true is set, Ansible will fail validation if the parameter is missing [6][4]. Behavior of Default The handling of default values differs significantly between Ansible modules and other contexts like role or play argument specifications [7]: 1. For Modules: The default value is used by the Ansible framework to populate the argument if it is omitted by the user, and the value is coerced to the specified type [7]. 2. For Roles and Plays: The default value is not automatically injected or applied to the variable [1][7]. In these contexts, default is primarily used for documentation purposes (e.g., when viewing documentation via ansible-doc) [7]. Ansible does not automatically assign this default value to your variable if it is missing [1][7]. In summary, if you mark a parameter as required: true, you should omit the default keyword entirely [1][3]. If you require a default value for a role or play variable, you must implement the logic for setting that default within your task or playbook (e.g., using the default() Jinja2 filter) because the argument_spec definition alone will not supply it [1][7].
Citations:
- 1: argument_spec: default value for dict entries do not work ansible/ansible#83287
- 2: Enhancing of validate_argument_spec documentaiton ansible/ansible#84164
- 3: https://docs.ansible.com/projects/ansible/8/dev%5Fguide/testing/sanity/validate-modules.html
- 4: https://docs.ansible.com/projects/ansible/latest/playbook_guide/playbooks_variables_validation.html
- 5: doc: explain default "required" value for argument_spec ansible/ansible#68678
- 6: https://docs.ansible.com/projects/ansible-core/devel/collections/ansible/builtin/validate_argument_spec_module.html
- 7: https://forum.ansible.com/t/are-module-role-and-play-argument-validation-the-same-argument-spec/45377
Enforce template_foo as required before relying on the documented API.
README.md states that template_foo has no default and is required, but meta/argument_specs.yml does not set required: true. Add it so validation rejects calls that omit the argument.
template_foo:
type: str
+ required: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template_foo: | |
| type: str | |
| template_foo: | |
| type: str | |
| required: true |
🤖 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 `@meta/argument_specs.yml` around lines 7 - 8, Update the template_foo argument
definition in the argument specifications to explicitly set required: true,
ensuring validation rejects calls that omit it while preserving its existing
string type.
There was a problem hiding this comment.
It's also fine to include such additional keywords like required:
There was a problem hiding this comment.
@spetrosi, thanks for confirming. required: true is a suitable addition here and keeps the argument specification aligned with the documented required input.
DonatSzabo
changed the title
DonatSzabo
changed the title
DonatSzabo
changed the title
spetrosi
left a comment
spetrosi
left a comment
There was a problem hiding this comment.
I suggest adding more variable types example so that it's easier for developers not familiar with argument_spec to fill it.
Including more complex examples like nested list where each list entry is a dict.
|
Ahh, okay. I wil do that. |
| A list of service configurations. Each entry is a dictionary | ||
| with its own set of options. Use this pattern when the role | ||
| takes a list of structured items. | ||
| options: |
There was a problem hiding this comment.
Nice, that's what I was talking about
| Name of the service. Mark a sub-option as required when | ||
| it must always be provided. | ||
| type: | ||
| type: str |
There was a problem hiding this comment.
I'd like the template to be explicit and required: always, regardless of whether it's true or false. Just for the sake of having the clearest and most readable high standards in the template role. For users who are not familiar with argument_spec it's not clear what missing required: keyword means.
2 participants
Enhancement: Added argument spec validation to template role.
Reason: Because it is a good addition to the linux-system-roles project.
Result: Successfully added it. Local tmt test also works.
Issue Tracker Tickets (Jira or BZ if any): linux-system-roles/postfix#206 https://redhat.atlassian.net/browse/RHELMISC-16008
Summary by CodeRabbit