feat: Add argument spec validation to template role - #222
Conversation
📝 WalkthroughWalkthroughChangesThe role adds defaults and argument specifications for new template inputs. The README documents these inputs and updates variable and playbook examples with numeric, path, state, package, raw, and service values. Role input contract
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 `@meta/argument_specs.yml`:
- Around line 25-29: Align template_config_path with the role’s actual
basename-based behavior: update its argument specification and description to
require or clearly document a basename rather than a filesystem path. Ensure the
validation or contract prevents absolute paths from being accepted, while
preserving the existing __template_foo_config template lookup and /etc/{{
__template_foo_config }} write behavior.
- Around line 57-86: Align the template_services schema with the
__template_services consumer: either normalize each dictionary into the service
name and enabled/type parameters before the loops in tasks/main.yml and
handlers/main.yml, updating service.name and enabled to use those values, or
simplify the schema and README to accept only plain service-name strings. Ensure
the chosen representation is consistent across argument validation and both
service task handlers.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cec6fec-f6bb-48b8-8dd2-87bd60a3530b
📒 Files selected for processing (3)
README.mddefaults/main.ymlmeta/argument_specs.yml
| template_config_path: | ||
| type: path | ||
| required: false | ||
| description: >- | ||
| Filesystem path to the configuration file. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
rg -n -C 6 'template_config_path|__template_foo_config' \
--glob '*.yml' --glob '*.yaml' . || trueRepository: linux-system-roles/template
Length of output: 2823
🏁 Script executed:
#!/usr/bin/env bash
set -u
echo "== tracked yml files =="
git ls-files '*.yml' '*.yaml' | sed -n '1,120p'
echo
echo "== variable references =="
rg -n -C 3 'template_config_path|__template_foo_config|template_foo|__template' \
--glob '*.yml' --glob '*.yaml' . || true
echo
echo "== tasks/main.yml =="
cat -n tasks/main.yml
echo
echo "== defaults/main.yml =="
cat -n defaults/main.yml
echo
echo "== vars/main.yml =="
cat -n vars/main.yml
echo
echo "== other main files =="
fd -a 'main\.ya?ml$' . | sed -n '1,80p'Repository: linux-system-roles/template
Length of output: 16112
🌐 Web query:
Ansible file mode path type normalize paths argument_specs path type
💡 Result:
In Ansible, the type 'path' is a specific attribute used within a module's argument_spec to manage filesystem paths [1][2]. When an argument is defined with type='path', Ansible automatically processes the input value by ensuring it is a string, and then expanding any shell variables and tilde characters (e.g., ~) [1][3][4]. This validation is handled by the check_type_path function within the module_utils validation framework [3][2]. While type='path' performs expansion, it does not explicitly guarantee normalization (such as removing redundant separators or resolving up-level references like '..') as part of the initial argument validation [1][5]. For explicit normalization of paths in Ansible, you should use the ansible.builtin.normpath filter in your tasks, which explicitly collapses redundant separators and up-level references [5]. Summary of Key Points: - Role of type='path': Ensures the value is a string and expands ~ and shell environment variables [1][2]. - Implementation: It is defined within the argument_spec dictionary in module code [3][6]. - Normalization: If formal normalization (collapsing separators/up-level refs) is required, apply the ansible.builtin.normpath filter to the path variable [5].
Citations:
- 1: https://docs.ansible.com/projects/ansible/latest/dev_guide/testing/sanity/use-argspec-type-path.html
- 2: https://deepwiki.com/ansible/ansible/5.2-argument-specification-and-validation
- 3: https://docs.ansible.com/projects/ansible/13/reference_appendices/module_utils.html
- 4: https://docs.ansible.com/projects/ansible/devel/dev%5Fguide/testing/sanity/use-argspec-type-path.html
- 5: https://docs.ansible.com/projects/ansible/latest/collections/ansible/builtin/normpath_filter.html
- 6: https://docs.ansible.com/projects/ansible/latest/dev_guide/developing_program_flow_modules.html
Align template_config_path with the role’s write path behavior.
The role only uses __template_foo_config as a basename: tasks/main.yml:21-24 reads {{ __template_foo_config }}.j2 and writes /etc/{{ __template_foo_config }}. If callers pass an absolute filesystem path through template_config_path, /etc/myapp/config.yml becomes /etc/myapp/config.yml.j2 and /etc/myapp/config.yml, which is inconsistent with the documented filesystem-path contract. Either restrict this option to basenames, or have the role write to the given filesystem path instead of /etc/{{ ... }}.
🤖 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 25 - 29, Align template_config_path
with the role’s actual basename-based behavior: update its argument
specification and description to require or clearly document a basename rather
than a filesystem path. Ensure the validation or contract prevents absolute
paths from being accepted, while preserving the existing __template_foo_config
template lookup and /etc/{{ __template_foo_config }} write behavior.
| template_services: | ||
| type: list | ||
| elements: dict | ||
| required: false | ||
| description: >- | ||
| 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: | ||
| name: | ||
| type: str | ||
| required: true | ||
| description: >- | ||
| Name of the service. Mark a sub-option as required when | ||
| it must always be provided. | ||
| type: | ||
| type: str | ||
| required: false | ||
| choices: | ||
| - simple | ||
| - forking | ||
| - oneshot | ||
| description: >- | ||
| Type of the service. Choices work inside nested options | ||
| the same way as at the top level. | ||
| enabled: | ||
| type: bool | ||
| required: false | ||
| description: >- | ||
| Whether the service should be enabled at boot. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -u
rg -n -C 8 'template_services|__template_services|service:|enabled:|type:' \
--glob '*.yml' --glob '*.yaml' . || trueRepository: linux-system-roles/template
Length of output: 10378
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf 'Tracked files:\n'
git ls-files | sed -n '1,120p'
printf '\nRole files likely containing normalization/main logic:\n'
for f in tasks/main.yml tasks/*.yml run.yml tasks/*.yaml tasks/*.sh vars/main.yml vars/empty_vars.yml meta/argument_specs.yml defaults/main.yml README.md; do
if [ -f "$f" ]; then
echo "--- $f $(wc -l < "$f") lines ---"
sed -n '1,220p' "$f"
fi
done
printf '\nSearch normalized/internal variable patterns:\n'
rg -n '__template_services|template_services|template_name|template_enabled|service:(name|state|enabled|type)|{%- set|- s?et\s+' --glob '*.yml' --glob '*.yaml' --glob '*.j2' --glob '*.sh' . || trueRepository: linux-system-roles/template
Length of output: 17130
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf 'Complete tasks and internal variable assignment search:\n'
find tasks vars defaults library meta examples tests -maxdepth 3 -type f \
\( -iname '*.yml' -o -iname '*.yaml' \) | sort | xargs -r -n1 sh -c 'echo "--- $0"; sed -n "1,220p" "$0"'
printf '\nService-related variable/task search:\n'
rg -n -C 6 'service:|template_services|__template_services|set_fact:|name:' tasks defaults vars examples tests --glob '*.yml' --glob '*.yaml' || true
printf '\nExamples:\n'
find examples -type f | sort | xargs -r sh -c 'echo "--- $0"; sed -n "1,220p" "$0"'Repository: linux-system-roles/template
Length of output: 33102
Align template_services with the role’s service consumer.
tasks/main.yml and handlers/main.yml loop over __template_services as a list of service names, passing item to service.name while hard-coding enabled: true. Add normalization from template_services dictionaries to strings/parameters, then use those extracted values in the service tasks, or remove the nested service options from the schema/README if only plain service names are supported.
🤖 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 57 - 86, Align the template_services
schema with the __template_services consumer: either normalize each dictionary
into the service name and enabled/type parameters before the loops in
tasks/main.yml and handlers/main.yml, updating service.name and enabled to use
those values, or simplify the schema and README to accept only plain
service-name strings. Ensure the chosen representation is consistent across
argument validation and both service task handlers.
3 participants
Enhancement: Added argument spec validation to template role. Added required: to all arguments to avoid ambiguity for developers reading the template.
Reason: Because it is a good addition to the linux-system-roles project.
Result: Successfully added it.
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