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

Allow using action/local_action on includes and imports #37260

Merged
merged 6 commits into from
May 4, 2018

Conversation

sivel
Copy link
Member

@sivel sivel commented Mar 9, 2018

SUMMARY

Allow using action/local_action on includes and imports. Fixes #28822

Using action or local_action on includes and imports doesn't work. This is due to playbooks/helpers.py doing things like if 'include_role' in task_ds: which fails because the task_ds may look like:

{'local_action': {'module': 'include_role', 'name': 'foo'}}

or

{'local_action': 'include_role name=foo'}

This PR uses mod_args to determine the action in a more robust way.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/playbooks/helpers.py

ANSIBLE VERSION
2.4
2.5
2.6
ADDITIONAL INFORMATION

@ansibot ansibot added 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. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 9, 2018
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Mar 13, 2018
@bcoca
Copy link
Member

bcoca commented Mar 13, 2018

i think we should go in opposite direction and make sure that the 'action' gets set correctly and compare to that.

There are things that should never workd i.e action: module={{varwithimport_tasksvalue}}, but the rest should work with action/local_action even if the 'local' part is irrelevant.

@gundalow gundalow added the ci_verified Changes made in this PR are causing tests to fail. label Mar 16, 2018
@sivel
Copy link
Member Author

sivel commented Mar 19, 2018

i think we should go in opposite direction and make sure that the 'action' gets set correctly and compare to that.

I did specifically mention that I didn't think that was worthwhile. We could, but it would involve a much larger refactor.

Right now we have a lot of code that just does if "include" in task_ds:. Because at this point we haven't actually parsed the task ds. That happens later, after we determine if it is one of these special cases.

We have at least 3 variants we would have to support:

  • if "include" in task_ds:
  • if task_ds.get('module') == "include": <- this one would need wrapped in a try/except for when the value is not a dict
  • Some parser to pull validate local_action: include foo.yml

I think we would end up either having to create a new parser, that is separate from Task, and effectively parse the task ds 2 times, or move parsing much farther forward.

@bcoca
Copy link
Member

bcoca commented Mar 19, 2018

modargsparser does this already, we might just want to refactor it to allow for 'external use'

@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 Mar 19, 2018
@sivel
Copy link
Member Author

sivel commented Mar 27, 2018

@bcoca I've updated the code to use ModuleArgsParser, my only concern is that without much more refactoring, we will end up calling ModuleArgsParser 2 times for the same task in some scenarios.

@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 27, 2018
@sivel sivel changed the title Prevent using action/local_action on includes and imports Allow using action/local_action on includes and imports Mar 27, 2018
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Mar 27, 2018
@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 Apr 4, 2018
@bcoca bcoca added this to Pending in include and import issues Apr 17, 2018
@bcoca bcoca moved this from Pending to In Progress in include and import issues Apr 17, 2018
@ansibot ansibot added support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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 Apr 23, 2018
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label May 1, 2018
@sivel sivel merged commit 3d5a7d6 into ansible:devel May 4, 2018
include and import issues automation moved this from In Progress to Done May 4, 2018
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 12, 2018
* Prevent using action/local_action on includes and imports. Fixes ansible#28822

* Use ModuleArgsParser to determine action instead of disallowing action/local_action with import/include

* Add to_native

* switch back to block in task_ds, use ModuleArgsParse otherwise

* var should be task_ds

* Add test validating action+include_tasks
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 14, 2018
* Prevent using action/local_action on includes and imports. Fixes ansible#28822

* Use ModuleArgsParser to determine action instead of disallowing action/local_action with import/include

* Add to_native

* switch back to block in task_ds, use ModuleArgsParse otherwise

* var should be task_ds

* Add test validating action+include_tasks
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 14, 2018
* Prevent using action/local_action on includes and imports. Fixes ansible#28822

* Use ModuleArgsParser to determine action instead of disallowing action/local_action with import/include

* Add to_native

* switch back to block in task_ds, use ModuleArgsParse otherwise

* var should be task_ds

* Add test validating action+include_tasks
tonal pushed a commit to tonal/ansible that referenced this pull request May 15, 2018
* Prevent using action/local_action on includes and imports. Fixes ansible#28822

* Use ModuleArgsParser to determine action instead of disallowing action/local_action with import/include

* Add to_native

* switch back to block in task_ds, use ModuleArgsParse otherwise

* var should be task_ds

* Add test validating action+include_tasks
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 15, 2018
* Prevent using action/local_action on includes and imports. Fixes ansible#28822

* Use ModuleArgsParser to determine action instead of disallowing action/local_action with import/include

* Add to_native

* switch back to block in task_ds, use ModuleArgsParse otherwise

* var should be task_ds

* Add test validating action+include_tasks
oolongbrothers pushed a commit to oolongbrothers/ansible that referenced this pull request May 15, 2018
* Prevent using action/local_action on includes and imports. Fixes ansible#28822

* Use ModuleArgsParser to determine action instead of disallowing action/local_action with import/include

* Add to_native

* switch back to block in task_ds, use ModuleArgsParse otherwise

* var should be task_ds

* Add test validating action+include_tasks
ilicmilan pushed a commit to ilicmilan/ansible that referenced this pull request Nov 7, 2018
* Prevent using action/local_action on includes and imports. Fixes ansible#28822

* Use ModuleArgsParser to determine action instead of disallowing action/local_action with import/include

* Add to_native

* switch back to block in task_ds, use ModuleArgsParse otherwise

* var should be task_ds

* Add test validating action+include_tasks
@ansible ansible locked and limited conversation to collaborators May 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug. 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. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
Development

Successfully merging this pull request may close these issues.

local_action: include_role fails with 'this is probably a bug'
4 participants