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

[WIP] fix using inventory and cache plugins in a collection #56469

Open
wants to merge 8 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@s-hertel
Copy link
Contributor

commented May 15, 2019

SUMMARY

Fix using custom inventory and cache plugins in an adjacent collection + tests.

Also fix corner case overlooking a plugin in a path set with config if there's a collection unless there's an explicit error (i.e. a plugin in the plugin path may have '.'s in the file name and would be overlooked by the collection without error, but then the plugin path set via config was not checked).

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins

@@ -219,7 +219,8 @@ def _read_config_data(self, path):
if not config:
# no data
raise AnsibleParserError("%s is empty" % (to_native(path)))
elif config.get('plugin') != self.NAME:
# If there's a '.' in the plugin name, only fail if there are also no matching collections
elif config.get('plugin') != self.NAME and config.get('plugin').split('.')[-1] != self.NAME:

This comment has been minimized.

Copy link
@s-hertel

s-hertel May 15, 2019

Author Contributor

Not sure of the best way to do this. The alternative would be for NAME in the inventory module to be required to be the full collection name. @bcoca thoughts?

This comment has been minimized.

Copy link
@bcoca

bcoca May 15, 2019

Member

you should not need to do this, plugin loader should take care of collection loading itself #56194 should fix the issue of finding 'adjacent' collections for 'non ansible-playbook' , like ansible-inventory

This comment has been minimized.

Copy link
@bcoca

bcoca May 15, 2019

Member

just looked at tests, if this is also an issue with playbook, its a bug in loader, the plugins themselves should not need to be 'collection aware'

@s-hertel

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@chrismeyersfsu if you get a chance to test this out, would love your feedback.

@@ -59,7 +59,8 @@ class BaseCacheModule(AnsiblePlugin):
_display = display

def __init__(self, *args, **kwargs):
self._load_name = self.__module__.split('.')[-1]
# get everything after ansible.plugins.cache
self._load_name = '.'.join(self.__module__.split('.')[3:])
super(BaseCacheModule, self).__init__()

This comment has been minimized.

Copy link
@bcoca

bcoca May 15, 2019

Member

i would say the bug is that the plugin inside the collection is not using a correct name, it needs to match the collection name, we should not have to make every other plugin or base class collection aware

This comment has been minimized.

Copy link
@s-hertel

s-hertel May 17, 2019

Author Contributor

This was true for the inventory plugin in the collection. Thanks for noticing that. Cache is a different case since they do not set their own name. Also kind of feeling like I wish inventory plugins did not set NAME: #56469 (comment)

I modified the loader instead to set self._load_name early enough for the plugin __init__

@ansibot ansibot added needs_revision and removed core_review labels May 15, 2019

s-hertel added some commits May 17, 2019

solution 1 - require caches to document their load_name
I think this is wrong because now the _load_name is specified twice. The user-given _load_name and actual _load_name can diverge.

Also fix the NAME for an inventory plugin in a collection (now requires no code change to inventory/__init__)
solution 2 - fix the loader instead of adding new, unecessary require…
…ments for cache plugins

Now _load_name is set prior to the plugin being instantiated and everything works.
@@ -52,7 +52,6 @@ class CacheModule(BaseFileCacheModule):
"""
A caching module backed by json files.
"""
NAME = 'testns.content_adj.custom_jsonfile'

This comment has been minimized.

Copy link
@s-hertel

s-hertel May 17, 2019

Author Contributor

If we require cache plugins to set a load name, then they also aren't portable between collections as NAME would need to be tailored to the collection name. The more I look at this the more I like using _load_name. I think inventory plugins should also get rid of NAME for the same reason.

@@ -588,6 +587,9 @@ def get(self, name, *args, **kwargs):

if not class_only:
try:
# A plugin may need to use its _load_name in __init__ (for example, to use self.set_options or self.get_option).
# Update the object before instantiating the class in case this is true
self._update_object(obj, name, path)

This comment has been minimized.

Copy link
@s-hertel

s-hertel May 17, 2019

Author Contributor

I left the _update_object call below for backwards compatibility in case an __init__ has a side effect.

This comment has been minimized.

Copy link
@s-hertel

s-hertel May 17, 2019

Author Contributor

An alternative to this line would be to use class_only=True where our code uses cache_loader (and any other loaders that might need _load_name in __init__) so _update_object is called before instantiation. I picked this because it also fixes possible third party code using the plugin loader.

@samdoran samdoran removed the needs_triage label May 21, 2019

@samdoran samdoran changed the title fix using inventory and cache plugins in a collection [WIP] fix using inventory and cache plugins in a collection May 21, 2019

@samdoran samdoran requested a review from nitzmahone May 21, 2019

@ansibot ansibot added the WIP label May 21, 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.