Skip to content

Commit

Permalink
Module validation: sanity check mutually_exclusive, required_if, requ…
Browse files Browse the repository at this point in the history 
…ired_xxx ... (#66961)

* required_if checks should have three or four parts.

* Validate mutually_exclusive, required_together, required_one_of, required_if and required_by.

* Simplify code.

* Improve messages.

* Add changelog.

* Sanity check.

* Update docs.

* Update ignore.txt.

* Don't continue with tests when terms are not strings.

* Remove ignore.txt entry.

* Make sure validate-modules doesn't choke on things already flagged by schema test.

* Check required_if requirements list for strings.
  • Loading branch information
@felixfontein
felixfontein committed Feb 19, 2020
1 parent 29ca9d2 commit 4373863
Show file tree
Hide file tree
Showing 5 changed files with 608 additions and 16 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/66961-ansible-test-required-mutually.yml
View file
@@ -0,0 +1,2 @@
minor_changes:
- "ansible-test - ``mutually_exclusive``, ``required_if``, ``required_by``, ``required_together`` and ``required_one_of`` in modules are now validated."
18 changes: 18 additions & 0 deletions docs/docsite/rst/dev_guide/testing_validate-modules.rst
View file
Expand Up @@ -141,4 +141,22 @@ Codes
use-run-command-not-os-call Imports Error ``os.call`` used instead of ``module.run_command``
use-run-command-not-popen Imports Error ``subprocess.Popen`` used instead of ``module.run_command``
use-short-gplv3-license Documentation Error GPLv3 license header should be the :ref:`short form <copyright>` for new modules
mutually_exclusive-type Documentation Error mutually_exclusive entry contains non-string value
mutually_exclusive-collision Documentation Error mutually_exclusive entry has repeated terms
mutually_exclusive-unknown Documentation Error mutually_exclusive entry contains option which does not appear in argument_spec (potentially an alias of an option?)
required_one_of-type Documentation Error required_one_of entry contains non-string value
required_one_of-collision Documentation Error required_one_of entry has repeated terms
required_one_of-unknown Documentation Error required_one_of entry contains option which does not appear in argument_spec (potentially an alias of an option?)
required_together-type Documentation Error required_together entry contains non-string value
required_together-collision Documentation Error required_together entry has repeated terms
required_together-unknown Documentation Error required_together entry contains option which does not appear in argument_spec (potentially an alias of an option?)
required_if-is_one_of-type Documentation Error required_if entry has a fourth value which is not a bool
required_if-requirements-type Documentation Error required_if entry has a third value (requirements) which is not a list or tuple
required_if-requirements-collision Documentation Error required_if entry has repeated terms in requirements
required_if-requirements-unknown Documentation Error required_if entry's requirements contains option which does not appear in argument_spec (potentially an alias of an option?)
required_if-unknown-key Documentation Error required_if entry's key does not appear in argument_spec (potentially an alias of an option?)
required_if-key-in-requirements Documentation Error required_if entry contains its key in requirements list/tuple
required_if-value-type Documentation Error required_if entry's value is not of the type specified for its key
required_by-collision Documentation Error required_by entry has repeated terms
required_by-unknown Documentation Error required_by entry contains option which does not appear in argument_spec (potentially an alias of an option?)
============================================================ ================== ==================== =========================================================================================
209 changes: 206 additions & 3 deletions test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py
View file
Expand Up @@ -50,7 +50,7 @@
from .utils import CaptureStd, NoArgsAnsibleModule, compare_unordered_lists, is_empty, parse_yaml
from voluptuous.humanize import humanize_error

from ansible.module_utils.six import PY3, with_metaclass
from ansible.module_utils.six import PY3, with_metaclass, string_types

if PY3:
# Because there is no ast.TryExcept in Python 3 ast module
Expand Down Expand Up @@ -1152,7 +1152,200 @@ def _validate_ansible_module_call(self, docs):

self._validate_argument_spec(docs, spec, kwargs)

def _validate_argument_spec(self, docs, spec, kwargs, context=None):
def _validate_list_of_module_args(self, name, terms, spec, context):
if terms is None:
return
if not isinstance(terms, (list, tuple)):
# This is already reported by schema checking
return
for check in terms:
if not isinstance(check, (list, tuple)):
# This is already reported by schema checking
continue
bad_term = False
for term in check:
if not isinstance(term, string_types):
msg = name
if context:
msg += " found in %s" % " -> ".join(context)
msg += " must contain strings in the lists or tuples; found value %r" % (term, )
self.reporter.error(
path=self.object_path,
code=name + '-type',
msg=msg,
)
bad_term = True
if bad_term:
continue
if len(set(check)) != len(check):
msg = name
if context:
msg += " found in %s" % " -> ".join(context)
msg += " has repeated terms"
self.reporter.error(
path=self.object_path,
code=name + '-collision',
msg=msg,
)
if not set(check) <= set(spec):
msg = name
if context:
msg += " found in %s" % " -> ".join(context)
msg += " contains terms which are not part of argument_spec: %s" % ", ".join(sorted(set(check).difference(set(spec))))
self.reporter.error(
path=self.object_path,
code=name + '-unknown',
msg=msg,
)

def _validate_required_if(self, terms, spec, context, module):
if terms is None:
return
if not isinstance(terms, (list, tuple)):
# This is already reported by schema checking
return
for check in terms:
if not isinstance(check, (list, tuple)) or len(check) not in [3, 4]:
# This is already reported by schema checking
continue
if len(check) == 4 and not isinstance(check[3], bool):
msg = "required_if"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " must have forth value omitted or of type bool; got %r" % (check[3], )
self.reporter.error(
path=self.object_path,
code='required_if-is_one_of-type',
msg=msg,
)
requirements = check[2]
if not isinstance(requirements, (list, tuple)):
msg = "required_if"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " must have third value (requirements) being a list or tuple; got type %r" % (requirements, )
self.reporter.error(
path=self.object_path,
code='required_if-requirements-type',
msg=msg,
)
continue
bad_term = False
for term in requirements:
if not isinstance(term, string_types):
msg = "required_if"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " must have only strings in third value (requirements); got %r" % (term, )
self.reporter.error(
path=self.object_path,
code='required_if-requirements-type',
msg=msg,
)
bad_term = True
if bad_term:
continue
if len(set(requirements)) != len(requirements):
msg = "required_if"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " has repeated terms in requirements"
self.reporter.error(
path=self.object_path,
code='required_if-requirements-collision',
msg=msg,
)
if not set(requirements) <= set(spec):
msg = "required_if"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " contains terms in requirements which are not part of argument_spec: %s" % ", ".join(sorted(set(requirements).difference(set(spec))))
self.reporter.error(
path=self.object_path,
code='required_if-requirements-unknown',
msg=msg,
)
key = check[0]
if key not in spec:
msg = "required_if"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " must have its key %s in argument_spec" % key
self.reporter.error(
path=self.object_path,
code='required_if-unknown-key',
msg=msg,
)
continue
if key in requirements:
msg = "required_if"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " contains its key %s in requirements" % key
self.reporter.error(
path=self.object_path,
code='required_if-key-in-requirements',
msg=msg,
)
value = check[1]
if value is not None:
_type = spec[key].get('type', 'str')
if callable(_type):
_type_checker = _type
else:
_type_checker = module._CHECK_ARGUMENT_TYPES_DISPATCHER.get(_type)
try:
with CaptureStd():
dummy = _type_checker(value)
except (Exception, SystemExit):
msg = "required_if"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " has value %r which does not fit to %s's parameter type %r" % (value, key, _type)
self.reporter.error(
path=self.object_path,
code='required_if-value-type',
msg=msg,
)

def _validate_required_by(self, terms, spec, context):
if terms is None:
return
if not isinstance(terms, Mapping):
# This is already reported by schema checking
return
for key, value in terms.items():
if isinstance(value, string_types):
value = [value]
if not isinstance(value, (list, tuple)):
# This is already reported by schema checking
continue
for term in value:
if not isinstance(term, string_types):
# This is already reported by schema checking
continue
if len(set(value)) != len(value) or key in value:
msg = "required_by"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " has repeated terms"
self.reporter.error(
path=self.object_path,
code='required_by-collision',
msg=msg,
)
if not set(value) <= set(spec) or key not in spec:
msg = "required_by"
if context:
msg += " found in %s" % " -> ".join(context)
msg += " contains terms which are not part of argument_spec: %s" % ", ".join(sorted(set(value).difference(set(spec))))
self.reporter.error(
path=self.object_path,
code='required_by-unknown',
msg=msg,
)

def _validate_argument_spec(self, docs, spec, kwargs, context=None, last_context_spec=None):
if not self.analyze_arg_spec:
return

Expand All @@ -1162,6 +1355,9 @@ def _validate_argument_spec(self, docs, spec, kwargs, context=None):
if context is None:
context = []

if last_context_spec is None:
last_context_spec = kwargs

try:
if not context:
add_fragments(docs, self.object_path, fragment_loader=fragment_loader)
Expand All @@ -1172,6 +1368,12 @@ def _validate_argument_spec(self, docs, spec, kwargs, context=None):
# Use this to access type checkers later
module = NoArgsAnsibleModule({})

self._validate_list_of_module_args('mutually_exclusive', last_context_spec.get('mutually_exclusive'), spec, context)
self._validate_list_of_module_args('required_together', last_context_spec.get('required_together'), spec, context)
self._validate_list_of_module_args('required_one_of', last_context_spec.get('required_one_of'), spec, context)
self._validate_required_if(last_context_spec.get('required_if'), spec, context, module)
self._validate_required_by(last_context_spec.get('required_by'), spec, context)

provider_args = set()
args_from_argspec = set()
deprecated_args_from_argspec = set()
Expand Down Expand Up @@ -1541,7 +1743,8 @@ def _validate_argument_spec(self, docs, spec, kwargs, context=None):
code='missing-suboption-docs',
msg=msg
)
self._validate_argument_spec({'options': doc_suboptions}, spec_suboptions, kwargs, context=context + [arg])
self._validate_argument_spec({'options': doc_suboptions}, spec_suboptions, kwargs,
context=context + [arg], last_context_spec=data)

for arg in args_from_argspec:
if not str(arg).isidentifier():
Expand Down
2 changes: 1 addition & 1 deletion test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/schema.py
View file
Expand Up @@ -75,7 +75,7 @@ def sequence_of_sequences(min=None, max=None):
'mutually_exclusive': sequence_of_sequences(min=2),
'required_together': sequence_of_sequences(min=2),
'required_one_of': sequence_of_sequences(min=2),
'required_if': sequence_of_sequences(min=3),
'required_if': sequence_of_sequences(min=3, max=4),
'required_by': Schema({str: Any(list_string_types, tuple_string_types, *string_types)}),
}

Expand Down

0 comments on commit 4373863

Please sign in to comment.