[E602] Don't compare to empty string - what's a better way?

[geerlingguy]

Issue Type

• Bug report

Ansible and Ansible Lint details

$ ansible --version
ansible 2.7.5
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/Users/jgeerling/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python2.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.15 (default, Jul 17 2018, 19:41:20) [GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)]

$ ansible-lint --version
ansible-lint 4.0.0

• ansible installation method: pip
• ansible-lint installation method: pip

Desired Behaviour

Using a string comparison to check for a string being empty should be A-OK:

- name: Do something with my_var.
  command: "do_something_with {{ my_var }}"
  when: my_var != ''

ansible-lint should throw no errors for this code.

Actual Behaviour (Bug report only)

ansible-lint throws the following:

[602] Don't compare to empty string

The problem is, if I set it to:

- name: Do something with my_var.
  command: "do_something_with {{ my_var }}"
  when: my_var

Then I get the error:

fatal: [localhost]: FAILED! =>
The conditional check 'my_var' failed. The error was: error while evaluating conditional (my_var): 'testing' is undefined

So I'm wondering—if we're not supposed to compare to empty strings in a duck-typed language like Python, what are we supposed to do? There are a ton of use cases for comparison to an empty string.

Workarounds

Currently there are two ways to get past this:

1. Head-in-the-sand:

   1. Add .ansible-lint file in your role/playbook.

   2. Add the following to skip the rule entirely:

      skip_list:
        - '602'
      

2. Bypass the rule's regex:

   when: my_var | length > 0
   

Number 1 is just ignoring the issue. Number 2 is more obtuse and makes for more unreadable code (at least IMO)... but is that preferred if we truly need to check if a string has a value?

[geerlingguy]

For reference, original discussion was from: #450

[geerlingguy]

So another workaround could be to use the bool filter (see https://docs.ansible.com/ansible/latest/user_guide/playbooks_filters.html):

when: my_var | bool

Is that the preferred way rather than a string comparison? I still don't think it's important / best-practice-y enough to force one way over the other, but that's a better compromise than the length filter at least—it makes more semantic sense.

[geerlingguy]

Hmm... but with the bool filter, it looks like there be dragons: ansible/ansible#25893

[Stolb27]

3. use not flat variable name in when expression, like vars.my_var (magic var vars)

[drybjed]

How about providing a "default wrapper" for the variable?

when: my_var|d()

That should behave as expected during Ansible execution and not trigger the E602 rule.

[sseide]

I for myself do not understand the rationale behind this rule...

Using the same example as Jeff Geerling above with my_var

1. my_var contains a string - following this linter rule Ansible playbook breaks and does not work at all.
2. my_var contains a boolean or number - why on earth should someone compare that again an empty string?
3. my_var contains a string and i have to use some of the - @geerlingguy and others, please do not take any offense - use some of the ugly work around above is neither elegant, comprehensible nor easy maintainable.

IMHO this rule should just be dropped. Or at least not flagged as error importing roles into Ansible Galaxy.

And finally - up to now no member of the Ansible team has come up with a explanation for this rule and the things it tries to solve...

[willthames]

Using when: my_var seems reasonable to me, and it's not clear why ansible then tries to evaluate the contents of my_var - seems like some double evaluation is going on.

Having said that, using when: my_var hasn't worked since 2.1 and I don't have an older version to test with, so I'd be tempted to drop this rule, it certainly doesn't work as I originally envisaged it (my guess is that it might have worked in 1.9, presumably I had a working alternative at some point)

This also illustrates that some rules are missing tests!

[tgadiev]

How about making when: my_var conditional working in Ansible for string variables? It would be good alternative to empty string comparison.

[sseide]

When Ansible would be changed to work that way (when: my_var), i would be happy to follow this rule...
IMHO not working with strings right now is the only big reason against this one...

[tgadiev]

Looks like it works now but only for hash variables. Using simple variables in conditionals causes trouble when: my_var, but hash variables in conditionals works as expected: when: my_hash.my_var. See
ansible/ansible#39414 for related details.

So, the good workaround for now is avoid using simple variables in string conditionals like when: my_var. If possible they should be changed to hashed ones. Several related variables can be grouped in one hash for example.

[geerlingguy]

@tgadiev - That workaround doesn't seem very helpful for this use case though; I often try to split my variables into simpler individual variables so if I want to override one in a playbook I don't have to end up overriding an entire hash/object with 3, 5, 10, or dozens of variables.

[tgadiev]

@geerlingguy Are you talking about overriding single key-value item of an entire hash? It's quite simple using combine filter. You don't have to override an entire hash/object to change single parameter.

[geerlingguy]

In the generalized use case of targeting an Ansible beginner, I do not like requiring them to use filters to change a simple string variable. And even in the use case of someone who knows how to merge and adjust hashes, it adds more complexity for no good reason (in the case of this lint rule). Why require me to compare a variable of my_hash.my_string_variable when my_string_variable has worked fine since before Ansible 1.0 and requires 8 fewer characters anywhere it's used?

(Don't want to get this issue off-track—but basically that is a valid workaround, but my strong opinion is that unless there's a demonstrably simpler way of doing a boolean when check on a string variable, then E602 seems very nitpicky and a narrow/not general lint rule.)

[tgadiev]

@geerlingguy There's no simpler way of doing such comparison to check for empty variable's value than when: my_var. I strongly agree that such kind of conditional check should work just like it works in Python. And from code quality point of view this approach should be used instead of compare to empty string or literal True/False, yes/no etc. That's the reason of rules E601 and E602 to be applied by ansible-lint.

Let's see if it works in practice. We have several variable types in Ansible:

• Array/List
• Hash/Dictionary
• Boolean
• Numeric (integer and float)
• String

With first four ones you can use simple comparison when: my_var without any issues. It works as expected. Empty lists and dictionaries treated like False, non-empty like True. Numeric: zero is False, non-zero is True. For boolean it's trivial.

With string variable there can be 4 possible ways of comparison:

1. when: not my_var, when my_var is empty string
2. when: not my_var, when my_var is non-empty string
3. when: my_var, when my_var is empty string
4. when: my_var, when my_var is non-empty string

First three cases work as expected. Empty string value treated as False, non-empty as True. The only issue is the last case when comparison to non-empty string causes exception of undefined variable. Yes, it's a bug and there are issues raised in Ansible for that. See ansible/ansible#39414 for example.

There are workarounds which can be used in case of checking non-empty string condition:

1. when: my_var | length
2. Using hash instead of simple variable
3. when: not (not my_var) - most funny :)

