From 8fb54885bf891a61516164594e1625be3925910d Mon Sep 17 00:00:00 2001 From: David Shrewsbury Date: Tue, 11 May 2021 10:29:47 -0400 Subject: [PATCH] Role argspec: allow new argument spec file (#74582) * support separate role argspec file in ansible-doc * support separate role argspec file in ansible-core * support both .yml and .yaml extensions on argspec file in ansible-doc * fix filename building bug and rename some argspec files to test variations * use yaml extensions from constants * add superfluous meta/main.yml files to tests * Update lib/ansible/cli/doc.py Co-authored-by: Sam Doran * update docs * ci_complete * add changelog and allow for main.yml variations * add collection role testing Co-authored-by: Sam Doran --- .../fragments/74582-role-argspec-new-file.yml | 2 + .../rst/user_guide/playbooks_reuse_roles.rst | 19 ++-- lib/ansible/cli/doc.py | 86 ++++++++++++++----- lib/ansible/playbook/role/__init__.py | 36 +++++++- .../roles/test_role1/meta/argument_specs.yml | 34 ++++++++ .../roles/test_role1/meta/main.yml | 42 ++------- .../ansible_collections/foo/bar/MANIFEST.json | 0 .../bar/roles/blah/meta/argument_specs.yml | 8 ++ .../foo/bar/roles/blah/tasks/main.yml | 3 + .../roles/a/meta/argument_specs.yml | 17 ++++ .../roles_arg_spec/roles/a/meta/main.yml | 14 ++- .../b/meta/{main.yml => argument_specs.yaml} | 0 .../meta/{main.yml => argument_specs.yml} | 0 .../meta/{main.yml => argument_specs.yml} | 0 .../targets/roles_arg_spec/test.yml | 33 +++++++ 15 files changed, 220 insertions(+), 74 deletions(-) create mode 100644 changelogs/fragments/74582-role-argspec-new-file.yml create mode 100644 test/integration/targets/ansible-doc/roles/test_role1/meta/argument_specs.yml create mode 100644 test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/MANIFEST.json create mode 100644 test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah/meta/argument_specs.yml create mode 100644 test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah/tasks/main.yml create mode 100644 test/integration/targets/roles_arg_spec/roles/a/meta/argument_specs.yml rename test/integration/targets/roles_arg_spec/roles/b/meta/{main.yml => argument_specs.yaml} (100%) rename test/integration/targets/roles_arg_spec/roles/role_with_no_tasks/meta/{main.yml => argument_specs.yml} (100%) rename test/integration/targets/roles_arg_spec/roles/test1/meta/{main.yml => argument_specs.yml} (100%) diff --git a/changelogs/fragments/74582-role-argspec-new-file.yml b/changelogs/fragments/74582-role-argspec-new-file.yml new file mode 100644 index 00000000000..b5504d7db22 --- /dev/null +++ b/changelogs/fragments/74582-role-argspec-new-file.yml @@ -0,0 +1,2 @@ +bugfixes: + - roles - allow for role arg specs in new meta file (https://github.com/ansible/ansible/issues/74525). diff --git a/docs/docsite/rst/user_guide/playbooks_reuse_roles.rst b/docs/docsite/rst/user_guide/playbooks_reuse_roles.rst index 8a860e4e09d..9da27d7b35d 100644 --- a/docs/docsite/rst/user_guide/playbooks_reuse_roles.rst +++ b/docs/docsite/rst/user_guide/playbooks_reuse_roles.rst @@ -260,9 +260,16 @@ Role argument validation ======================== Beginning with version 2.11, you may choose to enable role argument validation based on an argument -specification defined in the role ``meta/main.yml`` file. When this argument specification is defined, -a new task is inserted at the beginning of role execution that will validate the parameters supplied -for the role against the specification. If the parameters fail validation, the role will fail execution. +specification. This specification is defined in the ``meta/argument_specs.yml`` file (or with the ``.yaml`` +file extension). When this argument specification is defined, a new task is inserted at the beginning of role execution +that will validate the parameters supplied for the role against the specification. If the parameters fail +validation, the role will fail execution. + +.. note:: + + Ansible also supports role specifications defined in the role ``meta/main.yml`` file, as well. However, + any role that defines the specs within this file will not work on versions below 2.11. For this reason, + we recommend using the ``meta/argument_specs.yml`` file to maintain backward compatibility. .. note:: @@ -274,7 +281,7 @@ Specification format -------------------- The role argument specification must be defined in a top-level ``argument_specs`` block within the -role ``meta/main.yml`` file. All fields are lower-case. +role ``meta/argument_specs.yml`` file. All fields are lower-case. :entry-point-name: @@ -346,7 +353,7 @@ Sample specification .. code-block:: yaml - # roles/myapp/meta/main.yml + # roles/myapp/meta/argument_specs.yml --- argument_specs: # roles/myapp/tasks/main.yml entry point @@ -395,7 +402,7 @@ You have two options to force Ansible to run a role more than once. Passing different parameters ---------------------------- -If you pass different parameters in each role definition, Ansible runs the role more than once. Providing different variable values is not the same as passing different role parameters. You must use the ``roles`` keyword for this behavior, since ``import_role`` and ``include_role`` do not accept role parameters. +If you pass different parameters in each role definition, Ansible runs the role more than once. Providing different variable values is not the same as passing different role parameters. You must use the ``roles`` keyword for this behavior, since ``import_role`` and ``include_role`` do not accept role parameters. This playbook runs the ``foo`` role twice: diff --git a/lib/ansible/cli/doc.py b/lib/ansible/cli/doc.py index faab6ee9bdc..f0613647c98 100644 --- a/lib/ansible/cli/doc.py +++ b/lib/ansible/cli/doc.py @@ -78,16 +78,47 @@ class RoleMixin(object): Note: The methods for actual display of role data are not present here. """ - ROLE_ARGSPEC_FILE = 'main.yml' + # Potential locations of the role arg spec file in the meta subdir, with main.yml + # having the lowest priority. + ROLE_ARGSPEC_FILES = ['argument_specs' + e for e in C.YAML_FILENAME_EXTENSIONS] + ["main" + e for e in C.YAML_FILENAME_EXTENSIONS] def _load_argspec(self, role_name, collection_path=None, role_path=None): + """Load the role argument spec data from the source file. + + :param str role_name: The name of the role for which we want the argspec data. + :param str collection_path: Path to the collection containing the role. This + will be None for standard roles. + :param str role_path: Path to the standard role. This will be None for + collection roles. + + We support two files containing the role arg spec data: either meta/main.yml + or meta/argument_spec.yml. The argument_spec.yml file will take precedence + over the meta/main.yml file, if it exists. Data is NOT combined between the + two files. + + :returns: A dict of all data underneath the ``argument_specs`` top-level YAML + key in the argspec data file. Empty dict is returned if there is no data. + """ + if collection_path: - path = os.path.join(collection_path, 'roles', role_name, 'meta', self.ROLE_ARGSPEC_FILE) + meta_path = os.path.join(collection_path, 'roles', role_name, 'meta') elif role_path: - path = os.path.join(role_path, 'meta', self.ROLE_ARGSPEC_FILE) + meta_path = os.path.join(role_path, 'meta') else: raise AnsibleError("A path is required to load argument specs for role '%s'" % role_name) + path = None + + # Check all potential spec files + for specfile in self.ROLE_ARGSPEC_FILES: + full_path = os.path.join(meta_path, specfile) + if os.path.exists(full_path): + path = full_path + break + + if path is None: + return {} + try: with open(path, 'r') as f: data = from_yaml(f.read(), file_name=path) @@ -110,18 +141,25 @@ class RoleMixin(object): """ found = set() found_names = set() + for path in role_paths: if not os.path.isdir(path): continue - # Check each subdir for a meta/main.yml file + + # Check each subdir for an argument spec file for entry in os.listdir(path): role_path = os.path.join(path, entry) - full_path = os.path.join(role_path, 'meta', self.ROLE_ARGSPEC_FILE) - if os.path.exists(full_path): - if name_filters is None or entry in name_filters: - if entry not in found_names: - found.add((entry, role_path)) - found_names.add(entry) + + # Check all potential spec files + for specfile in self.ROLE_ARGSPEC_FILES: + full_path = os.path.join(role_path, 'meta', specfile) + if os.path.exists(full_path): + if name_filters is None or entry in name_filters: + if entry not in found_names: + found.add((entry, role_path)) + found_names.add(entry) + # select first-found + break return found def _find_all_collection_roles(self, name_filters=None, collection_filter=None): @@ -147,19 +185,23 @@ class RoleMixin(object): roles_dir = os.path.join(path, 'roles') if os.path.exists(roles_dir): for entry in os.listdir(roles_dir): - full_path = os.path.join(roles_dir, entry, 'meta', self.ROLE_ARGSPEC_FILE) - if os.path.exists(full_path): - if name_filters is None: - found.add((entry, collname, path)) - else: - # Name filters might contain a collection FQCN or not. - for fqcn in name_filters: - if len(fqcn.split('.')) == 3: - (ns, col, role) = fqcn.split('.') - if '.'.join([ns, col]) == collname and entry == role: + + # Check all potential spec files + for specfile in self.ROLE_ARGSPEC_FILES: + full_path = os.path.join(roles_dir, entry, 'meta', specfile) + if os.path.exists(full_path): + if name_filters is None: + found.add((entry, collname, path)) + else: + # Name filters might contain a collection FQCN or not. + for fqcn in name_filters: + if len(fqcn.split('.')) == 3: + (ns, col, role) = fqcn.split('.') + if '.'.join([ns, col]) == collname and entry == role: + found.add((entry, collname, path)) + elif fqcn == entry: found.add((entry, collname, path)) - elif fqcn == entry: - found.add((entry, collname, path)) + break return found def _build_summary(self, role, collection, argspec): diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py index 50fbfe3ea81..46b1e894b30 100644 --- a/lib/ansible/playbook/role/__init__.py +++ b/lib/ansible/playbook/role/__init__.py @@ -21,6 +21,7 @@ __metaclass__ = type import os +from ansible import constants as C from ansible.errors import AnsibleError, AnsibleParserError, AnsibleAssertionError from ansible.module_utils._text import to_text from ansible.module_utils.six import iteritems, binary_type, text_type @@ -256,7 +257,8 @@ class Role(Base, Conditional, Taggable, CollectionSearch): task_data = self._load_role_yaml('tasks', main=self._from_files.get('tasks')) if self._should_validate: - task_data = self._prepend_validation_task(task_data) + role_argspecs = self._get_role_argspecs() + task_data = self._prepend_validation_task(task_data, role_argspecs) if task_data: try: @@ -274,7 +276,32 @@ class Role(Base, Conditional, Taggable, CollectionSearch): raise AnsibleParserError("The handlers/main.yml file for role '%s' must contain a list of tasks" % self._role_name, obj=handler_data, orig_exc=e) - def _prepend_validation_task(self, task_data): + def _get_role_argspecs(self): + """Get the role argument spec data. + + Role arg specs can be in one of two files in the role meta subdir: argument_specs.yml + or main.yml. The former has precedence over the latter. Data is not combined + between the files. + + :returns: A dict of all data under the top-level ``argument_specs`` YAML key + in the argument spec file. An empty dict is returned if there is no + argspec data. + """ + base_argspec_path = os.path.join(self._role_path, 'meta', 'argument_specs') + + for ext in C.YAML_FILENAME_EXTENSIONS: + full_path = base_argspec_path + ext + if self._loader.path_exists(full_path): + # Note: _load_role_yaml() takes care of rebuilding the path. + argument_specs = self._load_role_yaml('meta', main='argument_specs') + return argument_specs.get('argument_specs', {}) + + # We did not find the meta/argument_specs.[yml|yaml] file, so use the spec + # dict from the role meta data, if it exists. Ansible 2.11 and later will + # have the 'argument_specs' attribute, but earlier versions will not. + return getattr(self._metadata, 'argument_specs', {}) + + def _prepend_validation_task(self, task_data, argspecs): '''Insert a role validation task if we have a role argument spec. This method will prepend a validation task to the front of the role task @@ -282,14 +309,15 @@ class Role(Base, Conditional, Taggable, CollectionSearch): exists for the entry point. Entry point defaults to `main`. :param task_data: List of tasks loaded from the role. + :param argspecs: The role argument spec data dict. :returns: The (possibly modified) task list. ''' - if self._metadata.argument_specs: + if argspecs: # Determine the role entry point so we can retrieve the correct argument spec. # This comes from the `tasks_from` value to include_role or import_role. entrypoint = self._from_files.get('tasks', 'main') - entrypoint_arg_spec = self._metadata.argument_specs.get(entrypoint) + entrypoint_arg_spec = argspecs.get(entrypoint) if entrypoint_arg_spec: validation_task = self._create_validation_task(entrypoint_arg_spec, entrypoint) diff --git a/test/integration/targets/ansible-doc/roles/test_role1/meta/argument_specs.yml b/test/integration/targets/ansible-doc/roles/test_role1/meta/argument_specs.yml new file mode 100644 index 00000000000..0315a1fd125 --- /dev/null +++ b/test/integration/targets/ansible-doc/roles/test_role1/meta/argument_specs.yml @@ -0,0 +1,34 @@ +--- +argument_specs: + main: + short_description: test_role1 from roles subdir + description: + - In to am attended desirous raptures B(declared) diverted confined at. Collected instantly remaining + up certainly to C(necessary) as. Over walk dull into son boy door went new. + - At or happiness commanded daughters as. Is I(handsome) an declared at received in extended vicinity + subjects. Into miss on he over been late pain an. Only week bore boy what fat case left use. Match round + scale now style far times. Your me past an much. + author: + - John Doe (@john) + - Jane Doe (@jane) + + options: + myopt1: + description: + - First option. + type: "str" + required: true + + myopt2: + description: + - Second option + type: "int" + default: 8000 + + myopt3: + description: + - Third option. + type: "str" + choices: + - choice1 + - choice2 diff --git a/test/integration/targets/ansible-doc/roles/test_role1/meta/main.yml b/test/integration/targets/ansible-doc/roles/test_role1/meta/main.yml index 9e6daeb2be9..19f6162b870 100644 --- a/test/integration/targets/ansible-doc/roles/test_role1/meta/main.yml +++ b/test/integration/targets/ansible-doc/roles/test_role1/meta/main.yml @@ -1,37 +1,13 @@ +# This meta/main.yml exists to test that it is NOT read, with preference being +# given to the meta/argument_specs.yml file. This spec contains additional +# entry points (groot, foo) that the argument_specs.yml does not. If this file +# were read, the additional entrypoints would show up in --list output, breaking +# tests. --- -dependencies: -galaxy_info: - argument_specs: main: short_description: test_role1 from roles subdir - description: - - In to am attended desirous raptures B(declared) diverted confined at. Collected instantly remaining - up certainly to C(necessary) as. Over walk dull into son boy door went new. - - At or happiness commanded daughters as. Is I(handsome) an declared at received in extended vicinity - subjects. Into miss on he over been late pain an. Only week bore boy what fat case left use. Match round - scale now style far times. Your me past an much. - author: - - John Doe (@john) - - Jane Doe (@jane) - - options: - myopt1: - description: - - First option. - type: "str" - required: true - - myopt2: - description: - - Second option - type: "int" - default: 8000 - - myopt3: - description: - - Third option. - type: "str" - choices: - - choice1 - - choice2 + groot: + short_description: I am Groot + foo: + short_description: I am Foo diff --git a/test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/MANIFEST.json b/test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/MANIFEST.json new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah/meta/argument_specs.yml b/test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah/meta/argument_specs.yml new file mode 100644 index 00000000000..3cb0a87b194 --- /dev/null +++ b/test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah/meta/argument_specs.yml @@ -0,0 +1,8 @@ +argument_specs: + main: + short_description: "The foo.bar.blah role" + options: + blah_str: + type: "str" + required: true + description: "A string value" diff --git a/test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah/tasks/main.yml b/test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah/tasks/main.yml new file mode 100644 index 00000000000..ecb4dac6e6b --- /dev/null +++ b/test/integration/targets/roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah/tasks/main.yml @@ -0,0 +1,3 @@ +- name: "First task of blah role" + debug: + msg: "The string is {{ blah_str }}" diff --git a/test/integration/targets/roles_arg_spec/roles/a/meta/argument_specs.yml b/test/integration/targets/roles_arg_spec/roles/a/meta/argument_specs.yml new file mode 100644 index 00000000000..cfc1a372380 --- /dev/null +++ b/test/integration/targets/roles_arg_spec/roles/a/meta/argument_specs.yml @@ -0,0 +1,17 @@ +argument_specs: + main: + short_description: Main entry point for role A. + options: + a_str: + type: "str" + required: true + + alternate: + short_description: Alternate entry point for role A. + options: + a_int: + type: "int" + required: true + + no_spec_entrypoint: + short_description: An entry point with no spec diff --git a/test/integration/targets/roles_arg_spec/roles/a/meta/main.yml b/test/integration/targets/roles_arg_spec/roles/a/meta/main.yml index cfc1a372380..90920dbbd68 100644 --- a/test/integration/targets/roles_arg_spec/roles/a/meta/main.yml +++ b/test/integration/targets/roles_arg_spec/roles/a/meta/main.yml @@ -1,3 +1,6 @@ +# This meta/main.yml exists to test that it is NOT read, with preference being +# given to the meta/argument_specs.yml file. This spec contains an extra required +# parameter, a_something, that argument_specs.yml does not. argument_specs: main: short_description: Main entry point for role A. @@ -5,13 +8,6 @@ argument_specs: a_str: type: "str" required: true - - alternate: - short_description: Alternate entry point for role A. - options: - a_int: - type: "int" + a_something: + type: "str" required: true - - no_spec_entrypoint: - short_description: An entry point with no spec diff --git a/test/integration/targets/roles_arg_spec/roles/b/meta/main.yml b/test/integration/targets/roles_arg_spec/roles/b/meta/argument_specs.yaml similarity index 100% rename from test/integration/targets/roles_arg_spec/roles/b/meta/main.yml rename to test/integration/targets/roles_arg_spec/roles/b/meta/argument_specs.yaml diff --git a/test/integration/targets/roles_arg_spec/roles/role_with_no_tasks/meta/main.yml b/test/integration/targets/roles_arg_spec/roles/role_with_no_tasks/meta/argument_specs.yml similarity index 100% rename from test/integration/targets/roles_arg_spec/roles/role_with_no_tasks/meta/main.yml rename to test/integration/targets/roles_arg_spec/roles/role_with_no_tasks/meta/argument_specs.yml diff --git a/test/integration/targets/roles_arg_spec/roles/test1/meta/main.yml b/test/integration/targets/roles_arg_spec/roles/test1/meta/argument_specs.yml similarity index 100% rename from test/integration/targets/roles_arg_spec/roles/test1/meta/main.yml rename to test/integration/targets/roles_arg_spec/roles/test1/meta/argument_specs.yml diff --git a/test/integration/targets/roles_arg_spec/test.yml b/test/integration/targets/roles_arg_spec/test.yml index faed945afb9..e7c1fae054b 100644 --- a/test/integration/targets/roles_arg_spec/test.yml +++ b/test/integration/targets/roles_arg_spec/test.yml @@ -270,3 +270,36 @@ # and validation wasn't performed up front (thus not returning 'argument_errors'). - "'argument_errors' not in ansible_failed_result" - "'The task includes an option with an undefined variable.' in ansible_failed_result.msg" + +- name: "New play to reset vars: Test collection-based role" + hosts: localhost + gather_facts: false + tasks: + - name: "Valid collection-based role usage" + import_role: + name: "foo.bar.blah" + vars: + blah_str: "some string" + + +- name: "New play to reset vars: Test collection-based role will fail" + hosts: localhost + gather_facts: false + tasks: + - block: + - name: "Invalid collection-based role usage" + import_role: + name: "foo.bar.blah" + - fail: + msg: "Should not get here" + rescue: + - debug: var=ansible_failed_result + - name: "Validate import_role failure for collection-based role" + assert: + that: + - ansible_failed_result.argument_errors | length == 1 + - "'missing required arguments: blah_str' in ansible_failed_result.argument_errors" + - ansible_failed_result.validate_args_context.argument_spec_name == "main" + - ansible_failed_result.validate_args_context.name == "blah" + - ansible_failed_result.validate_args_context.type == "role" + - "ansible_failed_result.validate_args_context.path is search('roles_arg_spec/collections/ansible_collections/foo/bar/roles/blah')"