improve conditional warnings #57190
improve conditional warnings #57190
Conversation
Fix bare variable warnings when the variable is a boolean
lib/ansible/playbook/conditional.py
Outdated
| # make sure the templar is using the variables specified with this method | ||
| templar.available_variables = all_vars | ||
|
|
||
| try: | ||
| # if the conditional is "unsafe", disable lookups | ||
| disable_lookups = hasattr(conditional, '__UNSAFE__') | ||
| conditional = templar.template(conditional, disable_lookups=disable_lookups) | ||
| if not isinstance(conditional, bool) and bare_vars_warning: |
There was a problem hiding this comment.
I debate whether issuing the warning here is catching some of the edge cases.
Take the following example:
- debug:
when: thing
vars:
thing: other
other: false
This is a little contrived for brevity, but is the users intent to evaluate the string "other" or have it double interpolated to the value of False?
There was a problem hiding this comment.
@sivel Good food for thought. I haven't altered what happens for the example task above on this branch versus devel (the var is double interpolated to the value of False and a warning is given for the bare var). But if brackets are used instead the bare variable warning doesn't occur.
- debug:
when: thing
vars:
thing: "{{ other }}"
other: false
In your example without brackets I'm unsure if the non-bracketed edge case has the wrong behavior. Since there is a bare var in the mix getting a warning for it seems like it could be appropriate. Thoughts?
There was a problem hiding this comment.
I feel pretty happy with this as it is. My reasoning is that if the behavior is going to stay for the user when CONDITIONAL_BARE_VARS changes then we don't need to warn them about anything.
|
I left a question on the commit. But looks like I did so in between commits, so it's marked as "outdated" and not shown on the "Files Changed" tab. |
|
@sivel thanks, found it :-) |
|
can you add a couple of test cases? one that triggered warning before and does not anymore and one that always triggers warning? |
ansibot
added
affects_2.9
s-hertel
removed
the
needs_triage
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.
s-hertel
force-pushed
the
fix_boolean_conditional
branch
from
6cab2c9
to
a548043
Compare
|
can we backport this one? |
* 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 (cherry picked from commit 21cd24a)
|
Backport: #57329 |
* 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 (cherry picked from commit 21cd24a)
SUMMARY
Fixes #53428 - Now only non-boolean when statements will trigger this warning
Fixes #56830 - Fix the order for warning on templating delimiters
ISSUE TYPE
COMPONENT NAME
lib/ansible/playbook/conditional.py