Do you think that it would be good approach to drop the rule just because of one use-case scenario which is not working as expected because of known bug while several workarounds are available at the moment?

[geerlingguy]

That particular bug report's been open for almost a year, and the bug has existed for at least a couple years, so yes, I believe this rule can and should be dropped unless that bug is actually fixed.

On a separate level (but again, unrelated to the discussion here), I don't particularly like duck typing, so explicitly comparing to a string is something I often do in any non-strict language (like PHP as well), and I'm not sure why there's a rule saying that is a bad thing...

[ViachaslauKabak]

@geerlingguy , even in PHP there is a method to check if string is empty
https://www.rosettacode.org/wiki/Empty_string#PHP
let's use the better code style)

[tgadiev]

@geerlingguy,
Ansible came from Python world. It's supposed to follow Zen of Python principles and related codestyle conventions. As for conditionals, Python code guides recommend to use direct truth value testing. See these recommendations for example. Short quote:

For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

Yes: if not seq:
     if seq:

No:  if len(seq):
     if not len(seq):

And below:

Don't compare boolean values to True or False using ==.

Yes:   if greeting:
No:    if greeting == True:
Worse: if greeting is True:

I guess Ansible code should also follow these recommendations. If you don't like it and prefer other program language code style - it's up to you. Ansible is quite flexible and allows various code styles. If you prefer php-like conditionals for string variables and you're happy with that approach - it's ok, your code will work. But why do you need ansible-lint for that? You can drop any rule you don't like on your own level just disabling it in config. Why do you ask to drop it on general level?

[sseide]

@tgadiev Thanks for your explanations of python code style...

Just to summaries this:

• There exists an long standing bug in Ansible Toplevel and Sublevel Variables Treated Differently by Conditionals ansible#39414 nobody seems to care about.
• This bug prevents us following this linter rule, which, as you said has its merrits

Most simple solution:
Ansible bug gets fixed, rule works as expected and all people are happy as there are no more workarounds needed. And if someone (for whatever reason) really does not like this rule it can be disabled...

Second solution:
Bug is ignored further and this rule creates lot of extra work for people completly destroying possible atempts to follow the zen of python... Than this rule should be dropped for beeing inconsistent.

Third solution:
Get everyone ignore the rule inside their playbooks. But whats the reason of a rule everyone ignores?

I definitely prefer the first one...

[tgadiev]

I definitely prefer the first one...

So do I

[geerlingguy]

