From ba801be6a607f551833bdf0fc144b92817627186 Mon Sep 17 00:00:00 2001 From: Matt Martz Date: Tue, 31 Jul 2018 17:51:53 -0500 Subject: [PATCH] Add AnsibleModule signature schema, and fix associated issues (#43512) (#43516) * Add AnsibleModule signature schema, and fix associated issues (#43512) (cherry picked from commit 01c0446cb54acca76a21a86a698b5937ffb879f2) * Address additional module issues --- .../dev_guide/testing_validate-modules.rst | 1 + .../modules/cloud/amazon/ecs_attribute.py | 2 +- .../modules/cloud/amazon/ecs_cluster.py | 2 +- .../cloud/amazon/elb_application_lb.py | 4 +-- .../cloud/amazon/elb_application_lb_facts.py | 2 +- .../modules/cloud/amazon/elb_target.py | 2 +- .../cloud/amazon/elb_target_group_facts.py | 2 +- .../digital_ocean_floating_ip.py | 8 ++--- .../modules/cloud/opennebula/one_service.py | 2 +- .../modules/clustering/k8s/_kubernetes.py | 2 +- .../modules/net_tools/basics/get_url.py | 2 +- lib/ansible/modules/net_tools/dnsimple.py | 4 +-- lib/ansible/modules/net_tools/dnsmadeeasy.py | 4 +-- .../modules/network/vyos/vyos_interface.py | 2 +- .../modules/packaging/language/pear.py | 3 +- test/sanity/validate-modules/main.py | 35 +++++++++++-------- test/sanity/validate-modules/schema.py | 33 ++++++++++++++++- 17 files changed, 73 insertions(+), 37 deletions(-) diff --git a/docs/docsite/rst/dev_guide/testing_validate-modules.rst b/docs/docsite/rst/dev_guide/testing_validate-modules.rst index c56169832ab..2e49a9c3316 100644 --- a/docs/docsite/rst/dev_guide/testing_validate-modules.rst +++ b/docs/docsite/rst/dev_guide/testing_validate-modules.rst @@ -117,6 +117,7 @@ Errors 328 Choices value from the documentation is not compatible with type defined in the argument_spec 329 Default value from the argument_spec is not compatible with type defined in the argument_spec 330 Choices value from the argument_spec is not compatible with type defined in the argument_spec + 332 ``AnsibleModule`` schema validation error .. --------- ------------------- **4xx** **Syntax** diff --git a/lib/ansible/modules/cloud/amazon/ecs_attribute.py b/lib/ansible/modules/cloud/amazon/ecs_attribute.py index ce8455ce3c7..3723e7b5963 100644 --- a/lib/ansible/modules/cloud/amazon/ecs_attribute.py +++ b/lib/ansible/modules/cloud/amazon/ecs_attribute.py @@ -257,7 +257,7 @@ def main(): attributes=dict(required=True, type='list'), )) - required_together = (['cluster', 'ec2_instance_id', 'attributes']) + required_together = [['cluster', 'ec2_instance_id', 'attributes']] module = AnsibleModule(argument_spec=argument_spec, supports_check_mode=True, required_together=required_together) diff --git a/lib/ansible/modules/cloud/amazon/ecs_cluster.py b/lib/ansible/modules/cloud/amazon/ecs_cluster.py index a4a9503d25a..eedd9892d8b 100644 --- a/lib/ansible/modules/cloud/amazon/ecs_cluster.py +++ b/lib/ansible/modules/cloud/amazon/ecs_cluster.py @@ -162,7 +162,7 @@ def main(): delay=dict(required=False, type='int', default=10), repeat=dict(required=False, type='int', default=10) )) - required_together = (['state', 'name']) + required_together = [['state', 'name']] module = AnsibleModule(argument_spec=argument_spec, supports_check_mode=True, required_together=required_together) diff --git a/lib/ansible/modules/cloud/amazon/elb_application_lb.py b/lib/ansible/modules/cloud/amazon/elb_application_lb.py index 42f31d7051b..31ffd6c9461 100644 --- a/lib/ansible/modules/cloud/amazon/elb_application_lb.py +++ b/lib/ansible/modules/cloud/amazon/elb_application_lb.py @@ -519,9 +519,9 @@ def main(): required_if=[ ('state', 'present', ['subnets', 'security_groups']) ], - required_together=( + required_together=[ ['access_logs_enabled', 'access_logs_s3_bucket', 'access_logs_s3_prefix'] - ) + ] ) # Quick check of listeners parameters diff --git a/lib/ansible/modules/cloud/amazon/elb_application_lb_facts.py b/lib/ansible/modules/cloud/amazon/elb_application_lb_facts.py index 268d96d52e5..0e601a66622 100644 --- a/lib/ansible/modules/cloud/amazon/elb_application_lb_facts.py +++ b/lib/ansible/modules/cloud/amazon/elb_application_lb_facts.py @@ -258,7 +258,7 @@ def main(): ) module = AnsibleModule(argument_spec=argument_spec, - mutually_exclusive=['load_balancer_arns', 'names'], + mutually_exclusive=[['load_balancer_arns', 'names']], supports_check_mode=True ) diff --git a/lib/ansible/modules/cloud/amazon/elb_target.py b/lib/ansible/modules/cloud/amazon/elb_target.py index 260be30fcac..6a3eabc9520 100644 --- a/lib/ansible/modules/cloud/amazon/elb_target.py +++ b/lib/ansible/modules/cloud/amazon/elb_target.py @@ -296,7 +296,7 @@ def main(): ) module = AnsibleModule(argument_spec=argument_spec, - mutually_exclusive=['target_group_arn', 'target_group_name'] + mutually_exclusive=[['target_group_arn', 'target_group_name']] ) if not HAS_BOTO3: diff --git a/lib/ansible/modules/cloud/amazon/elb_target_group_facts.py b/lib/ansible/modules/cloud/amazon/elb_target_group_facts.py index 95dbf928704..31bbc818476 100644 --- a/lib/ansible/modules/cloud/amazon/elb_target_group_facts.py +++ b/lib/ansible/modules/cloud/amazon/elb_target_group_facts.py @@ -247,7 +247,7 @@ def main(): ) module = AnsibleModule(argument_spec=argument_spec, - mutually_exclusive=['load_balancer_arn', 'target_group_arns', 'names'], + mutually_exclusive=[['load_balancer_arn', 'target_group_arns', 'names']], supports_check_mode=True ) diff --git a/lib/ansible/modules/cloud/digital_ocean/digital_ocean_floating_ip.py b/lib/ansible/modules/cloud/digital_ocean/digital_ocean_floating_ip.py index 398179e8a4b..890ad91c42a 100644 --- a/lib/ansible/modules/cloud/digital_ocean/digital_ocean_floating_ip.py +++ b/lib/ansible/modules/cloud/digital_ocean/digital_ocean_floating_ip.py @@ -302,12 +302,12 @@ def main(): validate_certs=dict(type='bool', default=True), timeout=dict(type='int', default=30), ), - required_if=([ + required_if=[ ('state', 'delete', ['ip']) - ]), - mutually_exclusive=( + ], + mutually_exclusive=[ ['region', 'droplet_id'] - ), + ], ) core(module) diff --git a/lib/ansible/modules/cloud/opennebula/one_service.py b/lib/ansible/modules/cloud/opennebula/one_service.py index 322f408a462..2e4dc0a766d 100644 --- a/lib/ansible/modules/cloud/opennebula/one_service.py +++ b/lib/ansible/modules/cloud/opennebula/one_service.py @@ -693,7 +693,7 @@ def main(): ['template_id', 'template_name', 'cardinality'], ['service_id', 'custom_attrs'] ], - required_together=['role', 'cardinality'], + required_together=[['role', 'cardinality']], supports_check_mode=True) auth = get_connection_info(module) diff --git a/lib/ansible/modules/clustering/k8s/_kubernetes.py b/lib/ansible/modules/clustering/k8s/_kubernetes.py index 5e57e69e71b..d635f6341ab 100644 --- a/lib/ansible/modules/clustering/k8s/_kubernetes.py +++ b/lib/ansible/modules/clustering/k8s/_kubernetes.py @@ -349,7 +349,7 @@ def main(): mutually_exclusive=(('file_reference', 'inline_data'), ('url_username', 'insecure'), ('url_password', 'insecure')), - required_one_of=(('file_reference', 'inline_data')), + required_one_of=(('file_reference', 'inline_data'),), ) if not HAS_LIB_YAML: diff --git a/lib/ansible/modules/net_tools/basics/get_url.py b/lib/ansible/modules/net_tools/basics/get_url.py index 65d23548ba6..38c48e2bc2c 100644 --- a/lib/ansible/modules/net_tools/basics/get_url.py +++ b/lib/ansible/modules/net_tools/basics/get_url.py @@ -398,7 +398,7 @@ def main(): argument_spec=argument_spec, add_file_common_args=True, supports_check_mode=True, - mutually_exclusive=(['checksum', 'sha256sum']), + mutually_exclusive=[['checksum', 'sha256sum']], ) url = module.params['url'] diff --git a/lib/ansible/modules/net_tools/dnsimple.py b/lib/ansible/modules/net_tools/dnsimple.py index 57e53553022..f0caa8d9b74 100644 --- a/lib/ansible/modules/net_tools/dnsimple.py +++ b/lib/ansible/modules/net_tools/dnsimple.py @@ -167,9 +167,9 @@ def main(): state=dict(required=False, choices=['present', 'absent']), solo=dict(required=False, type='bool'), ), - required_together=( + required_together=[ ['record', 'value'] - ), + ], supports_check_mode=True, ) diff --git a/lib/ansible/modules/net_tools/dnsmadeeasy.py b/lib/ansible/modules/net_tools/dnsmadeeasy.py index 29eccf9255c..da5f4e78615 100644 --- a/lib/ansible/modules/net_tools/dnsmadeeasy.py +++ b/lib/ansible/modules/net_tools/dnsmadeeasy.py @@ -562,9 +562,9 @@ def main(): ip5=dict(required=False), validate_certs=dict(default='yes', type='bool'), ), - required_together=( + required_together=[ ['record_value', 'record_ttl', 'record_type'] - ), + ], required_if=[ ['failover', True, ['autoFailover', 'port', 'protocol', 'ip1', 'ip2']], ['monitor', True, ['port', 'protocol', 'maxEmails', 'systemDescription', 'ip1']] diff --git a/lib/ansible/modules/network/vyos/vyos_interface.py b/lib/ansible/modules/network/vyos/vyos_interface.py index 78cc60fda43..63f441c6bb8 100644 --- a/lib/ansible/modules/network/vyos/vyos_interface.py +++ b/lib/ansible/modules/network/vyos/vyos_interface.py @@ -393,7 +393,7 @@ def main(): required_one_of = [['name', 'aggregate']] mutually_exclusive = [['name', 'aggregate']] - required_together = (['speed', 'duplex']) + required_together = [['speed', 'duplex']] module = AnsibleModule(argument_spec=argument_spec, required_one_of=required_one_of, mutually_exclusive=mutually_exclusive, diff --git a/lib/ansible/modules/packaging/language/pear.py b/lib/ansible/modules/packaging/language/pear.py index a284f3d3148..f18dd438c3d 100644 --- a/lib/ansible/modules/packaging/language/pear.py +++ b/lib/ansible/modules/packaging/language/pear.py @@ -201,10 +201,9 @@ def check_packages(module, packages, state): def main(): module = AnsibleModule( argument_spec=dict( - name=dict(aliases=['pkg']), + name=dict(aliases=['pkg'], required=True), state=dict(default='present', choices=['present', 'installed', "latest", 'absent', 'removed']), executable=dict(default=None, required=False, type='path')), - required_one_of=[['name']], supports_check_mode=True) p = module.params diff --git a/test/sanity/validate-modules/main.py b/test/sanity/validate-modules/main.py index 0524df22a10..8d3523461a2 100755 --- a/test/sanity/validate-modules/main.py +++ b/test/sanity/validate-modules/main.py @@ -43,7 +43,7 @@ from ansible.utils.plugin_docs import BLACKLIST, add_fragments, get_docstring from module_args import AnsibleModuleImportError, get_argument_spec -from schema import doc_schema, metadata_1_1_schema, return_schema +from schema import ansible_module_kwargs_schema, doc_schema, metadata_1_1_schema, return_schema from utils import CaptureStd, NoArgsAnsibleModule, compare_unordered_lists, is_empty, parse_yaml from voluptuous.humanize import humanize_error @@ -1002,19 +1002,7 @@ class ModuleValidator(Validator): msg='version_added should be %s. Currently %s' % (should_be, version_added) ) - def _validate_argument_spec(self, docs): - if not self.analyze_arg_spec: - return - - if docs is None: - docs = {} - - try: - add_fragments(docs, self.object_path, fragment_loader=fragment_loader) - except Exception: - # Cannot merge fragments - return - + def _validate_ansible_module_call(self, docs): try: spec, args, kwargs = get_argument_spec(self.path) except AnsibleModuleImportError as e: @@ -1029,6 +1017,23 @@ class ModuleValidator(Validator): ) return + self._validate_docs_schema(kwargs, ansible_module_kwargs_schema, 'AnsibleModule', 332) + + self._validate_argument_spec(docs, spec, kwargs) + + def _validate_argument_spec(self, docs, spec, kwargs): + if not self.analyze_arg_spec: + return + + if docs is None: + docs = {} + + try: + add_fragments(docs, self.object_path, fragment_loader=fragment_loader) + except Exception: + # Cannot merge fragments + return + # Use this to access type checkers later module = NoArgsAnsibleModule({}) @@ -1339,7 +1344,7 @@ class ModuleValidator(Validator): # FIXME if +2 then file should be empty? - maybe add this only in the future if self._python_module() and not self._just_docs() and not end_of_deprecation_should_be_docs_only: - self._validate_argument_spec(docs) + self._validate_ansible_module_call(docs) self._check_for_sys_exit() self._find_blacklist_imports() main = self._find_main_call() diff --git a/test/sanity/validate-modules/schema.py b/test/sanity/validate-modules/schema.py index 1ec667ba87e..06fde49a9f5 100644 --- a/test/sanity/validate-modules/schema.py +++ b/test/sanity/validate-modules/schema.py @@ -16,10 +16,41 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . -from voluptuous import PREVENT_EXTRA, Any, Required, Schema, Self +from voluptuous import PREVENT_EXTRA, All, Any, Length, Required, Schema, Self from ansible.module_utils.six import string_types list_string_types = list(string_types) + +def sequence_of_sequences(min=None, max=None): + return All( + Any( + None, + [Length(min=min, max=max)], + tuple([Length(min=min, max=max)]), + ), + Any( + None, + [Any(list, tuple)], + tuple([Any(list, tuple)]), + ), + ) + + +ansible_module_kwargs_schema = Schema( + { + 'argument_spec': dict, + 'bypass_checks': bool, + 'no_log': bool, + 'check_invalid_arguments': Any(None, bool), + 'mutually_exclusive': sequence_of_sequences(min=2), + 'required_together': sequence_of_sequences(min=2), + 'required_one_of': sequence_of_sequences(min=2), + 'add_file_common_args': bool, + 'supports_check_mode': bool, + 'required_if': sequence_of_sequences(min=3), + } +) + suboption_schema = Schema( { Required('description'): Any(list_string_types, *string_types),