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

WIP: Discourage boolean values in string fields #66631

Open
wants to merge 7 commits into
base: devel
Choose a base branch
from

Conversation

cristiklein
Copy link

@cristiklein cristiklein commented Jan 20, 2020

The YAML 1.1 specification mandates converting boolean-like values such
as 'yes' and 'no' to a booleans, unless quoted. If the user forgets to quote 'yes' in a module which offers this choice in a string parameter, then Ansible
gives a very confusing warning:

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

While one could probably come up with a way to work around this warning, I
think that Ansible should discourage module developers from using
boolean-like value choices in string fields.

This commit adds a deprecation warning in AnsibleModule. The only module
in my playlist that showed this warning was the apt module, which this
commit also fixes. Future version of Ansible should fail when
boolean-like values are valid choices in string parameters.

SUMMARY
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • module_utils
  • apt

The YAML 1.1 specification mandates converting boolean-like values such
as 'yes' and 'no' to a booleans, unless quoted. If the user forgets to quote 'yes' in a module which offers this choice in a string parameter, then Ansible
gives a very confusing warning:

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

While one could probably come up with a way to work around this warning, I
think that Ansible should discourage module developers from using
boolean-like value choices in string fields.

This commit adds a deprecation warning in AnsibleModule. The only module
in my playlist that showed this warning was the apt module, which this
commit also fixes. Future version of Ansible should fail when
boolean-like values are valid choices in string parameters.
@cristiklein cristiklein requested a review from sivel Jan 20, 2020
@ansibot
Copy link
Contributor

ansibot commented Jan 20, 2020

@ansibot ansibot added affects_2.10 core_review feature module needs_triage new_contributor packaging support:core labels Jan 20, 2020
@gundalow gundalow removed the request for review from sivel Jan 20, 2020
@ansibot
Copy link
Contributor

ansibot commented Jan 20, 2020

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

test/sanity/ignore.txt:5009:1: A100: Ignoring 'doc-default-does-not-match-spec' on 'lib/ansible/modules/packaging/os/apt.py' is unnecessary

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jan 20, 2020
@ansibot
Copy link
Contributor

ansibot commented Jan 21, 2020

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

test/sanity/ignore.txt:5009:1: A100: Ignoring 'doc-default-does-not-match-spec' on 'lib/ansible/modules/packaging/os/apt.py' is unnecessary

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Jan 21, 2020

@ansibot ansibot added the test label Jan 21, 2020
'Update the code and API for this module. AnsibleModule will '
'refuse to accept such choices in the '
'future').format(','.join(map(str, BOOLEANS)))
self.deprecate(msg, version='2.12')
Copy link
Contributor

@tremble tremble Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty short deprecation cycle, I thought we were supposed to wait for 2.14 at this point

- If full, performs an aptitude full-upgrade.
- If dist, performs an apt-get dist-upgrade.
- 'Note: This does not upgrade a specific package, use state=latest for that.'
- 'Note: Since 2.4, apt-get is used as a fall-back if aptitude is not present.'
version_added: "1.1"
choices: [ dist, full, 'no', safe, 'yes' ]
default: 'no'
choices: [ '', safe, dist, full ]
Copy link
Contributor

@tremble tremble Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be an instant break for anyone using 'yes' or 'no' and should probably be deprecated at the same cadence.

lib/ansible/module_utils/basic.py Outdated Show resolved Hide resolved
@cristiklein cristiklein changed the title Discourage boolean values in string fields WIP: Discourage boolean values in string fields Jan 21, 2020
@ansibot ansibot added the WIP label Jan 21, 2020
@ansibot
Copy link
Contributor

ansibot commented Jan 21, 2020

@ansibot ansibot removed the module label Jan 21, 2020
@ansibot
Copy link
Contributor

ansibot commented Jan 21, 2020

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/module_utils/basic.py:1609:39: bad-whitespace: No space allowed after bracket                         if param[k] in [ 'False', 'True' ]:                                        ^
lib/ansible/module_utils/basic.py:1609:57: bad-whitespace: No space allowed before bracket                         if param[k] in [ 'False', 'True' ]:                                                          ^

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/module_utils/basic.py:1609:41: E201: whitespace after '['
lib/ansible/module_utils/basic.py:1609:57: E202: whitespace before ']'
lib/ansible/module_utils/basic.py:1613:33: E123: closing bracket does not match indentation of opening bracket's line

click here for bot help

@cristiklein
Copy link
Author

cristiklein commented Jan 21, 2020

@tremble I made the changes we discussed. Mind taking another look? (I have the feeling that the CI failure is NOT due to external reasons. Please comment if I'm wrong.)

@samdoran samdoran self-assigned this Jan 28, 2020
@samdoran samdoran removed the needs_triage label Jan 28, 2020
@ansibot ansibot added the stale_ci label Feb 5, 2020
@ansibot ansibot added the needs_rebase label Feb 13, 2020
@samdoran samdoran removed their assignment Jul 13, 2020
@ansibot ansibot added pre_azp and removed stale_ci labels Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 feature needs_rebase needs_revision new_contributor packaging pre_azp support:core test WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants