From f510d59943f7f00f74e42ad78780e9e89787e991 Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Tue, 27 Aug 2019 18:11:21 -0700 Subject: [PATCH] Support relative imports in AnsiballZ. (#61196) --- changelogs/fragments/relative-import.yaml | 2 + .../rst/porting_guides/porting_guide_2.9.rst | 32 ++ hacking/test-module.py | 14 +- lib/ansible/executor/module_common.py | 446 ++++++++++++------ lib/ansible/module_utils/basic.py | 2 +- .../modules/cloud/amazon/lambda_event.py | 1 - lib/ansible/modules/system/setup.py | 2 +- .../modules/utilities/logic/async_wrapper.py | 7 +- .../ansiballz_python/library/custom_module.py | 19 + .../module_utils/custom_util.py | 6 + .../targets/ansiballz_python/tasks/main.yml | 7 + .../collections_relative_imports/aliases | 2 + .../my_col/plugins/module_utils/my_util1.py | 6 + .../my_col/plugins/module_utils/my_util2.py | 8 + .../my_col/plugins/module_utils/my_util3.py | 8 + .../my_ns/my_col/plugins/modules/my_module.py | 24 + .../my_ns/my_col/roles/test/tasks/main.yml | 4 + .../collections_relative_imports/runme.sh | 5 + .../collections_relative_imports/test.yml | 3 + .../_data/sanity/import/importer.py | 98 +++- .../validate_modules/module_args.py | 18 +- test/sanity/ignore.txt | 3 + .../module_common/test_module_common.py | 74 ++- .../module_common/test_recursive_finder.py | 79 ++-- 24 files changed, 681 insertions(+), 189 deletions(-) create mode 100644 changelogs/fragments/relative-import.yaml create mode 100644 test/integration/targets/ansiballz_python/library/custom_module.py create mode 100644 test/integration/targets/ansiballz_python/module_utils/custom_util.py create mode 100644 test/integration/targets/collections_relative_imports/aliases create mode 100644 test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util1.py create mode 100644 test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util2.py create mode 100644 test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util3.py create mode 100644 test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/modules/my_module.py create mode 100644 test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/roles/test/tasks/main.yml create mode 100755 test/integration/targets/collections_relative_imports/runme.sh create mode 100644 test/integration/targets/collections_relative_imports/test.yml diff --git a/changelogs/fragments/relative-import.yaml b/changelogs/fragments/relative-import.yaml new file mode 100644 index 0000000000..282654ba73 --- /dev/null +++ b/changelogs/fragments/relative-import.yaml @@ -0,0 +1,2 @@ +minor_changes: +- Ansible now supports relative imports of module_utils files in modules and module_utils. diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.9.rst b/docs/docsite/rst/porting_guides/porting_guide_2.9.rst index 8f4c663c67..cf3944120b 100644 --- a/docs/docsite/rst/porting_guides/porting_guide_2.9.rst +++ b/docs/docsite/rst/porting_guides/porting_guide_2.9.rst @@ -63,6 +63,38 @@ Modules * The ``win_get_url`` and ``win_uri`` module now sends requests with a default ``User-Agent`` of ``ansible-httpget``. This can be changed by using the ``http_agent`` key. +Writing modules +--------------- + +* Module and module_utils files can now use relative imports to include other module_utils files. + This is useful for shortening long import lines, especially in collections. + + Example of using a relative import in collections: + + .. code-block:: python + + # File: ansible_collections/my_namespace/my_collection/plugins/modules/my_module.py + # Old way to use an absolute import to import module_utils from the collection: + from ansible_collections.my_namespace.my_collection.plugins.module_utils import my_util + # New way using a relative import: + from ..module_utils import my_util + + Modules and module_utils shipped with Ansible can use relative imports as well but the savings + are smaller: + + .. code-block:: python + + # File: ansible/modules/system/ping.py + # Old way to use an absolute import to import module_utils from core: + from ansible.module_utils.basic import AnsibleModule + # New way using a relative import: + from ...module_utils.basic import AnsibleModule + + Each single dot (``.``) represents one level of the tree (equivalent to ``../`` in filesystem relative links). + + .. seealso:: `The Python Relative Import Docs `_ go into more detail of how to write relative imports. + + Modules removed --------------- diff --git a/hacking/test-module.py b/hacking/test-module.py index 72b387cc92..1a063082fd 100755 --- a/hacking/test-module.py +++ b/hacking/test-module.py @@ -28,6 +28,7 @@ # ./hacking/test-module.py -m lib/ansible/modules/files/lineinfile.py -a "dest=/etc/exports line='/srv/home hostname1(rw,sync)'" --check # ./hacking/test-module.py -m lib/ansible/modules/commands/command.py -a "echo hello" -n -o "test_hello" +import glob import optparse import os import subprocess @@ -193,8 +194,19 @@ def ansiballz_setup(modfile, modname, interpreters): sys.exit(err) debug_dir = lines[1].strip() + # All the directories in an AnsiBallZ that modules can live + core_dirs = glob.glob(os.path.join(debug_dir, 'ansible/modules')) + collection_dirs = glob.glob(os.path.join(debug_dir, 'ansible_collections/*/*/plugins/modules')) + + # There's only one module in an AnsiBallZ payload so look for the first module and then exit + for module_dir in core_dirs + collection_dirs: + for dirname, directories, filenames in os.walk(module_dir): + for filename in filenames: + if filename == modname + '.py': + modfile = os.path.join(dirname, filename) + break + argsfile = os.path.join(debug_dir, 'args') - modfile = os.path.join(debug_dir, '__main__.py') print("* ansiballz module detected; extracted module source to: %s" % debug_dir) return modfile, argsfile diff --git a/lib/ansible/executor/module_common.py b/lib/ansible/executor/module_common.py index 7f94f8c373..3b76ff7bbd 100644 --- a/lib/ansible/executor/module_common.py +++ b/lib/ansible/executor/module_common.py @@ -152,20 +152,14 @@ def _ansiballz_main(): sys.path = [p for p in sys.path if p != scriptdir] import base64 + import runpy import shutil import tempfile import zipfile if sys.version_info < (3,): - # imp is used on Python<3 - import imp - bytes = str - MOD_DESC = ('.py', 'U', imp.PY_SOURCE) PY3 = False else: - # importlib is only used on Python>=3 - import importlib.util - unicode = str PY3 = True ZIPDATA = """%(zipdata)s""" @@ -187,13 +181,6 @@ def _ansiballz_main(): zinfo.filename = 'sitecustomize.py' zinfo.date_time = ( %(year)i, %(month)i, %(day)i, %(hour)i, %(minute)i, %(second)i) z.writestr(zinfo, sitecustomize) - # Note: Remove the following section when we switch to zipimport - # Write the module to disk for imp.load_module - module = os.path.join(temp_path, '__main__.py') - with open(module, 'wb') as f: - f.write(z.read('__main__.py')) - f.close() - # End pre-zipimport section z.close() # Put the zipped up module_utils we got from the controller first in the python path so that we @@ -205,13 +192,7 @@ def _ansiballz_main(): basic._ANSIBLE_ARGS = json_params %(coverage)s # Run the module! By importing it as '__main__', it thinks it is executing as a script - if sys.version_info >= (3,): - spec = importlib.util.spec_from_file_location('__main__', module) - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) - else: - with open(module, 'rb') as mod: - imp.load_module('__main__', mod, module, MOD_DESC) + runpy.run_module(mod_name='%(module_fqn)s', init_globals=None, run_name='__main__', alter_sys=False) # Ansible modules must exit themselves print('{"msg": "New-style module did not handle its own exit", "failed": true}') @@ -253,7 +234,6 @@ def _ansiballz_main(): # Okay to use __file__ here because we're running from a kept file basedir = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'debug_dir') args_path = os.path.join(basedir, 'args') - script_path = os.path.join(basedir, '__main__.py') if command == 'excommunicate': print('The excommunicate debug command is deprecated and will be removed in 2.11. Use execute instead.') @@ -306,15 +286,7 @@ def _ansiballz_main(): basic._ANSIBLE_ARGS = json_params # Run the module! By importing it as '__main__', it thinks it is executing as a script - if PY3: - import importlib.util - spec = importlib.util.spec_from_file_location('__main__', script_path) - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) - else: - import imp - with open(script_path, 'r') as f: - imp.load_module('__main__', f, script_path, ('.py', 'r', imp.PY_SOURCE)) + runpy.run_module(mod_name='%(module_fqn)s', init_globals=None, run_name='__main__', alter_sys=False) # Ansible modules must exit themselves print('{"msg": "New-style module did not handle its own exit", "failed": true}') @@ -394,9 +366,11 @@ ANSIBALLZ_COVERAGE_TEMPLATE = ''' ANSIBALLZ_COVERAGE_CHECK_TEMPLATE = ''' try: if PY3: + import importlib.util if importlib.util.find_spec('coverage') is None: raise ImportError else: + import imp imp.find_module('coverage') except ImportError: print('{"msg": "Could not find `coverage` module.", "failed": true}') @@ -439,73 +413,131 @@ else: # ANSIBALLZ_TEMPLATE stripped of comments for smaller over the wire size ACTIVE_ANSIBALLZ_TEMPLATE = _strip_comments(ANSIBALLZ_TEMPLATE) +# dirname(dirname(dirname(site-packages/ansible/executor/module_common.py) == site-packages +# Do this instead of getting site-packages from distutils.sysconfig so we work when we +# haven't been installed +site_packages = os.path.dirname(os.path.dirname(os.path.dirname(__file__))) +CORE_LIBRARY_PATH_RE = re.compile(r'%s/(?Pansible/modules/.*)\.py$' % site_packages) +COLLECTION_PATH_RE = re.compile(r'/(?Pansible_collections/[^/]+/[^/]+/plugins/modules/.*)\.py$') + +# Detect new-style Python modules by looking for required imports: +# import ansible_collections.[my_ns.my_col.plugins.module_utils.my_module_util] +# from ansible_collections.[my_ns.my_col.plugins.module_utils import my_module_util] +# import ansible.module_utils[.basic] +# from ansible.module_utils[ import basic] +# from ansible.module_utils[.basic import AnsibleModule] +# from ..module_utils[ import basic] +# from ..module_utils[.basic import AnsibleModule] +NEW_STYLE_PYTHON_MODULE_RE = re.compile( + # Relative imports + br'(?:from +\.{2,} *module_utils.* +import |' + # Collection absolute imports: + br'from +ansible_collections\.[^.]+\.[^.]+\.plugins\.module_utils.* +import |' + br'import +ansible_collections\.[^.]+\.[^.]+\.plugins\.module_utils.*|' + # Core absolute imports + br'from +ansible\.module_utils.* +import |' + br'import +ansible\.module_utils\.)' +) + class ModuleDepFinder(ast.NodeVisitor): - # Caveats: - # This code currently does not handle: - # * relative imports from py2.6+ from . import urls - IMPORT_PREFIX_SIZE = len('ansible.module_utils.') - def __init__(self, *args, **kwargs): + def __init__(self, module_fqn, *args, **kwargs): """ Walk the ast tree for the python module. + :arg module_fqn: The fully qualified name to reach this module in dotted notation. + example: ansible.module_utils.basic Save submodule[.submoduleN][.identifier] into self.submodules + when they are from ansible.module_utils or ansible_collections packages self.submodules will end up with tuples like: - - ('basic',) - - ('urls', 'fetch_url') - - ('database', 'postgres') - - ('database', 'postgres', 'quote') + - ('ansible', 'module_utils', 'basic',) + - ('ansible', 'module_utils', 'urls', 'fetch_url') + - ('ansible', 'module_utils', 'database', 'postgres') + - ('ansible', 'module_utils', 'database', 'postgres', 'quote') + - ('ansible', 'module_utils', 'database', 'postgres', 'quote') + - ('ansible_collections', 'my_ns', 'my_col', 'plugins', 'module_utils', 'foo') It's up to calling code to determine whether the final element of the - dotted strings are module names or something else (function, class, or - variable names) + tuple are module names or something else (function, class, or variable names) + .. seealso:: :python3:class:`ast.NodeVisitor` """ super(ModuleDepFinder, self).__init__(*args, **kwargs) self.submodules = set() + self.module_fqn = module_fqn def visit_Import(self, node): - # import ansible.module_utils.MODLIB[.MODLIBn] [as asname] + """ + Handle import ansible.module_utils.MODLIB[.MODLIBn] [as asname] + + We save these as interesting submodules when the imported library is in ansible.module_utils + or ansible.collections + """ for alias in node.names: - if alias.name.startswith('ansible.module_utils.'): - py_mod = alias.name[self.IMPORT_PREFIX_SIZE:] - py_mod = tuple(py_mod.split('.')) + if (alias.name.startswith('ansible.module_utils.') or + alias.name.startswith('ansible_collections.')): + py_mod = tuple(alias.name.split('.')) self.submodules.add(py_mod) - elif alias.name.startswith('ansible_collections.'): - # keep 'ansible_collections.' as a sentinel prefix to trigger collection-loaded MU path - self.submodules.add(tuple(alias.name.split('.'))) self.generic_visit(node) def visit_ImportFrom(self, node): + """ + Handle from ansible.module_utils.MODLIB import [.MODLIBn] [as asname] + + Also has to handle relative imports + + We save these as interesting submodules when the imported library is in ansible.module_utils + or ansible.collections + """ + + # FIXME: These should all get skipped: + # from ansible.executor import module_common + # from ...executor import module_common + # from ... import executor (Currently it gives a non-helpful error) + if node.level > 0: + if self.module_fqn: + parts = tuple(self.module_fqn.split('.')) + if node.module: + # relative import: from .module import x + node_module = '.'.join(parts[:-node.level] + (node.module,)) + else: + # relative import: from . import x + node_module = '.'.join(parts[:-node.level]) + else: + # fall back to an absolute import + node_module = node.module + else: + # absolute import: from module import x + node_module = node.module + # Specialcase: six is a special case because of its # import logic + py_mod = None if node.names[0].name == '_six': self.submodules.add(('_six',)) - elif node.module.startswith('ansible.module_utils'): - where_from = node.module[self.IMPORT_PREFIX_SIZE:] - if where_from: - # from ansible.module_utils.MODULE1[.MODULEn] import IDENTIFIER [as asname] - # from ansible.module_utils.MODULE1[.MODULEn] import MODULEn+1 [as asname] - # from ansible.module_utils.MODULE1[.MODULEn] import MODULEn+1 [,IDENTIFIER] [as asname] - py_mod = tuple(where_from.split('.')) - for alias in node.names: - self.submodules.add(py_mod + (alias.name,)) - else: - # from ansible.module_utils import MODLIB [,MODLIB2] [as asname] - for alias in node.names: - self.submodules.add((alias.name,)) + elif node_module.startswith('ansible.module_utils'): + # from ansible.module_utils.MODULE1[.MODULEn] import IDENTIFIER [as asname] + # from ansible.module_utils.MODULE1[.MODULEn] import MODULEn+1 [as asname] + # from ansible.module_utils.MODULE1[.MODULEn] import MODULEn+1 [,IDENTIFIER] [as asname] + # from ansible.module_utils import MODULE1 [,MODULEn] [as asname] + py_mod = tuple(node_module.split('.')) - elif node.module.startswith('ansible_collections.'): - # TODO: finish out the subpackage et al cases - if node.module.endswith('plugins.module_utils'): + elif node_module.startswith('ansible_collections.'): + if node_module.endswith('plugins.module_utils') or '.plugins.module_utils.' in node_module: # from ansible_collections.ns.coll.plugins.module_utils import MODULE [as aname] [,MODULE2] [as aname] - py_mod = tuple(node.module.split('.')) - for alias in node.names: - self.submodules.add(py_mod + (alias.name,)) - else: # from ansible_collections.ns.coll.plugins.module_utils.MODULE import IDENTIFIER [as aname] - self.submodules.add(tuple(node.module.split('.'))) + # FIXME: Unhandled cornercase (needs to be ignored): + # from ansible_collections.ns.coll.plugins.[!module_utils].[FOO].plugins.module_utils import IDENTIFIER + py_mod = tuple(node_module.split('.')) + else: + # Not from module_utils so ignore. for instance: + # from ansible_collections.ns.coll.plugins.lookup import IDENTIFIER + pass + + if py_mod: + for alias in node.names: + self.submodules.add(py_mod + (alias.name,)) self.generic_visit(node) @@ -601,73 +633,126 @@ class ModuleInfo: self._info[0].close() return _slurp(self.path) + def __repr__(self): + return 'ModuleInfo: py_src=%s, pkg_dir=%s, path=%s' % (self.py_src, self.pkg_dir, self.path) -def recursive_finder(name, data, py_module_names, py_module_cache, zf): + +class CollectionModuleInfo(ModuleInfo): + def __init__(self, name, paths): + self._mod_name = name + self.py_src = True + # FIXME: Implement pkg_dir so that we can place __init__.py files + self.pkg_dir = False + + for path in paths: + self._package_name = '.'.join(path.split('/')) + try: + self.get_source() + except FileNotFoundError: + pass + else: + self.path = os.path.join(path, self._mod_name) + '.py' + break + else: + # FIXME (nitz): implement package fallback code + raise ImportError('unable to load collection-hosted module_util' + ' {0}.{1}'.format(to_native(self._package_name), + to_native(name))) + + def get_source(self): + # FIXME (nitz): need this in py2 for some reason TBD, but we shouldn't (get_data delegates + # to wrong loader without it) + pkg = import_module(self._package_name) + data = pkgutil.get_data(to_native(self._package_name), to_native(self._mod_name + '.py')) + return data + + +def recursive_finder(name, module_fqn, data, py_module_names, py_module_cache, zf): """ Using ModuleDepFinder, make sure we have all of the module_utils files that - the module its module_utils files needs. + the module and its module_utils files needs. + :arg name: Name of the python module we're examining + :arg module_fqn: Fully qualified name of the python module we're scanning + :arg py_module_names: set of the fully qualified module names represented as a tuple of their + FQN with __init__ appended if the module is also a python package). Presence of a FQN in + this set means that we've already examined it for module_util deps. + :arg py_module_cache: map python module names (represented as a tuple of their FQN with __init__ + appended if the module is also a python package) to a tuple of the code in the module and + the pathname the module would have inside of a Python toplevel (like site-packages) + :arg zf: An open :python:class:`zipfile.ZipFile` object that holds the Ansible module payload + which we're assembling """ # Parse the module and find the imports of ansible.module_utils try: tree = ast.parse(data) except (SyntaxError, IndentationError) as e: raise AnsibleError("Unable to import %s due to %s" % (name, e.msg)) - finder = ModuleDepFinder() + + finder = ModuleDepFinder(module_fqn) finder.visit(tree) # # Determine what imports that we've found are modules (vs class, function. # variable names) for packages # + module_utils_paths = [p for p in module_utils_loader._get_paths(subdirs=False) if os.path.isdir(p)] + # FIXME: Do we still need this? It feels like module-utils_loader should include + # _MODULE_UTILS_PATH + module_utils_paths.append(_MODULE_UTILS_PATH) normalized_modules = set() # Loop through the imports that we've found to normalize them # Exclude paths that match with paths we've already processed # (Have to exclude them a second time once the paths are processed) - module_utils_paths = [p for p in module_utils_loader._get_paths(subdirs=False) if os.path.isdir(p)] - module_utils_paths.append(_MODULE_UTILS_PATH) for py_module_name in finder.submodules.difference(py_module_names): module_info = None - if py_module_name[0] == 'six': - # Special case the python six library because it messes up the + if py_module_name[0:3] == ('ansible', 'module_utils', 'six'): + # Special case the python six library because it messes with the # import process in an incompatible way module_info = ModuleInfo('six', module_utils_paths) - py_module_name = ('six',) + py_module_name = ('ansible', 'module_utils', 'six') idx = 0 - elif py_module_name[0] == '_six': - # Special case the python six library because it messes up the + elif py_module_name[0:3] == ('ansible', 'module_utils', '_six'): + # Special case the python six library because it messes with the # import process in an incompatible way module_info = ModuleInfo('_six', [os.path.join(p, 'six') for p in module_utils_paths]) - py_module_name = ('six', '_six') + py_module_name = ('ansible', 'module_utils', 'six', '_six') idx = 0 elif py_module_name[0] == 'ansible_collections': - # FIXME: replicate module name resolution like below for granular imports - # this is a collection-hosted MU; look it up with get_data - package_name = '.'.join(py_module_name[:-1]) - resource_name = py_module_name[-1] + '.py' - try: - # FIXME: need this in py2 for some reason TBD, but we shouldn't (get_data delegates to wrong loader without it) - pkg = import_module(package_name) - module_info = pkgutil.get_data(package_name, resource_name) - except FileNotFoundError: - # FIXME: implement package fallback code - raise AnsibleError('unable to load collection-hosted module_util {0}.{1}'.format(to_native(package_name), - to_native(resource_name))) - idx = 0 - else: - # Check whether either the last or the second to last identifier is - # a module name + # FIXME (nitz): replicate module name resolution like below for granular imports for idx in (1, 2): if len(py_module_name) < idx: break try: - module_info = ModuleInfo(py_module_name[-idx], - [os.path.join(p, *py_module_name[:-idx]) for p in module_utils_paths]) + # this is a collection-hosted MU; look it up with pkgutil.get_data() + module_info = CollectionModuleInfo(py_module_name[-idx], + [os.path.join(*py_module_name[:-idx])]) break except ImportError: continue + elif py_module_name[0:2] == ('ansible', 'module_utils'): + # Need to remove ansible.module_utils because PluginLoader may find different paths + # for us to look in + relative_module_utils_dir = py_module_name[2:] + # Check whether either the last or the second to last identifier is + # a module name + for idx in (1, 2): + if len(relative_module_utils_dir) < idx: + break + try: + module_info = ModuleInfo(py_module_name[-idx], + [os.path.join(p, *relative_module_utils_dir[:-idx]) for p in module_utils_paths]) + break + except ImportError: + continue + else: + # If we get here, it's because of a bug in ModuleDepFinder. If we get a reproducer we + # should then fix ModuleDepFinder + display.warning('ModuleDepFinder improperly found a non-module_utils import %s' + % [py_module_name]) + continue # Could not find the module. Construct a helpful error message. if module_info is None: @@ -678,10 +763,15 @@ def recursive_finder(name, data, py_module_names, py_module_cache, zf): msg.append(py_module_name[-1]) raise AnsibleError(' '.join(msg)) - if isinstance(module_info, bytes): # collection-hosted, just the code + if isinstance(module_info, CollectionModuleInfo): + if idx == 2: + # We've determined that the last portion was an identifier and + # thus, not part of the module name + py_module_name = py_module_name[:-1] + # HACK: maybe surface collection dirs in here and use existing find_module code? normalized_name = py_module_name - normalized_data = module_info + normalized_data = module_info.get_source() normalized_path = os.path.join(*py_module_name) py_module_cache[normalized_name] = (normalized_data, normalized_path) normalized_modules.add(normalized_name) @@ -738,13 +828,18 @@ def recursive_finder(name, data, py_module_names, py_module_cache, zf): py_module_cache[normalized_name] = (normalized_data, module_info.path) normalized_modules.add(normalized_name) + # # Make sure that all the packages that this module is a part of # are also added + # for i in range(1, len(py_module_name)): py_pkg_name = py_module_name[:-i] + ('__init__',) if py_pkg_name not in py_module_names: - pkg_dir_info = ModuleInfo(py_pkg_name[-1], - [os.path.join(p, *py_pkg_name[:-1]) for p in module_utils_paths]) + # Need to remove ansible.module_utils because PluginLoader may find + # different paths for us to look in + relative_module_utils = py_pkg_name[2:] + pkg_dir_info = ModuleInfo(relative_module_utils[-1], + [os.path.join(p, *relative_module_utils[:-1]) for p in module_utils_paths]) normalized_modules.add(py_pkg_name) py_module_cache[py_pkg_name] = (pkg_dir_info.get_source(), pkg_dir_info.path) @@ -758,10 +853,10 @@ def recursive_finder(name, data, py_module_names, py_module_cache, zf): # want basic.py can import that instead. AnsibleModule will need to change to import the vars # from the separate python module and mirror the args into its global variable for backwards # compatibility. - if ('basic',) not in py_module_names: + if ('ansible', 'module_utils', 'basic',) not in py_module_names: pkg_dir_info = ModuleInfo('basic', module_utils_paths) - normalized_modules.add(('basic',)) - py_module_cache[('basic',)] = (pkg_dir_info.get_source(), pkg_dir_info.path) + normalized_modules.add(('ansible', 'module_utils', 'basic',)) + py_module_cache[('ansible', 'module_utils', 'basic',)] = (pkg_dir_info.get_source(), pkg_dir_info.path) # End of AnsiballZ hack # @@ -773,17 +868,11 @@ def recursive_finder(name, data, py_module_names, py_module_cache, zf): unprocessed_py_module_names = normalized_modules.difference(py_module_names) for py_module_name in unprocessed_py_module_names: - # HACK: this seems to work as a way to identify a collections-based import, but a stronger identifier would be better - if not py_module_cache[py_module_name][1].startswith('/'): - dir_prefix = '' - else: - dir_prefix = 'ansible/module_utils' py_module_path = os.path.join(*py_module_name) py_module_file_name = '%s.py' % py_module_path - zf.writestr(os.path.join(dir_prefix, - py_module_file_name), py_module_cache[py_module_name][0]) + zf.writestr(py_module_file_name, py_module_cache[py_module_name][0]) display.vvvvv("Using module_utils file %s" % py_module_cache[py_module_name][1]) # Add the names of the files we're scheduling to examine in the loop to @@ -792,7 +881,9 @@ def recursive_finder(name, data, py_module_names, py_module_cache, zf): py_module_names.update(unprocessed_py_module_names) for py_module_file in unprocessed_py_module_names: - recursive_finder(py_module_file, py_module_cache[py_module_file][0], py_module_names, py_module_cache, zf) + next_fqn = '.'.join(py_module_file) + recursive_finder(py_module_file[-1], next_fqn, py_module_cache[py_module_file][0], + py_module_names, py_module_cache, zf) # Save memory; the file won't have to be read again for this ansible module. del py_module_cache[py_module_file] @@ -803,6 +894,67 @@ def _is_binary(b_module_data): return bool(start.translate(None, textchars)) +def _get_ansible_module_fqn(module_path): + """ + Get the fully qualified name for an ansible module based on its pathname + + remote_module_fqn is the fully qualified name. Like ansible.modules.system.ping + Or ansible_collections.Namespace.Collection_name.plugins.modules.ping + .. warning:: This function is for ansible modules only. It won't work for other things + (non-module plugins, etc) + """ + remote_module_fqn = None + + # Is this a core module? + match = CORE_LIBRARY_PATH_RE.search(module_path) + if not match: + # Is this a module in a collection? + match = COLLECTION_PATH_RE.search(module_path) + + # We can tell the FQN for core modules and collection modules + if match: + path = match.group('path') + if '.' in path: + # FQNs must be valid as python identifiers. This sanity check has failed. + # we could check other things as well + raise ValueError('Module name (or path) was not a valid python identifier') + + remote_module_fqn = '.'.join(path.split('/')) + else: + # Currently we do not handle modules in roles so we can end up here for that reason + raise ValueError("Unable to determine module's fully qualified name") + + return remote_module_fqn + + +def _add_module_to_zip(zf, remote_module_fqn, b_module_data): + """Add a module from ansible or from an ansible collection into the module zip""" + module_path_parts = remote_module_fqn.split('.') + + # Write the module + module_path = '/'.join(module_path_parts) + '.py' + zf.writestr(module_path, b_module_data) + + # Write the __init__.py's necessary to get there + if module_path_parts[0] == 'ansible': + # The ansible namespace is setup as part of the module_utils setup... + start = 2 + existing_paths = frozenset() + else: + # ... but ansible_collections and other toplevels are not + start = 1 + existing_paths = frozenset(zf.namelist()) + + for idx in range(start, len(module_path_parts)): + package_path = '/'.join(module_path_parts[:idx]) + '/__init__.py' + # If a collections module uses module_utils from a collection then most packages will have already been added by recursive_finder. + if package_path in existing_paths: + continue + # Note: We don't want to include more than one ansible module in a payload at this time + # so no need to fill the __init__.py with namespace code + zf.writestr(package_path, b'') + + def _find_module_utils(module_name, b_module_data, module_path, module_args, task_vars, templar, module_compression, async_timeout, become, become_method, become_user, become_password, become_flags, environment): """ @@ -825,9 +977,7 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas module_style = 'new' module_substyle = 'python' b_module_data = b_module_data.replace(REPLACER, b'from ansible.module_utils.basic import *') - # FUTURE: combined regex for this stuff, or a "looks like Python, let's inspect further" mechanism - elif b'from ansible.module_utils.' in b_module_data or b'from ansible_collections.' in b_module_data\ - or b'import ansible_collections.' in b_module_data: + elif NEW_STYLE_PYTHON_MODULE_RE.search(b_module_data): module_style = 'new' module_substyle = 'python' elif REPLACER_WINDOWS in b_module_data: @@ -869,6 +1019,17 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas display.warning(u'Bad module compression string specified: %s. Using ZIP_STORED (no compression)' % module_compression) compression_method = zipfile.ZIP_STORED + try: + remote_module_fqn = _get_ansible_module_fqn(module_path) + except ValueError: + # Modules in roles currently are not found by the fqn heuristic so we + # fallback to this. This means that relative imports inside a module from + # a role may fail. Absolute imports should be used for future-proofness. + # People should start writing collections instead of modules in roles so we + # may never fix this + display.debug('ANSIBALLZ: Could not determine module FQN') + remote_module_fqn = 'ansible.modules.%s' % module_name + lookup_path = os.path.join(C.DEFAULT_LOCAL_TMP, 'ansiballz_cache') cached_module_filename = os.path.join(lookup_path, "%s-%s" % (module_name, module_compression)) @@ -900,18 +1061,42 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas # Create the module zip data zipoutput = BytesIO() zf = zipfile.ZipFile(zipoutput, mode='w', compression=compression_method) - # Note: If we need to import from release.py first, - # remember to catch all exceptions: https://github.com/ansible/ansible/issues/16523 - zf.writestr('ansible/__init__.py', - b'from pkgutil import extend_path\n__path__=extend_path(__path__,__name__)\n__version__="' + - to_bytes(__version__) + b'"\n__author__="' + - to_bytes(__author__) + b'"\n') - zf.writestr('ansible/module_utils/__init__.py', b'from pkgutil import extend_path\n__path__=extend_path(__path__,__name__)\n') - zf.writestr('__main__.py', b_module_data) + # py_module_cache maps python module names to a tuple of the code in the module + # and the pathname to the module. See the recursive_finder() documentation for + # more info. + # Here we pre-load it with modules which we create without bothering to + # read from actual files (In some cases, these need to differ from what ansible + # ships because they're namespace packages in the module) + py_module_cache = { + ('ansible', '__init__',): ( + b'from pkgutil import extend_path\n' + b'__path__=extend_path(__path__,__name__)\n' + b'__version__="' + to_bytes(__version__) + + b'"\n__author__="' + to_bytes(__author__) + b'"\n', + 'ansible/__init__.py'), + ('ansible', 'module_utils', '__init__',): ( + b'from pkgutil import extend_path\n' + b'__path__=extend_path(__path__,__name__)\n', + 'ansible/module_utils/__init__.py')} + + for (py_module_name, (file_data, filename)) in py_module_cache.items(): + zf.writestr(filename, file_data) + # py_module_names keeps track of which modules we've already scanned for + # module_util dependencies + py_module_names.add(py_module_name) + + # Returning the ast tree is a temporary hack. We need to know if the module has + # a main() function or not as we are deprecating new-style modules without + # main(). Because parsing the ast is expensive, return it from recursive_finder + # instead of reparsing. Once the deprecation is over and we remove that code, + # also remove returning of the ast tree. + recursive_finder(module_name, remote_module_fqn, b_module_data, py_module_names, + py_module_cache, zf) + + display.debug('ANSIBALLZ: Writing module into payload') + _add_module_to_zip(zf, remote_module_fqn, b_module_data) - py_module_cache = {('__init__',): (b'', '[builtin]')} - recursive_finder(module_name, b_module_data, py_module_names, py_module_cache, zf) zf.close() zipdata = base64.b64encode(zipoutput.getvalue()) @@ -950,11 +1135,6 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas if shebang is None: shebang = u'#!/usr/bin/python' - # Enclose the parts of the interpreter in quotes because we're - # substituting it into the template as a Python string - interpreter_parts = interpreter.split(u' ') - interpreter = u"'{0}'".format(u"', '".join(interpreter_parts)) - # FUTURE: the module cache entry should be invalidated if we got this value from a host-dependent source rlimit_nofile = C.config.get_config_value('PYTHON_MODULE_RLIMIT_NOFILE', variables=task_vars) @@ -991,9 +1171,9 @@ def _find_module_utils(module_name, b_module_data, module_path, module_args, tas output.write(to_bytes(ACTIVE_ANSIBALLZ_TEMPLATE % dict( zipdata=zipdata, ansible_module=module_name, + module_fqn=remote_module_fqn, params=python_repred_params, shebang=shebang, - interpreter=interpreter, coding=ENCODING_STRING, year=now.year, month=now.month, diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 1e11bea4e5..31f32755c6 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -79,7 +79,7 @@ except ImportError: # Python2 & 3 way to get NoneType NoneType = type(None) -from ansible.module_utils._text import to_native, to_bytes, to_text +from ._text import to_native, to_bytes, to_text from ansible.module_utils.common.text.converters import ( jsonify, container_to_bytes as json_dict_unicode_to_bytes, diff --git a/lib/ansible/modules/cloud/amazon/lambda_event.py b/lib/ansible/modules/cloud/amazon/lambda_event.py index 37442bdc5f..cfd074c8c5 100644 --- a/lib/ansible/modules/cloud/amazon/lambda_event.py +++ b/lib/ansible/modules/cloud/amazon/lambda_event.py @@ -393,7 +393,6 @@ def lambda_event_stream(module, aws): def main(): """Produce a list of function suffixes which handle lambda events.""" - this_module = sys.modules[__name__] source_choices = ["stream", "sqs"] argument_spec = ec2_argument_spec() diff --git a/lib/ansible/modules/system/setup.py b/lib/ansible/modules/system/setup.py index 7e4e5af41f..cea3c8ed05 100644 --- a/lib/ansible/modules/system/setup.py +++ b/lib/ansible/modules/system/setup.py @@ -131,7 +131,7 @@ EXAMPLES = """ """ # import module snippets -from ansible.module_utils.basic import AnsibleModule +from ...module_utils.basic import AnsibleModule from ansible.module_utils.facts.namespace import PrefixFactNamespace from ansible.module_utils.facts import ansible_collector diff --git a/lib/ansible/modules/utilities/logic/async_wrapper.py b/lib/ansible/modules/utilities/logic/async_wrapper.py index 586438ff13..26f0ef08b5 100644 --- a/lib/ansible/modules/utilities/logic/async_wrapper.py +++ b/lib/ansible/modules/utilities/logic/async_wrapper.py @@ -199,8 +199,7 @@ def _run_module(wrapped_cmd, jid, job_path): os.rename(tmp_job_path, job_path) -if __name__ == '__main__': - +def main(): if len(sys.argv) < 5: print(json.dumps({ "failed": True, @@ -334,3 +333,7 @@ if __name__ == '__main__': "msg": "FATAL ERROR: %s" % e })) sys.exit(1) + + +if __name__ == '__main__': + main() diff --git a/test/integration/targets/ansiballz_python/library/custom_module.py b/test/integration/targets/ansiballz_python/library/custom_module.py new file mode 100644 index 0000000000..625823eabe --- /dev/null +++ b/test/integration/targets/ansiballz_python/library/custom_module.py @@ -0,0 +1,19 @@ +#!/usr/bin/python + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +from ..module_utils.basic import AnsibleModule # pylint: disable=relative-beyond-top-level +from ..module_utils.custom_util import forty_two # pylint: disable=relative-beyond-top-level + + +def main(): + module = AnsibleModule( + argument_spec=dict() + ) + + module.exit_json(answer=forty_two()) + + +if __name__ == '__main__': + main() diff --git a/test/integration/targets/ansiballz_python/module_utils/custom_util.py b/test/integration/targets/ansiballz_python/module_utils/custom_util.py new file mode 100644 index 0000000000..0393db473f --- /dev/null +++ b/test/integration/targets/ansiballz_python/module_utils/custom_util.py @@ -0,0 +1,6 @@ +from __future__ import absolute_import, division, print_function +__metaclass__ = type + + +def forty_two(): + return 42 diff --git a/test/integration/targets/ansiballz_python/tasks/main.yml b/test/integration/targets/ansiballz_python/tasks/main.yml index f9ca58898a..d6efad8017 100644 --- a/test/integration/targets/ansiballz_python/tasks/main.yml +++ b/test/integration/targets/ansiballz_python/tasks/main.yml @@ -32,6 +32,10 @@ ansible_python_module_rlimit_nofile: '{{ rlimit_original_return.rlimit_nofile[1] + 1 }}' register: rlimit_above_hard_return +- name: run a role module which uses a role module_util using relative imports + custom_module: + register: custom_module_return + - assert: that: # make sure ansible-test was able to set the limit unless it exceeds the hard limit or the value is lower on macOS @@ -54,3 +58,6 @@ # setting the limit higher than the existing hard limit should use the hard limit (except on macOS) - rlimit_above_hard_return.rlimit_nofile[0] == rlimit_original_return.rlimit_nofile[1] or ansible_distribution == 'MacOSX' + + # custom module returned the correct answer + - custom_module_return.answer == 42 diff --git a/test/integration/targets/collections_relative_imports/aliases b/test/integration/targets/collections_relative_imports/aliases new file mode 100644 index 0000000000..54ea5a3b07 --- /dev/null +++ b/test/integration/targets/collections_relative_imports/aliases @@ -0,0 +1,2 @@ +shippable/posix/group1 +skip/python2.6 diff --git a/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util1.py b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util1.py new file mode 100644 index 0000000000..196b4abf54 --- /dev/null +++ b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util1.py @@ -0,0 +1,6 @@ +from __future__ import absolute_import, division, print_function +__metaclass__ = type + + +def one(): + return 1 diff --git a/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util2.py b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util2.py new file mode 100644 index 0000000000..0d985bf37c --- /dev/null +++ b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util2.py @@ -0,0 +1,8 @@ +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +from .my_util1 import one + + +def two(): + return one() * 2 diff --git a/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util3.py b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util3.py new file mode 100644 index 0000000000..1529d7b274 --- /dev/null +++ b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util3.py @@ -0,0 +1,8 @@ +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +from . import my_util2 + + +def three(): + return my_util2.two() + 1 diff --git a/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/modules/my_module.py b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/modules/my_module.py new file mode 100644 index 0000000000..0cdf50089a --- /dev/null +++ b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/modules/my_module.py @@ -0,0 +1,24 @@ +#!/usr/bin/python +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +from ansible.module_utils.basic import AnsibleModule +from ..module_utils.my_util2 import two +from ..module_utils import my_util3 + + +def main(): + module = AnsibleModule( + argument_spec=dict(), + supports_check_mode=True + ) + + result = dict( + two=two(), + three=my_util3.three(), + ) + module.exit_json(**result) + + +if __name__ == '__main__': + main() diff --git a/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/roles/test/tasks/main.yml b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/roles/test/tasks/main.yml new file mode 100644 index 0000000000..9ba0f7ed74 --- /dev/null +++ b/test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/roles/test/tasks/main.yml @@ -0,0 +1,4 @@ +- name: fully qualified module usage with relative imports + my_ns.my_col.my_module: +- name: collection relative module usage with relative imports + my_module: diff --git a/test/integration/targets/collections_relative_imports/runme.sh b/test/integration/targets/collections_relative_imports/runme.sh new file mode 100755 index 0000000000..5800750358 --- /dev/null +++ b/test/integration/targets/collections_relative_imports/runme.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash + +set -eux + +ANSIBLE_COLLECTIONS_PATHS="${PWD}/collection_root" ansible-playbook test.yml -i ../../inventory "$@" diff --git a/test/integration/targets/collections_relative_imports/test.yml b/test/integration/targets/collections_relative_imports/test.yml new file mode 100644 index 0000000000..d1c3f1b7a5 --- /dev/null +++ b/test/integration/targets/collections_relative_imports/test.yml @@ -0,0 +1,3 @@ +- hosts: testhost + roles: + - my_ns.my_col.test diff --git a/test/lib/ansible_test/_data/sanity/import/importer.py b/test/lib/ansible_test/_data/sanity/import/importer.py index 561ebf9b59..d530ddb8bc 100755 --- a/test/lib/ansible_test/_data/sanity/import/importer.py +++ b/test/lib/ansible_test/_data/sanity/import/importer.py @@ -38,6 +38,15 @@ def main(): # noinspection PyPep8Naming AnsibleCollectionLoader = None + # These are the public attribute sof a doc-only module + doc_keys = ('ANSIBLE_METADATA', + 'DOCUMENTATION', + 'EXAMPLES', + 'RETURN', + 'absolute_import', + 'division', + 'print_function') + class ImporterAnsibleModuleException(Exception): """Exception thrown during initialization of ImporterAnsibleModule.""" @@ -80,17 +89,21 @@ def main(): if not path.startswith('lib/ansible/modules/'): return + # __init__ in module directories is empty (enforced by a different test) + if path.endswith('__init__.py'): + return + # async_wrapper is not an Ansible module if path == 'lib/ansible/modules/utilities/logic/async_wrapper.py': return - # run code protected by __name__ conditional - name = '__main__' + name = calculate_python_module_name(path) # show the Ansible module responsible for the exception, even if it was thrown in module_utils filter_dir = os.path.join(base_dir, 'lib/ansible/modules') else: - # do not run code protected by __name__ conditional - name = 'module_import_test' + # Calculate module name + name = calculate_python_module_name(path) + # show the Ansible file responsible for the exception, even if it was thrown in 3rd party code filter_dir = base_dir @@ -98,15 +111,58 @@ def main(): try: if imp: - with open(path, 'r') as module_fd: - with capture_output(capture): - imp.load_module(name, module_fd, os.path.abspath(path), ('.py', 'r', imp.PY_SOURCE)) + with capture_output(capture): + # On Python2 without absolute_import we have to import parent modules all + # the way up the tree + full_path = os.path.abspath(path) + parent_mod = None + + py_packages = name.split('.') + # BIG HACK: reimporting module_utils breaks the monkeypatching of basic we did + # above and also breaks modules which import names directly from module_utils + # modules (you'll get errors like ERROR: + # lib/ansible/modules/storage/netapp/na_ontap_vserver_cifs_security.py:151:0: + # AttributeError: 'module' object has no attribute 'netapp'). + # So when we import a module_util here, use a munged name. + if 'module_utils' in py_packages: + # Avoid accidental double underscores by using _1 as a prefix + py_packages[-1] = '_1%s' % py_packages[-1] + name = '.'.join(py_packages) + + for idx in range(1, len(py_packages)): + parent_name = '.'.join(py_packages[:idx]) + if parent_mod is None: + toplevel_end = full_path.find('ansible/module') + toplevel = full_path[:toplevel_end] + parent_mod_info = imp.find_module(parent_name, [toplevel]) + else: + parent_mod_info = imp.find_module(py_packages[idx - 1], parent_mod.__path__) + + parent_mod = imp.load_module(parent_name, *parent_mod_info) + # skip distro due to an apparent bug or bad interaction in + # imp.load_module() with our distro/__init__.py. + # distro/__init__.py sets sys.modules['ansible.module_utils.distro'] + # = _distro.pyc + # but after running imp.load_module(), + # sys.modules['ansible.module_utils.distro._distro'] = __init__.pyc + # (The opposite of what we set) + # This does not affect runtime so regular import seems to work. It's + # just imp.load_module() + if name == 'ansible.module_utils.distro._1__init__': + return + + with open(path, 'r') as module_fd: + module = imp.load_module(name, module_fd, full_path, ('.py', 'r', imp.PY_SOURCE)) + if ansible_module: + run_if_really_module(module) else: spec = importlib.util.spec_from_file_location(name, os.path.abspath(path)) module = importlib.util.module_from_spec(spec) with capture_output(capture): spec.loader.exec_module(module) + if ansible_module: + run_if_really_module(module) capture_report(path, capture, messages) except ImporterAnsibleModuleException: @@ -153,6 +209,34 @@ def main(): report_message(error, messages) + def run_if_really_module(module): + # Module was removed + if ('removed' not in module.ANSIBLE_METADATA['status'] and + # Documentation only module + [attr for attr in + (frozenset(module.__dict__.keys()).difference(doc_keys)) + if not (attr.startswith('__') and attr.endswith('__'))]): + # Run main() code for ansible_modules + module.main() + + def calculate_python_module_name(path): + name = None + try: + idx = path.index('ansible/modules') + except ValueError: + try: + idx = path.index('ansible/module_utils') + except ValueError: + try: + idx = path.index('ansible_collections') + except ValueError: + # Default + name = 'module_import_test' + if name is None: + name = path[idx:-len('.py')].replace('/', '.') + + return name + class Capture: """Captured output and/or exception.""" def __init__(self): 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 fb8fdd95f3..d7a6f1de69 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 @@ -105,13 +105,25 @@ def get_ps_argument_spec(filename): def get_py_argument_spec(filename): - with setup_env(filename) as fake: + # Calculate the module's name so that relative imports work correctly + name = None + try: + idx = filename.index('ansible/modules') + except ValueError: try: - # We use ``module`` here instead of ``__main__`` + 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('/', '.') + + with setup_env(filename) as fake: + try: with CaptureStd(): - mod = imp.load_source('module', filename) + mod = imp.load_source(name, filename) if not fake.called: mod.main() except AnsibleModuleCallError: diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index 8e2f98db3f..652d7af421 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -6027,6 +6027,9 @@ test/integration/targets/async_fail/library/async_test.py future-import-boilerpl test/integration/targets/async_fail/library/async_test.py metaclass-boilerplate test/integration/targets/aws_lambda/files/mini_lambda.py future-import-boilerplate test/integration/targets/aws_lambda/files/mini_lambda.py metaclass-boilerplate +test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util2.py pylint:relative-beyond-top-level +test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/module_utils/my_util3.py pylint:relative-beyond-top-level +test/integration/targets/collections_relative_imports/collection_root/ansible_collections/my_ns/my_col/plugins/modules/my_module.py pylint:relative-beyond-top-level test/integration/targets/expect/files/test_command.py future-import-boilerplate test/integration/targets/expect/files/test_command.py metaclass-boilerplate test/integration/targets/get_url/files/testserver.py future-import-boilerplate diff --git a/test/units/executor/module_common/test_module_common.py b/test/units/executor/module_common/test_module_common.py index 10b4f0cf55..04bae85d33 100644 --- a/test/units/executor/module_common/test_module_common.py +++ b/test/units/executor/module_common/test_module_common.py @@ -19,6 +19,8 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +import os.path + import pytest import ansible.errors @@ -28,7 +30,7 @@ from ansible.executor.interpreter_discovery import InterpreterDiscoveryRequiredE from ansible.module_utils.six import PY2 -class TestStripComments(object): +class TestStripComments: def test_no_changes(self): no_comments = u"""def some_code(): return False""" @@ -69,7 +71,7 @@ def test(arg): assert amc._strip_comments(mixed) == mixed_results -class TestSlurp(object): +class TestSlurp: def test_slurp_nonexistent(self, mocker): mocker.patch('os.path.exists', side_effect=lambda x: False) with pytest.raises(ansible.errors.AnsibleError): @@ -96,14 +98,14 @@ class TestSlurp(object): @pytest.fixture def templar(): - class FakeTemplar(object): + class FakeTemplar: def template(self, template_string, *args, **kwargs): return template_string return FakeTemplar() -class TestGetShebang(object): +class TestGetShebang: """Note: We may want to change the API of this function in the future. It isn't a great API""" def test_no_interpreter_set(self, templar): # normally this would return /usr/bin/python, but so long as we're defaulting to auto python discovery, we'll get @@ -129,3 +131,67 @@ class TestGetShebang(object): def test_python_via_env(self, templar): assert amc._get_shebang(u'/usr/bin/python', {u'ansible_python_interpreter': u'/usr/bin/env python'}, templar) == \ (u'#!/usr/bin/env python', u'/usr/bin/env python') + + +class TestDetectionRegexes: + ANSIBLE_MODULE_UTIL_STRINGS = ( + # Absolute collection imports + b'import ansible_collections.my_ns.my_col.plugins.module_utils.my_util', + b'from ansible_collections.my_ns.my_col.plugins.module_utils import my_util', + b'from ansible_collections.my_ns.my_col.plugins.module_utils.my_util import my_func', + # Absolute core imports + b'import ansible.module_utils.basic', + b'from ansible.module_utils import basic', + b'from ansible.module_utils.basic import AnsibleModule', + # Relative imports + b'from ..module_utils import basic', + b'from .. module_utils import basic', + b'from ....module_utils import basic', + b'from ..module_utils.basic import AnsibleModule', + ) + NOT_ANSIBLE_MODULE_UTIL_STRINGS = ( + b'from ansible import release', + b'from ..release import __version__', + b'from .. import release', + b'from ansible.modules.system import ping', + b'from ansible_collecitons.my_ns.my_col.plugins.modules import function', + ) + + OFFSET = os.path.dirname(os.path.dirname(amc.__file__)) + CORE_PATHS = ( + ('%s/modules/from_role.py' % OFFSET, 'ansible/modules/from_role'), + ('%s/modules/system/ping.py' % OFFSET, 'ansible/modules/system/ping'), + ('%s/modules/cloud/amazon/s3.py' % OFFSET, 'ansible/modules/cloud/amazon/s3'), + ) + + COLLECTION_PATHS = ( + ('/root/ansible_collections/ns/col/plugins/modules/ping.py', + 'ansible_collections/ns/col/plugins/modules/ping'), + ('/root/ansible_collections/ns/col/plugins/modules/subdir/ping.py', + 'ansible_collections/ns/col/plugins/modules/subdir/ping'), + ) + + @pytest.mark.parametrize('testcase', ANSIBLE_MODULE_UTIL_STRINGS) + def test_detect_new_style_python_module_re(self, testcase): + assert amc.NEW_STYLE_PYTHON_MODULE_RE.search(testcase) + + @pytest.mark.parametrize('testcase', NOT_ANSIBLE_MODULE_UTIL_STRINGS) + def test_no_detect_new_style_python_module_re(self, testcase): + assert not amc.NEW_STYLE_PYTHON_MODULE_RE.search(testcase) + + # pylint bug: https://github.com/PyCQA/pylint/issues/511 + @pytest.mark.parametrize('testcase, result', CORE_PATHS) # pylint: disable=undefined-variable + def test_detect_core_library_path_re(self, testcase, result): + assert amc.CORE_LIBRARY_PATH_RE.search(testcase).group('path') == result + + @pytest.mark.parametrize('testcase', (p[0] for p in COLLECTION_PATHS)) # pylint: disable=undefined-variable + def test_no_detect_core_library_path_re(self, testcase): + assert not amc.CORE_LIBRARY_PATH_RE.search(testcase) + + @pytest.mark.parametrize('testcase, result', COLLECTION_PATHS) # pylint: disable=undefined-variable + def test_detect_collection_path_re(self, testcase, result): + assert amc.COLLECTION_PATH_RE.search(testcase).group('path') == result + + @pytest.mark.parametrize('testcase', (p[0] for p in CORE_PATHS)) # pylint: disable=undefined-variable + def test_no_detect_collection_path_re(self, testcase): + assert not amc.COLLECTION_PATH_RE.search(testcase) diff --git a/test/units/executor/module_common/test_recursive_finder.py b/test/units/executor/module_common/test_recursive_finder.py index 0866623b7e..4a6d43b353 100644 --- a/test/units/executor/module_common/test_recursive_finder.py +++ b/test/units/executor/module_common/test_recursive_finder.py @@ -19,6 +19,7 @@ from __future__ import (absolute_import, division, print_function) __metaclass__ = type +import os import pytest import zipfile @@ -35,27 +36,29 @@ from ansible.module_utils.six import PY2 # when basic.py gains new imports # We will remove these when we modify AnsiBallZ to store its args in a separate file instead of in # basic.py -MODULE_UTILS_BASIC_IMPORTS = frozenset((('_text',), - ('basic',), - ('common', '__init__'), - ('common', '_collections_compat'), - ('common', '_json_compat'), - ('common', 'collections'), - ('common', 'file'), - ('common', 'parameters'), - ('common', 'process'), - ('common', 'sys_info'), - ('common', 'text', '__init__'), - ('common', 'text', 'converters'), - ('common', 'text', 'formatters'), - ('common', 'validation'), - ('common', '_utils'), - ('distro', '__init__'), - ('distro', '_distro'), - ('parsing', '__init__'), - ('parsing', 'convert_bool'), - ('pycompat24',), - ('six', '__init__'), +MODULE_UTILS_BASIC_IMPORTS = frozenset((('ansible', '__init__'), + ('ansible', 'module_utils', '__init__'), + ('ansible', 'module_utils', '_text'), + ('ansible', 'module_utils', 'basic'), + ('ansible', 'module_utils', 'common', '__init__'), + ('ansible', 'module_utils', 'common', '_collections_compat'), + ('ansible', 'module_utils', 'common', '_json_compat'), + ('ansible', 'module_utils', 'common', 'collections'), + ('ansible', 'module_utils', 'common', 'file'), + ('ansible', 'module_utils', 'common', 'parameters'), + ('ansible', 'module_utils', 'common', 'process'), + ('ansible', 'module_utils', 'common', 'sys_info'), + ('ansible', 'module_utils', 'common', 'text', '__init__'), + ('ansible', 'module_utils', 'common', 'text', 'converters'), + ('ansible', 'module_utils', 'common', 'text', 'formatters'), + ('ansible', 'module_utils', 'common', 'validation'), + ('ansible', 'module_utils', 'common', '_utils'), + ('ansible', 'module_utils', 'distro', '__init__'), + ('ansible', 'module_utils', 'distro', '_distro'), + ('ansible', 'module_utils', 'parsing', '__init__'), + ('ansible', 'module_utils', 'parsing', 'convert_bool'), + ('ansible', 'module_utils', 'pycompat24',), + ('ansible', 'module_utils', 'six', '__init__'), )) MODULE_UTILS_BASIC_FILES = frozenset(('ansible/module_utils/_text.py', @@ -84,15 +87,19 @@ MODULE_UTILS_BASIC_FILES = frozenset(('ansible/module_utils/_text.py', 'ansible/module_utils/six/__init__.py', )) -ONLY_BASIC_IMPORT = frozenset((('basic',),)) +ONLY_BASIC_IMPORT = frozenset((('ansible', '__init__'), + ('ansible', 'module_utils', '__init__'), + ('ansible', 'module_utils', 'basic',),)) ONLY_BASIC_FILE = frozenset(('ansible/module_utils/basic.py',)) +ANSIBLE_LIB = os.path.join(os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(__file__)))), 'lib', 'ansible') + @pytest.fixture def finder_containers(): FinderContainers = namedtuple('FinderContainers', ['py_module_names', 'py_module_cache', 'zf']) - py_module_names = set() + py_module_names = set((('ansible', '__init__'), ('ansible', 'module_utils', '__init__'))) # py_module_cache = {('__init__',): b''} py_module_cache = {} @@ -107,7 +114,7 @@ class TestRecursiveFinder(object): def test_no_module_utils(self, finder_containers): name = 'ping' data = b'#!/usr/bin/python\nreturn \'{\"changed\": false}\'' - recursive_finder(name, data, *finder_containers) + recursive_finder(name, os.path.join(ANSIBLE_LIB, 'modules', 'system', 'ping.py'), data, *finder_containers) assert finder_containers.py_module_names == set(()).union(MODULE_UTILS_BASIC_IMPORTS) assert finder_containers.py_module_cache == {} assert frozenset(finder_containers.zf.namelist()) == MODULE_UTILS_BASIC_FILES @@ -116,14 +123,14 @@ class TestRecursiveFinder(object): name = 'fake_module' data = b'#!/usr/bin/python\ndef something(:\n pass\n' with pytest.raises(ansible.errors.AnsibleError) as exec_info: - recursive_finder(name, data, *finder_containers) + recursive_finder(name, os.path.join(ANSIBLE_LIB, 'modules', 'system', 'fake_module.py'), data, *finder_containers) assert 'Unable to import fake_module due to invalid syntax' in str(exec_info.value) def test_module_utils_with_identation_error(self, finder_containers): name = 'fake_module' data = b'#!/usr/bin/python\n def something():\n pass\n' with pytest.raises(ansible.errors.AnsibleError) as exec_info: - recursive_finder(name, data, *finder_containers) + recursive_finder(name, os.path.join(ANSIBLE_LIB, 'modules', 'system', 'fake_module.py'), data, *finder_containers) assert 'Unable to import fake_module due to unexpected indent' in str(exec_info.value) def test_from_import_toplevel_package(self, finder_containers, mocker): @@ -140,10 +147,10 @@ class TestRecursiveFinder(object): name = 'ping' data = b'#!/usr/bin/python\nfrom ansible.module_utils import foo' - recursive_finder(name, data, *finder_containers) + recursive_finder(name, os.path.join(ANSIBLE_LIB, 'modules', 'system', 'ping.py'), data, *finder_containers) mocker.stopall() - assert finder_containers.py_module_names == set((('foo', '__init__'),)).union(ONLY_BASIC_IMPORT) + assert finder_containers.py_module_names == set((('ansible', 'module_utils', 'foo', '__init__'),)).union(ONLY_BASIC_IMPORT) assert finder_containers.py_module_cache == {} assert frozenset(finder_containers.zf.namelist()) == frozenset(('ansible/module_utils/foo/__init__.py',)).union(ONLY_BASIC_FILE) @@ -158,10 +165,10 @@ class TestRecursiveFinder(object): name = 'ping' data = b'#!/usr/bin/python\nfrom ansible.module_utils import foo' - recursive_finder(name, data, *finder_containers) + recursive_finder(name, os.path.join(ANSIBLE_LIB, 'modules', 'system', 'ping.py'), data, *finder_containers) mocker.stopall() - assert finder_containers.py_module_names == set((('foo',),)).union(ONLY_BASIC_IMPORT) + assert finder_containers.py_module_names == set((('ansible', 'module_utils', 'foo',),)).union(ONLY_BASIC_IMPORT) assert finder_containers.py_module_cache == {} assert frozenset(finder_containers.zf.namelist()) == frozenset(('ansible/module_utils/foo.py',)).union(ONLY_BASIC_FILE) @@ -171,23 +178,23 @@ class TestRecursiveFinder(object): def test_from_import_six(self, finder_containers): name = 'ping' data = b'#!/usr/bin/python\nfrom ansible.module_utils import six' - recursive_finder(name, data, *finder_containers) - assert finder_containers.py_module_names == set((('six', '__init__'),)).union(MODULE_UTILS_BASIC_IMPORTS) + recursive_finder(name, os.path.join(ANSIBLE_LIB, 'modules', 'system', 'ping.py'), data, *finder_containers) + assert finder_containers.py_module_names == set((('ansible', 'module_utils', 'six', '__init__'),)).union(MODULE_UTILS_BASIC_IMPORTS) assert finder_containers.py_module_cache == {} assert frozenset(finder_containers.zf.namelist()) == frozenset(('ansible/module_utils/six/__init__.py', )).union(MODULE_UTILS_BASIC_FILES) def test_import_six(self, finder_containers): name = 'ping' data = b'#!/usr/bin/python\nimport ansible.module_utils.six' - recursive_finder(name, data, *finder_containers) - assert finder_containers.py_module_names == set((('six', '__init__'),)).union(MODULE_UTILS_BASIC_IMPORTS) + recursive_finder(name, os.path.join(ANSIBLE_LIB, 'modules', 'system', 'ping.py'), data, *finder_containers) + assert finder_containers.py_module_names == set((('ansible', 'module_utils', 'six', '__init__'),)).union(MODULE_UTILS_BASIC_IMPORTS) assert finder_containers.py_module_cache == {} assert frozenset(finder_containers.zf.namelist()) == frozenset(('ansible/module_utils/six/__init__.py', )).union(MODULE_UTILS_BASIC_FILES) def test_import_six_from_many_submodules(self, finder_containers): name = 'ping' data = b'#!/usr/bin/python\nfrom ansible.module_utils.six.moves.urllib.parse import urlparse' - recursive_finder(name, data, *finder_containers) - assert finder_containers.py_module_names == set((('six', '__init__'),)).union(MODULE_UTILS_BASIC_IMPORTS) + recursive_finder(name, os.path.join(ANSIBLE_LIB, 'modules', 'system', 'ping.py'), data, *finder_containers) + assert finder_containers.py_module_names == set((('ansible', 'module_utils', 'six', '__init__'),)).union(MODULE_UTILS_BASIC_IMPORTS) assert finder_containers.py_module_cache == {} assert frozenset(finder_containers.zf.namelist()) == frozenset(('ansible/module_utils/six/__init__.py',)).union(MODULE_UTILS_BASIC_FILES)