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

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**

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
 

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:

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

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

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',
+            mock.patch('%s.__init__' % module_class, new=module_mock)
-                       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()
+    yield module_mock
-    try:
-        finder.run_script(filename)
-    except:
-        pass
 
-    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)
+    with add_mocks(filename) as module_mock:
-
+        try:
-    try:
+            mod = imp.load_source('module', filename)
-        mod = imp.load_source('module', filename)
+            if not module_mock.call_args:
-        if not module_mock.call_args:
+                mod.main()
-            mod.main()
+        except AnsibleModuleCallError:
-    except AnsibleModuleCallError:
+            pass
-        pass
+        except Exception as e:
-    except Exception:
+            reraise(AnsibleModuleImportError, AnsibleModuleImportError('%s' % e), sys.exc_info()[2])
-        # We can probably remove this branch, it is here for use while testing
-        pass
-
-    remove_mocks(mocks, sys_mocks)
 
     try:
         args, kwargs = module_mock.call_args