Skip to content
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

Move type checking methods out of basic.py #53687

Merged
merged 19 commits into from Mar 21, 2019

Conversation

Projects
None yet
4 participants
@samdoran
Copy link
Member

samdoran commented Mar 12, 2019

SUMMARY

Move the following type checking methods out of basic.py into lib/ansible/module_utils/common/validation.py as standalone functions:

  • check_type_str()
  • check_type_list()
  • safe_eval()
  • check_type_dict()
  • check_type_bool()
  • check_type_int()
  • check_type_float()
  • check_type_path()
  • check_type_raw()
  • check_type_bytes()
  • check_type_bits()
  • check_type_jsonarg()

Move the following out of basic.py and into lib/ansible/module_utils/common/text/formatters.py:

  • lenient_lowercase()
  • human_to_bytes()
  • bytes_to_human()

Move the following out of basic.py and into lib/ansible/module_utils/common/text/converters.py:

  • jsonify()
  • json_dict_to_unicode_to_bytes() --> container_to_bytes()
  • json_dict_bytes_to_unicode() --> container_to_text()

Create common code for testing json imports in lib/ansible/module_utils/common/_json_compat.py

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/module_utils/common/_json_compat.py
lib/ansible/module_utils/common/validation.py
lib/ansible/module_utils/basic.py

@samdoran samdoran force-pushed the samdoran:arg_spec/type_checking branch from 6e334ed to 4113a4e Mar 12, 2019

@samdoran samdoran force-pushed the samdoran:arg_spec/type_checking branch 2 times, most recently from 505177c to ac1c805 Mar 12, 2019

@ansibot

This comment was marked as outdated.

Copy link
Contributor

ansibot commented Mar 12, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/module_utils/common/validation.py:45:0: syntax-error invalid syntax (<unknown>, line 45)

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with the error:

Command "ansible-doc -t become doas dzdo enable ksu machinectl pbrun pfexec pmrun runas sesu su sudo" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (validation.py, line 45)

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with the error:

Command "ansible-doc -t become doas dzdo enable ksu machinectl pbrun pfexec pmrun runas sesu su sudo" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (validation.py, line 45)

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with the error:

Command "ansible-doc -t become doas dzdo enable ksu machinectl pbrun pfexec pmrun runas sesu su sudo" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (validation.py, line 45)

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with the error:

Command "ansible-doc -t become doas dzdo enable ksu machinectl pbrun pfexec pmrun runas sesu su sudo" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (validation.py, line 45)

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with the error:

Command "ansible-doc -t become doas dzdo enable ksu machinectl pbrun pfexec pmrun runas sesu su sudo" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (validation.py, line 45)

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with the error:

