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

Fix the module redirect_list for action plugins #73864

Merged
merged 10 commits into from May 27, 2021

Conversation

s-hertel
Copy link
Contributor

@s-hertel s-hertel commented Mar 11, 2021

SUMMARY

Fixes #72918

Might separate gather_facts into its own PR or move it to #73863 as it's kind of broken anyway and I'm not sure it should be backported. Fixing it will probably be kind of be featureish for the other action plugins so I'm not sure if that should be backported either. This still needs tests and changelog.

I fixed gather_facts without adding anything new by just checking if the source of the value was 'default'.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

gather_facts
package
service

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.11 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 11, 2021
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Mar 11, 2021
@s-hertel s-hertel force-pushed the fix_action_plugin_module_defaults branch from 2dedc67 to 63a4ea1 Compare March 17, 2021 17:56
@s-hertel s-hertel changed the title [WIP] Fix the module redirect_list for action plugins Fix the module redirect_list for action plugins Mar 17, 2021
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community. core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 17, 2021
@s-hertel s-hertel force-pushed the fix_action_plugin_module_defaults branch from d1c4f3f to 6d8c08b Compare March 17, 2021 18:15
@ansibot

This comment has been minimized.

@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 Mar 17, 2021
@s-hertel s-hertel force-pushed the fix_action_plugin_module_defaults branch from 6d8c08b to 4261a5a Compare March 17, 2021 19:11
@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 Mar 17, 2021
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. 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. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 26, 2021
@s-hertel s-hertel force-pushed the fix_action_plugin_module_defaults branch from 4261a5a to ddcec0f Compare March 30, 2021 17:05
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label May 21, 2021
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

Maybe give a shot to putting the ansible.legacy equivalency check in get_action_args_with_defaults- that might obviate the need for all the rest of the changes.

@@ -41,7 +41,16 @@ def _get_module_args(self, fact_module, task_vars):
mod_args = dict((k, v) for k, v in mod_args.items() if v is not None)

# handle module defaults
mod_args = get_action_args_with_defaults(fact_module, mod_args, self._task.module_defaults, self._templar, self._task._ansible_internal_redirect_list)
if fact_module == 'ansible.legacy.setup' and self._use_setup_defaults:
Copy link
Member

Choose a reason for hiding this comment

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

I think probably all the changes to the actions can go away if you were to change get_action_args_with_defaults to say:

  • if action is prefixed with ansible.legacy. and the same ansible.legacy-prefixed action appears in the redirect list, remove the prefix from both places before looping over to apply module_defautls

@ansibot ansibot added 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. and removed 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. labels May 27, 2021
@s-hertel s-hertel force-pushed the fix_action_plugin_module_defaults branch from 5886120 to 7d70f08 Compare May 27, 2021 18:36
… fully qualified ansible.legacy actions

ci_complete
@s-hertel s-hertel force-pushed the fix_action_plugin_module_defaults branch from 7d70f08 to f3ec6bd Compare May 27, 2021 19:01
@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 May 27, 2021
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

Fix LGTM- a couple questions/suggestions about the tests, but good to merge either way.

test/units/plugins/action/test_gather_facts.py Outdated Show resolved Hide resolved
test/units/plugins/action/test_gather_facts.py Outdated Show resolved Hide resolved
@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 May 27, 2021
@ansibot ansibot added 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. and removed 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. labels May 27, 2021
@s-hertel s-hertel merged commit 5640093 into ansible:devel May 27, 2021
@s-hertel
Copy link
Contributor Author

Thanks for the reviews!

s-hertel added a commit to s-hertel/ansible that referenced this pull request May 27, 2021
…ansible#73864)

* Fix module-specific defaults in the gather_facts, package, and service action plugins.

* Handle ansible.legacy actions better in get_action_args_with_defaults

* Add tests for each action plugin

* Changelog

Fixes ansible#72918

(cherry picked from commit 5640093)
s-hertel added a commit to s-hertel/ansible that referenced this pull request May 27, 2021
…ansible#73864)

* Fix module-specific defaults in the gather_facts, package, and service action plugins.

* Handle ansible.legacy actions better in get_action_args_with_defaults

* Add tests for each action plugin

* Changelog

Fixes ansible#72918

(cherry picked from commit 5640093)
tsabirgaliev added a commit to tsabirgaliev/kubespray that referenced this pull request May 30, 2021
relrod pushed a commit that referenced this pull request Jun 14, 2021
…74850)

* Use the module redirect_list when getting defaults for action plugins (#73864)

* Fix module-specific defaults in the gather_facts, package, and service action plugins.

* Handle ansible.legacy actions better in get_action_args_with_defaults

* Add tests for each action plugin

* Changelog

Fixes #72918

(cherry picked from commit 5640093)

* Fix tests for < 3.8

(cherry picked from commit 267b721)
relrod pushed a commit that referenced this pull request Jun 14, 2021
…74849)

* Use the module redirect_list when getting defaults for action plugins (#73864)

* Fix module-specific defaults in the gather_facts, package, and service action plugins.

* Handle ansible.legacy actions better in get_action_args_with_defaults

* Add tests for each action plugin

* Changelog

Fixes #72918

(cherry picked from commit 5640093)

* Fix tests for < 3.8
@ansible ansible locked and limited conversation to collaborators Jun 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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. 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.

module_defaults for yum do not get picked up when invoked via package
4 participants