From 6f76a48f59e4d1936f3f3bd1711b3999e1f3869b Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Mon, 16 Dec 2019 11:28:24 -0500 Subject: [PATCH] Make sorting in collection_loader match plugin loader (#65776) * Simply sorting of Windows files below other plugin types Using the sort method with a custom key function uses less memory than creating multiple lists then joining them. This seemed to be an acceptable use of a lamdba, even though I geneally try to avoid them. * Fix sorting of plugins inside of collections Explicitly sort Windows files below others, mimicking what we do in plugin/loader.py * Add documentation about ansible.builtin and ansible.legacy Also document to the two different methods used for searching based on the candidate type. * Add changelog * Add integration test * Update comment with expected sort order --- .../collection_loader-sort-plugins.yaml | 2 ++ lib/ansible/plugins/loader.py | 29 +++++++++++-------- lib/ansible/utils/collection_loader.py | 9 ++++++ .../testcoll/roles/testrole/tasks/main.yml | 6 ++++ 4 files changed, 34 insertions(+), 12 deletions(-) create mode 100644 changelogs/fragments/collection_loader-sort-plugins.yaml diff --git a/changelogs/fragments/collection_loader-sort-plugins.yaml b/changelogs/fragments/collection_loader-sort-plugins.yaml new file mode 100644 index 00000000000..efe1277dbd6 --- /dev/null +++ b/changelogs/fragments/collection_loader-sort-plugins.yaml @@ -0,0 +1,2 @@ +bugfixes: + - collection_loader - sort Windows modules below other plugin types so the correct builtin plugin inside a role is selected (https://github.com/ansible/ansible/issues/65298) diff --git a/lib/ansible/plugins/loader.py b/lib/ansible/plugins/loader.py index 74e7be2c492..c383a5eb322 100644 --- a/lib/ansible/plugins/loader.py +++ b/lib/ansible/plugins/loader.py @@ -271,19 +271,15 @@ class PluginLoader: # PLUGIN_PATHS_CACHE, and MODULE_CACHE. Since those three dicts key # on the class_name and neither regular modules nor powershell modules # would have class_names, they would not work as written. - reordered_paths = [] - win_dirs = [] - - for path in ret: - if path.endswith('windows'): - win_dirs.append(path) - else: - reordered_paths.append(path) - reordered_paths.extend(win_dirs) + # + # The expected sort order is paths in the order in 'ret' with paths ending in '/windows' at the end, + # also in the original order they were found in 'ret'. + # The .sort() method is guaranteed to be stable, so original order is preserved. + ret.sort(key=lambda p: p.endswith('/windows')) # cache and return the result - self._paths = reordered_paths - return reordered_paths + self._paths = ret + return ret def _load_config_defs(self, name, module, path): ''' Reads plugin docs to find configuration setting definitions, to push to config manager for later use ''' @@ -317,6 +313,10 @@ class PluginLoader: display.debug('Added %s to loader search path' % (directory)) def _find_fq_plugin(self, fq_name, extension): + """Search builtin paths to find a plugin. No external paths are searched, + meaning plugins inside roles inside collections will be ignored. + """ + plugin_type = AnsibleCollectionRef.legacy_plugin_dir_to_plugin_type(self.subdir) acr = AnsibleCollectionRef.from_fqcr(fq_name, plugin_type) @@ -397,10 +397,12 @@ class PluginLoader: try: # HACK: refactor this properly if candidate_name.startswith('ansible.legacy'): - # just pass the raw name to the old lookup function to check in all the usual locations + # 'ansible.legacy' refers to the plugin finding behavior used before collections existed. + # They need to search 'library' and the various '*_plugins' directories in order to find the file. full_name = name p = self._find_plugin_legacy(name.replace('ansible.legacy.', '', 1), ignore_deprecated, check_aliases, suffix) else: + # 'ansible.builtin' should be handled here. This means only internal, or builtin, paths are searched. full_name, p = self._find_fq_plugin(candidate_name, suffix) if p: return full_name, p @@ -417,6 +419,9 @@ class PluginLoader: return name, self._find_plugin_legacy(name, ignore_deprecated, check_aliases, suffix) def _find_plugin_legacy(self, name, ignore_deprecated=False, check_aliases=False, suffix=None): + """Search library and various *_plugins paths in order to find the file. + This was behavior prior to the existence of collections. + """ if check_aliases: name = self.aliases.get(name, name) diff --git a/lib/ansible/utils/collection_loader.py b/lib/ansible/utils/collection_loader.py index ec496524b5b..4c32162cc90 100644 --- a/lib/ansible/utils/collection_loader.py +++ b/lib/ansible/utils/collection_loader.py @@ -290,6 +290,15 @@ class AnsibleFlatMapLoader(object): for root, dirs, files in os.walk(root_path): # add all files in this dir that don't have a blacklisted extension flat_files.extend(((root, f) for f in files if not any((f.endswith(ext) for ext in self._extension_blacklist)))) + + # HACK: Put Windows modules at the end of the list. This makes collection_loader behave + # the same way as plugin loader, preventing '.ps1' from modules being selected before '.py' + # modules simply because '.ps1' files may be above '.py' files in the flat_files list. + # + # The expected sort order is paths in the order they were in 'flat_files' + # with paths ending in '/windows' at the end, also in the original order they were + # in 'flat_files'. The .sort() method is guaranteed to be stable, so original order is preserved. + flat_files.sort(key=lambda p: p[0].endswith('/windows')) self._dirtree = flat_files def find_file(self, filename): diff --git a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole/tasks/main.yml b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole/tasks/main.yml index 31e3af5e4f2..7c05abb127d 100644 --- a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole/tasks/main.yml +++ b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole/tasks/main.yml @@ -1,3 +1,9 @@ +# test using builtin module of multiple types in a role in a collection +# https://github.com/ansible/ansible/issues/65298 +- name: Run setup module because there is both setup.ps1 and setup.py + setup: + gather_subset: min + - name: check collections list from role meta plugin_lookup: register: pluginlookup_out