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.

Sign up for GitHub

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

Jump to bottom

Be strict about what is a boolean #67625

Merged
merged 5 commits into from Apr 28, 2020
Merged

Be strict about what is a boolean #67625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c35295
be strict about what is a boolean
bcoca Feb 20, 2020
2515ec8
added clog
bcoca Feb 21, 2020
1b6fd93
fix typo
bcoca Feb 25, 2020
56bd130
added porting note
bcoca Feb 25, 2020
445be8e
added integration tests
bcoca Apr 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/play_bools_strict.yml
View file
Open in desktop
@@ -0,0 +1,2 @@
bugfixes:
- Ensure that keywords defined as booleans are correctly interpreting their input, before patch any random string would be interpreted as False
samccann marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion docs/docsite/rst/porting_guides/porting_guide_2.10.rst
View file
Open in desktop
Expand Up @@ -23,7 +23,8 @@ This document is part of a collection on porting. The complete list of porting g
Playbook
========

No notable changes
* Fixed a bug on boolean keywords that made random strings return 'False', now they should return an error if they are not a proper boolean
samccann marked this conversation as resolved.
Show resolved Hide resolved
Example: `diff: yes-` was returning `False`.


Command Line
Expand Down
2 changes: 1 addition & 1 deletion lib/ansible/playbook/base.py
View file
Open in desktop
Expand Up @@ -343,7 +343,7 @@ def get_validated_value(self, name, attribute, value, templar):
elif attribute.isa == 'float':
value = float(value)
elif attribute.isa == 'bool':
value = boolean(value, strict=False)
value = boolean(value, strict=True)
bcoca marked this conversation as resolved.
Show resolved Hide resolved
elif attribute.isa == 'percent':
# special value, which may be an integer or float
# with an optional '%' at the end
Expand Down
2 changes: 1 addition & 1 deletion test/integration/targets/incidental_xml/tasks/test-children-elements-xml.yml
View file
Open in desktop
Expand Up @@ -19,7 +19,7 @@
src: results/test-add-children-elements.xml
dest: /tmp/ansible-xml-beers.xml
check_mode: yes
diff: yes·
diff: yes
register: comparison

- name: Test expected result
Expand Down
1 change: 1 addition & 0 deletions test/integration/targets/playbook/aliases
View file
Open in desktop
@@ -0,0 +1 @@
shippable/posix/group1
6 changes: 6 additions & 0 deletions test/integration/targets/playbook/runme.sh
View file
Open in desktop
@@ -0,0 +1,6 @@
#!/usr/bin/env bash

set -eux

# run type tests
ansible-playbook -i ../../inventory types.yml -v "$@"
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a role instead of a script based test?

Copy link
Member Author

Choose a reason for hiding this comment

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

mostly because i thought that was the 'new norm', but also because these are easier to expand and less subject to other vars/effects created in same invocation.

Copy link
Member

Choose a reason for hiding this comment

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

We're only using them as needed. There's nothing to be "separate" from. Each role is executed in its own playbook, so using a script doesn't provide any more isolation than a role.

Copy link
Member Author

@bcoca bcoca Apr 22, 2020 •
edited

Choose a reason for hiding this comment

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

but all tests for playbook would have to be in same role, that is what i meant. But if this posses a problem i'm fine with changing it, just confused since lately you had advised me to use runme.sh type tests.

Copy link
Member

Choose a reason for hiding this comment

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

If you're planning on adding additional tests in separate playbooks for this target, then using a script makes sense. I was mainly asking because right now there's just the one ansible-playbook invocation, so no benefit to using a script.

Copy link
Member Author

Choose a reason for hiding this comment

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

right now this just checks booleans (and a specific field at that), but my thought was that we should test all the rest also.

21 changes: 21 additions & 0 deletions test/integration/targets/playbook/types.yml
View file
Open in desktop
@@ -0,0 +1,21 @@
- hosts: testhost
gather_facts: false
tasks:
- name: try to set 'diff' a boolean
debug: msg="not important"
diff: yes
ignore_errors: True
register: good_diff

- name: try to set 'diff' a boolean to a string (. would make it non boolean)
debug: msg="not important"
diff: yes.
ignore_errors: True
register: bad_diff

- name: Check we did error out
assert:
that:
- good_diff is success
- bad_diff is failed
- "'is not a valid boolean' in bad_diff['msg']"