From dcc05093db395eb1b7c42f46ef5b5506de67da6b Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Thu, 11 Jan 2018 17:41:53 -0600 Subject: [PATCH] Validate modules arg spec fixes (#34477) * Update validate-modules arg_spec introspection to be faster, by only mocking the imports we explicitly list * The use of types.MethodType in redhat_subscription wasn't py3 compatible, use partial instead * Remove argument_spec import hacks, make them errors, we can ignore them with ansible-test * Enable the --arg-spec flag for validate-modules --- .../dev_guide/testing_validate-modules.rst | 1 + .../packaging/os/redhat_subscription.py | 4 +- test/runner/lib/sanity/validate_modules.py | 1 + test/sanity/validate-modules/ignore.txt | 103 ++++++++++++++++++ test/sanity/validate-modules/main.py | 24 +++- test/sanity/validate-modules/module_args.py | 79 ++++---------- 6 files changed, 149 insertions(+), 63 deletions(-) diff --git a/docs/docsite/rst/dev_guide/testing_validate-modules.rst b/docs/docsite/rst/dev_guide/testing_validate-modules.rst index 625c1ed265..70ccc5008a 100644 --- a/docs/docsite/rst/dev_guide/testing_validate-modules.rst +++ b/docs/docsite/rst/dev_guide/testing_validate-modules.rst @@ -106,6 +106,7 @@ Errors 318 Module deprecated, but DOCUMENTATION.deprecated is missing 319 ``RETURN`` fragments missing or invalid 320 ``DOCUMENTATION.options`` must be a dictionary/hash when used + 321 ``Exception`` attempting to import module for ``argument_spec`` introspection .. --------- ------------------- **4xx** **Syntax** diff --git a/lib/ansible/modules/packaging/os/redhat_subscription.py b/lib/ansible/modules/packaging/os/redhat_subscription.py index e7e0937079..017127b0b5 100644 --- a/lib/ansible/modules/packaging/os/redhat_subscription.py +++ b/lib/ansible/modules/packaging/os/redhat_subscription.py @@ -231,7 +231,7 @@ import os import re import shutil import tempfile -import types +import functools from ansible.module_utils.basic import AnsibleModule from ansible.module_utils._text import to_native @@ -314,7 +314,7 @@ class Rhsm(RegistrationBase): else: return default - cp.get_option = types.MethodType(get_option_default, cp, configparser.ConfigParser) + cp.get_option = functools.partial(get_option_default, cp) return cp diff --git a/test/runner/lib/sanity/validate_modules.py b/test/runner/lib/sanity/validate_modules.py index e52fd9990c..1491e38d2c 100644 --- a/test/runner/lib/sanity/validate_modules.py +++ b/test/runner/lib/sanity/validate_modules.py @@ -57,6 +57,7 @@ class ValidateModulesTest(SanitySingleVersion): 'python%s' % args.python_version, 'test/sanity/validate-modules/validate-modules', '--format', 'json', + '--arg-spec', ] + paths with open(VALIDATE_SKIP_PATH, 'r') as skip_fd: diff --git a/test/sanity/validate-modules/ignore.txt b/test/sanity/validate-modules/ignore.txt index e69de29bb2..f8db1e98d4 100644 --- a/test/sanity/validate-modules/ignore.txt +++ b/test/sanity/validate-modules/ignore.txt @@ -0,0 +1,103 @@ +lib/ansible/modules/cloud/amazon/ec2_vpc_subnet.py E317 +lib/ansible/modules/cloud/amazon/iam.py E317 +lib/ansible/modules/cloud/amazon/iam_policy.py E317 +lib/ansible/modules/cloud/amazon/kinesis_stream.py E317 +lib/ansible/modules/cloud/amazon/lambda_alias.py E317 +lib/ansible/modules/cloud/amazon/lambda_event.py E317 +lib/ansible/modules/cloud/amazon/sts_assume_role.py E317 +lib/ansible/modules/cloud/atomic/atomic_container.py E317 +lib/ansible/modules/cloud/azure/azure_rm_dnsrecordset.py E321 +lib/ansible/modules/cloud/azure/azure_rm_storageaccount.py E321 +lib/ansible/modules/cloud/centurylink/clc_alert_policy.py E317 +lib/ansible/modules/cloud/centurylink/clc_firewall_policy.py E317 +lib/ansible/modules/cloud/dimensiondata/dimensiondata_network.py E321 +lib/ansible/modules/cloud/ovirt/ovirt_affinity_label.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_cluster.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_datacenter.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_external_provider.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_external_provider_facts.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_host_networks.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_host_pm.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_mac_pools.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_networks.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_tags.py E317 +lib/ansible/modules/cloud/ovirt/ovirt_vmpools.py E317 +lib/ansible/modules/cloud/smartos/imgadm.py E317 +lib/ansible/modules/cloud/webfaction/webfaction_app.py E321 +lib/ansible/modules/cloud/webfaction/webfaction_db.py E321 +lib/ansible/modules/cloud/webfaction/webfaction_domain.py E321 +lib/ansible/modules/cloud/webfaction/webfaction_mailbox.py E321 +lib/ansible/modules/cloud/webfaction/webfaction_site.py E321 +lib/ansible/modules/clustering/k8s/k8s_raw.py E321 +lib/ansible/modules/clustering/k8s/k8s_scale.py E321 +lib/ansible/modules/clustering/openshift/openshift_raw.py E321 +lib/ansible/modules/clustering/openshift/openshift_scale.py E321 +lib/ansible/modules/database/mongodb/mongodb_parameter.py E317 +lib/ansible/modules/monitoring/logicmonitor.py E317 +lib/ansible/modules/monitoring/logicmonitor_facts.py E317 +lib/ansible/modules/monitoring/nagios.py E317 +lib/ansible/modules/monitoring/zabbix/zabbix_maintenance.py E317 +lib/ansible/modules/net_tools/cloudflare_dns.py E317 +lib/ansible/modules/net_tools/haproxy.py E317 +lib/ansible/modules/net_tools/omapi_host.py E317 +lib/ansible/modules/network/cloudengine/ce_reboot.py E317 +lib/ansible/modules/network/f5/bigip_asm_policy.py E321 +lib/ansible/modules/network/f5/bigip_config.py E321 +lib/ansible/modules/network/f5/bigip_configsync_action.py E321 +lib/ansible/modules/network/f5/bigip_configsync_actions.py E321 +lib/ansible/modules/network/f5/bigip_device_connectivity.py E321 +lib/ansible/modules/network/f5/bigip_device_dns.py E321 +lib/ansible/modules/network/f5/bigip_device_httpd.py E321 +lib/ansible/modules/network/f5/bigip_device_ntp.py E321 +lib/ansible/modules/network/f5/bigip_device_sshd.py E321 +lib/ansible/modules/network/f5/bigip_device_trust.py E321 +lib/ansible/modules/network/f5/bigip_gtm_datacenter.py E321 +lib/ansible/modules/network/f5/bigip_gtm_facts.py E321 +lib/ansible/modules/network/f5/bigip_gtm_pool.py E321 +lib/ansible/modules/network/f5/bigip_gtm_server.py E321 +lib/ansible/modules/network/f5/bigip_gtm_wide_ip.py E321 +lib/ansible/modules/network/f5/bigip_hostname.py E317 +lib/ansible/modules/network/f5/bigip_iapplx_package.py E321 +lib/ansible/modules/network/f5/bigip_partition.py E321 +lib/ansible/modules/network/f5/bigip_policy.py E321 +lib/ansible/modules/network/f5/bigip_policy_rule.py E321 +lib/ansible/modules/network/f5/bigip_pool.py E321 +lib/ansible/modules/network/f5/bigip_profile_client_ssl.py E321 +lib/ansible/modules/network/f5/bigip_provision.py E321 +lib/ansible/modules/network/f5/bigip_qkview.py E321 +lib/ansible/modules/network/f5/bigip_remote_syslog.py E321 +lib/ansible/modules/network/f5/bigip_security_port_list.py E321 +lib/ansible/modules/network/f5/bigip_selfip.py E321 +lib/ansible/modules/network/f5/bigip_snat_pool.py E321 +lib/ansible/modules/network/f5/bigip_snmp.py E321 +lib/ansible/modules/network/f5/bigip_snmp_trap.py E321 +lib/ansible/modules/network/f5/bigip_software_update.py E321 +lib/ansible/modules/network/f5/bigip_ssl_certificate.py E321 +lib/ansible/modules/network/f5/bigip_ssl_key.py E321 +lib/ansible/modules/network/f5/bigip_sys_db.py E321 +lib/ansible/modules/network/f5/bigip_sys_global.py E321 +lib/ansible/modules/network/f5/bigip_traffic_group.py E321 +lib/ansible/modules/network/f5/bigip_ucs.py E321 +lib/ansible/modules/network/f5/bigip_user.py E321 +lib/ansible/modules/network/f5/bigip_vcmp_guest.py E321 +lib/ansible/modules/network/f5/bigip_virtual_address.py E321 +lib/ansible/modules/network/f5/bigip_virtual_server.py E321 +lib/ansible/modules/network/f5/bigip_vlan.py E321 +lib/ansible/modules/network/f5/bigip_wait.py E321 +lib/ansible/modules/network/f5/bigiq_regkey_license.py E321 +lib/ansible/modules/network/f5/bigiq_regkey_pool.py E321 +lib/ansible/modules/network/illumos/dladm_linkprop.py E317 +lib/ansible/modules/network/illumos/ipadm_addrprop.py E317 +lib/ansible/modules/network/illumos/ipadm_ifprop.py E317 +lib/ansible/modules/network/radware/vdirect_commit.py E321 +lib/ansible/modules/network/radware/vdirect_file.py E321 +lib/ansible/modules/network/radware/vdirect_runnable.py E321 +lib/ansible/modules/notification/rocketchat.py E317 +lib/ansible/modules/notification/snow_record.py E317 +lib/ansible/modules/remote_management/stacki/stacki_host.py E317 +lib/ansible/modules/storage/netapp/na_cdot_volume.py E317 +lib/ansible/modules/system/make.py E317 +lib/ansible/modules/utilities/helper/_accelerate.py E305 +lib/ansible/modules/utilities/logic/async_status.py E310 +lib/ansible/modules/web_infrastructure/apache2_mod_proxy.py E317 +lib/ansible/modules/web_infrastructure/django_manage.py E317 diff --git a/test/sanity/validate-modules/main.py b/test/sanity/validate-modules/main.py index 9fe715f120..4d87bfa2f2 100755 --- a/test/sanity/validate-modules/main.py +++ b/test/sanity/validate-modules/main.py @@ -40,7 +40,7 @@ from ansible import __version__ as ansible_version from ansible.executor.module_common import REPLACER_WINDOWS from ansible.utils.plugin_docs import BLACKLIST, get_docstring -from module_args import get_argument_spec +from module_args import AnsibleModuleImportError, get_argument_spec from schema import doc_schema, metadata_1_1_schema, return_schema @@ -247,7 +247,7 @@ class ModuleValidator(Validator): self.length = len(self.text.splitlines()) try: self.ast = ast.parse(self.text) - except: + except Exception: self.ast = None if base_branch: @@ -264,7 +264,7 @@ class ModuleValidator(Validator): try: os.remove(self.base_module) - except: + except Exception: pass @property @@ -971,7 +971,21 @@ class ModuleValidator(Validator): def _validate_argument_spec(self): if not self.analyze_arg_spec: return - spec = get_argument_spec(self.path) + + try: + spec = get_argument_spec(self.path) + except AnsibleModuleImportError: + self.reporter.error( + path=self.object_path, + code=321, + msg='Exception attempting to import module for argument_spec introspection' + ) + self.reporter.trace( + path=self.object_path, + tracebk=traceback.format_exc() + ) + return + for arg, data in spec.items(): if data.get('required') and data.get('default', object) != object: self.reporter.error( @@ -1048,7 +1062,7 @@ class ModuleValidator(Validator): (option, version_added)) ) continue - except: + except Exception: # If there is any other exception it should have been caught # in schema validation, so we won't duplicate errors by # listing it again diff --git a/test/sanity/validate-modules/module_args.py b/test/sanity/validate-modules/module_args.py index d25d7b906d..50a9224154 100644 --- a/test/sanity/validate-modules/module_args.py +++ b/test/sanity/validate-modules/module_args.py @@ -19,20 +19,15 @@ import imp import sys -from modulefinder import ModuleFinder +from contextlib import contextmanager import mock +from ansible.module_utils.six import reraise + MODULE_CLASSES = [ 'ansible.module_utils.basic.AnsibleModule', - 'ansible.module_utils.vca.VcaAnsibleModule', - 'ansible.module_utils.network.nxos.nxos.NetworkModule', - 'ansible.module_utils.network.eos.eos.NetworkModule', - 'ansible.module_utils.network.ios.ios.NetworkModule', - 'ansible.module_utils.network.iosxr.iosxr.NetworkModule', - 'ansible.module_utils.network.junos.junos.NetworkModule', - 'ansible.module_utils.openswitch.NetworkModule', ] @@ -40,6 +35,11 @@ class AnsibleModuleCallError(RuntimeError): pass +class AnsibleModuleImportError(ImportError): + pass + + +@contextmanager def add_mocks(filename): gp = mock.patch('ansible.module_utils.basic.get_platform').start() gp.return_value = 'linux' @@ -48,64 +48,31 @@ def add_mocks(filename): mocks = [] for module_class in MODULE_CLASSES: mocks.append( - mock.patch('ansible.module_utils.basic.AnsibleModule', - new=module_mock) + mock.patch('%s.__init__' % module_class, new=module_mock) ) for m in mocks: p = m.start() - p.side_effect = AnsibleModuleCallError() + p.side_effect = AnsibleModuleCallError('AnsibleModuleCallError') + mocks.append( + mock.patch('ansible.module_utils.basic._load_params').start() + ) - finder = ModuleFinder() - try: - finder.run_script(filename) - except: - pass + yield module_mock - sys_mock = mock.MagicMock() - sys_mock.__version__ = '0.0.0' - sys_mocks = [] - for module, sources in finder.badmodules.items(): - if module in sys.modules: - continue - if [s for s in sources if s[:7] in ['ansible', '__main_']]: - parts = module.split('.') - for i in range(len(parts)): - dotted = '.'.join(parts[:i + 1]) - # Never mock out ansible or ansible.module_utils - # we may get here if a specific module_utils file needed mocked - if dotted in ('ansible', 'ansible.module_utils',): - continue - sys.modules[dotted] = sys_mock - sys_mocks.append(dotted) - - return module_mock, mocks, sys_mocks - - -def remove_mocks(mocks, sys_mocks): for m in mocks: m.stop() - for m in sys_mocks: - try: - del sys.modules[m] - except KeyError: - pass - def get_argument_spec(filename): - module_mock, mocks, sys_mocks = add_mocks(filename) - - try: - mod = imp.load_source('module', filename) - if not module_mock.call_args: - mod.main() - except AnsibleModuleCallError: - pass - except Exception: - # We can probably remove this branch, it is here for use while testing - pass - - remove_mocks(mocks, sys_mocks) + with add_mocks(filename) as module_mock: + try: + mod = imp.load_source('module', filename) + if not module_mock.call_args: + mod.main() + except AnsibleModuleCallError: + pass + except Exception as e: + reraise(AnsibleModuleImportError, AnsibleModuleImportError('%s' % e), sys.exc_info()[2]) try: args, kwargs = module_mock.call_args