From 04e816e13b4f3adda09a9a0f20266349bf89c870 Mon Sep 17 00:00:00 2001 From: John R Barker Date: Mon, 13 Mar 2017 19:49:27 +0000 Subject: [PATCH] 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. --- .../developing_modules_checklist.rst | 31 +++--- .../developing_modules_documenting.rst | 56 +++++++++-- hacking/templates/rst.j2 | 45 ++++++++- test/sanity/validate-modules/schema.py | 98 ++++++++++++++----- test/sanity/validate-modules/validate-modules | 16 +-- 5 files changed, 183 insertions(+), 63 deletions(-) diff --git a/docs/docsite/rst/dev_guide/developing_modules_checklist.rst b/docs/docsite/rst/dev_guide/developing_modules_checklist.rst index 8c6fd95005..bb81898581 100644 --- a/docs/docsite/rst/dev_guide/developing_modules_checklist.rst +++ b/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 -_old_cloud.py. This keeps the module available, but hides it from the primary docs and listing. +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. -When deprecating a module, set the `ANSIBLE_METADATA` `status` to `deprecated`. -In the `DOCUMENTATION` section, add a `deprecated` field along the lines of:: +When deprecating a module: + +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:: diff --git a/docs/docsite/rst/dev_guide/developing_modules_documenting.rst b/docs/docsite/rst/dev_guide/developing_modules_documenting.rst index 9157bb46d0..1c719e6d53 100644 --- a/docs/docsite/rst/dev_guide/developing_modules_documenting.rst +++ b/docs/docsite/rst/dev_guide/developing_modules_documenting.rst @@ -143,9 +143,13 @@ 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: - + :option-name: * Declarative operation (not CRUD)–this makes it easy for a user not to care what the existing state is, just about the final state, for example `online:`, rather than `is_online:`. @@ -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,14 +170,18 @@ 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"``. - :requirements: - List of requirements, and minimum versions (if applicable) - :notes: + :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: Details of any important information that doesn't fit in one of the above sections; for example if ``check_mode`` isn't supported, or a link to external documentation. @@ -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:: diff --git a/hacking/templates/rst.j2 b/hacking/templates/rst.j2 index 012e6ee7ce..f401fda6fa 100644 --- a/hacking/templates/rst.j2 +++ b/hacking/templates/rst.j2 @@ -70,17 +70,54 @@ Options {% for k in option_keys %} {% set v = options[k] %} - - @{ k }@
{% if v['version_added'] %} (added in @{v['version_added']}@){% endif %}
+ {% if not v['suboptions'] %} + @{ k }@
{% if v['version_added'] %} (added in @{v['version_added']}@){% endif %}
{% if v.get('required', False) %}yes{% else %}no{% endif %} {% if v['default'] %}@{ v['default'] }@{% endif %} {% if v.get('type', 'not_bool') == 'bool' %} {% else %} - + {% if v['choices'] %}{% endif %} {% endif %} {% for desc in v.description -%}
@{ desc | replace('\n', '\n ') | html_ify }@
{% endfor -%} {% if 'aliases' in v and v.aliases -%}
-
aliases: @{ v.aliases|join(', ') }@
{%- endif %} +
aliases: @{ v.aliases|join(', ') }@
{%- endif %} + {% else %} + @{ k }@
{% if v['version_added'] %} (added in @{v['version_added']}@){% endif %}
+ {% if v.get('required', False) %}yes{% else %}no{% endif %} + + {% for desc in v.description -%}
@{ desc | replace('\n', '\n ') | html_ify }@
{% endfor -%} {% if 'aliases' in v and v.aliases -%}
+
aliases: @{ v.aliases|join(', ') }@
{%- endif %} + + + + + + + + + + + + + {% for k2 in v['suboptions'] %} + {% set v2 = v['suboptions'] [k2] %} + + + + {% if v2.get('type', 'not_bool') == 'bool' %} + + {% else %} + + {% endif %} + + {% endfor %} +
Dictionary object @{ k }@
parameterrequireddefaultchoicescomments
@{ k2 }@
{% if v2['version_added'] %} (added in @{v2['version_added']}@){% endif %}
{% if v2.get('required', False) %}yes{% else %}no{% endif %}{% if v2['default'] %}@{ v2['default'] }@{% endif %}
  • yes
  • no
{% if v2['choices'] %}
    {% for choice in v2.get('choices',[]) -%}
  • @{ choice }@
    • {% endfor -%}
{% endif %}		{% for desc in v2.description -%}
@{ desc | replace('\n', '\n ') | html_ify }@
{% endfor -%} {% if 'aliases' in v and v2.aliases -%}
+
aliases: @{ v2.aliases|join(', ') }@
{%- endif %} +
+ + + {% endif %} + {% endfor %}
diff --git a/test/sanity/validate-modules/schema.py b/test/sanity/validate-modules/schema.py index 7aef6e7e61..72db0eb4e8 100644 --- a/test/sanity/validate-modules/schema.py +++ b/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 . -from voluptuous import ALLOW_EXTRA, Any, Required, Schema +from voluptuous import PREVENT_EXTRA, Any, Required, Schema + +suboption_schema = Schema( + { + Required('description'): Any(basestring, [basestring]), + 'required': bool, + 'choices': list, + 'aliases': Any(basestring, list), + '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=PREVENT_EXTRA +) option_schema = Schema( { Required('description'): Any(basestring, [basestring]), 'required': bool, 'choices': list, - 'aliases': list, - 'version_added': Any(basestring, float) - }, - extra=ALLOW_EXTRA -) - -doc_schema = Schema( - { - Required('module'): basestring, - 'short_description': basestring, - 'description': Any(basestring, [basestring]), + 'aliases': Any(basestring, list), 'version_added': Any(basestring, float), - 'author': Any(None, basestring, [basestring]), - 'notes': Any(None, [basestring]), - 'requirements': [basestring], - 'options': Any(None, dict), - 'extends_documentation_fragment': Any(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=ALLOW_EXTRA + extra=PREVENT_EXTRA ) -metadata_schema = Schema( - { - Required('status'): [Any('stableinterface', 'preview', 'deprecated', - 'removed')], - Required('version'): '1.0', - Required('supported_by'): Any('core', 'community', 'unmaintained', - 'committer') - } -) +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], + 'todo': Any(None, basestring, [basestring]), + 'options': Any(None, {basestring: option_schema}), + 'extends_documentation_fragment': Any(basestring, [basestring]) + }, + extra=PREVENT_EXTRA + ) + +def metadata_schema(deprecated): + valid_status = Any('stableinterface', 'preview', 'deprecated', 'removed') + if deprecated: + valid_status = Any('deprecated') + + return Schema( + { + Required('status'): [valid_status], + 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) diff --git a/test/sanity/validate-modules/validate-modules b/test/sanity/validate-modules/validate-modules index 11dfc2ef9e..07bcc1b3c9 100755 --- a/test/sanity/validate-modules/validate-modules +++ b/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