Stricter module documentation validation (#22353)

Raise the bar for module `DOCUMENTAION`
This validator update was used to find the issues in https://github.com/ansible/ansible/pull/22297/files

**Validation**
* Updated Validation and docs to enforce more (items fixed in https://github.com/ansible/ansible/pull/22297/files)
* Use `suboptions` to document complex options 
* Validate module name
* Validate deprecated modules have correct ANSIBLE_METADATA

**Module Documentation Generation**
* Document `suboptions:` Example https://gist.github.com/gundalow/4bdc3669d696268328ccc18528cc6718
* Tidy up HTML generation (valid HTML, no empty lists, etc)
 
**Documentation**
* Clarify the steps for deprecating a module
* Use correct RST headings
* Document `suboptions:` (options)
* Document `contains:` (returns)


**Details**
The aim is to get this (and corresponding module updates) complete by the time `devel` becomes `2.4`, as this allows us to raise the bar for new modules

Example `suboptions` https://gist.github.com/gundalow/4bdc3669d696268328ccc18528cc6718

The aim is to get this PR integrated into `devel` *before* we branch `stable-2.3`, this will allows us to:
* Raise the bar for new modules in 2.4
* Ensure the generated module documentation for 2.3 and higher is improved, important as we will be doing versioned docs moving forward.

docs/docsite/rst/dev_guide/developing_modules_checklist.rst

@@ -1,7 +1,8 @@
 .. _module_contribution:
 
+===================================
 Contributing Your Module to Ansible
-```````````````````````````````````
+===================================
 
 High-quality modules with minimal dependencies
 can be included in Ansible, but modules (just due to the programming
@@ -14,8 +15,9 @@ gives them slightly higher development priority (though they'll work in exactly
 
 .. formerly marked with _module_dev_testing:
 
+------------------------------
 Contributing Modules Checklist
-``````````````````````````````
+------------------------------
 
 The following  checklist items are important guidelines for people who want to contribute to the development of modules to Ansible on GitHub. Please read the guidelines before you submit your PR/proposal.
 
@@ -133,7 +135,7 @@ The complete module metadata specification is here: https://github.com/ansible/p
   like :envvar:`API_USERNAME` would conflict between modules.
 
 Windows modules checklist
-`````````````````````````
+=========================
 * Favour native powershell and .net ways of doing things over calls to COM libraries or calls to native executables which may or may not be present in all versions of Windows
 * modules are in powershell (.ps1 files) but the docs reside in same name python file (.py)
 * look at ansible/lib/ansible/module_utils/powershell.ps1 for common code, avoid duplication
@@ -158,7 +160,7 @@ To parse all arguments into a variable modules generally use:
     $params = Parse-Args $args
 
 Arguments
-+++++++++
+---------
 
 * Try and use state present and state absent like other modules
 * You need to check that all your mandatory args are present. You can do this using the builtin Get-AnsibleParam function.
@@ -175,7 +177,7 @@ Required arguments with name validation:
         $state = Get-AnsibleParam -obj $params -name "State" -ValidateSet "Present","Absent" -resultobj $resultobj -failifempty $true
 
 Optional arguments with name validation
-+++++++++++++++++++++++++++++++++++++++
+---------------------------------------
 
 .. code-block:: powershell
 
@@ -186,7 +188,7 @@ Optional arguments with name validation
 * Look at existing modules for more examples of argument checking.
 
 Results
-+++++++
+-------
 * The result object should always contain an attribute called changed set to either $true or $false
 * Create your result object like this
 
@@ -208,17 +210,22 @@ Results
 * Have you tested for powershell 3.0 and 4.0 compliance?
 
 Deprecating and making module aliases
-``````````````````````````````````````
+======================================
 
-Starting in 1.8, you can deprecate modules by renaming them with a preceding _, i.e. old_cloud.py to
+Starting in 1.8, you can deprecate modules by renaming them with a preceding ``_``, i.e. ``old_cloud.py`` to
-_old_cloud.py. This keeps the module available, but hides it from the primary docs and listing.
+``_old_cloud.py``. This keeps the module available, but hides it from the primary docs and listing.
 
-When deprecating a module, set the `ANSIBLE_METADATA` `status` to `deprecated`.
+When deprecating a module:
-In the `DOCUMENTATION` section, add a `deprecated` field along the lines of::
+
+1) Set the ``ANSIBLE_METADATA`` `status` to `deprecated`.
+2) In the ``DOCUMENTATION`` section, add a `deprecated` field along the lines of::
 
     deprecated: Deprecated in 2.3. Use M(whatmoduletouseinstead) instead.
 
