From 4d984613f5e16e205434cdf7290572e62b40bf62 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Thu, 17 Mar 2022 20:16:40 +0100 Subject: [PATCH] validate-modules for plugins: do some more schema validations so that some issues that are currently reported for modules are also reported for plugins (#77268) * Add more sanity tests on schema level (so they also work for plugins). * Fix various issues the sanity test reported. * Add changelog fragment. * Fix function name. --- .../fragments/77268-validate-plugins.yml | 2 + lib/ansible/modules/add_host.py | 1 + lib/ansible/modules/assert.py | 1 + lib/ansible/modules/group_by.py | 1 + lib/ansible/modules/include_vars.py | 2 + lib/ansible/modules/reboot.py | 1 + lib/ansible/modules/user.py | 2 +- lib/ansible/modules/yum_repository.py | 20 +-- lib/ansible/plugins/become/su.py | 1 + lib/ansible/plugins/connection/winrm.py | 1 + .../plugins/doc_fragments/shell_common.py | 3 + .../plugins/doc_fragments/shell_windows.py | 1 + lib/ansible/plugins/inventory/yaml.py | 1 + lib/ansible/plugins/lookup/first_found.py | 2 + lib/ansible/plugins/lookup/url.py | 1 + lib/ansible/plugins/vars/host_group_vars.py | 1 + .../validate-modules/validate_modules/main.py | 10 -- .../validate_modules/schema.py | 146 +++++++++++++++++- test/sanity/ignore.txt | 1 - 19 files changed, 173 insertions(+), 25 deletions(-) create mode 100644 changelogs/fragments/77268-validate-plugins.yml diff --git a/changelogs/fragments/77268-validate-plugins.yml b/changelogs/fragments/77268-validate-plugins.yml new file mode 100644 index 00000000000..6889bd39642 --- /dev/null +++ b/changelogs/fragments/77268-validate-plugins.yml @@ -0,0 +1,2 @@ +minor_changes: + - "ansible-test validate-modules sanity test - add more schema checks to improve quality of plugin documentation (https://github.com/ansible/ansible/pull/77268)." diff --git a/lib/ansible/modules/add_host.py b/lib/ansible/modules/add_host.py index 5d08a7f69c3..b446df59530 100644 --- a/lib/ansible/modules/add_host.py +++ b/lib/ansible/modules/add_host.py @@ -28,6 +28,7 @@ options: description: - The groups to add the hostname to. type: list + elements: str aliases: [ group, groupname ] extends_documentation_fragment: - action_common_attributes diff --git a/lib/ansible/modules/assert.py b/lib/ansible/modules/assert.py index 52e5e36cf6e..71b2424e766 100644 --- a/lib/ansible/modules/assert.py +++ b/lib/ansible/modules/assert.py @@ -20,6 +20,7 @@ options: description: - A list of string expressions of the same form that can be passed to the 'when' statement. type: list + elements: str required: true fail_msg: description: diff --git a/lib/ansible/modules/group_by.py b/lib/ansible/modules/group_by.py index 41004ea359b..ef641f2cf82 100644 --- a/lib/ansible/modules/group_by.py +++ b/lib/ansible/modules/group_by.py @@ -31,6 +31,7 @@ options: description: - The list of the parent groups. type: list + elements: str default: all version_added: "2.4" attributes: diff --git a/lib/ansible/modules/include_vars.py b/lib/ansible/modules/include_vars.py index 1286bef5c07..d341df0050e 100644 --- a/lib/ansible/modules/include_vars.py +++ b/lib/ansible/modules/include_vars.py @@ -54,11 +54,13 @@ options: description: - List of file names to ignore. type: list + elements: str version_added: "2.2" extensions: description: - List of file extensions to read when using C(dir). type: list + elements: str default: [ json, yaml, yml ] version_added: "2.3" ignore_unknown_extensions: diff --git a/lib/ansible/modules/reboot.py b/lib/ansible/modules/reboot.py index d3575e8be8e..71e6294eac2 100644 --- a/lib/ansible/modules/reboot.py +++ b/lib/ansible/modules/reboot.py @@ -59,6 +59,7 @@ options: - Paths to search on the remote machine for the C(shutdown) command. - I(Only) these paths will be searched for the C(shutdown) command. C(PATH) is ignored in the remote node when searching for the C(shutdown) command. type: list + elements: str default: ['/sbin', '/bin', '/usr/sbin', '/usr/bin', '/usr/local/sbin'] version_added: '2.8' diff --git a/lib/ansible/modules/user.py b/lib/ansible/modules/user.py index 346d91e618d..b247ba3682e 100644 --- a/lib/ansible/modules/user.py +++ b/lib/ansible/modules/user.py @@ -146,8 +146,8 @@ options: ssh_key_bits: description: - Optionally specify number of bits in SSH key to create. + - The default value depends on ssh-keygen. type: int - default: default set by ssh-keygen version_added: "0.9" ssh_key_type: description: diff --git a/lib/ansible/modules/yum_repository.py b/lib/ansible/modules/yum_repository.py index cc1866e0369..fed1e4bc81b 100644 --- a/lib/ansible/modules/yum_repository.py +++ b/lib/ansible/modules/yum_repository.py @@ -34,7 +34,7 @@ options: (bytes/sec) then this option is ignored. Default is C(0) (no bandwidth throttling). type: str - default: 0 + default: '0' baseurl: description: - URL to the directory where the yum repository's 'repodata' directory @@ -49,7 +49,7 @@ options: - Relative cost of accessing this repository. Useful for weighing one repo's packages as greater/less than any other. type: str - default: 1000 + default: '1000' deltarpm_metadata_percentage: description: - When the relative size of deltarpm metadata vs pkgs is larger than @@ -58,14 +58,14 @@ options: required to be half the size of the packages. Use C(0) to turn off this check, and always download metadata. type: str - default: 100 + default: '100' deltarpm_percentage: description: - When the relative size of delta vs pkg is larger than this, delta is not used. Use C(0) to turn off delta rpm processing. Local repositories (with file:// I(baseurl)) have delta rpms turned off by default. type: str - default: 75 + default: '75' description: description: - A human readable string describing the repository. This option corresponds to the "name" property in the repo file. @@ -160,7 +160,7 @@ options: - Determines how yum resolves host names. - C(4) or C(IPv4) - resolve to IPv4 addresses only. - C(6) or C(IPv6) - resolve to IPv6 addresses only. - choices: [4, 6, IPv4, IPv6, whatever] + choices: ['4', '6', IPv4, IPv6, whatever] type: str default: whatever keepalive: @@ -182,7 +182,7 @@ options: - Time (in seconds) after which the metadata will expire. - Default value is 6 hours. type: str - default: 21600 + default: '21600' metadata_expire_filter: description: - Filter the I(metadata_expire) time, allowing a trade of speed for @@ -224,7 +224,7 @@ options: expire. - Default value is 6 hours. type: str - default: 21600 + default: '21600' name: description: - Unique repository ID. This option builds the section name of the repository in the repo file. @@ -242,7 +242,7 @@ options: from 1 to 99. - This option only works if the YUM Priorities plugin is installed. type: str - default: 99 + default: '99' protect: description: - Protect packages from updates from other repositories. @@ -277,7 +277,7 @@ options: - Set the number of times any attempt to retrieve a file should retry before returning an error. Setting this to C(0) makes yum try forever. type: str - default: 10 + default: '10' s3_enabled: description: - Enables support for S3 repositories. @@ -341,7 +341,7 @@ options: description: - Number of seconds to wait for a connection before timing out. type: str - default: 30 + default: '30' ui_repoid_vars: description: - When a repository id is displayed, append these yum variables to the diff --git a/lib/ansible/plugins/become/su.py b/lib/ansible/plugins/become/su.py index e12a1bc892a..6e54bc336e2 100644 --- a/lib/ansible/plugins/become/su.py +++ b/lib/ansible/plugins/become/su.py @@ -81,6 +81,7 @@ DOCUMENTATION = """ if you add another one in your string, your prompt will fail with a "Timeout" error. default: [] type: list + elements: string ini: - section: su_become_plugin key: localized_prompts diff --git a/lib/ansible/plugins/connection/winrm.py b/lib/ansible/plugins/connection/winrm.py index 5aad5ee560d..f4720238f45 100644 --- a/lib/ansible/plugins/connection/winrm.py +++ b/lib/ansible/plugins/connection/winrm.py @@ -78,6 +78,7 @@ DOCUMENTATION = """ - If None (the default) the plugin will try to automatically guess the correct list - The choices available depend on your version of pywinrm type: list + elements: string vars: - name: ansible_winrm_transport kerberos_command: diff --git a/lib/ansible/plugins/doc_fragments/shell_common.py b/lib/ansible/plugins/doc_fragments/shell_common.py index c1c7f0b15a1..fe1ae4ee8f2 100644 --- a/lib/ansible/plugins/doc_fragments/shell_common.py +++ b/lib/ansible/plugins/doc_fragments/shell_common.py @@ -43,6 +43,7 @@ options: the first one from the list will be used instead. default: [ /var/tmp, /tmp ] type: list + elements: string env: [{name: ANSIBLE_SYSTEM_TMPDIRS}] ini: - section: defaults @@ -61,6 +62,7 @@ options: - name: ansible_async_dir environment: type: list + elements: dictionary default: [{}] description: - List of dictionaries of environment variables and their values to use when executing commands. @@ -68,6 +70,7 @@ options: - name: environment admin_users: type: list + elements: string default: ['root', 'toor'] description: - list of users to be expected to have admin privileges. This is used by the controller to diff --git a/lib/ansible/plugins/doc_fragments/shell_windows.py b/lib/ansible/plugins/doc_fragments/shell_windows.py index e4d648c7af3..af4de955fff 100644 --- a/lib/ansible/plugins/doc_fragments/shell_windows.py +++ b/lib/ansible/plugins/doc_fragments/shell_windows.py @@ -46,5 +46,6 @@ options: keyword: - name: environment type: list + elements: dictionary default: [{}] """ diff --git a/lib/ansible/plugins/inventory/yaml.py b/lib/ansible/plugins/inventory/yaml.py index aac89d31e0d..409970cf3d0 100644 --- a/lib/ansible/plugins/inventory/yaml.py +++ b/lib/ansible/plugins/inventory/yaml.py @@ -21,6 +21,7 @@ DOCUMENTATION = ''' yaml_extensions: description: list of 'valid' extensions for files containing YAML type: list + elements: string default: ['.yaml', '.yml', '.json'] env: - name: ANSIBLE_YAML_FILENAME_EXT diff --git a/lib/ansible/plugins/lookup/first_found.py b/lib/ansible/plugins/lookup/first_found.py index f09ff98f56e..5b94b103a4e 100644 --- a/lib/ansible/plugins/lookup/first_found.py +++ b/lib/ansible/plugins/lookup/first_found.py @@ -24,10 +24,12 @@ DOCUMENTATION = """ files: description: A list of file names. type: list + elements: string default: [] paths: description: A list of paths in which to look for the files. type: list + elements: string default: [] skip: type: boolean diff --git a/lib/ansible/plugins/lookup/url.py b/lib/ansible/plugins/lookup/url.py index 010b69a9878..9e2d911e1b8 100644 --- a/lib/ansible/plugins/lookup/url.py +++ b/lib/ansible/plugins/lookup/url.py @@ -138,6 +138,7 @@ options: unredirected_headers: description: A list of headers to not attach on a redirected request type: list + elements: string version_added: "2.10" vars: - name: ansible_lookup_url_unredir_headers diff --git a/lib/ansible/plugins/vars/host_group_vars.py b/lib/ansible/plugins/vars/host_group_vars.py index 960ae828742..7eb02949c07 100644 --- a/lib/ansible/plugins/vars/host_group_vars.py +++ b/lib/ansible/plugins/vars/host_group_vars.py @@ -48,6 +48,7 @@ DOCUMENTATION = ''' - key: yaml_valid_extensions section: defaults type: list + elements: string extends_documentation_fragment: - vars_plugin_staging ''' diff --git a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py index 36846233115..0bdd9dee21c 100644 --- a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py +++ b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py @@ -1927,16 +1927,6 @@ class ModuleValidator(Validator): doc_elements = doc_options_arg.get('elements', None) doc_type = doc_options_arg.get('type', 'str') data_elements = data.get('elements', None) - if (doc_elements and not doc_type == 'list'): - msg = "Argument '%s' " % arg - if context: - msg += " found in %s" % " -> ".join(context) - msg += " defines parameter elements as %s but it is valid only when value of parameter type is list" % doc_elements - self.reporter.error( - path=self.object_path, - code='doc-elements-invalid', - msg=msg - ) if (doc_elements or data_elements) and not (doc_elements == data_elements): msg = "Argument '%s' in argument_spec" % arg if context: diff --git a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/schema.py b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/schema.py index b628d7856a8..67e44b790a8 100644 --- a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/schema.py +++ b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/schema.py @@ -14,6 +14,8 @@ from urllib.parse import urlparse from voluptuous import ALLOW_EXTRA, PREVENT_EXTRA, All, Any, Invalid, Length, Required, Schema, Self, ValueInvalid, Exclusive from ansible.module_utils.six import string_types from ansible.module_utils.common.collections import is_iterable +from ansible.module_utils.parsing.convert_bool import boolean +from ansible.parsing.quoting import unquote from ansible.utils.version import SemanticVersion from ansible.release import __version__ @@ -384,6 +386,134 @@ def version_added(v, error_code='version-added-invalid', accept_historical=False return v +def check_option_elements(v): + # Check whether elements is there iff type == 'list' + v_type = v.get('type') + v_elements = v.get('elements') + if v_type == 'list' and v_elements is None: + raise _add_ansible_error_code( + Invalid('Argument defines type as list but elements is not defined'), + error_code='parameter-list-no-elements') # FIXME: adjust error code? + if v_type != 'list' and v_elements is not None: + raise _add_ansible_error_code( + Invalid('Argument defines parameter elements as %s but it is valid only when value of parameter type is list' % (v_elements, )), + error_code='doc-elements-invalid') + return v + + +def get_type_checker(v): + v_type = v.get('type') + if v_type == 'list': + elt_checker, elt_name = get_type_checker({'type': v.get('elements')}) + + def list_checker(value): + if isinstance(value, string_types): + value = [unquote(x.strip()) for x in value.split(',')] + if not isinstance(value, list): + raise ValueError('Value must be a list') + if elt_checker: + for elt in value: + try: + elt_checker(elt) + except Exception as exc: + raise ValueError('Entry %r is not of type %s: %s' % (elt, elt_name, exc)) + + return list_checker, ('list of %s' % elt_name) if elt_checker else 'list' + + if v_type in ('boolean', 'bool'): + return partial(boolean, strict=False), v_type + + if v_type in ('integer', 'int'): + return int, v_type + + if v_type == 'float': + return float, v_type + + if v_type == 'none': + def none_checker(value): + if value not in ('None', None): + raise ValueError('Value must be "None" or none') + + return none_checker, v_type + + if v_type in ('str', 'string', 'path', 'tmp', 'temppath', 'tmppath'): + def str_checker(value): + if not isinstance(value, string_types): + raise ValueError('Value must be string') + + return str_checker, v_type + + if v_type in ('pathspec', 'pathlist'): + def path_list_checker(value): + if not isinstance(value, string_types) and not is_iterable(value): + raise ValueError('Value must be string or list of strings') + + return path_list_checker, v_type + + if v_type in ('dict', 'dictionary'): + def dict_checker(value): + if not isinstance(value, dict): + raise ValueError('Value must be dictionary') + + return dict_checker, v_type + + return None, 'unknown' + + +def check_option_choices(v): + # Check whether choices have the correct type + v_choices = v.get('choices') + if not is_iterable(v_choices): + return v + + if v.get('type') == 'list': + # choices for a list type means that every list element must be one of these choices + type_checker, type_name = get_type_checker({'type': v.get('elements')}) + else: + type_checker, type_name = get_type_checker(v) + if type_checker is None: + return v + + for value in v_choices: + try: + type_checker(value) + except Exception as exc: + raise _add_ansible_error_code( + Invalid( + 'Argument defines choices as (%r) but this is incompatible with argument type %s: %s' % (value, type_name, exc)), + error_code='doc-choices-incompatible-type') + + return v + + +def check_option_default(v): + # Check whether default is only present if required=False, and whether default has correct type + v_default = v.get('default') + if v.get('required') and v_default is not None: + raise _add_ansible_error_code( + Invalid( + 'Argument is marked as required but specifies a default.' + ' Arguments with a default should not be marked as required'), + error_code='no-default-for-required-parameter') # FIXME: adjust error code? + + if v_default is None: + return v + + type_checker, type_name = get_type_checker(v) + if type_checker is None: + return v + + try: + type_checker(v_default) + except Exception as exc: + raise _add_ansible_error_code( + Invalid( + 'Argument defines default as (%r) but this is incompatible with parameter type %s: %s' % (v_default, type_name, exc)), + error_code='incompatible-default-type') + + return v + + def list_dict_option_schema(for_collection, plugin_type): if plugin_type == 'module': option_types = Any(None, 'bits', 'bool', 'bytes', 'dict', 'float', 'int', 'json', 'jsonarg', 'list', 'path', 'raw', 'sid', 'str') @@ -433,7 +563,7 @@ def list_dict_option_schema(for_collection, plugin_type): partial(check_removal_version, version_field='version', collection_name_field='collection_name', - error_code='invalid-removal-version') + error_code='invalid-removal-version'), ) env_schema = All( Schema({ @@ -496,7 +626,12 @@ def list_dict_option_schema(for_collection, plugin_type): # Recursive suboptions 'suboptions': Any(None, *list({str_type: Self} for str_type in string_types)), }) - suboption_schema = Schema(suboption_schema, extra=PREVENT_EXTRA) + suboption_schema = Schema(All( + suboption_schema, + check_option_elements, + check_option_choices, + check_option_default, + ), extra=PREVENT_EXTRA) # This generates list of dicts with keys from string_types and suboption_schema value # for example in Python 3: {str: suboption_schema} @@ -506,7 +641,12 @@ def list_dict_option_schema(for_collection, plugin_type): option_schema.update({ 'suboptions': Any(None, *list_dict_suboption_schema), }) - option_schema = Schema(option_schema, extra=PREVENT_EXTRA) + option_schema = Schema(All( + option_schema, + check_option_elements, + check_option_choices, + check_option_default, + ), extra=PREVENT_EXTRA) option_version_added = Schema( All({ diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index 268aeefc94f..e99ce827cd8 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -96,7 +96,6 @@ lib/ansible/modules/systemd.py validate-modules:return-syntax-error lib/ansible/modules/sysvinit.py validate-modules:return-syntax-error lib/ansible/modules/uri.py validate-modules:doc-required-mismatch lib/ansible/modules/user.py validate-modules:doc-default-does-not-match-spec -lib/ansible/modules/user.py validate-modules:doc-default-incompatible-type lib/ansible/modules/user.py validate-modules:use-run-command-not-popen lib/ansible/modules/yum.py pylint:disallowed-name lib/ansible/modules/yum.py validate-modules:parameter-invalid