Role argspec: allow new argument spec file (#74582) (#74656)

* 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
* update docs
* add changelog and allow for main.yml variations
* add collection role testing

Co-authored-by: Sam Doran <sdoran@redhat.com>
(cherry picked from commit 8fb54885bf)

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).

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:
 

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):

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)

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

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

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"

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 }}"

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

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

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')"