-Add the deprecation to CHANGELOG.md
+3) Add the deprecation to CHANGELOG.md under the ``###Deprecations:`` section.
+
+Alias module names
+------------------
 
 You can also rename modules and keep an alias to the old name by using a symlink that starts with _.
 This example allows the stat module to be called with fileinfo, making the following examples equivalent::

docs/docsite/rst/dev_guide/developing_modules_documenting.rst

@@ -143,6 +143,10 @@ The following fields can be used and are all required unless specified otherwise
   This is a `string`, and not a float, i.e. ``version_added: "2.1"``
 :author:
   Name of the module author in the form ``First Last (@GitHubID)``. Use a multi-line list if there is more than one author.
+:deprecated:
+  If this module is deprecated, detail when that happened, and what to use instead, e.g.
+  `Deprecated in 2.3. Use M(whatmoduletouseinstead) instead.`
+  Ensure `CHANGELOG.md` is updated to reflect this.
 :options:
   One per module argument:
 
@@ -155,7 +159,6 @@ The following fields can be used and are all required unless specified otherwise
 
     * Detailed explanation of what this option does. It should be written in full sentences.
     * Should not list the options values (that's what ``choices:`` is for, though it should explain `what` the values do if they aren't obvious.
-    * If an argument takes both True)/False and Yes)/No, the documentation should use True and False.
     * If an optional parameter is sometimes required this need to be reflected in the documentation, e.g. "Required when I(state=present)."
     * Mutually exclusive options must be documented as the final sentence on each of the options.
   :required:
@@ -167,11 +170,15 @@ The following fields can be used and are all required unless specified otherwise
     * The default option must not be listed as part of the description.
   :choices:
     List of option values. Should be absent if empty.
+  :type:
+    If an argument is ``type='bool'``, this field should be set to ``type: bool`` and no ``choices`` should be specified.
   :aliases:
     List of option name aliases; generally not needed.
   :version_added:
     Only needed if this option was extended after initial Ansible release, i.e. this is greater than the top level `version_added` field.
     This is a string, and not a float, i.e. ``version_added: "2.3"``.
+  :suboptions:
+    If this option takes a dict, you can define it here. See `azure_rm_securitygroup`, `os_ironic_node` for examples.
 :requirements:
   List of requirements, and minimum versions (if applicable)
 :notes:
@@ -182,8 +189,6 @@ The following fields can be used and are all required unless specified otherwise
 
    - The above fields are are all in lowercase.
 
