Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Make sure that none is not accepted for required module options #72248

Draft
wants to merge 15 commits into
base: devel
Choose a base branch
from
Draft

[WIP] Make sure that none is not accepted for required module options #72248

wants to merge 15 commits into from
+342 −36

Conversation

felixfontein
Copy link
Contributor

SUMMARY

If a value that is required (by required, required_if, required_together, required_one_of, required_by) is specified as none/null/~/empty, it is only accepted when the option explicitly declares allow_none_value=True.

Fixes #69190 more closely to https://meetbot.fedoraproject.org/ansible-meeting/2020-05-05/ansible_core_public_irc_meeting.2020-05-05-19.05.log.html than #72243.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/basic.py
lib/ansible/module_utils/common/validation.py

@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 18, 2020
@ansibot
Copy link
Contributor

ansibot commented Oct 18, 2020

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/argspec-required-mutually_exclusive.yml:0:0: (WARNING/2) Inline literal start-string without end-string.

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Oct 18, 2020
tremble
tremble suggested changes Oct 19, 2020
View changes
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

I don't see any tests for the allow_none_value=True behaviour. Given that this was the original behaviour there should probably be some tests that it's possible to go back to the old behaviour.

Similarly I may have missed it but I don't see any update to the arg_spec sanity tests to ensure its use doesn't trigger a sanity warning.

@tremble tremble mentioned this pull request Oct 19, 2020
Closed
@felixfontein
Copy link
Contributor Author

@treble that's kind of expected, since I only stayed up to long to finish the main part of the PR before I forgot my plans for it ;)

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. packaging Packaging category test This PR relates to tests. docs This issue/PR relates to or includes documentation. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 19, 2020
@ansibot
Copy link
Contributor

ansibot commented Oct 19, 2020

The test ansible-test sanity --test changelog [explain] failed with 1 error:

changelogs/fragments/argspec-required-mutually_exclusive.yml:0:0: (WARNING/2) Inline literal start-string without end-string.

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Oct 19, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Oct 19, 2020
@felixfontein
Copy link
Contributor Author

/rebuild_failed

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. 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. labels Feb 14, 2021
@ansibot
Copy link
Contributor

ansibot commented Feb 14, 2021

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/module_utils/common/validation.py:173:30: undefined-variable: Undefined variable 'module_parameters'
lib/ansible/module_utils/common/validation.py:173:64: undefined-variable: Undefined variable 'module_parameters'

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Feb 14, 2021
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. labels Feb 14, 2021
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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 22, 2021
@felixfontein felixfontein changed the title Make sure that none is not accepted for required module options [WIP] Make sure that none is not accepted for required module options Mar 20, 2021
@felixfontein
Copy link
Contributor Author

This needs a complete rewrite since the argument spec handling was rewritten / completely refactored.

@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Mar 20, 2021
@felixfontein felixfontein mentioned this pull request Mar 22, 2021
Closed
@samdoran samdoran removed their assignment Jul 6, 2021
@felixfontein felixfontein marked this pull request as draft Nov 26, 2022
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. 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 Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tremble tremble

Assignees
No one assigned
Labels
affects_2.11 bug This issue/PR relates to a bug. docs This issue/PR relates to or includes documentation. has_issue needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. packaging Packaging category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Strange behaviour with required / required_* module parameters
6 participants
@felixfontein @ansibot @samdoran @mkrizek @tremble @relrod