improve conditional warnings (#57190)

* Fix order for warning on templated conditionals

Fix bare variable warnings when the variable is a boolean

* changelog

* Add tests for cases that should and should not give warnings

If the behavior may change when the default behavior for CONDITIONAL_BARE_VARS becomes False there should be a warning. Boolean type conditionals will not change in behavior so don't warn.

* oops, forgot to add files

* typo

changelogs/fragments/57190-improve-warnings-for-when.yaml

@@ -0,0 +1,3 @@
+bugfixes:
+  - Only warn for bare variables if they are not type boolean (https://github.com/ansible/ansible/issues/53428)
+  - Fix regression warning on jinja2 delimiters in when statements (https://github.com/ansible/ansible/issues/56830)

lib/ansible/playbook/conditional.py

@@ -114,16 +114,17 @@ class Conditional:
         if isinstance(conditional, bool):
             return conditional
 
-        if C.CONDITIONAL_BARE_VARS:
-            if conditional in all_vars and VALID_VAR_REGEX.match(conditional):
-                display.deprecated('evaluating %s as a bare variable, this behaviour will go away and you might need to add |bool'
-                                   ' to the expression in the future. Also see CONDITIONAL_BARE_VARS configuration toggle.' % conditional, "2.12")
-                conditional = all_vars[conditional]
-
         if templar.is_template(conditional):
             display.warning('conditional statements should not include jinja2 '
                             '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
 
@@ -131,6 +132,9 @@ 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 %s as a bare variable, this behaviour will go away and you might need to add |bool'
+                                   ' to the expression in the future. Also see CONDITIONAL_BARE_VARS configuration toggle.' % conditional, "2.12")
             if not isinstance(conditional, text_type) or conditional == "":
                 return conditional
 

test/integration/targets/conditionals/runme.sh

@@ -4,3 +4,12 @@ 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

test/integration/targets/conditionals/test_no_warnings.yml

@@ -0,0 +1,18 @@
+- 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

test/integration/targets/conditionals/test_warnings.yml

@@ -0,0 +1,14 @@
+- 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