-   - There is no need to document the ``type:`` of an option.
-
    - If the module doesn't doesn't have any options (for example, it's a ``_facts`` module), you can use ``options: {}``.
 
 EXAMPLES block
@@ -216,6 +221,41 @@ The RETURN section documents what the module returns, and is required for all ne
 For each value returned, provide a ``description``, in what circumstances the value is ``returned``,
 the ``type`` of the value and a ``sample``.  For example, from the ``copy`` module::
 
+
+The following fields can be used and are all required unless specified otherwise.
+
+:return name:
+  Name of the returned field.
+
+  :description:
+    Detailed description of what this value represents.
+  :returned:
+    When this value is returned, such as `always`, on `success`, `always`
+  :type:
+    Data type
+  :sample:
+    One or more examples.
+  :contains:
+    Optional, if you set `type: complex` you can detail the dictionary here by repeating the above elements.
+
+    :return name:
+      One per return
+
+      :description:
+        Detailed description of what this value represents.
+      :returned:
+        When this value is returned, such as `always`, on `success`, `always`
+      :type:
+        Data type
+      :sample:
+        One or more examples.
+
+
+For complex nested returns type can be specified as ``type: complex``.
+
+Example::
+
+
     RETURN = '''
     dest:
         description: destination file/path
@@ -233,7 +273,7 @@ the ``type`` of the value and a ``sample``.  For example, from the ``copy`` modu
         type: string
         sample: 2a5aeecc61dc98c4d780b14b330e3282
     ...
-    '''
+
 
 .. note::
 

hacking/templates/rst.j2

@@ -70,17 +70,54 @@ Options
     </tr>
     {% for k in option_keys %}
     {% set v = options[k] %}
-    <tr>
+    {% if not v['suboptions'] %}
-    <td>@{ k }@<br/><div style="font-size: small;">{% if v['version_added'] %} (added in @{v['version_added']}@){% endif %}</div></td>
+    <tr><td>@{ k }@<br/><div style="font-size: small;">{% if v['version_added'] %} (added in @{v['version_added']}@){% endif %}</div></td>
     <td>{% if v.get('required', False) %}yes{% else %}no{% endif %}</td>
     <td>{% if v['default'] %}@{ v['default'] }@{% endif %}</td>
     {% if v.get('type', 'not_bool') == 'bool' %}
     <td><ul><li>yes</li><li>no</li></ul></td>
     {% else %}
-    <td><ul>{% for choice in v.get('choices',[]) -%}<li>@{ choice }@</li>{% endfor -%}</ul></td>
+    <td>{% if v['choices'] %}<ul>{% for choice in v.get('choices',[]) -%}<li>@{ choice }@</li>{% endfor -%}</ul>{% endif %}</td>
     {% endif %}
     <td>{% for desc in v.description -%}<div>@{ desc | replace('\n', '\n    ') | html_ify }@</div>{% endfor -%} {% if 'aliases' in v and v.aliases -%}</br>
-        <div style="font-size: small;">aliases: @{ v.aliases|join(', ') }@<div>{%- endif %}</td></tr>
+    <div style="font-size: small;">aliases: @{ v.aliases|join(', ') }@<div>{%- endif %}
+    {% else %}
+    <tr><td rowspan="2">@{ k }@<br/><div style="font-size: small;">{% if v['version_added'] %} (added in @{v['version_added']}@){% endif %}</div></td>
+    <td>{% if v.get('required', False) %}yes{% else %}no{% endif %}</td>
+    <td></td><td></td>
+    <td> {% for desc in v.description -%}<div>@{ desc | replace('\n', '\n    ') | html_ify }@</div>{% endfor -%} {% if 'aliases' in v and v.aliases -%}</br>
+    <div style="font-size: small;">aliases: @{ v.aliases|join(', ') }@<div>{%- endif %}
+    </tr>
+    <tr>
+    <td colspan="5">
+    <table border=1 cellpadding=4>
+    <caption><b>Dictionary object @{ k }@</b></caption>
+    <tr>
+    <th class="head">parameter</th>
+    <th class="head">required</th>
+    <th class="head">default</th>
+    <th class="head">choices</th>
+    <th class="head">comments</th>
+    </tr>
+    {% for k2 in v['suboptions'] %}
+        {% set v2 = v['suboptions'] [k2] %}
+        <tr><td>@{ k2 }@<br/><div style="font-size: small;">{% if v2['version_added'] %} (added in @{v2['version_added']}@){% endif %}</div></td>
+        <td>{% if v2.get('required', False) %}yes{% else %}no{% endif %}</td>
+        <td>{% if v2['default'] %}@{ v2['default'] }@{% endif %}</td>
+        {% if v2.get('type', 'not_bool') == 'bool' %}
+        <td><ul><li>yes</li><li>no</li></ul></td>
+        {% else %}
+        <td>{% if v2['choices'] %}<ul>{% for choice in v2.get('choices',[]) -%}<li>@{ choice }@</li>{% endfor -%}</ul>{% endif %}</td>
+        {% endif %}
+        <td>{% for desc in v2.description -%}<div>@{ desc | replace('\n', '\n    ') | html_ify }@</div>{% endfor -%} {% if 'aliases' in v and v2.aliases -%}</br>
+        <div style="font-size: small;">aliases: @{ v2.aliases|join(', ') }@<div>{%- endif %}
+        </td></tr>
+    {% endfor %}
+    </table>
+    </td>
+    </tr>
+    {% endif %}
+    </td></tr>
     {% endfor %}
     </table>
     </br>

test/sanity/validate-modules/schema.py

@@ -16,40 +16,84 @@
 #    You should have received a copy of the GNU General Public License
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from voluptuous import ALLOW_EXTRA, Any, Required, Schema
+from voluptuous import PREVENT_EXTRA, Any, Required, Schema
 
-option_schema = Schema(
+suboption_schema = Schema(
     {
         Required('description'): Any(basestring, [basestring]),
         'required': bool,
         'choices': list,
-        'aliases': list,
+        'aliases': Any(basestring, list),
-        'version_added': Any(basestring, float)
+        'version_added': Any(basestring, float),
+        'default': Any(None, basestring, float, int, bool, list, dict),
+        # Note: Types are strings, not literal bools, such as True or False
+        'type': Any(None, "bool")
     },
-    extra=ALLOW_EXTRA
+    extra=PREVENT_EXTRA
 )
 
-doc_schema = Schema(
+option_schema = Schema(
     {
-        Required('module'): basestring,
+        Required('description'): Any(basestring, [basestring]),
-        'short_description': basestring,
+        'required': bool,
-        'description': Any(basestring, [basestring]),
+        'choices': list,
+        'aliases': Any(basestring, list),
         'version_added': Any(basestring, float),
-        'author': Any(None, basestring, [basestring]),
+        'default': Any(None, basestring, float, int, bool, list, dict),
+        'suboptions': Any(None, {basestring: suboption_schema,}),
+        # Note: Types are strings, not literal bools, such as True or False
+        'type': Any(None, "bool")
+    },
+    extra=PREVENT_EXTRA
+)
+
+def doc_schema(module_name):
+    if module_name.startswith('_'):
+        module_name = module_name[1:]
+    return Schema(
+        {
+            Required('module'): module_name,
+            'deprecated': basestring,
+            Required('short_description'): basestring,
+            Required('description'): Any(basestring, [basestring]),
+            Required('version_added'): Any(basestring, float),
+            Required('author'): Any(None, basestring, [basestring]),
             'notes': Any(None, [basestring]),
             'requirements': [basestring],
-        'options': Any(None, dict),
+            'todo': Any(None, basestring, [basestring]),
+            'options': Any(None, {basestring: option_schema}),
             'extends_documentation_fragment': Any(basestring, [basestring])
         },
-    extra=ALLOW_EXTRA
+        extra=PREVENT_EXTRA
     )
 
-metadata_schema = Schema(
+def metadata_schema(deprecated):
+    valid_status = Any('stableinterface', 'preview', 'deprecated', 'removed')
+    if deprecated:
+        valid_status = Any('deprecated')
+
+    return Schema(
         {
-        Required('status'): [Any('stableinterface', 'preview', 'deprecated',
+            Required('status'): [valid_status],
-                                 'removed')],
             Required('version'): '1.0',
             Required('supported_by'): Any('core', 'community', 'unmaintained',
                                           'committer')
         }
     )
+
+
+
+# Things to add soon
+####################
+# 1) Validate RETURN, including `contains` if `type: complex`
+#    This will improve documentation, though require fair amount of module tidyup
+
+# Possible Future Enhancements
+##############################
+
+# 1) Don't allow empty options for choices, aliases, etc
+# 2) If type: bool ensure choices isn't set - perhaps use Exclusive
+# 3) both version_added should be quoted floats
+# 4) Use Recursive Schema: https://github.com/alecthomas/voluptuous/issues/128 though don't allow two layers
+
+#  Tool that takes JSON and generates RETURN skeleton (needs to support complex structures)

test/sanity/validate-modules/validate-modules

@@ -609,16 +609,6 @@ class ModuleValidator(Validator):
                 error.data = doc
             errors.extend(e.errors)
 
-        options = doc.get('options', {})
-        for key, option in (options or {}).items():
-            try:
-                option_schema(option)
-            except Exception as e:
-                for error in e.errors:
-                    error.path[:0] = ['options', key]
-                    error.data = option
-                errors.extend(e.errors)
-
         for error in errors:
             path = [str(p) for p in error.path]
 
@@ -668,14 +658,16 @@ class ModuleValidator(Validator):
                          'with DOCUMENTATION.extends_documentation_fragment')
                     ))
 
+                deprecated = False
                 if self.object_name.startswith('_') and not os.path.islink(self.object_path):
+                    deprecated = True
                     if 'deprecated' not in doc or not doc.get('deprecated'):
                         self.errors.append((
                             318,
                             'Module deprecated, but DOCUMENTATION.deprecated is missing'
                         ))
 
-                self._validate_docs_schema(doc, doc_schema, 'DOCUMENTATION', 305)
+                self._validate_docs_schema(doc, doc_schema(self.object_name.split('.')[0]), 'DOCUMENTATION', 305)
                 self._check_version_added(doc)
                 self._check_for_new_args(doc)
 
@@ -718,7 +710,7 @@ class ModuleValidator(Validator):
                 self.traces.extend(traces)
 
             if metadata:
-                self._validate_docs_schema(metadata, metadata_schema,
+                self._validate_docs_schema(metadata, metadata_schema(deprecated),
                                            'ANSIBLE_METADATA', 316)
 
         return doc_info