Skip to content

validate_argument_spec - fix validating the argument spec before using it to validate vars#79657

Draft
s-hertel wants to merge 1 commit intoansible:develfrom
s-hertel:warn-badly-formatted-role-arg-spec
Draft

validate_argument_spec - fix validating the argument spec before using it to validate vars#79657
s-hertel wants to merge 1 commit intoansible:develfrom
s-hertel:warn-badly-formatted-role-arg-spec

Conversation

@s-hertel
Copy link
Copy Markdown
Contributor

@s-hertel s-hertel commented Jan 4, 2023

SUMMARY

add a spec for the spec

This resolves the error in #79624, but replaces it with a warning because no_log in the role's argument spec has no bearing on whether or not the tasks in the role divulge it. The role currently needs to manage things like no_log/aliases for variables itself.

Fixes #79624

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

validate_argument_spec

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.15 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. labels Jan 4, 2023
@ansibot

This comment was marked as resolved.

@ansibot ansibot removed the module This issue/PR relates to a module. label Jan 5, 2023
@ansibot

This comment was marked as resolved.

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jan 5, 2023
@s-hertel s-hertel marked this pull request as ready for review January 5, 2023 16:20
@ansibot ansibot removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. ci_verified Changes made in this PR are causing tests to fail. labels Jan 5, 2023
@s-hertel s-hertel force-pushed the warn-badly-formatted-role-arg-spec branch 2 times, most recently from 9bac965 to 2b8745f Compare January 5, 2023 17:36
@s-hertel s-hertel marked this pull request as draft January 5, 2023 19:00
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Jan 5, 2023
@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Jan 5, 2023
@nitzmahone nitzmahone self-requested a review January 5, 2023 20:15
)
for block in validation_blocks:
for task in block.block:
task.implicit = True
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't change how the validation task is displayed by default. Callbacks only ignore implicit Tasks (unless they set the attr wants_implicit_tasks), and always get the TaskResults, even from implicit Tasks. https://github.com/ansible/ansible/blob/devel/lib/ansible/executor/task_queue_manager.py#L442-L443

Comment thread lib/ansible/plugins/action/validate_argument_spec.py Outdated
@s-hertel s-hertel marked this pull request as ready for review January 6, 2023 16:52
@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Jan 6, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 14, 2023
Copy link
Copy Markdown
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs portion LGTM

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Feb 2, 2023
Comment thread lib/ansible/plugins/action/validate_argument_spec.py Outdated
Comment thread changelogs/fragments/79657-warn-for-unsupported-role-arg-spec-fields.yml Outdated
@ansibot ansibot removed the shipit This PR is ready to be merged by Core label Feb 21, 2023
@s-hertel s-hertel force-pushed the warn-badly-formatted-role-arg-spec branch from a5df33a to bbeb55c Compare October 16, 2024 01:20
@ansibot ansibot added the module This issue/PR relates to a module. label Oct 16, 2024
@s-hertel s-hertel changed the title give a warning for unsupported role argument spec fields validate_argument_spec - fix validating the argument spec before using it to validate vars Oct 16, 2024
@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 16, 2024
@ansibot

This comment was marked as outdated.

@s-hertel s-hertel force-pushed the warn-badly-formatted-role-arg-spec branch from bbeb55c to 4ba445b Compare October 16, 2024 14:09
@ansibot ansibot removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. ci_verified Changes made in this PR are causing tests to fail. labels Oct 16, 2024
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 30, 2024
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Apr 15, 2025
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Oct 24, 2025
@s-hertel s-hertel force-pushed the warn-badly-formatted-role-arg-spec branch from 4ba445b to e737139 Compare January 15, 2026 21:17
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_pr This PR has not been pushed to for more than one year. labels Jan 15, 2026
@s-hertel s-hertel force-pushed the warn-badly-formatted-role-arg-spec branch from e737139 to a2dad29 Compare January 15, 2026 21:24
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jan 15, 2026
@ansibot

This comment was marked as resolved.

@webknjaz webknjaz added the ci_verified Changes made in this PR are causing tests to fail. label Jan 19, 2026
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 29, 2026
@s-hertel s-hertel force-pushed the warn-badly-formatted-role-arg-spec branch from a2dad29 to 8d8ee8d Compare March 16, 2026 15:47
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 16, 2026
@s-hertel s-hertel force-pushed the warn-badly-formatted-role-arg-spec branch from 8d8ee8d to 9a7709c Compare March 16, 2026 16:20
passing it to the ArgumentSpecValidator to prevent unhandled errors
during variable validation.

Make role argument spec errors non-fatal at runtime. Display a warning
and include details with -vvv. Pass ArgumentSpecValidator a pruned
argument spec containing the valid portion.

Give an error and return argument_spec_errors in the task result for
non-implicit validate_argument_spec tasks.

Add tests for role argument spec errors and validate_argument_spec
argument_spec errors.
@s-hertel s-hertel force-pushed the warn-badly-formatted-role-arg-spec branch from 9a7709c to 9c9725d Compare March 16, 2026 17:56
fields.pop(field, None)

if fields.get('type') in ('list', 'dict') and fields.get('options'):
validate_argspec(validator, context, fields['options'], errors, prune)
Copy link
Copy Markdown
Contributor Author

@s-hertel s-hertel Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: suboptions can use required_if, mutually_exclusive, etc, so not supporting those for this code path would be a breaking change. Maybe this needs a deprecation period just to be safe. It would be simple just to allow those fields for top level options (instead of only suboptions) to fix the inconsistency, but the plan was to implement a better UX.

@s-hertel s-hertel marked this pull request as draft March 16, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

affects_2.18 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. has_issue module This issue/PR relates to a module. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Argument Spec Validation Failure with vault + no_log

6 participants