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

Validate plugin option type 'dict' / 'dictionary' #71928

Merged
merged 5 commits into from
Sep 29, 2020

Conversation

felixfontein
Copy link
Contributor

SUMMARY

Until now, the type dict/dictionary was not validated in ensure_type() in lib/ansible/config/manager.py, even though that type is used also in plugins included with ansible/ansible (see for example https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/lookup/url.py#L39).

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/config/manager.py

felixfontein added a commit to felixfontein/ansible that referenced this pull request Sep 24, 2020
@ansibot ansibot added affects_2.11 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. 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. 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 Sep 24, 2020
@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. labels Sep 25, 2020
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. core_review In order to be merged, this PR must follow the core review workflow. labels Sep 25, 2020
@felixfontein
Copy link
Contributor Author

/rebuild_failed

@ansibot ansibot added has_issue 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 Sep 25, 2020
@@ -56,7 +56,7 @@ class ModuleDocFragment(object):
vars:
- name: ansible_async_dir
environment:
type: dict
type: dict or list
Copy link
Member

Choose a reason for hiding this comment

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

these are for modules, handled by argspec, this change should not be part of this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Also I'm not aware of any module using these docs fragments.)

Copy link
Contributor

@s-hertel s-hertel Sep 29, 2020

Choose a reason for hiding this comment

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

type: dict or list skips validation because that isn't a known type. The convention we've used for that in the past is type: raw. But environment should just be a list, since it's coming from the field attribute anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to type: list as discussed in the meeting.

@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Sep 29, 2020
felixfontein added a commit to felixfontein/ansible that referenced this pull request Sep 29, 2020
@s-hertel s-hertel merged commit 8893a24 into ansible:devel Sep 29, 2020
@felixfontein felixfontein deleted the plugin-option-type-dict branch September 29, 2020 21:17
@felixfontein
Copy link
Contributor Author

@bcoca @s-hertel thanks a lot for discussing and reviewing and merging this!

felixfontein added a commit to felixfontein/ansible that referenced this pull request Oct 2, 2020
heiderich pushed a commit to heiderich/ansible that referenced this pull request Oct 4, 2020
* Validate option type 'dict' / 'dictionary'.

* Add changelog fragment.

* Change type of 'environment' to list.
felixfontein added a commit to felixfontein/ansible that referenced this pull request Oct 6, 2020
@ansible ansible locked and limited conversation to collaborators Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.11 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. has_issue 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants