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

[tmshn]

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

ISSUE TYPE

• Feature Pull Request

COMPONENT NAMES

• _docker
• docker_container
• docker_image
• _nova_compute
• cl_img_install
• apt

I did not modify uri module, since 'yes'/'no' choices of follow_redirects argument is deprecated.

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:

• _nova_compute

Example: _nova_compute module's case

---
# This still works,
- name: launch an instance
  nova_compute: wait=yes # snip...

---
# and this still works too,
- name: launch an instance
  nova_compute:
    wait: "yes"
    # snip...

---
# AND! this will work now.
- name: launch an instance
  nova_compute:
    wait: yes
    # snip...

2. Add options to catch boolean

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:

• _docker
• docker_container
• docker_image
• apt

Example: apt module's case

The choices of upgrade argument of apt modules contain "safe", "full" and "dist", so it cannot be treated as boolean.

---
# This still works,
- name: upgrade packages
  apt: upgrade=yes update_cache=yes force=yes

---
# and this still works too,
- name: upgrade packages
  apt:
    upgrade: "yes"
    update_cache: yes
    force: yes

---
# AND! this will work now.
- name: upgrade packages
  apt:
    upgrade: yes
    update_cache: yes
    force: yes

---
# Of course, this
- name: upgrade packages
  apt: upgrade=dist

---
# or this still works.
- name: upgrade packages
  apt:
    upgrade: dist

[gregdek]

Thanks @tmshn for this PR. This module is maintained by the Ansible core team, so it can take a while for patches to be reviewed. Thanks for your patience.

Core team: please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with 'needs_revision' or merge as appropriate.

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

[gundalow]

Hi,

Some initial thoughts:

1. _docker.py and _nova_compute are deprecated modules (they start with _)
2. Using type='bool'is generally the right option
3. Have a look at mk_boolean in `basic.py

Having an option that accepts bool(ish) and values just seems confusing

[abadger]

@tmshn The type='bool' is definitely acceptable. I'll be proposing a patch to ansible/ansible that addresses some cases where type='bool' is the wrong answer in a moment. If that works for the majority of the cases we can look at any remaining cases to see what the least kludgey way to solve them is.

For others reading this, the problem is that if you write your playbook and specify:

tasks:
  - docker:
      use_tls: no

Then the yaml parser converts that no into a boolean. Then the module gets {use_tls: False}. The argument handling first transforms that boolean False into the string 'False' and then tries to find 'False' in the choices which is currently failing.

Arguably, the right way to fix this is to be explicit in your playbook that you're handling a string here:

tasks:
  - docker:
      use_tls: "no"

key=value should also work although we don't usually like promoting its use:

tasks:
  - docker: use_tls=no

[abadger]

@tmshn See if ansible/ansible#16961 fixes most of the modules in case (2) for you and doesn't cause further problems. If it looks good we can merge that and you can cut this PR down to the modules that still have issues. If they're all in case (1) I think it will then be good to merge. If there's still things in case(2) we'll have to evaluate each of them to see what the best strategy is. (Same thing for ansible/ansible-modules-extras#2593 )

[tmshn]

@abadger Your strategy in ansible/ansible#16191 looks great! I'll wait it to be merged, and then revise this PR. Thus, this pended PR needs_revision now.

[tmshn]

@gundalow

1. _docker.py and _nova_compute are deprecated modules (they start with _)

You mean I don't need to (or should not) update these modules?

[gundalow]

@tmshn Normally it doesn't make sense to update depreciated modules unless there is a major bug/security concern. People should be moving over to the newer modules.

[gregdek]

Thanks @tmshn for this PR. Unfortunately, it is not mergeable in its current state due to merge conflicts. Please rebase your PR. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

For help on how to do this cleanly please see http://docs.ansible.com/ansible/community.html#contributing-code-features-or-bugfixes

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

[abadger]

ansible/ansible#16191 has been merged. Good to continue on this one.

[tmshn]

Thanks to @abadger's commit (and the advice from @gundalow that _nova_compute module is deprecated), this PR no longer has meaning. Closing.