New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Module validation: sanity check mutually_exclusive, required_if, required_xxx ... #66961
Conversation
The errors found by the check (which resulted in the
|
This comment has been minimized.
This comment has been minimized.
d1f22ad
to
5981f98
Compare
(Rebasing to resolve conflicts.) |
5981f98
to
8839b5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor things.
test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py
Show resolved
Hide resolved
try: | ||
with CaptureStd(): | ||
dummy = _type_checker(value) | ||
except (Exception, SystemExit): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to catch TypeError
here rather than Exception
. Though I suppose a custom callable as a type checker could raise something other than TypeError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the same catch
expression as
ansible/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py
Lines 1490 to 1497 in 8839b5b
try: | |
doc_default = None | |
if 'default' in doc_options_arg and not is_empty(doc_options_arg['default']): | |
with CaptureStd(): | |
doc_default = _type_checker(doc_options_arg['default']) | |
elif doc_options_arg.get('default') is None and _type == 'bool' and 'suboptions' not in doc_options_arg: | |
doc_default = False | |
except (Exception, SystemExit): |
ansible/test/lib/ansible_test/_data/sanity/validate-modules/validate_modules/main.py
Lines 1450 to 1453 in 8839b5b
try: | |
with CaptureStd(): | |
arg_default = _type_checker(data['default']) | |
except (Exception, SystemExit): |
1965cb7
to
1869ddd
Compare
@felixfontein , i've tried to test it manually
(
Is it expected or i did anything wrong? |
@Andersson007 it's not expected! (Potentially
for your example. |
cool! now that works ok in this part
i'm going to check the other options |
i made a mistake passing the option as
but probably it makes sense to check |
@Andersson007 this is already caught by the schema test; I've added more code which ignores bad data which is reported by the schema test. |
passed
ran
got
|
That's it. The other options work ok. |
@Andersson007 I've added a check for types in |
@samdoran @Andersson007 thanks for all the reviewing and testing and merging! |
SUMMARY
Adds some basic checks for
mutually_exclusive
,required_if
,required_by
,required_together
,required_one_of
. The checks test for whether the options actually exist in argument_spec, whether there are repeated keys, and whether types are correct (like value forrequired_if
has to satisfy type of option).This found at least one real bug: https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/podman/podman_image.py#L713-L714 mentions
authfile
, which is an alias ofpath
. If the user combinespath
withusername
orpassword
,AnsibleModule
will accept this.Quite some ignore.txt entries come from things inserted in common module_utils code, like for azure and kubevirt.
ISSUE TYPE
COMPONENT NAME
ansible-test