Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Allow value to be bool where 'yes'/'no' are in choices #2593

Merged
merged 4 commits into from
Aug 17, 2016

Conversation

tmshn
Copy link
Contributor

@tmshn tmshn commented Jul 20, 2016

NOTE This pull request work on same issue as #4220 on ansible-modules-core. Therefore, below comment is almost copy-and-paste.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAMES
  • ecs_service_facts
  • virt_net
  • virt_pool
  • portage
  • nmcli
  • debconf
ANSIBLE VERSION
ansible 2.1.0.0
  config file =
  configured module search path = Default w/o overrides
SUMMARY

Many arguments from many modules has limited choices including 'yes' and/or 'no' as a option. However, in some case, these are treated as string type; boolean values cause error.

This pull request fix this problem, in the following two way:

1. Change type to bool

If choices are only 'yes' and 'no', this argument can be easily modified to bool type. Even parameter is passed as string, it will be converted to boolean, so it is 100% backward compatible.

Modules:

  • ecs_service_facts
  • virt_net
  • virt_pool
  • nmcli

Example: ecs_service_facts module's case

---
# This still works,
- name: Basic listing example
  ecs_service_facts: details=true # snip...

---
# and this still works too,
- name: Basic listing example
  ecs_service_facts:
    details: "true"
    # snip...

---
# AND! this will work now.
- name: Basic listing example
  ecs_service_facts:
    details: true
    # snip...
2. Add options to catch boolean

UPDATED Thanks to ansible/ansible#16961, this case doesn't need to be considered any more.

If choices contains other values than 'yes' or 'no', this argument should be kept to be treated as string. In this case, if you pass boolean argument here, it will be converted to 'True' or 'False' respectively. Therefore, adding them to the choices will solve the problem.

Modules:

  • portage

Example: portage module's case

The choices of sync argument of portage modules contain "web", so it cannot be treated as boolean.

---
# This still works,
- name: sync repositories
  portage: sync=yes

---
# and this still works too,
- name: sync repositories
  portage:
    sync: "yes"

---
# AND! this will work now.
- name: sync repositories
  portage:
    sync: yes

---
# Of course, this
- name: sync repositories via web
  portage: sync=web

---
# or this still works.
- name: sync repositories via web
  portage:
    sync: web

Another example: debconf module's case
The value argument of debconf modules does not have any explicit choices in the modules source code, but actually its choices are limited when vtype is 'boolean'. So I added conversion process here.

@tmshn tmshn changed the title Allow yesno bool Allow value to be bool where 'yes'/'no' are in choices Jul 20, 2016
@gregdek
Copy link
Contributor

gregdek commented Jul 20, 2016

Thanks @tmshn. To the current maintainers, @sayap, @Java1Guy, @alcamie101, @drybjed, @bcoca please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit', 'needs_revision' or 'close_me' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

abadger added a commit to abadger/ansible that referenced this pull request Aug 4, 2016
  uri:
    follow_redirects: no

Will lead yaml to set follow_redirects=False.  This is problematic when
the module parameter is not a boolean value but a string.  For instance:

  follow_redirects = dict(required=False, default='safe', choices=['all', 'safe', 'none', 'yes', 'no']),

Our parameter validation code ends up getting follow_redirects="False"
instead of "no".  The 100% fix is for the user to quote their strings in
playbooks like:
  uri:
    follow_redirects: "no"

But we can fix quite a few common cases by trying to switch "False" back
into the string that it was specified as.  We only do this if there is
only one correct choices value that could have been specified.  In the
follow_redirects example, a value of "True" only maps back to "yes" and
a value of "False" only maps back to "no" so we can do this.  If choices
also contained "on" and "off" then we couldn't map back safely and would
need to force the module author to change the module to handle this
case.

Fixes parts of the following PRs:

* ansible/ansible-modules-core#4220
* ansible/ansible-modules-extras#2593
@tmshn
Copy link
Contributor Author

tmshn commented Aug 5, 2016

Same as ansible/ansible-modules-core#4220, this PR is pended until ansible/ansible#16961 will be merged. needs_revision

abadger added a commit to ansible/ansible that referenced this pull request Aug 5, 2016
uri:
    follow_redirects: no

Will lead yaml to set follow_redirects=False.  This is problematic when
the module parameter is not a boolean value but a string.  For instance:

  follow_redirects = dict(required=False, default='safe', choices=['all', 'safe', 'none', 'yes', 'no']),

Our parameter validation code ends up getting follow_redirects="False"
instead of "no".  The 100% fix is for the user to quote their strings in
playbooks like:
  uri:
    follow_redirects: "no"

But we can fix quite a few common cases by trying to switch "False" back
into the string that it was specified as.  We only do this if there is
only one correct choices value that could have been specified.  In the
follow_redirects example, a value of "True" only maps back to "yes" and
a value of "False" only maps back to "no" so we can do this.  If choices
also contained "on" and "off" then we couldn't map back safely and would
need to force the module author to change the module to handle this
case.

Fixes parts of the following PRs:

* ansible/ansible-modules-core#4220
* ansible/ansible-modules-extras#2593
abadger added a commit to ansible/ansible that referenced this pull request Aug 5, 2016
uri:
    follow_redirects: no

Will lead yaml to set follow_redirects=False.  This is problematic when
the module parameter is not a boolean value but a string.  For instance:

  follow_redirects = dict(required=False, default='safe', choices=['all', 'safe', 'none', 'yes', 'no']),

Our parameter validation code ends up getting follow_redirects="False"
instead of "no".  The 100% fix is for the user to quote their strings in
playbooks like:
  uri:
    follow_redirects: "no"

But we can fix quite a few common cases by trying to switch "False" back
into the string that it was specified as.  We only do this if there is
only one correct choices value that could have been specified.  In the
follow_redirects example, a value of "True" only maps back to "yes" and
a value of "False" only maps back to "no" so we can do this.  If choices
also contained "on" and "off" then we couldn't map back safely and would
need to force the module author to change the module to handle this
case.

Fixes parts of the following PRs:

* ansible/ansible-modules-core#4220
* ansible/ansible-modules-extras#2593
(cherry picked from commit 6db6edf)
@tmshn
Copy link
Contributor Author

tmshn commented Aug 14, 2016

Thanks to @abadger (ansible/ansible#16961), modification for case 2 is meaningless any more. Thus, I dropped commit on packaging/os/portage.py.

Other than that, below modules are welcome for reviews.

Case 1

Case 3

  • debconf (1ee56fb)
    • In debconf's case, the choices are implicit (invisible from ansible module), so still needs conversion.

@abadger abadger merged commit 5fbb0de into ansible:devel Aug 17, 2016
abadger pushed a commit that referenced this pull request Aug 17, 2016
* Changed type of 'details' argument to bool on ecs_service_facts module.

* Changed type of 'autostart' argument to bool on virt_* modules.

* Changed types of 'autoconnect' and 'stp' argument to bool on nmcli module.
('create_connection_bridge(self)' and 'modify_connection_bridge(self)' are not implemented yet?)

* Added conversion of 'value' argument when 'vtype' is boolean on debconf module.
@abadger
Copy link
Contributor

abadger commented Aug 17, 2016

Thanks @tmshn ! Merged to devel and cherry-picked to stable-2.1.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants