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

Ansiballz explicit module name #58504

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@sivel
Copy link
Member

commented Jun 28, 2019

SUMMARY

Make Ansiballz pass the module name explicitly to basic for
an intermediate module name until arguments can be parsed. This should
be correct in almost all cases, but will fall back to ansible-module-basic
in the event arguments are not parsed, and the _name attribute is needed.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/executor/module_common.py
lib/ansible/module_utils/basic.py

ADDITIONAL INFORMATION

@bcoca

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

a follow up might be using thsi module name for errors vs the _ansible_module_name which contains 'task action' (both might be needed in error, but former is more correts)

@bcoca

bcoca approved these changes Jun 28, 2019

@ansibot ansibot added needs_revision and removed core_review labels Jun 28, 2019

@ansibot

This comment has been minimized.

@nitzmahone

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@sivel Just out of curiosity, what's the context that's driving this change- is it around conditional facts->info behavior, or something else? Increasing the amount of stuff we're monkeypatching into module globals makes me a little nervous for some of the persistent interpreter things I'm playing with for the future (we'll already have to solve some of those anyway, but adding more doesn't feel great at first blush).

@nitzmahone
Copy link
Member

left a comment

Impl looks fine if we really need it, but still not sure of the context

@bcoca

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

@nitzmahone this is long standing issue, if args are not parsed correctly we fallback to 'name' to display an error, this is currently 'basic.py' but should actually reflect the module

@ansibot ansibot removed the core_review label Jun 28, 2019

@sivel

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2019

@nitzmahone this issue was raised by @abadger a while ago, I don't think via an actual "issue". Part of the problem is that __file__ is almost never correct. There were a few other attempts to access the module name by implicit implementation details of ansiballz, as opposed to explicit behavior.

I proposed this change the other day, as an explicit means of trying to be as correct as possible, and @abadger was in favor of the approach.

@abadger might be able to provide more insight as well.

@ansibot ansibot added core_review and removed needs_revision labels Jul 6, 2019

@ansibot ansibot added the stale_ci label Jul 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.