Handle errors during ansible-doc --metadata-dump more gracefully (#77035)

* Add option --no-fail-on-errors to return errors for ansible-doc --metadata-dump in JSON result instead of failing.

* Adjust changelog fragment.

* Add basic tests.

11 files changed, 450 insertions, 21 deletions

diff --git a/changelogs/fragments/77035-ansible-doc-metadata-dump-errors.yml b/changelogs/fragments/77035-ansible-doc-metadata-dump-errors.yml
new file mode 100644
index 0000000000..045e80a571
--- /dev/null
+++ b/changelogs/fragments/77035-ansible-doc-metadata-dump-errors.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - "ansible-doc metadata dump - add option ``--no-fail-on-errors`` which allows to not fail the ansible-doc invocation when errors happen during docs parsing or processing. Instead they are reported in the JSON result in an ``error`` key for the affected plugins (https://github.com/ansible/ansible/pull/77035)."
diff --git a/lib/ansible/cli/doc.py b/lib/ansible/cli/doc.py
index 314e517a47..98ff8705a6 100755
--- a/lib/ansible/cli/doc.py
+++ b/lib/ansible/cli/doc.py
@@ -255,7 +255,7 @@ class RoleMixin(object):
 
         return (fqcn, doc)
 
-    def _create_role_list(self):
+    def _create_role_list(self, fail_on_errors=True):
         """Return a dict describing the listing of all roles with arg specs.
 
         :param role_paths: A tuple of one or more role paths.
@@ -296,22 +296,37 @@ class RoleMixin(object):
         result = {}
 
         for role, role_path in roles:
-            argspec = self._load_argspec(role, role_path=role_path)
-            fqcn, summary = self._build_summary(role, '', argspec)
-            result[fqcn] = summary
+            try:
+                argspec = self._load_argspec(role, role_path=role_path)
+                fqcn, summary = self._build_summary(role, '', argspec)
+                result[fqcn] = summary
+            except Exception as e:
+                if fail_on_errors:
+                    raise
+                result[role] = {
+                    'error': 'Error while loading role argument spec: %s' % to_native(e),
+                }
 
         for role, collection, collection_path in collroles:
-            argspec = self._load_argspec(role, collection_path=collection_path)
-            fqcn, summary = self._build_summary(role, collection, argspec)
-            result[fqcn] = summary
+            try:
+                argspec = self._load_argspec(role, collection_path=collection_path)
+                fqcn, summary = self._build_summary(role, collection, argspec)
+                result[fqcn] = summary
+            except Exception as e:
+                if fail_on_errors:
+                    raise
+                result['%s.%s' % (collection, role)] = {
+                    'error': 'Error while loading role argument spec: %s' % to_native(e),
+                }
 
         return result
 
-    def _create_role_doc(self, role_names, entry_point=None):
+    def _create_role_doc(self, role_names, entry_point=None, fail_on_errors=True):
         """
         :param role_names: A tuple of one or more role names.
         :param role_paths: A tuple of one or more role paths.
         :param entry_point: A role entry point name for filtering.
+        :param fail_on_errors: When set to False, include errors in the JSON output instead of raising errors
 
         :returns: A dict indexed by role name, with 'collection', 'entry_points', and 'path' keys per role.
         """
@@ -322,16 +337,26 @@ class RoleMixin(object):
         result = {}
 
         for role, role_path in roles:
-            argspec = self._load_argspec(role, role_path=role_path)
-            fqcn, doc = self._build_doc(role, role_path, '', argspec, entry_point)
-            if doc:
-                result[fqcn] = doc
+            try:
+                argspec = self._load_argspec(role, role_path=role_path)
+                fqcn, doc = self._build_doc(role, role_path, '', argspec, entry_point)
+                if doc:
+                    result[fqcn] = doc
+            except Exception as e:  # pylint:disable=broad-except
+                result[role] = {
+                    'error': 'Error while processing role: %s' % to_native(e),
+                }
 
         for role, collection, collection_path in collroles:
-            argspec = self._load_argspec(role, collection_path=collection_path)
-            fqcn, doc = self._build_doc(role, collection_path, collection, argspec, entry_point)
-            if doc:
-                result[fqcn] = doc
+            try:
+                argspec = self._load_argspec(role, collection_path=collection_path)
+                fqcn, doc = self._build_doc(role, collection_path, collection, argspec, entry_point)
+                if doc:
+                    result[fqcn] = doc
+            except Exception as e:  # pylint:disable=broad-except
+                result['%s.%s' % (collection, role)] = {
+                    'error': 'Error while processing role: %s' % to_native(e),
+                }
 
         return result
 
@@ -438,6 +463,10 @@ class DocCLI(CLI, RoleMixin):
         exclusive.add_argument("--metadata-dump", action="store_true", default=False, dest='dump',
                                help='**For internal use only** Dump json metadata for all entries, ignores other options.')
 
+        self.parser.add_argument("--no-fail-on-errors", action="store_true", default=False, dest='no_fail_on_errors',
+                                 help='**For internal use only** Only used for --metadata-dump. '
+                                      'Do not fail on errors. Report the error message in the JSON instead.')
+
     def post_process_args(self, options):
         options = super(DocCLI, self).post_process_args(options)
 
@@ -626,7 +655,7 @@ class DocCLI(CLI, RoleMixin):
             self.plugin_list = set()  # reset for next iteration
         return results
 
-    def _get_plugins_docs(self, plugin_type, names):
+    def _get_plugins_docs(self, plugin_type, names, fail_on_errors=True):
 
         loader = DocCLI._prep_loader(plugin_type)
         search_paths = DocCLI.print_paths(loader)
@@ -640,6 +669,11 @@ class DocCLI(CLI, RoleMixin):
                 display.warning("%s %s not found in:\n%s\n" % (plugin_type, plugin, search_paths))
                 continue
             except Exception as e:
+                if not fail_on_errors:
+                    plugin_docs[plugin] = {
+                        'error': 'Missing documentation or could not parse documentation: %s' % to_native(e),
+                    }
+                    continue
                 display.vvv(traceback.format_exc())
                 raise AnsibleError("%s %s missing documentation (or could not parse"
                                    " documentation): %s\n" %
@@ -647,9 +681,24 @@ class DocCLI(CLI, RoleMixin):
 
             if not doc:
                 # The doc section existed but was empty
+                if not fail_on_errors:
+                    plugin_docs[plugin] = {
+                        'error': 'No valid documentation found',
+                    }
                 continue
 
-            plugin_docs[plugin] = DocCLI._combine_plugin_doc(plugin, plugin_type, doc, plainexamples, returndocs, metadata)
+            docs = DocCLI._combine_plugin_doc(plugin, plugin_type, doc, plainexamples, returndocs, metadata)
+            if not fail_on_errors:
+                # Check whether JSON serialization would break
+                try:
+                    json.dumps(docs, cls=AnsibleJSONEncoder)
+                except Exception as e:  # pylint:disable=broad-except
+                    plugin_docs[plugin] = {
+                        'error': 'Cannot serialize documentation as JSON: %s' % to_native(e),
+                    }
+                    continue
+
+            plugin_docs[plugin] = docs
 
         return plugin_docs
 
@@ -719,14 +768,16 @@ class DocCLI(CLI, RoleMixin):
             docs['all'] = {}
             for ptype in ptypes:
                 if ptype == 'role':
-                    roles = self._create_role_list()
-                    docs['all'][ptype] = self._create_role_doc(roles.keys(), context.CLIARGS['entry_point'])
+                    roles = self._create_role_list(fail_on_errors=not context.CLIARGS['no_fail_on_errors'])
+                    docs['all'][ptype] = self._create_role_doc(
+                        roles.keys(), context.CLIARGS['entry_point'], fail_on_errors=not context.CLIARGS['no_fail_on_errors'])
                 elif ptype == 'keyword':
                     names = DocCLI._list_keywords()
                     docs['all'][ptype] = DocCLI._get_keywords_docs(names.keys())
                 else:
                     plugin_names = self._list_plugins(ptype, None)
-                    docs['all'][ptype] = self._get_plugins_docs(ptype, plugin_names)
+                    docs['all'][ptype] = self._get_plugins_docs(
+                        ptype, plugin_names, fail_on_errors=not context.CLIARGS['no_fail_on_errors'])
                     # reset list after each type to avoid polution
         elif listing:
             if plugin_type == 'keyword':
diff --git a/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/MANIFEST.json b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/MANIFEST.json
new file mode 100644
index 0000000000..243a5e4372
--- /dev/null
+++ b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/MANIFEST.json
@@ -0,0 +1,30 @@
+{
+ "collection_info": {
+  "description": null, 
+  "repository": "",
+  "tags": [], 
+  "dependencies": {}, 
+  "authors": [
+   "Ansible (https://ansible.com)"
+  ], 
+  "issues": "",
+  "name": "testcol", 
+  "license": [
+   "GPL-3.0-or-later"
+  ], 
+  "documentation": "",
+  "namespace": "testns", 
+  "version": "0.1.1231", 
+  "readme": "README.md", 
+  "license_file": "COPYING", 
+  "homepage": "",
+ }, 
+ "file_manifest_file": {
+  "format": 1, 
+  "ftype": "file", 
+  "chksum_sha256": "4c15a867ceba8ba1eaf2f4a58844bb5dbb82fec00645fc7eb74a3d31964900f6", 
+  "name": "FILES.json", 
+  "chksum_type": "sha256"
+ }, 
+ "format": 1
+}
diff --git a/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/cache/notjsonfile.py b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/cache/notjsonfile.py
new file mode 100644
index 0000000000..9fa25b400a
--- /dev/null
+++ b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/cache/notjsonfile.py
@@ -0,0 +1,70 @@
+# (c) 2020 Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+# Make coding more python3-ish
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+DOCUMENTATION = '''
+    cache: notjsonfile
+        broken:
+    short_description: JSON formatted files.
+    description:
+        - This cache uses JSON formatted, per host, files saved to the filesystem.
+    author: Ansible Core (@ansible-core)
+    version_added: 0.7.0
+    options:
+      _uri:
+        required: True
+        description:
+          - Path in which the cache plugin will save the JSON files
+        env:
+          - name: ANSIBLE_CACHE_PLUGIN_CONNECTION
+            version_added: 1.2.0
+        ini:
+          - key: fact_caching_connection
+            section: defaults
+            deprecated:
+              alternative: none
+              why: Test deprecation
+              version: '2.0.0'
+      _prefix:
+        description: User defined prefix to use when creating the JSON files
+        env:
+          - name: ANSIBLE_CACHE_PLUGIN_PREFIX
+            version_added: 1.1.0
+        ini:
+          - key: fact_caching_prefix
+            section: defaults
+        deprecated:
+          alternative: none
+          why: Another test deprecation
+          removed_at_date: '2050-01-01'
+      _timeout:
+        default: 86400
+        description: Expiration timeout for the cache plugin data
+        env:
+          - name: ANSIBLE_CACHE_PLUGIN_TIMEOUT
+        ini:
+          - key: fact_caching_timeout
+            section: defaults
+        vars:
+          - name: notsjonfile_fact_caching_timeout
+            version_added: 1.5.0
+            deprecated:
+              alternative: do not use a variable
+              why: Test deprecation
+              version: '3.0.0'
+        type: integer
+    extends_documentation_fragment:
+        - testns.testcol2.plugin
+'''
+
+from ansible.plugins.cache import BaseFileCacheModule
+
+
+class CacheModule(BaseFileCacheModule):
+    """
+    A caching module backed by json files.
+    """
+    pass
diff --git a/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/inventory/statichost.py b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/inventory/statichost.py
new file mode 100644
index 0000000000..caec2ed699
--- /dev/null
+++ b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/inventory/statichost.py
@@ -0,0 +1,36 @@
+# Copyright (c) 2018 Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+DOCUMENTATION = '''
+    inventory: statichost
+        broken:
+    short_description: Add a single host
+    description: Add a single host
+    extends_documentation_fragment:
+      - inventory_cache
+    options:
+      plugin:
+        description: plugin name (must be statichost)
+        required: true
+      hostname:
+        description: Toggle display of stderr even when script was successful
+        required: True
+'''
+
+from ansible.errors import AnsibleParserError
+from ansible.plugins.inventory import BaseInventoryPlugin, Cacheable
+
+
+class InventoryModule(BaseInventoryPlugin, Cacheable):
+
+    NAME = 'testns.content_adj.statichost'
+
+    def verify_file(self, path):
+        pass
+
+    def parse(self, inventory, loader, path, cache=None):
+
+        pass
diff --git a/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/lookup/noop.py b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/lookup/noop.py
new file mode 100644
index 0000000000..d456986957
--- /dev/null
+++ b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/lookup/noop.py
@@ -0,0 +1,45 @@
+# (c) 2020 Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+# Make coding more python3-ish
+from __future__ import (absolute_import, division, print_function)
+
+__metaclass__ = type
+
+DOCUMENTATION = """
+    lookup: noop
+        broken:
+    author: Ansible core team
+    short_description: returns input
+    description:
+      - this is a noop
+    deprecated:
+        alternative: Use some other lookup
+        why: Test deprecation
+        removed_in: '3.0.0'
+    extends_documentation_fragment:
+        - testns.testcol2.version_added
+"""
+
+EXAMPLES = """
+- name: do nothing
+  debug: msg="{{ lookup('testns.testcol.noop', [1,2,3,4] }}"
+"""
+
+RETURN = """
+  _list:
+    description: input given
+    version_added: 1.0.0
+"""
+
+from ansible.module_utils.common._collections_compat import Sequence
+from ansible.plugins.lookup import LookupBase
+from ansible.errors import AnsibleError
+
+
+class LookupModule(LookupBase):
+
+    def run(self, terms, **kwargs):
+        if not isinstance(terms, Sequence):
+            raise AnsibleError("testns.testcol.noop expects a list")
+        return terms
diff --git a/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/fakemodule.py b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/fakemodule.py
new file mode 100644
index 0000000000..a1caeb148b
--- /dev/null
+++ b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/fakemodule.py
@@ -0,0 +1,28 @@
+#!/usr/bin/python
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+
+DOCUMENTATION = """
+    module: fakemodule
+        broken:
+    short_desciption: fake module
+    description:
+        - this is a fake module
+    version_added: 1.0.0
+    options:
+        _notreal:
+            description:  really not a real option
+    author:
+        - me
+"""
+
+import json
+
+
+def main():
+    print(json.dumps(dict(changed=False, source='testns.testcol.fakemodule')))
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/notrealmodule.py b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/notrealmodule.py
new file mode 100644
index 0000000000..4479f23fa5
--- /dev/null
+++ b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/notrealmodule.py
@@ -0,0 +1,13 @@
+#!/usr/bin/python
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import json
+
+
+def main():
+    print(json.dumps(dict(changed=False, source='testns.testcol.notrealmodule')))
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/randommodule.py b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/randommodule.py
new file mode 100644
index 0000000000..fb0e319d14
--- /dev/null
+++ b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/modules/randommodule.py
@@ -0,0 +1,96 @@
+#!/usr/bin/python
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+
+DOCUMENTATION = '''
+---
+module: randommodule
+short_description: A random module
+description:
+    - A random module.
+author:
+    - Ansible Core Team
+version_added: 1.0.0
+deprecated:
+    alternative: Use some other module
+    why: Test deprecation
+    removed_in: '3.0.0'
+options:
+    test:
+        description: Some text.
+        type: str
+        version_added: 1.2.0
+    sub:
+        description: Suboptions.
+        type: dict
+        suboptions:
+            subtest:
+                description: A suboption.
+                type: int
+                version_added: 1.1.0
+        # The following is the wrong syntax, and should not get processed
+        # by add_collection_to_versions_and_dates()
+        options:
+            subtest2:
+                description: Another suboption.
+                type: float
+                version_added: 1.1.0
+        # The following is not supported in modules, and should not get processed
+        # by add_collection_to_versions_and_dates()
+        env:
+            - name: TEST_ENV
+              version_added: 1.0.0
+              deprecated:
+                alternative: none
+                why: Test deprecation
+                removed_in: '2.0.0'
+                version: '2.0.0'
+extends_documentation_fragment:
+    - testns.testcol2.module
+'''
+
+EXAMPLES = '''
+'''
+
+RETURN = '''
+z_last:
+    description: A last result.
+        broken:
+    type: str
+    returned: success
+    version_added: 1.3.0
+
+m_middle:
+    description:
+        - This should be in the middle.
+        - Has some more data
+    type: dict
+    returned: success and 1st of month
+    contains:
+        suboption:
+            description: A suboption.
+            type: str
+            choices: [ARF, BARN, c_without_capital_first_letter]
+            version_added: 1.4.0
+
+a_first:
+    description: A first result.
+    type: str
+    returned: success
+'''
+
+
+from ansible.module_utils.basic import AnsibleModule
+
+
+def main():
+    module = AnsibleModule(
+        argument_spec=dict(),
+    )
+
+    module.exit_json()
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/vars/noop_vars_plugin.py b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/vars/noop_vars_plugin.py
new file mode 100644
index 0000000000..ae0f75e0b3
--- /dev/null
+++ b/test/integration/targets/ansible-doc/broken-docs/collections/ansible_collections/testns/testcol/plugins/vars/noop_vars_plugin.py
@@ -0,0 +1,30 @@
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+DOCUMENTATION = '''
+    vars: noop_vars_plugin
+        broken:
+    short_description: Do NOT load host and group vars
+    description: don't test loading host and group vars from a collection
+    options:
+      stage:
+        default: all
+        choices: ['all', 'inventory', 'task']
+        type: str
+        ini:
+          - key: stage
+            section: testns.testcol.noop_vars_plugin
+        env:
+          - name: ANSIBLE_VARS_PLUGIN_STAGE
+    extends_documentation_fragment:
+        - testns.testcol2.deprecation
+'''
+
+from ansible.plugins.vars import BaseVarsPlugin
+
+
+class VarsModule(BaseVarsPlugin):
+
+    def get_vars(self, loader, path, entities, cache=True):
+        super(VarsModule, self).get_vars(loader, path, entities)
+        return {'collection': 'yes', 'notreal': 'value'}
diff --git a/test/integration/targets/ansible-doc/runme.sh b/test/integration/targets/ansible-doc/runme.sh
index bab614bc97..81eba61946 100755
--- a/test/integration/targets/ansible-doc/runme.sh
+++ b/test/integration/targets/ansible-doc/runme.sh
@@ -103,3 +103,31 @@ test "$current_out" == "$expected_out"
 
 # just ensure it runs
 ANSIBLE_LIBRARY='./nolibrary' ansible-doc --metadata-dump --playbook-dir /dev/null
+
+# create broken role argument spec
+mkdir -p broken-docs/collections/ansible_collections/testns/testcol/roles/testrole/meta
+cat <<EOF > broken-docs/collections/ansible_collections/testns/testcol/roles/testrole/meta/main.yml
+---
+dependencies:
+galaxy_info:
+
+argument_specs:
+    main:
+        short_description: testns.testcol.testrole short description for main entry point
+        description:
+            - Longer description for testns.testcol.testrole main entry point.
+        author: Ansible Core (@ansible)
+        options:
+            opt1:
+                description: opt1 description
+                    broken:
+                type: "str"
+                required: true
+EOF
+
+# ensure that --metadata-dump does not fail when --no-fail-on-errors is supplied
+ANSIBLE_LIBRARY='./nolibrary' ansible-doc --metadata-dump --no-fail-on-errors --playbook-dir broken-docs testns.testcol
+
+# ensure that --metadata-dump does fail when --no-fail-on-errors is not supplied
+output=$(ANSIBLE_LIBRARY='./nolibrary' ansible-doc --metadata-dump --playbook-dir broken-docs testns.testcol 2>&1 | grep -c 'ERROR!' || true)
+test "$output" -eq 1