From e9f8a34dce4576cdbbeb10c9c03c495b4475eda9 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Wed, 30 Oct 2019 09:48:21 -0700 Subject: [PATCH] Fixes for validate-modules import handling. (#63932) * Fix validate-modules support for collections. - Relative imports now work correctly. - The collection loader is now used. - Modules are invoked as `__main__`. * Remove obsolete validate-modules code ignores. * Handle sys.exit in validate-modules. * Add check for AnsibleModule initialization. * Remove `missing-module-utils-import` check. This check does not support relative imports or collections. Instead of trying to overhaul the test, we can rely on the `ansible-module-not-initialized` test instead. * Fix badly named error codes with `c#` in the name. The `#` conflicts with comments in the sanity test ignore files. * Add changelog entries. --- .../ansible-test-validate-modules-fixes.yml | 8 +++ .../dev_guide/testing_validate-modules.rst | 6 +- .../validate-modules/validate_modules/main.py | 64 +++++++++++++++---- .../validate_modules/module_args.py | 39 +++++------ .../validate_modules/utils.py | 15 +++++ .../_internal/sanity/validate_modules.py | 8 --- 6 files changed, 95 insertions(+), 45 deletions(-) create mode 100644 changelogs/fragments/ansible-test-validate-modules-fixes.yml diff --git a/changelogs/fragments/ansible-test-validate-modules-fixes.yml b/changelogs/fragments/ansible-test-validate-modules-fixes.yml new file mode 100644 index 00000000000..4e56fa637fd --- /dev/null +++ b/changelogs/fragments/ansible-test-validate-modules-fixes.yml @@ -0,0 +1,8 @@ +bugfixes: + - ansible-test validate-modules sanity test now properly handles collections imports using the Ansible collection loader. + - ansible-test validate-modules sanity test now properly handles relative imports. + - ansible-test validate-modules sanity test now properly invokes Ansible modules as scripts. + - ansible-test validate-modules sanity test now properly handles sys.exit in modules. + - ansible-test validate-modules sanity test now checks for AnsibleModule initialization instead of module_utils imports, which did not work in many cases. + - ansible-test validate-modules sanity test code ``multiple-c#-utils-per-requires`` is now ``multiple-csharp-utils-per-requires`` (fixes ignore bug). + - ansible-test validate-modules sanity test code ``missing-module-utils-import-c#-requirements`` is now ``missing-module-utils-import-csharp-requirements`` (fixes ignore bug). diff --git a/docs/docsite/rst/dev_guide/testing_validate-modules.rst b/docs/docsite/rst/dev_guide/testing_validate-modules.rst index 38b862158ed..59c3cdbf69c 100644 --- a/docs/docsite/rst/dev_guide/testing_validate-modules.rst +++ b/docs/docsite/rst/dev_guide/testing_validate-modules.rst @@ -61,6 +61,7 @@ Codes ============================================================ ================== ==================== ========================================================================================= **Error Code** **Type** **Level** **Sample Message** ------------------------------------------------------------ ------------------ -------------------- ----------------------------------------------------------------------------------------- + ansible-module-not-initialized Syntax Error Execution of the module did not result in initialization of AnsibleModule deprecation-mismatch Documentation Error Module marked as deprecated or removed in at least one of the filename, its metadata, or in DOCUMENTATION (setting DOCUMENTATION.deprecated for deprecation or removing all Documentation for removed) but not in all three places. doc-choices-do-not-match-spec Documentation Error Value for "choices" from the argument_spec does not match the documentation doc-choices-incompatible-type Documentation Error Choices value from the documentation is not compatible with type defined in the argument_spec @@ -98,8 +99,7 @@ Codes missing-main-call Syntax Error Did not find a call to ``main()`` (or ``removed_module()`` in the case of deprecated & docs only modules) missing-metadata Documentation Error No ``ANSIBLE_METADATA`` provided missing-module-utils-basic-import Imports Warning Did not find ``ansible.module_utils.basic`` import - missing-module-utils-import Imports Error Did not find a ``module_utils`` import - missing-module-utils-import-c# Imports Error No ``Ansible.ModuleUtils`` or C# Ansible util requirements/imports found + missing-module-utils-import-csharp-requirements Imports Error No ``Ansible.ModuleUtils`` or C# Ansible util requirements/imports found missing-powershell-interpreter Syntax Error Interpreter line is not ``#!powershell`` missing-python-doc Naming Error Missing python documentation file missing-python-interpreter Syntax Error Interpreter line is not ``#!/usr/bin/python`` @@ -110,7 +110,7 @@ Codes module-invalid-version-added Documentation Error Module level ``version_added`` is not a valid version number module-utils-specific-import Imports Error ``module_utils`` imports should import specific components, not ``*`` multiple-utils-per-requires Imports Error ``Ansible.ModuleUtils`` requirements do not support multiple modules per statement - multiple-c#-utils-per-requires Imports Error Ansible C# util requirements do not support multiple utils per statement + multiple-csharp-utils-per-requires Imports Error Ansible C# util requirements do not support multiple utils per statement no-default-for-required-parameter Documentation Error Option is marked as required but specifies a default. Arguments with a default should not be marked as required nonexistent-parameter-documented Documentation Error Argument is listed in DOCUMENTATION.options, but not accepted by the module option-incorrect-version-added Documentation Error ``version_added`` for new option is incorrect diff --git a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py index 4e39100468b..694ce36ccc5 100644 --- a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py +++ b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py @@ -38,10 +38,12 @@ from fnmatch import fnmatch from ansible import __version__ as ansible_version from ansible.executor.module_common import REPLACER_WINDOWS from ansible.module_utils.common._collections_compat import Mapping +from ansible.module_utils._text import to_bytes from ansible.plugins.loader import fragment_loader +from ansible.utils.collection_loader import AnsibleCollectionLoader from ansible.utils.plugin_docs import BLACKLIST, add_fragments, get_docstring -from .module_args import AnsibleModuleImportError, get_argument_spec +from .module_args import AnsibleModuleImportError, AnsibleModuleNotInitialized, get_argument_spec from .schema import ansible_module_kwargs_schema, doc_schema, metadata_1_1_schema, return_schema @@ -517,13 +519,7 @@ class ModuleValidator(Validator): name.name == 'basic'): found_basic = True - if not linenos: - self.reporter.error( - path=self.object_path, - code='missing-module-utils-import', - msg='Did not find a module_utils import' - ) - elif not found_basic: + if not found_basic: self.reporter.warning( path=self.object_path, code='missing-module-utils-basic-import', @@ -751,7 +747,7 @@ class ModuleValidator(Validator): if len(module_list) > 1: self.reporter.error( path=self.object_path, - code='multiple-c#-utils-per-requires', + code='multiple-csharp-utils-per-requires', msg='Ansible C# util requirements do not support multiple utils per statement: "%s"' % req_stmt.group(0) ) continue @@ -769,7 +765,7 @@ class ModuleValidator(Validator): if not found_requires and REPLACER_WINDOWS not in self.text: self.reporter.error( path=self.object_path, - code='missing-module-utils-import-c#-requirements', + code='missing-module-utils-import-csharp-requirements', msg='No Ansible.ModuleUtils or C# Ansible util requirements/imports found' ) @@ -1133,7 +1129,14 @@ class ModuleValidator(Validator): def _validate_ansible_module_call(self, docs): try: - spec, args, kwargs = get_argument_spec(self.path) + spec, args, kwargs = get_argument_spec(self.path, self.collection) + except AnsibleModuleNotInitialized: + self.reporter.error( + path=self.object_path, + code='ansible-module-not-initialized', + msg="Execution of the module did not result in initialization of AnsibleModule", + ) + return except AnsibleModuleImportError as e: self.reporter.error( path=self.object_path, @@ -1698,6 +1701,42 @@ class PythonPackageValidator(Validator): ) +def setup_collection_loader(): + def get_source(self, fullname): + mod = sys.modules.get(fullname) + if not mod: + mod = self.load_module(fullname) + + with open(to_bytes(mod.__file__), 'rb') as mod_file: + source = mod_file.read() + + return source + + def get_code(self, fullname): + return compile(source=self.get_source(fullname), filename=self.get_filename(fullname), mode='exec', flags=0, dont_inherit=True) + + def is_package(self, fullname): + return self.get_filename(fullname).endswith('__init__.py') + + def get_filename(self, fullname): + mod = sys.modules.get(fullname) or self.load_module(fullname) + + return mod.__file__ + + # monkeypatch collection loader to work with runpy + # remove this (and the associated code above) once implemented natively in the collection loader + AnsibleCollectionLoader.get_source = get_source + AnsibleCollectionLoader.get_code = get_code + AnsibleCollectionLoader.is_package = is_package + AnsibleCollectionLoader.get_filename = get_filename + + collection_loader = AnsibleCollectionLoader() + + # allow importing code from collections when testing a collection + # noinspection PyCallingNonCallable + sys.meta_path.insert(0, collection_loader) + + def re_compile(value): """ Argparse expects things to raise TypeError, re.compile raises an re.error @@ -1745,6 +1784,9 @@ def run(): check_dirs = set() + if args.collection: + setup_collection_loader() + for module in args.modules: if os.path.isfile(module): path = module diff --git a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/module_args.py b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/module_args.py index d7a6f1de699..8d1b7ecf9a0 100644 --- a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/module_args.py +++ b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/module_args.py @@ -18,7 +18,7 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type -import imp +import runpy import json import os import subprocess @@ -28,7 +28,7 @@ from contextlib import contextmanager from ansible.module_utils.six import reraise -from .utils import CaptureStd, find_executable +from .utils import CaptureStd, find_executable, get_module_name_from_filename class AnsibleModuleCallError(RuntimeError): @@ -39,6 +39,10 @@ class AnsibleModuleImportError(ImportError): pass +class AnsibleModuleNotInitialized(Exception): + pass + + class _FakeAnsibleModuleInit: def __init__(self): self.args = tuple() @@ -104,33 +108,22 @@ def get_ps_argument_spec(filename): return kwargs['argument_spec'], (), kwargs -def get_py_argument_spec(filename): - # Calculate the module's name so that relative imports work correctly - name = None - try: - idx = filename.index('ansible/modules') - except ValueError: - try: - idx = filename.index('ansible_collections/') - except ValueError: - # We default to ``module`` here instead of ``__main__`` - # which helps with some import issues in this tool - # where modules may import things that conflict - name = 'module' - if name is None: - name = filename[idx:-len('.py')].replace('/', '.') +def get_py_argument_spec(filename, collection): + name = get_module_name_from_filename(filename, collection) with setup_env(filename) as fake: try: with CaptureStd(): - mod = imp.load_source(name, filename) - if not fake.called: - mod.main() + runpy.run_module(name, run_name='__main__') except AnsibleModuleCallError: pass - except Exception as e: + except BaseException as e: + # we want to catch all exceptions here, including sys.exit reraise(AnsibleModuleImportError, AnsibleModuleImportError('%s' % e), sys.exc_info()[2]) + if not fake.called: + raise AnsibleModuleNotInitialized() + try: try: # for ping kwargs == {'argument_spec':{'data':{'type':'str','default':'pong'}}, 'supports_check_mode':True} @@ -141,8 +134,8 @@ def get_py_argument_spec(filename): return {}, (), {} -def get_argument_spec(filename): +def get_argument_spec(filename, collection): if filename.endswith('.py'): - return get_py_argument_spec(filename) + return get_py_argument_spec(filename, collection) else: return get_ps_argument_spec(filename) diff --git a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/utils.py b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/utils.py index 3a083497131..f3a755de0c9 100644 --- a/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/utils.py +++ b/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/utils.py @@ -115,6 +115,21 @@ class CaptureStd(): return self.stdout.buffer.getvalue(), self.stderr.buffer.getvalue() +def get_module_name_from_filename(filename, collection): + # Calculate the module's name so that relative imports work correctly + if collection: + # collection is a relative path, example: ansible_collections/my_namespace/my_collection + # filename is a relative path, example: plugins/modules/my_module.py + path = os.path.join(collection, filename) + else: + # filename is a relative path, example: lib/ansible/modules/system/ping.py + path = os.path.relpath(filename, 'lib') + + name = os.path.splitext(path)[0].replace(os.path.sep, '.') + + return name + + def parse_yaml(value, lineno, module, name, load_all=False): traces = [] errors = [] diff --git a/test/lib/ansible_test/_internal/sanity/validate_modules.py b/test/lib/ansible_test/_internal/sanity/validate_modules.py index 3f90b419554..8cc65b94338 100644 --- a/test/lib/ansible_test/_internal/sanity/validate_modules.py +++ b/test/lib/ansible_test/_internal/sanity/validate_modules.py @@ -60,13 +60,6 @@ class ValidateModulesTest(SanitySingleVersion): :type python_version: str :rtype: TestResult """ - if data_context().content.is_ansible: - ignore_codes = () - else: - ignore_codes = (( - 'E502', # only ansible content requires __init__.py for module subdirectories - )) - env = ansible_environment(args, color=False) settings = self.load_processor(args) @@ -121,7 +114,6 @@ class ValidateModulesTest(SanitySingleVersion): message=item['msg'], )) - errors = [error for error in errors if error.code not in ignore_codes] errors = settings.process_errors(errors, paths) if errors: