From 19aeb4706d7f7a984daf2a70f16d69a7dccde484 Mon Sep 17 00:00:00 2001 From: Rick Elrod Date: Tue, 13 Apr 2021 10:12:10 -0500 Subject: [PATCH] [conditional] Remove support for bare variables (#74208) Change: - Variables used in ``when`` conditionals are no longer parsed and attempted to be converted to booleans. All non-empty strings are considered true (empty strings, false). Test Plan: - Updated existing tests - Added a bunch of new tests with various kinds of truthy/falsy values. Tickets: - Fixes #74134 Signed-off-by: Rick Elrod --- .../fragments/conditional-bare-vars.yml | 2 + .../porting_guide_core_2.12.rst | 14 +- lib/ansible/config/base.yml | 14 -- lib/ansible/playbook/conditional.py | 13 +- .../integration/targets/conditionals/play.yml | 150 ++++++++++++++++-- .../integration/targets/conditionals/runme.sh | 12 +- .../targets/conditionals/test_no_warnings.yml | 18 --- .../targets/conditionals/test_warnings.yml | 14 -- .../targets/conditionals/vars/main.yml | 7 + test/sanity/ignore.txt | 1 - 10 files changed, 157 insertions(+), 88 deletions(-) create mode 100644 changelogs/fragments/conditional-bare-vars.yml delete mode 100644 test/integration/targets/conditionals/test_no_warnings.yml delete mode 100644 test/integration/targets/conditionals/test_warnings.yml diff --git a/changelogs/fragments/conditional-bare-vars.yml b/changelogs/fragments/conditional-bare-vars.yml new file mode 100644 index 00000000000..4d246c83d29 --- /dev/null +++ b/changelogs/fragments/conditional-bare-vars.yml @@ -0,0 +1,2 @@ +breaking_changes: + - conditionals - ``when`` conditionals no longer automatically parse string booleans such as ``"true"`` and ``"false"`` into actual booleans. Any non-empty string is now considered true. The ``CONDITIONAL_BARE_VARS`` configuration variable no longer has any effect. diff --git a/docs/docsite/rst/porting_guides/porting_guide_core_2.12.rst b/docs/docsite/rst/porting_guides/porting_guide_core_2.12.rst index 868597ab7a5..c3da04fbe2a 100644 --- a/docs/docsite/rst/porting_guides/porting_guide_core_2.12.rst +++ b/docs/docsite/rst/porting_guides/porting_guide_core_2.12.rst @@ -31,7 +31,19 @@ No notable changes Deprecated ========== -No notable changes +* Bare variables in conditionals: ``when`` conditionals no longer automatically parse string booleans such as ``"true"`` and ``"false"`` into actual booleans. Any variable containing a non-empty string is considered true. This was previously configurable with the ``CONDITIONAL_BARE_VARS`` configuration option (and the ``ANSIBLE_CONDITIONAL_BARE_VARS`` environment variable). This setting no longer has any effect. Users can work around the issue by using the ``|bool`` filter: + +.. code-block:: yaml + + vars: + teardown: 'false' + + tasks: + - include_tasks: teardown.yml + when: teardown | bool + + - include_tasks: provision.yml + when: not teardown | bool Modules diff --git a/lib/ansible/config/base.yml b/lib/ansible/config/base.yml index 2a0019d7ed7..1b088a1cd14 100644 --- a/lib/ansible/config/base.yml +++ b/lib/ansible/config/base.yml @@ -330,20 +330,6 @@ COLOR_WARN: env: [{name: ANSIBLE_COLOR_WARN}] ini: - {key: warn, section: colors} -CONDITIONAL_BARE_VARS: - name: Allow bare variable evaluation in conditionals - default: False - type: boolean - description: - - With this setting on (True), running conditional evaluation 'var' is treated differently than 'var.subkey' as the first is evaluated - directly while the second goes through the Jinja2 parser. But 'false' strings in 'var' get evaluated as booleans. - - With this setting off they both evaluate the same but in cases in which 'var' was 'false' (a string) it won't get evaluated as a boolean anymore. - - Currently this setting defaults to 'True' but will soon change to 'False' and the setting itself will be removed in the future. - - Expect that this setting eventually will be deprecated after 2.12 - env: [{name: ANSIBLE_CONDITIONAL_BARE_VARS}] - ini: - - {key: conditional_bare_variables, section: defaults} - version_added: "2.8" COVERAGE_REMOTE_OUTPUT: name: Sets the output directory and filename prefix to generate coverage run info. description: diff --git a/lib/ansible/playbook/conditional.py b/lib/ansible/playbook/conditional.py index 5653437941c..293013a2816 100644 --- a/lib/ansible/playbook/conditional.py +++ b/lib/ansible/playbook/conditional.py @@ -127,12 +127,6 @@ class Conditional: 'templating delimiters such as {{ }} or {%% %%}. ' 'Found: %s' % conditional) - bare_vars_warning = False - if C.CONDITIONAL_BARE_VARS: - if conditional in all_vars and VALID_VAR_REGEX.match(conditional): - conditional = all_vars[conditional] - bare_vars_warning = True - # make sure the templar is using the variables specified with this method templar.available_variables = all_vars @@ -140,12 +134,7 @@ class Conditional: # if the conditional is "unsafe", disable lookups disable_lookups = hasattr(conditional, '__UNSAFE__') conditional = templar.template(conditional, disable_lookups=disable_lookups) - if bare_vars_warning and not isinstance(conditional, bool): - display.deprecated('evaluating %r as a bare variable, this behaviour will go away and you might need to add " | bool"' - ' (if you would like to evaluate input string from prompt) or " is truthy"' - ' (if you would like to apply Python\'s evaluation method) to the expression in the future. ' - 'Also see CONDITIONAL_BARE_VARS configuration toggle' % original, - version="2.12", collection_name='ansible.builtin') + if not isinstance(conditional, text_type) or conditional == "": return conditional diff --git a/test/integration/targets/conditionals/play.yml b/test/integration/targets/conditionals/play.yml index c6bb381527b..455818c97c0 100644 --- a/test/integration/targets/conditionals/play.yml +++ b/test/integration/targets/conditionals/play.yml @@ -6,10 +6,6 @@ vars_files: - vars/main.yml tasks: - - name: set conditional bare vars status - set_fact: - bare: "{{lookup('config', 'CONDITIONAL_BARE_VARS')|bool}}" - - name: test conditional '==' shell: echo 'testing' when: 1 == 1 @@ -164,6 +160,136 @@ - "result.stdout == 'testing'" - "result.rc == 0" + - name: not test bare conditional + shell: echo 'testing' + when: not test_bare + register: result + + - name: assert did not run + assert: + that: + - result is skipped + + - name: empty string is false + shell: echo 'testing' + when: string_lit_empty + register: result + + - name: assert did not run + assert: + that: + - result is skipped + + - name: not empty string is true + shell: echo 'testing' + when: not string_lit_empty + register: result + + - name: assert ran + assert: + that: + - result is not skipped + + - name: literal 0 is false + shell: echo 'testing' + when: int_lit_0 + register: result + + - name: assert did not run + assert: + that: + - result is skipped + + - name: not literal 0 is true + shell: echo 'testing' + when: not int_lit_0 + register: result + + - name: assert ran + assert: + that: + - result is not skipped + + - name: literal 1 is true + shell: echo 'testing' + when: int_lit_1 + register: result + + - name: assert ran + assert: + that: + - result is not skipped + + - name: not literal 1 is false + shell: echo 'testing' + when: not int_lit_1 + register: result + + - name: assert did not run + assert: + that: + - result is skipped + + - name: null is false + shell: echo 'testing' + when: lit_null + register: result + + - name: assert did not run + assert: + that: + - result is skipped + + - name: literal string "true" is true + shell: echo 'testing' + when: string_lit_true + register: result + + - name: assert ran + assert: + that: + - result is not skipped + + - name: not literal string "true" is false + shell: echo 'testing' + when: not string_lit_true + register: result + + - name: assert did not run + assert: + that: + - result is skipped + + - name: literal string "false" is true (nonempty string) + shell: echo 'testing' + when: string_lit_false + register: result + + - name: assert ran + assert: + that: + - result is not skipped + + - name: not literal string "false" is false + shell: echo 'testing' + when: not string_lit_false + register: result + + - name: assert did not run + assert: + that: + - result is skipped + + - name: not literal string "true" is false + shell: echo 'testing' + when: not string_lit_true + register: result + + - name: assert did not run + assert: + that: + - result is skipped + - name: test conditional using a variable shell: echo 'testing' when: test_bare_var == 123 @@ -195,23 +321,13 @@ - debug: var={{item}} loop: - - bare - result - test_bare_nested_bad - - name: assert that the bad nested conditional is skipped since 'bare' since 'string' template is resolved to 'false' + - name: assert that the bad nested conditional ran (it is a non-empty string, so truthy) assert: that: - - result is skipped - - when: bare|bool - - - name: assert that the bad nested conditional did run since non bare 'string' is untemplated but 'trueish' - assert: - that: - - result is skipped - when: not bare|bool - - result is changed + - result is not skipped - name: test bad conditional based on nested variables with bool filter shell: echo 'testing' @@ -223,6 +339,7 @@ that: - result is skipped + #----------------------------------------------------------------------- # proper booleanification tests (issue #8629) @@ -382,7 +499,6 @@ - name: Deal with multivar equality tags: ['leveldiff'] - when: not bare|bool vars: toplevel_hash: hash_var_one: justastring diff --git a/test/integration/targets/conditionals/runme.sh b/test/integration/targets/conditionals/runme.sh index 934443a5238..4858fbf2a6d 100755 --- a/test/integration/targets/conditionals/runme.sh +++ b/test/integration/targets/conditionals/runme.sh @@ -2,14 +2,4 @@ set -eux -ANSIBLE_CONDITIONAL_BARE_VARS=1 ansible-playbook -i ../../inventory play.yml "$@" -ANSIBLE_CONDITIONAL_BARE_VARS=0 ansible-playbook -i ../../inventory play.yml "$@" - -export ANSIBLE_CONDITIONAL_BARE_VARS=1 -export ANSIBLE_DEPRECATION_WARNINGS=True - -# No warnings for conditionals that are already type bool -test "$(ansible-playbook -i ../../inventory test_no_warnings.yml "$@" 2>&1 | grep -c '\[DEPRECATION WARNING\]')" = 0 - -# Warn for bare vars of other types since they may be interpreted differently when CONDITIONAL_BARE_VARS defaults to False -test "$(ansible-playbook -i ../../inventory test_warnings.yml "$@" 2>&1 | grep -c '\[DEPRECATION WARNING\]')" = 2 +ansible-playbook -i ../../inventory play.yml "$@" diff --git a/test/integration/targets/conditionals/test_no_warnings.yml b/test/integration/targets/conditionals/test_no_warnings.yml deleted file mode 100644 index 932804476be..00000000000 --- a/test/integration/targets/conditionals/test_no_warnings.yml +++ /dev/null @@ -1,18 +0,0 @@ -- hosts: testhost - gather_facts: false - vars: - boolean_var: false - nested: - bool_var: false - tasks: - - name: Run tasks with previous warnings requesting the bool filter on type boolean vars - block: - - debug: - when: boolean_var - - debug: - when: nested.bool_var - - debug: - when: double_interpolated - vars: - double_interpolated: "{{ other }}" - other: false diff --git a/test/integration/targets/conditionals/test_warnings.yml b/test/integration/targets/conditionals/test_warnings.yml deleted file mode 100644 index 4186cd01127..00000000000 --- a/test/integration/targets/conditionals/test_warnings.yml +++ /dev/null @@ -1,14 +0,0 @@ -- hosts: testhost - gather_facts: false - vars: - str_boolean_var: 'false' - tasks: - - name: Run tasks with warnings for conditionals that will change in behavior depending on CONDITIONAL_BARE_VARS - block: - - debug: - when: str_boolean_var - - debug: - when: double_interpolated - vars: - double_interpolated: other - other: false diff --git a/test/integration/targets/conditionals/vars/main.yml b/test/integration/targets/conditionals/vars/main.yml index d62214781e7..2af6cee2db7 100644 --- a/test/integration/targets/conditionals/vars/main.yml +++ b/test/integration/targets/conditionals/vars/main.yml @@ -20,3 +20,10 @@ test_bare: true test_bare_var: 123 test_bare_nested_good: "test_bare_var == 123" test_bare_nested_bad: "{{test_bare_var}} == 321" + +string_lit_true: "true" +string_lit_false: "false" +string_lit_empty: "" +lit_null: null +int_lit_0: 0 +int_lit_1: 1 diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt index e132b5db6f3..3f25b212852 100644 --- a/test/sanity/ignore.txt +++ b/test/sanity/ignore.txt @@ -144,7 +144,6 @@ lib/ansible/modules/yum_repository.py validate-modules:undocumented-parameter lib/ansible/parsing/vault/__init__.py pylint:blacklisted-name lib/ansible/playbook/base.py pylint:blacklisted-name lib/ansible/playbook/collectionsearch.py required-and-default-attributes # https://github.com/ansible/ansible/issues/61460 -lib/ansible/playbook/conditional.py pylint:ansible-deprecated-version lib/ansible/playbook/helpers.py pylint:ansible-deprecated-version lib/ansible/playbook/helpers.py pylint:blacklisted-name lib/ansible/playbook/play_context.py pylint:ansible-deprecated-version