Plugin/module docs: parse return values, add collection names in them (version_added_collection), and format them nicely in ansible-doc (#69796)

* Tag return value docs if they are a dict (and not str/None).

* Try to parse return docs as YAML.

* Properly dump return values in ansible-doc.

* Adjust plugin formatter.

* Add changelog fragment.

* Don't add 'default' for return values.

* Fix plugin_formatter.

* Only try to parse return docs if they are still a string.

* Add tests.

* Warn if RETURN cannot be parsed.

* Adjust tests. Also test for warning.

* if -> elif (otherwise EXAMPLE will be parsed too).

* Always parse return documentation, and fail if it is invalid YAML.

* Polishing.

* Mostly re-enable ansible-doc tests.

Listing from the local collection seems to be somewhat broken. I assume this
is why the test was disabled.

* Lint and make tests work with Python 2.

* Keep FQCNs in plugins (not modules), i.e. restore previous state.

changelogs/fragments/69796-ansible-doc-return-values.yml

@@ -0,0 +1,4 @@
+minor_changes:
+- "ansible-doc - return values will be properly formatted (https://github.com/ansible/ansible/pull/69796)."
+major_changes:
+- "Both ansible-doc and ansible-console's help command will error for modules and plugins whose return documentation cannot be parsed as YAML. All modules and plugins passing ``ansible-test sanity --test yamllint`` will not be affected by this."

hacking/build_library/build_ansible/command_plugins/plugin_formatter.py

@@ -527,12 +527,8 @@ def process_plugins(module_map, templates, outputname, output_dir, ansible_versi
 
         display.vvvvv(pp.pformat(module_map[module]))
         if module_map[module]['returndocs']:
-            try:
+            doc['returndocs'] = module_map[module]['returndocs']
-                doc['returndocs'] = yaml.safe_load(module_map[module]['returndocs'])
             process_returndocs(doc['returndocs'])
-            except Exception as e:
-                print("%s:%s:yaml error:%s:returndocs=%s" % (fname, module, e, module_map[module]['returndocs']))
-                doc['returndocs'] = None
         else:
             doc['returndocs'] = None
 

lib/ansible/cli/doc.py

@@ -235,13 +235,6 @@ class DocCLI(CLI):
                 plugin_docs[plugin] = {'doc': doc, 'examples': plainexamples, 'return': returndocs, 'metadata': metadata}
 
             if do_json:
-                # Some changes to how json docs are formatted
-                for plugin, doc_data in plugin_docs.items():
-                    try:
-                        doc_data['return'] = yaml.safe_load(doc_data['return'])
-                    except Exception:
-                        pass
-
                 jdump(plugin_docs)
 
             else:
@@ -346,6 +339,8 @@ class DocCLI(CLI):
         # TODO: do we really want this?
         # add_collection_to_versions_and_dates(doc, '(unknown)', is_module=(plugin_type == 'module'))
         # remove_current_collection_from_versions_and_dates(doc, collection_name, is_module=(plugin_type == 'module'))
+        # remove_current_collection_from_versions_and_dates(
+        #     returndocs, collection_name, is_module=(plugin_type == 'module'), return_docs=True)
 
         # assign from other sections
         doc['plainexamples'] = plainexamples
@@ -515,7 +510,7 @@ class DocCLI(CLI):
                                                    Dumper=AnsibleDumper).split('\n')]))
 
     @staticmethod
-    def add_fields(text, fields, limit, opt_indent, base_indent=''):
+    def add_fields(text, fields, limit, opt_indent, return_values=False, base_indent=''):
 
         for o in sorted(fields):
             opt = fields[o]
@@ -552,6 +547,7 @@ class DocCLI(CLI):
                     choices = "(Choices: " + ", ".join(to_text(i) for i in opt['choices']) + ")"
                 del opt['choices']
             default = ''
+            if not return_values:
                 if 'default' in opt or not required:
                     default = "[Default: %s" % to_text(opt.pop('default', '(null)')) + "]"
 
@@ -591,7 +587,7 @@ class DocCLI(CLI):
             for subkey, subdata in suboptions:
                 text.append('')
                 text.append("%s%s:\n" % (opt_indent, subkey.upper()))
