Repository: ansible/ansible
Pull request: Role argspec: allow new argument spec file
##### SUMMARY

Roles containing a defined argument spec in `meta/main.yml` do not currently work on ansible-core versions less than 2.11 because those versions do not allow for the `argument_specs` key in `meta/main.yml`. To solve this, we will allow the role arg specs to be defined in a new file (`meta/argument_specs.[yml|yaml]`) which will allow updated roles to still work on older versions of ansible-core.

Argument specs in `meta/main.yml` will still be supported, but roles choosing to use this file will work only on ansible-core >= 2.11.  The new spec file will have precedence over `meta/main.yml`. Data will not be combined between the two files, if both exist.

This also adds a new test for collection-based roles that tests both validation success _and_ validation failure.

Fixes #74525 

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
roles



Problem statement:
Ansible < 2.11 fails on roles that contain argument specification
### Summary

If the role contains an argument specification, Ansible < 2.11 fails to use it and reports "ERROR! 'argument_specs' is not a valid attribute for a RoleMetadata".

### Issue Type

Bug Report

### Component Name

include_role

### Ansible Version

```console
$ ansible --version
ansible 2.9.18
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/tadej/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.9/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.9.4 (default, Apr  6 2021, 00:00:00) [GCC 10.2.1 20201125 (Red Hat 10.2.1-9)]
```


### Configuration

```console
$ ansible-config dump --only-changed
```


### OS / Environment

Fedora 33

### Steps to Reproduce

```
$ mkdir -p /tmp/cols/ansible_collections/sensu/
$ cd cols/ansible_collections/sensu/
$ git clone --branch role-argspec --depth 1 git@github.com:xlab-steampunk/sensu-go-ansible.git sensu_go
$ cd sensu_go
$ ANSIBLE_COLLECTIONS_PATHS=/tmp/cols ansible -m include_role -a name=sensu.sensu_go.install -vvv localhost
```


### Expected Results

Ansible should ignore the `argument_specs` and run the role without parameter validation.

### Actual Results

```console
ansible 2.9.18
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/tadej/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.9/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.9.4 (default, Apr  6 2021, 00:00:00) [GCC 10.2.1 20201125 (Red Hat 10.2.1-9)]
Using /etc/ansible/ansible.cfg as config file
host_list declined parsing /etc/ansible/hosts as it did not pass its verify_file() method
script declined parsing /etc/ansible/hosts as it did not pass its verify_file() method
auto declined parsing /etc/ansible/hosts as it did not pass its verify_file() method
Parsed /etc/ansible/hosts inventory source with ini plugin
Skipping callback 'actionable', as we already have a stdout callback.
Skipping callback 'counter_enabled', as we already have a stdout callback.
Skipping callback 'debug', as we already have a stdout callback.
Skipping callback 'dense', as we already have a stdout callback.
Skipping callback 'dense', as we already have a stdout callback.
Skipping callback 'full_skip', as we already have a stdout callback.
Skipping callback 'json', as we already have a stdout callback.
Skipping callback 'minimal', as we already have a stdout callback.
Skipping callback 'null', as we already have a stdout callback.
Skipping callback 'oneline', as we already have a stdout callback.
Skipping callback 'selective', as we already have a stdout callback.
Skipping callback 'skippy', as we already have a stdout callback.
Skipping callback 'stderr', as we already have a stdout callback.
Skipping callback 'unixy', as we already have a stdout callback.
Skipping callback 'yaml', as we already have a stdout callback.
META: ran handlers
localhost | SUCCESS => {
    "changed": false,
    "include_args": {
        "name": "sensu.sensu_go.install"
    }
}
ERROR! 'argument_specs' is not a valid attribute for a RoleMetadata

The error appears to be in '/tmp/cols/ansible_collections/sensu/sensu_go/roles/install/meta/main.yml': line 1, column 1, but may
be elsewhere in the file depending on the exact syntax problem.

The offending line appears to be:


galaxy_info:
^ here
```


### Code of Conduct

- [X] I agree to follow the Ansible Code of Conduct

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/playbook/role/include.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/playbook/role/include.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
related: PR #73120
This is definitely an issue. We are discussing how best to address this.
We discussed this in the Core IRC meeting today and decided on the following:

Role argument specs will move to `meta/argument_specs.yml` in the next release of Ansible 2.11.

The `argument_specs` key in `meta/main.yml` will continue to work but the `meta/argument_specs.yml` file will be preferred over `meta/main.yml`.

Commit message: fix filename building bug and rename some argspec files to test variations

