From 8d93ba91208ca5a0d2919acb1a5697e71f64d2f9 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Thu, 11 Jun 2020 20:03:43 +0200 Subject: [PATCH] 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. --- .../69796-ansible-doc-return-values.yml | 4 ++ .../command_plugins/plugin_formatter.py | 8 +-- lib/ansible/cli/doc.py | 23 +++----- lib/ansible/parsing/plugin_docs.py | 4 +- lib/ansible/utils/plugin_docs.py | 10 +++- test/integration/targets/ansible-doc/aliases | 1 - .../testcol/plugins/vars/noop_vars_plugin.py | 2 +- .../ansible-doc/library/test_docs_returns.py | 56 +++++++++++++++++++ .../library/test_docs_returns_broken.py | 40 +++++++++++++ test/integration/targets/ansible-doc/runme.sh | 11 +--- test/integration/targets/ansible-doc/test.yml | 40 +++++++++++++ .../ansible-doc/test_docs_returns.output | 37 ++++++++++++ .../ansible-doc/test_docs_suboptions.output | 5 +- 13 files changed, 203 insertions(+), 38 deletions(-) create mode 100644 changelogs/fragments/69796-ansible-doc-return-values.yml create mode 100644 test/integration/targets/ansible-doc/library/test_docs_returns.py create mode 100644 test/integration/targets/ansible-doc/library/test_docs_returns_broken.py create mode 100644 test/integration/targets/ansible-doc/test_docs_returns.output diff --git a/changelogs/fragments/69796-ansible-doc-return-values.yml b/changelogs/fragments/69796-ansible-doc-return-values.yml new file mode 100644 index 00000000000..c8b6a41405b --- /dev/null +++ b/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." diff --git a/hacking/build_library/build_ansible/command_plugins/plugin_formatter.py b/hacking/build_library/build_ansible/command_plugins/plugin_formatter.py index e6cb3913db6..475d86d97ce 100644 --- a/hacking/build_library/build_ansible/command_plugins/plugin_formatter.py +++ b/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'] = 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 + doc['returndocs'] = module_map[module]['returndocs'] + process_returndocs(doc['returndocs']) else: doc['returndocs'] = None diff --git a/lib/ansible/cli/doc.py b/lib/ansible/cli/doc.py index bcd52dc12ac..cb06de7bdf3 100644 --- a/lib/ansible/cli/doc.py +++ b/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,8 +547,9 @@ class DocCLI(CLI): choices = "(Choices: " + ", ".join(to_text(i) for i in opt['choices']) + ")" del opt['choices'] default = '' - if 'default' in opt or not required: - default = "[Default: %s" % to_text(opt.pop('default', '(null)')) + "]" + if not return_values: + if 'default' in opt or not required: + default = "[Default: %s" % to_text(opt.pop('default', '(null)')) + "]" text.append(textwrap.fill(DocCLI.tty_ify(aliases + choices + default), limit, initial_indent=opt_indent, subsequent_indent=opt_indent)) @@ -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): - text.append(doc.pop('returndocs')) - else: - text.append(yaml.dump(doc.pop('returndocs'), indent=2, default_flow_style=False)) + DocCLI.add_fields(text, doc.pop('returndocs'), limit, opt_indent, return_values=True) return "\n".join(text) diff --git a/lib/ansible/parsing/plugin_docs.py b/lib/ansible/parsing/plugin_docs.py index 468fed14077..bdbde6eb7b8 100644 --- a/lib/ansible/parsing/plugin_docs.py +++ b/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']: - # examples 'can' be yaml, return must be, but even if so, we dont want to parse as such here + if theid == 'EXAMPLES': + # 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: diff --git a/lib/ansible/utils/plugin_docs.py b/lib/ansible/utils/plugin_docs.py index 26388476316..02c6fb46363 100644 --- a/lib/ansible/utils/plugin_docs.py +++ b/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'] diff --git a/test/integration/targets/ansible-doc/aliases b/test/integration/targets/ansible-doc/aliases index 268da2c7020..a6dafcf8cd8 100644 --- a/test/integration/targets/ansible-doc/aliases +++ b/test/integration/targets/ansible-doc/aliases @@ -1,2 +1 @@ shippable/posix/group1 -disabled diff --git a/test/integration/targets/ansible-doc/collections/ansible_collections/testns/testcol/plugins/vars/noop_vars_plugin.py b/test/integration/targets/ansible-doc/collections/ansible_collections/testns/testcol/plugins/vars/noop_vars_plugin.py index ccb33b04dd2..2ff181ab3b7 100644 --- a/test/integration/targets/ansible-doc/collections/ansible_collections/testns/testcol/plugins/vars/noop_vars_plugin.py +++ b/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: diff --git a/test/integration/targets/ansible-doc/library/test_docs_returns.py b/test/integration/targets/ansible-doc/library/test_docs_returns.py new file mode 100644 index 00000000000..77c1376453f --- /dev/null +++ b/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() diff --git a/test/integration/targets/ansible-doc/library/test_docs_returns_broken.py b/test/integration/targets/ansible-doc/library/test_docs_returns_broken.py new file mode 100644 index 00000000000..d6d62643f0a --- /dev/null +++ b/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() diff --git a/test/integration/targets/ansible-doc/runme.sh b/test/integration/targets/ansible-doc/runme.sh index 84a69ee103b..7cc6bfe449f 100755 --- a/test/integration/targets/ansible-doc/runme.sh +++ b/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) - post=$(ansible-doc -l -t ${ptype} --playbook-dir ./|wc -l) - test "$pre" -eq $((post - 1)) + # FIXME pre=$(ansible-doc -l -t ${ptype}|wc -l) + # FIXME post=$(ansible-doc -l -t ${ptype} --playbook-dir ./|wc -l) + # 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) diff --git a/test/integration/targets/ansible-doc/test.yml b/test/integration/targets/ansible-doc/test.yml index b0e04d918f3..e03fb49a48e 100644 --- a/test/integration/targets/ansible-doc/test.yml +++ b/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 diff --git a/test/integration/targets/ansible-doc/test_docs_returns.output b/test/integration/targets/ansible-doc/test_docs_returns.output new file mode 100644 index 00000000000..9fbbc8c7f4a --- /dev/null +++ b/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 diff --git a/test/integration/targets/ansible-doc/test_docs_suboptions.output b/test/integration/targets/ansible-doc/test_docs_suboptions.output index fb2a993f237..52b51d9d276 100644 --- a/test/integration/targets/ansible-doc/test_docs_suboptions.output +++ b/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: -