-                DocCLI.add_fields(text, subdata, limit, opt_indent + '    ', opt_indent)
+                DocCLI.add_fields(text, subdata, limit, opt_indent + '    ', return_values, opt_indent)
             if not suboptions:
                 text.append('')
 
@@ -705,9 +701,6 @@ class DocCLI(CLI):
 
         if doc.get('returndocs', False):
             text.append("RETURN VALUES:")
-            if isinstance(doc['returndocs'], string_types):
+            DocCLI.add_fields(text, doc.pop('returndocs'), limit, opt_indent, return_values=True)
-                text.append(doc.pop('returndocs'))
-            else:
-                text.append(yaml.dump(doc.pop('returndocs'), indent=2, default_flow_style=False))
 
         return "\n".join(text)

lib/ansible/parsing/plugin_docs.py

@@ -56,8 +56,8 @@ def read_docstring(filename, verbose=True, ignore_errors=True):
                         if isinstance(child.value, ast.Dict):
                             data[varkey] = ast.literal_eval(child.value)
                         else:
-                            if theid in ['EXAMPLES', 'RETURN']:
+                            if theid == 'EXAMPLES':
-                                # examples 'can' be yaml, return must be, but even if so, we dont want to parse as such here
+                                # examples 'can' be yaml, but even if so, we dont want to parse as such here
                                 # as it can create undesired 'objects' that don't display well as docs.
                                 data[varkey] = to_text(child.value.s)
                             else:

lib/ansible/utils/plugin_docs.py

@@ -85,6 +85,9 @@ def _process_versions_and_dates(fragment, is_module, return_docs, callback):
             if isinstance(return_value.get('contains'), MutableMapping):
                 process_return_values(return_value['contains'])
 
+    if not fragment:
+        return
+
     if return_docs:
         process_return_values(fragment)
         return
@@ -203,13 +206,18 @@ def get_docstring(filename, fragment_loader, verbose=False, ignore_errors=False,
     data = read_docstring(filename, verbose=verbose, ignore_errors=ignore_errors)
 
     if data.get('doc', False):
-        # tag version_added
+        # add collection name to versions and dates
         if collection_name is not None:
             add_collection_to_versions_and_dates(data['doc'], collection_name, is_module=is_module)
 
         # add fragments to documentation
         add_fragments(data['doc'], filename, fragment_loader=fragment_loader, is_module=is_module)
 
+    if data.get('returndocs', False):
+        # add collection name to versions and dates
+        if collection_name is not None:
+            add_collection_to_versions_and_dates(data['returndocs'], collection_name, is_module=is_module, return_docs=True)
+
     return data['doc'], data['plainexamples'], data['returndocs'], data['metadata']
 
 

test/integration/targets/ansible-doc/aliases

@@ -1,2 +1 @@
 shippable/posix/group1
-disabled

test/integration/targets/ansible-doc/collections/ansible_collections/testns/testcol/plugins/vars/noop_vars_plugin.py

@@ -2,7 +2,7 @@ from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
 DOCUMENTATION = '''
-    vars: noop_vars_plugin
+    vars: testns.testcol.noop_vars_plugin
     short_description: Do NOT load host and group vars
     description: don't test loading host and group vars from a collection
     options:

test/integration/targets/ansible-doc/library/test_docs_returns.py

@@ -0,0 +1,56 @@
+#!/usr/bin/python
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+
+DOCUMENTATION = '''
+---
+module: test_docs_returns
+short_description: Test module
+description:
+    - Test module
+author:
+    - Ansible Core Team
+'''
+
+EXAMPLES = '''
+'''
+
+RETURN = '''
+z_last:
+    description: A last result.
+    type: str
+    returned: success
+
+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]
+
+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()

test/integration/targets/ansible-doc/library/test_docs_returns_broken.py

@@ -0,0 +1,40 @@
+#!/usr/bin/python
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+
+DOCUMENTATION = '''
+---
+module: test_docs_returns_broken
+short_description: Test module
+description:
+    - Test module
+author:
+    - Ansible Core Team
+'''
+
+EXAMPLES = '''
+'''
+
+RETURN = '''
+test:
+    description: A test return value.
+   type: str
+
+broken_key: [
+'''
+
+
+from ansible.module_utils.basic import AnsibleModule
+
+
+def main():
+    module = AnsibleModule(
+        argument_spec=dict(),
+    )
+
+    module.exit_json()
+
+
+if __name__ == '__main__':
+    main()