Command "ansible-doc -t become doas dzdo enable ksu machinectl pbrun pfexec pmrun runas sesu su sudo" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (validation.py, line 45)

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Command 'make singlehtmldocs' failed with status code: 2
--> Standard Output
PYTHONPATH=../../lib ../bin/dump_config.py --template-file=../templates/config.rst.j2 --output-dir=rst/reference_appendices/ -d ../../lib/ansible/config/base.yml
mkdir -p rst/cli
PYTHONPATH=../../lib ../bin/generate_man.py --template-file=../templates/cli_rst.j2 --output-dir=rst/cli/ --output-format rst ../../lib/ansible/cli/*.py
Makefile:83: recipe for target 'cli' failed
--> Standard Error
Traceback (most recent call last):
  File "../bin/generate_man.py", line 253, in <module>
    allvars[cli_name] = opts_docs(cli_class_name, cli_name)
  File "../bin/generate_man.py", line 104, in opts_docs
    fromlist=[cli_class_name]), cli_class_name)
  File "/root/ansible/lib/ansible/cli/__init__.py", line 26, in <module>
    from ansible.parsing.dataloader import DataLoader
  File "/root/ansible/lib/ansible/parsing/dataloader.py", line 17, in <module>
    from ansible.module_utils.basic import is_executable
  File "/root/ansible/lib/ansible/module_utils/basic.py", line 170, in <module>
    from ansible.module_utils.common.validation import (
  File "/root/ansible/lib/ansible/module_utils/common/validation.py", line 45
    >>>>>>> 16aa946b27... Move safe_eval() out of basic.py
     ^
SyntaxError: invalid syntax
make: *** [cli] Error 1

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/module_utils/common/validation.py:45:2: SyntaxError: >>>>>>> 16aa946b27... Move safe_eval() out of basic.py

The test ansible-test sanity --test compile --python 2.7 [explain] failed with 1 error:

lib/ansible/module_utils/common/validation.py:45:2: SyntaxError: >>>>>>> 16aa946b27... Move safe_eval() out of basic.py

The test ansible-test sanity --test compile --python 3.5 [explain] failed with 1 error:

lib/ansible/module_utils/common/validation.py:45:2: SyntaxError: >>>>>>> 16aa946b27... Move safe_eval() out of basic.py

The test ansible-test sanity --test compile --python 3.6 [explain] failed with 1 error:

lib/ansible/module_utils/common/validation.py:45:2: SyntaxError: >>>>>>> 16aa946b27... Move safe_eval() out of basic.py

The test ansible-test sanity --test compile --python 3.7 [explain] failed with 1 error:

lib/ansible/module_utils/common/validation.py:45:2: SyntaxError: >>>>>>> 16aa946b27... Move safe_eval() out of basic.py

The test ansible-test sanity --test compile --python 3.8 [explain] failed with 1 error:

lib/ansible/module_utils/common/validation.py:45:1: SyntaxError: >>>>>>> 16aa946b27... Move safe_eval() out of basic.py

click here for bot help

@samdoran samdoran force-pushed the samdoran:arg_spec/type_checking branch 6 times, most recently from 5ea61bd to 4398af9 Mar 12, 2019

Rework check_type_str()
Add allow_conversion option to make the function more self-contained.
Move the messaging back to basic.py since those error messages are more relevant to using this function in the context of AnsibleModule and not when using the function in isolation.

@samdoran samdoran force-pushed the samdoran:arg_spec/type_checking branch from ad59784 to f0be3d4 Mar 16, 2019

@ansibot ansibot added core_review and removed needs_revision labels Mar 19, 2019

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Mar 19, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/module_utils/common/validation/test_check_type_int.py:35:1: W391 blank line at end of file

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Mar 19, 2019

@samdoran samdoran force-pushed the samdoran:arg_spec/type_checking branch from 02b52cc to 52c3404 Mar 19, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 19, 2019

@samdoran samdoran force-pushed the samdoran:arg_spec/type_checking branch 2 times, most recently from 3ac72e1 to 1384623 Mar 19, 2019

@ansibot ansibot added core_review and removed needs_revision labels Mar 19, 2019

msg = common_msg.capitalize()
raise TypeError(to_native(msg))
elif self._string_conversion_action == 'warn':
msg = ('The value {0!r} (type {0.__class__.__name__}) in a string field was converted to {1!r} (type string). '

This comment has been minimized.

@Akasurde

Akasurde Mar 20, 2019

Member

Can we add a key name as well ? It is very confusing message if we have multiple warnings like this -

[WARNING]: The value 600 (type int) in a string field was converted to u'600' (type string). If this does not look like what you expect,
quote the entire value to ensure it does not change.

 [WARNING]: The value {'Name': 'aws-blorp_rhel7'} (type dict) in a string field was converted to u"{'Name': 'aws-blorp_rhel8'}" (type
string). If this does not look like what you expect, quote the entire value to ensure it does not change.

 [WARNING]: The value 300 (type int) in a string field was converted to u'300' (type string). If this does not look like what you expect,
quote the entire value to ensure it does not change.

Maybe something like -

Suggested change
msg = ('The value {0!r} (type {0.__class__.__name__}) in a string field was converted to {1!r} (type string). '
msg = ('The value {0!r} (type {0.__class__.__name__}) in a string field of argument {1} was converted to {2!r} (type string). '

This comment has been minimized.

@samdoran

samdoran Mar 20, 2019

Author Member

I'm not sure of a good way to do this since we don't have the name of the key available, only the value to check or convert.

This comment has been minimized.

@Akasurde

Akasurde Mar 20, 2019

Member

Got it. But this is very confusing error message. anyways, not a blocker as such.

This comment has been minimized.

@samdoran

samdoran Mar 20, 2019

Author Member

I figured the value would be enough to track down the problematic field. Even if you have multiple fields with the same value, the error only prints once (a byproduct of having the same error message text).

But I see that as a warning, it gets printed at the top of the play and that removes a lot of context. 🤔 As an error, it's much more clear what is happening.

There is room for improvement here, for sure.

@samdoran samdoran force-pushed the samdoran:arg_spec/type_checking branch from 1384623 to 262d4b7 Mar 20, 2019

@sivel

sivel approved these changes Mar 20, 2019

@ansibot ansibot added needs_revision and removed core_review labels Mar 20, 2019

@samdoran samdoran merged commit ff88bd8 into ansible:devel Mar 21, 2019

1 check passed

Shippable Run 115209 status is SUCCESS.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.