Patch to review:
diff --git a/lib/ansible/cli/doc.py b/lib/ansible/cli/doc.py
index faab6ee9bdc7dd..4739a5de5197fc 100644
--- a/lib/ansible/cli/doc.py
+++ b/lib/ansible/cli/doc.py
@@ -78,16 +78,49 @@ class RoleMixin(object):
     Note: The methods for actual display of role data are not present here.
     """
 
-    ROLE_ARGSPEC_FILE = 'main.yml'
+    ROLE_META_FILE = 'main.yml'
+    ROLE_ARGSPEC_BASE = 'argument_specs'
 
     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)
+            argspec_base = os.path.join(collection_path, 'roles', role_name, 'meta', self.ROLE_ARGSPEC_BASE)
+            meta_path = os.path.join(collection_path, 'roles', role_name, 'meta', self.ROLE_META_FILE)
         elif role_path:
-            path = os.path.join(role_path, 'meta', self.ROLE_ARGSPEC_FILE)
+            argspec_base = os.path.join(role_path, 'meta', self.ROLE_ARGSPEC_BASE)
+            meta_path = os.path.join(role_path, 'meta', self.ROLE_META_FILE)
         else:
             raise AnsibleError("A path is required to load argument specs for role '%s'" % role_name)
 
+        path = None
+
+        # Allow for .yml or .yaml file extensions
+        for extension in ('.yml', '.yaml'):
+            full_path = argspec_base + extension
+            if os.path.exists(full_path):
+                path = full_path
+                break
+        if path is None and os.path.exists(meta_path):
+            path = meta_path
+        elif path is None:
+            return {}
+
         try:
             with open(path, 'r') as f:
                 data = from_yaml(f.read(), file_name=path)
@@ -113,15 +146,22 @@ def _find_all_normal_roles(self, role_paths, name_filters=None):
         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)
+
+                # Role argspecs can be in one of two files in meta subdir
+                potential_files = [self.ROLE_ARGSPEC_BASE + e for e in ('.yml', '.yaml')]
+                potential_files.append(self.ROLE_META_FILE)
+                for specfile in potential_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)
+                        break
         return found
 
     def _find_all_collection_roles(self, name_filters=None, collection_filter=None):
@@ -147,19 +187,23 @@ def _find_all_collection_roles(self, name_filters=None, collection_filter=None):
             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:
+
+                    # Role argspecs can be in one of two files in meta subdir
+                    for specfile in (self.ROLE_ARGSPEC_BASE, self.ROLE_META_FILE):
+                        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 50fbfe3ea81d7b..d1257f404c1347 100644
--- a/lib/ansible/playbook/role/__init__.py
+++ b/lib/ansible/playbook/role/__init__.py
@@ -256,7 +256,8 @@ def _load_role_data(self, role_include, parent_role=None):
         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 +275,33 @@ def _load_role_data(self, role_include, parent_role=None):
                 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 precendence 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.
+        '''
+        extensions = ('.yml', '.yaml')
+        base_argspec_path = os.path.join(self._role_path, 'meta', 'argument_specs')
+
+        for ext in 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 @@ def _prepend_validation_task(self, task_data):
         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/main.yml b/test/integration/targets/ansible-doc/roles/test_role1/meta/argument_specs.yml
similarity index 100%
rename from test/integration/targets/ansible-doc/roles/test_role1/meta/main.yml
rename to test/integration/targets/ansible-doc/roles/test_role1/meta/argument_specs.yml
diff --git a/test/integration/targets/roles_arg_spec/roles/a/meta/main.yml b/test/integration/targets/roles_arg_spec/roles/a/meta/argument_specs.yml
similarity index 100%
rename from test/integration/targets/roles_arg_spec/roles/a/meta/main.yml
rename to test/integration/targets/roles_arg_spec/roles/a/meta/argument_specs.yml
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


Code review comments:
diff_hunk: @@ -147,19 +187,23 @@ def _find_all_collection_roles(self, name_filters=None, collection_filter=None):
             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:
+
+                    # Role argspecs can be in one of two files in meta subdir
+                    for specfile in (self.ROLE_ARGSPEC_BASE, self.ROLE_META_FILE):
path: lib/ansible/cli/doc.py
text: @author:
This needs to be updated to use file extensions.

@user1:
use `C.YAML_FILENAME_EXTENSIONS ` for that, in most cases we also allow 'no extension' and/or a directory, but I'm not sure we want to go that far

diff_hunk: @@ -274,22 +275,49 @@ def _load_role_data(self, role_include, parent_role=None):
                 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.
path: lib/ansible/playbook/role/__init__.py
text: @user1:
PEP 257 suggest triple double quotes for docstrings.
```suggestion
        """Get the role argument spec data.
```

diff_hunk: @@ -78,16 +78,49 @@ class RoleMixin(object):
     Note: The methods for actual display of role data are not present here.
     """
 
-    ROLE_ARGSPEC_FILE = 'main.yml'
+    ROLE_META_FILE = 'main.yml'
+    ROLE_ARGSPEC_BASE = 'argument_specs'
 
     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
path: lib/ansible/cli/doc.py
text: @user1:
```suggestion
        :returns: A dict of all data underneath the ``argument_specs`` top-level YAML
```

diff_hunk: @@ -274,22 +275,49 @@ def _load_role_data(self, role_include, parent_role=None):
                 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 precendence 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.
+        '''
+        extensions = ('.yml', '.yaml')
path: lib/ansible/playbook/role/__init__.py
text: @user1:
I'm wondering if it would be better to store this list centrally somewhere. We have it defined in: 

https://github.com/ansible/ansible/blob/aea0c4dd1bf04cfff12b6a1e01d99653787048c3/lib/ansible/utils/collection_loader/_collection_finder.py#L73

But that's probably not where it should be referenced from. Maybe `lib/ansible/constants.py`?

@user2:
we already have this covered, use the existing `C.YAML_FILENAME_EXTENSIONS `

@user1:
Awesome! I didn't know that.