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

inter-related collection loader fixes #62470

Closed
wants to merge 1 commit into from

Conversation

@nitzmahone
Copy link
Member

commented Sep 18, 2019

SUMMARY
  • fix for relative imports (PEP302 compliance- verifying our loader can find a module before returning itself to do so)
  • ensure that internal pluginloader caches use the full name of the code object so same-named objects in different collections can't clobber each other
  • ensure that Jinja2 plugin loader doesn't look things up in function cache before it's fully-populated
  • add relative import tests for controller-hosted plugins (JInja2 plugins, connections)
  • supersedes #60317

needs backport to 2.9

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

collection_loader

ADDITIONAL INFORMATION
* fix for relative imports (PEP302 compliance- verifying our loader can find a module before returning itself to do so)
* ensure that internal pluginloader caches use the full name of the code object so same-named objects in different collections can't clobber each other
* ensure that Jinja2 plugin loader doesn't look things up in function cache before it's fully-populated
* add relative import tests for controller-hosted plugins (JInja2 plugins, connections)
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/connection/relimp_conn.py:6:0: relative-beyond-top-level: Attempted relative import beyond top-level package

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 2 errors:

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/plugins/lookup/lookup_no_future_boilerplate.py:0:0: missing: from __future__ import (absolute_import, division, print_function)
test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/connection/relimp_conn.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Sep 18, 2019
@nitzmahone nitzmahone added P2 and removed needs_triage labels Sep 18, 2019
@@ -534,7 +544,13 @@ def get(self, name, *args, **kwargs):
collection_list = kwargs.pop('collection_list', None)
if name in self.aliases:
name = self.aliases[name]
path = self.find_plugin(name, collection_list=collection_list)
plugin_load_detail = dict()
path = self.find_plugin(name, collection_list=collection_list, load_detail_out=plugin_load_detail)

This comment has been minimized.

Copy link
@abadger

abadger Sep 18, 2019

Member

Oops.... When you were talking about this yesterday, I thought you were going to change the return value to be a dict rather than passing in a dict. Mea culpa. I just read what you said you were planning to do wrong. I don't think operating by side effect, is the best way to write this, especially when we have a side effect and returning a value from the function.

Since we're under time pressure I could see doing it this way for 2.9.x but I think it should get changed to being the return value in devel. (Unless it's simple to take what @mattclay did to identify the locations that need to be updated and then use a dist instead of a tuple for all of those).

@mattclay mattclay added this to In progress in Testing Collections via automation Sep 18, 2019
@nitzmahone nitzmahone closed this Sep 19, 2019
Testing Collections automation moved this from In progress to Done Sep 19, 2019
@nitzmahone nitzmahone removed the P2 label Sep 19, 2019
@abadger abadger moved this from To do to Done in 2.9 post beta firedrills Oct 11, 2019
@abadger abadger moved this from Done to To do in 2.9 post beta firedrills Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.