diff --git a/changelogs/fragments/86079-ansible-test-validate-modules-bad-return-keys.yml b/changelogs/fragments/86079-ansible-test-validate-modules-bad-return-keys.yml new file mode 100644 index 00000000000..a9475ec4f32 --- /dev/null +++ b/changelogs/fragments/86079-ansible-test-validate-modules-bad-return-keys.yml @@ -0,0 +1,2 @@ +minor_changes: + - "ansible-test validate-modules sanity test - now reports bad return value keys that cannot be used with the dot notation in Jinja expressions (https://github.com/ansible/ansible/issues/86079)." diff --git a/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/constants.py b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/constants.py new file mode 100644 index 00000000000..18cbf665347 --- /dev/null +++ b/test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/constants.py @@ -0,0 +1,67 @@ +# -*- coding: utf-8 -*- +# +# Copyright (C) 2015 Matt Martz +# Copyright (C) 2015 Rackspace US, Inc. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +from __future__ import annotations + +import re + + +REJECTLIST_DIRS = frozenset(('.git', 'test', '.github', '.idea')) +SYS_EXIT_REGEX = re.compile(r'[^#]*sys.exit\s*\(.*') +NO_LOG_REGEX = re.compile(r'(?:pass(?!ive)|secret|token|key)', re.I) + +# Everything that should not be used in a dictionary of a return value, +# since it will make user's life harder. +FORBIDDEN_DICTIONARY_KEYS = frozenset([ + 'clear', + 'copy', + 'fromkeys', + 'get', + 'items', + 'keys', + 'pop', + 'popitem', + 'setdefault', + 'update', + 'values', +]) + + +REJECTLIST_IMPORTS = { + 'requests': { + 'new_only': True, + 'error': { + 'code': 'use-module-utils-urls', + 'msg': ('requests import found, should use ' + 'ansible.module_utils.urls instead') + } + }, + r'boto(?:\.|$)': { + 'new_only': True, + 'error': { + 'code': 'use-boto3', + 'msg': 'boto import found, new modules should use boto3' + } + }, +} +SUBPROCESS_REGEX = re.compile(r'subprocess\.Po.*') +OS_CALL_REGEX = re.compile(r'os\.call.*') + + +PLUGINS_WITH_RETURN_VALUES = ('module', ) +PLUGINS_WITH_EXAMPLES = ('module', ) +PLUGINS_WITH_YAML_EXAMPLES = ('module', ) 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 341c37f3789..e9a15377f02 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 @@ -85,6 +85,19 @@ from .schema import ( from .utils import CaptureStd, NoArgsAnsibleModule, compare_unordered_lists, parse_yaml, parse_isodate +from .constants import ( + REJECTLIST_DIRS, + SYS_EXIT_REGEX, + NO_LOG_REGEX, + FORBIDDEN_DICTIONARY_KEYS, + REJECTLIST_IMPORTS, + SUBPROCESS_REGEX, + OS_CALL_REGEX, + PLUGINS_WITH_RETURN_VALUES, + PLUGINS_WITH_EXAMPLES, + PLUGINS_WITH_YAML_EXAMPLES, +) + # Because there is no ast.TryExcept in Python 3 ast module TRY_EXCEPT = ast.Try @@ -92,41 +105,9 @@ TRY_EXCEPT = ast.Try # string but we need unicode for Python 3 REPLACER_WINDOWS = _REPLACER_WINDOWS.decode('utf-8') -REJECTLIST_DIRS = frozenset(('.git', 'test', '.github', '.idea')) -INDENT_REGEX = re.compile(r'([\t]*)') -SYS_EXIT_REGEX = re.compile(r'[^#]*sys.exit\s*\(.*') -NO_LOG_REGEX = re.compile(r'(?:pass(?!ive)|secret|token|key)', re.I) - - -REJECTLIST_IMPORTS = { - 'requests': { - 'new_only': True, - 'error': { - 'code': 'use-module-utils-urls', - 'msg': ('requests import found, should use ' - 'ansible.module_utils.urls instead') - } - }, - r'boto(?:\.|$)': { - 'new_only': True, - 'error': { - 'code': 'use-boto3', - 'msg': 'boto import found, new modules should use boto3' - } - }, -} -SUBPROCESS_REGEX = re.compile(r'subprocess\.Po.*') -OS_CALL_REGEX = re.compile(r'os\.call.*') - - LOOSE_ANSIBLE_VERSION = LooseVersion('.'.join(ansible_version.split('.')[:3])) -PLUGINS_WITH_RETURN_VALUES = ('module', ) -PLUGINS_WITH_EXAMPLES = ('module', ) -PLUGINS_WITH_YAML_EXAMPLES = ('module', ) - - def is_potential_secret_option(option_name): if not NO_LOG_REGEX.search(option_name): return False @@ -905,6 +886,27 @@ class ModuleValidator(Validator): msg=msg, ) + def _validate_return_docs(self, returns: object, context: list[str] | None = None) -> None: + if not isinstance(returns, dict): + return + if context is None: + context = [] + + for rv, data in returns.items(): + if isinstance(data, dict) and "contains" in data: + self._validate_return_docs(data["contains"], context + [rv]) + + if str(rv) in FORBIDDEN_DICTIONARY_KEYS or not str(rv).isidentifier(): + msg = f"Return value key {rv!r}" + if context: + msg += " found in %s" % " -> ".join(context) + msg += " should not be used for return values since it cannot be accessed with dot notation in Jinja" + self.reporter.error( + path=self.object_path, + code='bad-return-value-key', + msg=msg, + ) + def _validate_docs(self): doc = None # We have three ways of marking deprecated/removed files. Have to check each one @@ -1145,6 +1147,7 @@ class ModuleValidator(Validator): returns, return_schema(for_collection=bool(self.collection), plugin_type=self.plugin_type), 'RETURN', 'return-syntax-error') + self._validate_return_docs(returns) elif self.plugin_type in PLUGINS_WITH_RETURN_VALUES: if self._is_new_module(): diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index 5afea3523e8..bb296a812d9 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -18,6 +18,7 @@ lib/ansible/modules/copy.py validate-modules:undocumented-parameter lib/ansible/modules/dnf.py validate-modules:parameter-invalid lib/ansible/modules/dnf5.py validate-modules:parameter-invalid lib/ansible/modules/file.py validate-modules:undocumented-parameter +lib/ansible/modules/getent.py validate-modules:bad-return-value-key # used for documentation, not a real key lib/ansible/modules/git.py use-argspec-type-path lib/ansible/modules/git.py validate-modules:doc-required-mismatch lib/ansible/modules/package_facts.py validate-modules:doc-choices-do-not-match-spec diff --git a/test/units/ansible_test/_util/controller/sanity/validate-modules/validate_modules/conftest.py b/test/units/ansible_test/_util/controller/sanity/validate-modules/validate_modules/conftest.py new file mode 100644 index 00000000000..54bdf330890 --- /dev/null +++ b/test/units/ansible_test/_util/controller/sanity/validate-modules/validate_modules/conftest.py @@ -0,0 +1,17 @@ +from __future__ import annotations + + +import pytest +import sys + +from pathlib import Path + + +@pytest.fixture(autouse=True, scope='session') +def inject_ansible_test_validate_modules() -> None: + """Make ansible_test's validate-modules available on `sys.path` for unit testing ansible-test.""" + test_lib = ( + Path(__file__).parent / ".." / ".." / ".." / ".." / ".." / ".." / ".." + / "lib" / "ansible_test" / "_util" / "controller" / "sanity" / "validate-modules" + ) + sys.path.insert(0, str(test_lib)) diff --git a/test/units/ansible_test/_util/controller/sanity/validate-modules/validate_modules/test_main.py b/test/units/ansible_test/_util/controller/sanity/validate-modules/validate_modules/test_main.py new file mode 100644 index 00000000000..f8b41aecf5f --- /dev/null +++ b/test/units/ansible_test/_util/controller/sanity/validate-modules/validate_modules/test_main.py @@ -0,0 +1,10 @@ +"""Tests for validate-module's main module.""" +from __future__ import annotations + + +def test_dict_members() -> None: + from validate_modules.constants import FORBIDDEN_DICTIONARY_KEYS # type: ignore[import-not-found] + + expected_keys = [key for key in dict.__dict__ if not key.startswith("__")] + + assert FORBIDDEN_DICTIONARY_KEYS == frozenset(expected_keys)