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.

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

No need to evaluate a variable two times. #2

Merged
merged 1 commit into from Oct 15, 2015

Conversation

ypid
Copy link
Member

@ypid ypid commented Oct 15, 2015

Example: docker_upstream|d() and docker_upstream

  • When docker_upstream is undefined it defaults to false using the
    Jinja default filter. The same applies when docker_upstream is False.

  • When docker_upstream is True, it it tested two times for True.

  • Additionally this change now correctly handles:

    false_var: False
    docker_upstream: '{{ false_var }}'

Example: `docker_upstream|d() and docker_upstream`

* When `docker_upstream` is undefined it defaults to false using the
  Jinja `default` filter. The same applies when `docker_upstream` is False.
* When `docker_upstream` is True, it it tested two times for True.
* Additionally this change now correctly handles:

  ```YAML
  false_var: False
  docker_upstream: '{{ false_var }}'
  ```
@drybjed
Copy link
Member

drybjed commented Oct 15, 2015

Looks good. I guess it's time to unlearn another thing... :-)

drybjed added a commit that referenced this pull request Oct 15, 2015
No need to evaluate a variable two times.
@drybjed drybjed merged commit fadfbf3 into debops:master Oct 15, 2015
@ypid
Copy link
Member Author

ypid commented Oct 15, 2015

At least this is shorter and it worked in all my test cases :)

@dddpaul
Copy link
Contributor

dddpaul commented Nov 30, 2015

Hi. You've merged definitely buggy commit :)

For example, this commit turns {% if docker_bridge|d() and docker_bridge %} into {% if docker_bridge|d() | bool %}.

But docker_bridge is a string, not boolean! So {% if docker_bridge|d() | bool %} evaluates to True only if name of docker bridge is True or true or Yes etc.

The same is for the lists. You can't use {% if docker_listen|d() | bool %} with docker_listen being a list. Because for every empty/non-empty list it evaluates to False.

You can use var | d() | bool construction only if var a boolean.

I have another proposal for conditional brevity:

  1. Use var instead var is defined and var, see my post about that.
  2. Get rid of d() completely, it has no sense.
  3. Use var | bool when var is boolean and var otherwise (for strings and lists).

For example:

  • when: ((nginx_local_servers is defined and nginx_local_servers) and (item.0 is defined and item.0)) -> when: nginx_local_servers and item.0 (regardless of type);
  • {% if docker_bridge|d() and docker_bridge %} -> {% if docker_bridge %} (docker_bridge is a string);
  • {% if docker_listen|d() and docker_listen %} -> {% if docker_listen %} (docker_listen is a list);
  • {% if docker_pki|d() and docker_pki | bool %} -> {% if docker_pki| bool %} (docker_pki is a boolean).

@drybjed
Copy link
Member

drybjed commented Nov 30, 2015

I agree that recent (as in, the 2.0 Ansible tree, not recent in time) changes to Ansible and Jinja2 parsing made the parser more strict. I guess it's time to unlearn some things. Lots of d() and is defined tests came from the fact that Ansible by default treats undefined variables as errors, which in DebOps isn't always the case, for example in multiple places not defined values in YAML lists have a default (check debops.users for a good example of this). So I started testing every variable after noticing this and I guess it went out of hand after some time. :-)

Thanks for noticing this and detailed explanations in your blog. :)

@dddpaul
Copy link
Contributor

dddpaul commented Nov 30, 2015

I was very wrong about uselessness of default filter. It turns out that Ansible throws AnsibleUndefinedVariable if variable is undefined and not protected by d().

So my propositions are slightly simplified :)

  1. Use var instead var is defined and var.
  2. Use var | d() | bool when var is boolean and var | d() otherwise (for strings and lists).

@drybjed
Copy link
Member

drybjed commented Nov 30, 2015

@dddpaul Yup, that's exactly how I'm using that now. Also remember that d() does not mean is defined but default(), and sometimes this might be significant, for example if you want to check if a given key in a dict is defined but handle it differently depending if it's a string, or a list, or a mapping.

@ypid
Copy link
Member Author

ypid commented Nov 30, 2015

@dddpaul Thanks for finding my wrong use of the bool filter. Seems I kind of overdid it there …

But for the docker_upstream|d() | bool case: I actually discussed this with @drybjed prior and I think there are some good reasons for using it (but only when docker_upstream is supposed to be a boolean, as you noted):

  1. When passing variables like

    my_custom_boolean: True
    docker_upstream: '{{ my_custom_boolean }}'

    docker_upstream is actually a string containing True. The bool filter converts it back to its boolean meaning

  2. Variables can be undefined and Ansible will complain about them. So I find it quite naturally to default them to something in this case. Example in condition (for string): https://github.com/debops-contrib/ansible-checkmk_agent/blob/fff2bea89f1668524a08e6cf03e6b60b72b35f5a/tasks/setup_plugins.yml#L38

    Using default() (empty argument) uses the default_value from the default filter which evaluates to False.

Use var instead var is defined and var.

You might not be aware that there can be unintended side effects here: debops/ansible-owncloud#26
Not yet sure how this can be better expressed/handled.

Thanks for the blogposts 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants