From d81ae27a4aa106c68c19331361a52887131840d4 Mon Sep 17 00:00:00 2001 From: Matt Davis Date: Thu, 29 Aug 2019 02:25:44 -0700 Subject: [PATCH] Collection role relative deps (#61517) * default collection support * playbooks run from inside a registered collection will set that collection as the first item in the search order (as will all non-collection roles) * this allows easy migration of runme.sh style playbook/role integration tests to collections without the playbooks/roles needing to know the name of their enclosing collection * disable default collection test under Windows * enable collection search for role dependencies * unqualified role deps in collection-hosted roles will first search the containing collection * if the calling role has specified a collections search list in metadata, it will be appended to the search order for unqualified role deps * disable cycle detection unit test * failing on 3.7+, needs proper cycle detection * see #61527 --- .../fragments/collection_dep_search.yml | 4 ++ lib/ansible/playbook/helpers.py | 19 ++++--- lib/ansible/playbook/play.py | 3 +- lib/ansible/playbook/role/__init__.py | 4 ++ lib/ansible/playbook/role/metadata.py | 19 ++++++- lib/ansible/utils/collection_loader.py | 6 +-- .../meta/main.yml | 2 + .../tasks/main.yml | 7 +++ .../subdir_testrole/tasks/main.yml | 3 ++ .../testcoll/roles/testrole/tasks/main.yml | 3 ++ .../roles/testrole_main_yaml/tasks/main.yml | 3 ++ .../integration/targets/collections/posix.yml | 53 ++++++++++++++++++- .../collections/roles/testrole/tasks/main.yml | 3 ++ test/units/playbook/role/test_role.py | 7 ++- 14 files changed, 120 insertions(+), 16 deletions(-) create mode 100644 changelogs/fragments/collection_dep_search.yml create mode 100644 test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/calls_intra_collection_dep_role_unqualified/meta/main.yml create mode 100644 test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/calls_intra_collection_dep_role_unqualified/tasks/main.yml diff --git a/changelogs/fragments/collection_dep_search.yml b/changelogs/fragments/collection_dep_search.yml new file mode 100644 index 00000000000..97759e6acb0 --- /dev/null +++ b/changelogs/fragments/collection_dep_search.yml @@ -0,0 +1,4 @@ +minor_changes: + - collection role dependencies will first search for unqualified role names in the containing collection. + - roles that define a collections search list in metadata will attempt to use the defined search list + when resolving unqualified role names. diff --git a/lib/ansible/playbook/helpers.py b/lib/ansible/playbook/helpers.py index 197329bc749..4eff569943b 100644 --- a/lib/ansible/playbook/helpers.py +++ b/lib/ansible/playbook/helpers.py @@ -368,12 +368,17 @@ def load_list_of_tasks(ds, play, block=None, role=None, task_include=None, use_h return task_list -def load_list_of_roles(ds, play, current_role_path=None, variable_manager=None, loader=None): - ''' - Loads and returns a list of RoleInclude objects from the datastructure - list of role definitions - ''' - +def load_list_of_roles(ds, play, current_role_path=None, variable_manager=None, loader=None, collection_search_list=None): + """ + Loads and returns a list of RoleInclude objects from the ds list of role definitions + :param ds: list of roles to load + :param play: calling Play object + :param current_role_path: path of the owning role, if any + :param variable_manager: varmgr to use for templating + :param loader: loader to use for DS parsing/services + :param collection_search_list: list of collections to search for unqualified role names + :return: + """ # we import here to prevent a circular dependency with imports from ansible.playbook.role.include import RoleInclude @@ -383,7 +388,7 @@ def load_list_of_roles(ds, play, current_role_path=None, variable_manager=None, roles = [] for role_def in ds: i = RoleInclude.load(role_def, play=play, current_role_path=current_role_path, variable_manager=variable_manager, - loader=loader, collection_list=play.collections) + loader=loader, collection_list=collection_search_list) roles.append(i) return roles diff --git a/lib/ansible/playbook/play.py b/lib/ansible/playbook/play.py index 2d6e3d4e48c..85f12132f24 100644 --- a/lib/ansible/playbook/play.py +++ b/lib/ansible/playbook/play.py @@ -192,7 +192,8 @@ class Play(Base, Taggable, CollectionSearch): ds = [] try: - role_includes = load_list_of_roles(ds, play=self, variable_manager=self._variable_manager, loader=self._loader) + role_includes = load_list_of_roles(ds, play=self, variable_manager=self._variable_manager, + loader=self._loader, collection_search_list=self.collections) except AssertionError as e: raise AnsibleParserError("A malformed role declaration was encountered.", obj=self._ds, orig_exc=e) diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py index d335d270346..eb734f88c7e 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -162,6 +162,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch): role_obj.add_parent(parent_role) return role_obj + # TODO: need to fix cycle detection in role load (maybe use an empty dict + # for the in-flight in role cache as a sentinel that we're already trying to load + # that role?) + # see https://github.com/ansible/ansible/issues/61527 r = Role(play=play, from_files=from_files, from_include=from_include) r._load_role_data(role_include, parent_role=parent_role) diff --git a/lib/ansible/playbook/role/metadata.py b/lib/ansible/playbook/role/metadata.py index fd1d873734a..1c5c5203479 100644 --- a/lib/ansible/playbook/role/metadata.py +++ b/lib/ansible/playbook/role/metadata.py @@ -84,12 +84,27 @@ class RoleMetadata(Base, CollectionSearch): raise AnsibleParserError(to_native(exc), obj=role_def, orig_exc=exc) current_role_path = None + collection_search_list = None + if self._owner: current_role_path = os.path.dirname(self._owner._role_path) + # if the calling role has a collections search path defined, consult it + collection_search_list = self._owner.collections[:] or [] + + # if the calling role is a collection role, ensure that its containing collection is searched first + owner_collection = self._owner._role_collection + if owner_collection: + collection_search_list = [c for c in collection_search_list if c != owner_collection] + collection_search_list.insert(0, owner_collection) + # ensure fallback role search works + if 'ansible.legacy' not in collection_search_list: + collection_search_list.append('ansible.legacy') + try: - return load_list_of_roles(roles, play=self._owner._play, current_role_path=current_role_path, variable_manager=self._variable_manager, - loader=self._loader) + return load_list_of_roles(roles, play=self._owner._play, current_role_path=current_role_path, + variable_manager=self._variable_manager, loader=self._loader, + collection_search_list=collection_search_list) except AssertionError as e: raise AnsibleParserError("A malformed list of role dependencies was encountered.", obj=self._ds, orig_exc=e) diff --git a/lib/ansible/utils/collection_loader.py b/lib/ansible/utils/collection_loader.py index daa64b60e8b..76334ba2750 100644 --- a/lib/ansible/utils/collection_loader.py +++ b/lib/ansible/utils/collection_loader.py @@ -173,7 +173,7 @@ class AnsibleCollectionLoader(with_metaclass(Singleton, object)): is_package = True location = None # check for implicit sub-package first - if os.path.isdir(candidate_child_path): + if os.path.isdir(to_bytes(candidate_child_path)): # Py3.x implicit namespace packages don't have a file location, so they don't support get_data # (which assumes the parent dir or that the loader has an internal mapping); so we have to provide # a bogus leaf file on the __file__ attribute for pkgutil.get_data to strip off @@ -181,10 +181,10 @@ class AnsibleCollectionLoader(with_metaclass(Singleton, object)): else: for source_path in [os.path.join(candidate_child_path, '__init__.py'), candidate_child_path + '.py']: - if not os.path.isfile(source_path): + if not os.path.isfile(to_bytes(source_path)): continue - with open(source_path, 'rb') as fd: + with open(to_bytes(source_path), 'rb') as fd: source = fd.read() code_object = compile(source=source, filename=source_path, mode='exec', flags=0, dont_inherit=True) diff --git a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/calls_intra_collection_dep_role_unqualified/meta/main.yml b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/calls_intra_collection_dep_role_unqualified/meta/main.yml new file mode 100644 index 00000000000..b3a88198658 --- /dev/null +++ b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/calls_intra_collection_dep_role_unqualified/meta/main.yml @@ -0,0 +1,2 @@ +dependencies: + - testrole # since testrole lives in this collection, we'll check there first diff --git a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/calls_intra_collection_dep_role_unqualified/tasks/main.yml b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/calls_intra_collection_dep_role_unqualified/tasks/main.yml new file mode 100644 index 00000000000..99297f70ced --- /dev/null +++ b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/calls_intra_collection_dep_role_unqualified/tasks/main.yml @@ -0,0 +1,7 @@ +- debug: + msg: '{{ outer_role_input | default("(undefined)") }}' + register: outer_role_output + +- assert: + that: + - outer_role_input is not defined or outer_role_input == outer_role_output.msg diff --git a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/role_subdir/subdir_testrole/tasks/main.yml b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/role_subdir/subdir_testrole/tasks/main.yml index 98445ce3aaf..64f5242b959 100644 --- a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/role_subdir/subdir_testrole/tasks/main.yml +++ b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/role_subdir/subdir_testrole/tasks/main.yml @@ -2,6 +2,9 @@ msg: '{{ test_role_input | default("(undefined)") }}' register: test_role_output +- set_fact: + testrole_source: collection + - assert: that: - test_role_input is not defined or test_role_input == test_role_output.msg 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 0fa5302eefd..31e3af5e4f2 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 @@ -19,6 +19,9 @@ msg: '{{ test_role_input | default("(undefined)") }}' register: test_role_output +- set_fact: + testrole_source: collection + # FIXME: add tests to ensure that block/task level stuff in a collection-hosted role properly inherit role default/meta values - assert: diff --git a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole_main_yaml/tasks/main.yml b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole_main_yaml/tasks/main.yml index 0fa5302eefd..31e3af5e4f2 100644 --- a/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole_main_yaml/tasks/main.yml +++ b/test/integration/targets/collections/collection_root_user/ansible_collections/testns/testcoll/roles/testrole_main_yaml/tasks/main.yml @@ -19,6 +19,9 @@ msg: '{{ test_role_input | default("(undefined)") }}' register: test_role_output +- set_fact: + testrole_source: collection + # FIXME: add tests to ensure that block/task level stuff in a collection-hosted role properly inherit role default/meta values - assert: diff --git a/test/integration/targets/collections/posix.yml b/test/integration/targets/collections/posix.yml index eac73e2d990..b94cee777b8 100644 --- a/test/integration/targets/collections/posix.yml +++ b/test/integration/targets/collections/posix.yml @@ -208,6 +208,7 @@ assert: that: - test_role_output.msg == test_role_input + - testrole_source == 'collection' # test dynamic execution of a FQ collection-backed role @@ -225,7 +226,7 @@ assert: that: - test_role_output.msg == test_role_input - + - testrole_source == 'collection' # test task-static execution of a FQ collection-backed role - name: verify collection-backed role execution (task static) @@ -241,6 +242,7 @@ assert: that: - test_role_output.msg == test_role_input + - testrole_source == 'collection' # test a legacy playbook-adjacent role, ensure that play collections config is not inherited @@ -259,6 +261,8 @@ assert: that: - test_role_output.msg == test_role_input + - testrole_source == 'legacy roles dir' + # test dynamic execution of a FQ collection-backed role - name: verify collection-backed role execution in subdir (include) @@ -272,6 +276,53 @@ assert: that: - test_role_output.msg == test_role_input + - testrole_source == 'collection' + + +# test collection-relative role deps (keyword static) +- name: verify collection-relative role deps + hosts: testhost + vars: + outer_role_input: keyword static outer + test_role_input: keyword static inner + roles: + - testns.testcoll.calls_intra_collection_dep_role_unqualified + tasks: + - assert: + that: + - outer_role_output.msg == outer_role_input + - test_role_output.msg == test_role_input + - testrole_source == 'collection' + +# test collection-relative role deps (task static) +- name: verify collection-relative role deps + hosts: testhost + vars: + outer_role_input: task static outer + test_role_input: task static inner + tasks: + - import_role: + name: testns.testcoll.calls_intra_collection_dep_role_unqualified + - assert: + that: + - outer_role_output.msg == outer_role_input + - test_role_output.msg == test_role_input + - testrole_source == 'collection' + +# test collection-relative role deps (task dynamic) +- name: verify collection-relative role deps + hosts: testhost + vars: + outer_role_input: task dynamic outer + test_role_input: task dynamic inner + tasks: + - include_role: + name: testns.testcoll.calls_intra_collection_dep_role_unqualified + - assert: + that: + - outer_role_output.msg == outer_role_input + - test_role_output.msg == test_role_input + - testrole_source == 'collection' - name: validate static task include behavior diff --git a/test/integration/targets/collections/roles/testrole/tasks/main.yml b/test/integration/targets/collections/roles/testrole/tasks/main.yml index d904cf19f6f..cbf6b8e770a 100644 --- a/test/integration/targets/collections/roles/testrole/tasks/main.yml +++ b/test/integration/targets/collections/roles/testrole/tasks/main.yml @@ -17,6 +17,9 @@ msg: '{{ test_role_input | default("(undefined)") }}' register: test_role_output +- set_fact: + testrole_source: legacy roles dir + - assert: that: - coll_module_out.source == 'user' diff --git a/test/units/playbook/role/test_role.py b/test/units/playbook/role/test_role.py index ba5d9245fd5..5abad1c66e7 100644 --- a/test/units/playbook/role/test_role.py +++ b/test/units/playbook/role/test_role.py @@ -401,8 +401,11 @@ class TestRole(unittest.TestCase): i = RoleInclude.load('bad2_metadata', play=mock_play, loader=fake_loader) self.assertRaises(AnsibleParserError, Role.load, i, play=mock_play) - i = RoleInclude.load('recursive1_metadata', play=mock_play, loader=fake_loader) - self.assertRaises(AnsibleError, Role.load, i, play=mock_play) + # TODO: re-enable this test once Ansible has proper role dep cycle detection + # that doesn't rely on stack overflows being recoverable (as they aren't in Py3.7+) + # see https://github.com/ansible/ansible/issues/61527 + # i = RoleInclude.load('recursive1_metadata', play=mock_play, loader=fake_loader) + # self.assertRaises(AnsibleError, Role.load, i, play=mock_play) @patch('ansible.playbook.role.definition.unfrackpath', mock_unfrackpath_noop) def test_load_role_complex(self):