test/integration/targets/ansible-doc/runme.sh

@@ -12,18 +12,13 @@ current_out="$(ansible-doc --playbook-dir ./ testns.testcol.fakemodule)"
 expected_out="$(cat fakemodule.output)"
 test "$current_out" == "$expected_out"
 
-# test module docs from collection
-current_out="$(ansible-doc --playbook-dir ./ test_docs_suboptions)"
-expected_out="$(cat test_docs_suboptions.output)"
-test "$current_out" == "$expected_out"
-
 # test listing diff plugin types from collection
 for ptype in cache inventory lookup vars
 do
 	# each plugin type adds 1 from collection
-	pre=$(ansible-doc -l -t ${ptype}|wc -l)
+	# FIXME pre=$(ansible-doc -l -t ${ptype}|wc -l)
-	post=$(ansible-doc -l -t ${ptype} --playbook-dir ./|wc -l)
+	# FIXME post=$(ansible-doc -l -t ${ptype} --playbook-dir ./|wc -l)
-	test "$pre" -eq $((post - 1))
+	# FIXME test "$pre" -eq $((post - 1))
 
 	# ensure we ONLY list from the collection
 	justcol=$(ansible-doc -l -t ${ptype} --playbook-dir ./ testns.testcol|wc -l)

test/integration/targets/ansible-doc/test.yml

@@ -3,6 +3,46 @@
   environment:
     ANSIBLE_LIBRARY: "{{ playbook_dir }}/library"
   tasks:
+    - name: module with suboptions
+      command: ansible-doc test_docs_suboptions
+      register: result
+      ignore_errors: true
+
+    - set_fact:
+        actual_output: >-
+          {{ result.stdout | regex_replace('^(> [A-Z_]+ +\().+library/([a-z_]+.py)\)$', '\1library/\2)', multiline=true) }}
+        expected_output: "{{ lookup('file', 'test_docs_suboptions.output') }}"
+
+    - assert:
+        that:
+          - result is succeeded
+          - actual_output == expected_output
+
+    - name: module with return docs
+      command: ansible-doc test_docs_returns
+      register: result
+      ignore_errors: true
+
+    - set_fact:
+        actual_output: >-
+          {{ result.stdout | regex_replace('^(> [A-Z_]+ +\().+library/([a-z_]+.py)\)$', '\1library/\2)', multiline=true) }}
+        expected_output: "{{ lookup('file', 'test_docs_returns.output') }}"
+
+    - assert:
+        that:
+          - result is succeeded
+          - actual_output == expected_output
+
+    - name: module with broken return docs
+      command: ansible-doc test_docs_returns_broken
+      register: result
+      ignore_errors: true
+
+    - assert:
+        that:
+          - result is failed
+          - '"ERROR! module test_docs_returns_broken missing documentation (or could not parse documentation)" in result.stderr'
+
     - name: non-existent module
       command: ansible-doc test_does_not_exist
       register: result

test/integration/targets/ansible-doc/test_docs_returns.output

@@ -0,0 +1,37 @@
+> TEST_DOCS_RETURNS    (library/test_docs_returns.py)
+
+        Test module
+
+AUTHOR: Ansible Core Team
+
+EXAMPLES:
+
+
+
+
+RETURN VALUES:
+- a_first
+        A first result.
+
+        returned: success
+        type: str
+
+- m_middle
+        This should be in the middle.
+        Has some more data
+
+        returned: success and 1st of month
+        type: dict
+
+        CONTAINS:
+
+        - suboption
+            A suboption.
+            (Choices: ARF, BARN, c_without_capital_first_letter)
+            type: str
+
+- z_last
+        A last result.
+
+        returned: success
+        type: str

test/integration/targets/ansible-doc/test_docs_suboptions.output

@@ -1,4 +1,4 @@
-> TEST_DOCS_SUBOPTIONS    (/home/felix/projects/code/github-cloned/ansible/test/integration/targets/ansible-doc/library/test_docs_suboptions.py)
+> TEST_DOCS_SUBOPTIONS    (library/test_docs_suboptions.py)
 
         Test module
 
@@ -41,6 +41,3 @@ EXAMPLES:
 
 
 
-
-RETURN VALUES:
-