Ditto. I don’t care if we prefer that style, but if Ansible breaks if we follow that style and this lint rule slaps me on the wrist if I don’t follow that style (I know I can disable it... in all my 150+ roles and 500+ playbooks individually; that’s a lot of (meaningless) toil), then I agree either that bug needs to be elevated in priority or this rule is toothless.

[tgadiev]

Looks like the fixes are coming. Let's wait for related PR #51030 to be merged.

[insideClaw]

+1, just had to fix the error with the aforementioned
when myvar | length > 0
and it doesn't seem like anything but dirty and convoluted when you consider that the discussed "if variable:" Pythonic approach does not work here.

[tgadiev]

Related PR #51030 has been merged to develop.

[tgadiev]

Who wants to test?

[sjpb]

Ah, asking around, I discovered that the | bool filter only returns True for truthy string values (so like 'true', or 'True' or 1), but not for non-empty strings. Big miss on my part.

Except its not really a big miss seeing as this seems to be totally undocumented!

[ssbarnea]

@geerlingguy Do you think we have any chance to come to some conclusions on this famous ticket? I would really want to close it and while removal of the rule is a quick way to achieve it, I am bit worried that this rule prevented introduction of some bugs in the past.

[geerlingguy]

In my own work, I can't think of a time when this rule ever helped prevent a mistake. It only annoyed me because there are many times where a comparison to empty string is the only possible way to do it right because of Python's duck typing.

I just add it to the skip list on all my playbooks now :(

[nixfu]

Why is this so controversial? This should be the way to be able to check if a value exists/is empty or not.

when: MyString
when: not MyString

This is functionally the same as the very common python usage of
if MyString:
if not MyString:

See this how to do it right(the way python does it): https://docs.python.org/3/library/stdtypes.html#truth-value-testing

[phemmer]

Why is this so controversial? This should be the way to be able to check if a value exists/is empty or not.

when: MyString
when: not MyString

This is functionally the same as the very common python usage of
if MyString:
if not MyString:

See this how to do it right(the way python does it): https://docs.python.org/3/library/stdtypes.html#truth-value-testing

See #457 (comment)

[elafontaine]

Maybe we could get a filter to make things "truthy" or "falsy" like python does... this way, we could abstract that problem away and get people to be explicit.

Something like

when: not truthy(string)

or even

when: not filter("truthy", string)

[cognifloyd]

Does anyone have examples where the empty-string-compare rule prevents bugs?

I asked in chat to understand why comparing with an empty string is dangerous, as described in @ssbarnea's comment above:

I am bit worried that this rule prevented introduction of some bugs in the past.

My question:

The empty-string-compare rule (ansible-lint) checks for var == "" and var != "" (and similar). It suggests using var|length == 0 or var|length > 0 instead. First, why is comparing with an empty string dangerous? Second with the introduction of truthy and falsy tests in ansible 2.10, would var is falsy and var is truthy be a better recommendation?
... You said that empty-string-comapre: "prevented introduction of some bugs in the past". Do you have any examples of bugs that the rule prevented?
... I'm not clear on the purpose of the rule, so I'm not sure why |length checks are the recommended fix.

@ssbarnea said:

sadly there is no solution fits all for all string compare
I will have to reopen the subject at some point and document all options with pros and cons because last discussions were long time ago and I forgot some of the nuances.

So, can we create such a document together? What are the pros/cons of various string comparison alternatives? What bugs are possible (or common) with each alternative?

[IronTooch]

My two cents on the topic is that this rule should be removed. It makes an assumption that the user is an experienced Python user who knows that mystring="" evaluates to false. One of the major benefits of Ansible, I feel like, is that it abstracts away the Python for the most part. So assuming that the user is aware of that feature of Python is (IMHO) maybe overzealous. Particularly from people who work in super-strongly-typed languages. Those folks might (reasonably) make the assumption that "" is true and declared appropriately, and undeclared or uninitialized variables only evaluate to false. At no point does == "" result in LESS code clarity. Arguably it's more declarative. I (personally) feel code clarity should be a major point of the linting solution.

[ssbarnea]

@awcrosby @sivel @bcoca Can you please help us make a decision regarding this confusing rule?

While I do know for sure that it did catch some bugs in the past it also seems to give false recommendations in some cases.

Unless we can fix it by getting a recommended use pattern that is safe to use I would see it demoting it to an opt-in in upcoming v6.

[bcoca]

opt-in seems best, there are many cases you want to be specific and compare to empty string (also confirms 'type')

[ssbarnea]

v6 already does this as opt-in but I think that we should look into improving the rule with positive and negative cases and look into getting it out of opt-in once we address these.