owncloud-ansible / owncloud

Ansible role for ownCloud 10
https://owncloud-ansible.github.io
Apache License 2.0
6 stars 7 forks source link

Change include conditionals (support jinja2_native=True) #36

Closed max-frank closed 3 years ago

max-frank commented 3 years ago

When using the jinja2_native option the files lookups will correctly return null when no match is found. This lead to the playbook crashing for such cases. This commit changes the conditions to simply check the truthyness of the result. Which could be a greater than 0 length string when a match was found, or either an empty string (jinja2_native=false) or null (jinja2_native=true) when no match was found.

See the Ansible Config Ref and the Jinja2 Doc for details on the native environment.

The difference in observed values for the negative case (i.e., no files found) and the correctness of the proposed change can also be easily checked by running the following play. (Note that you will have to add file matching the format to check the positive case.)

- name: PoC
  hosts: localhost
  gather_facts: yes
  vars:
    params:
      files:
        - "configure-{{ ansible_os_family | lower }}.yml"
      paths:
        - "tasks"
    task_files: "{{ lookup('first_found', params, errors='ignore') }}"
  tasks:
    - debug:
        var: params
    - debug:
        var: task_files
    - name: conditional
      debug:
        msg: executed conditional
      when: task_files
# ansible.cfg
...
# This option preserves variable types during template operations. This requires Jinja2 >= 2.10.
jinja2_native = True # False is the default
...

The same "issue" is found in all other owncloud ansible roles. For now I have only opened a PR for this role and owncloud-ansible/php, but am willing open PRs for the remaining roles if you OK these PRs.

See https://github.com/owncloud-ansible/php/pull/16

xoxys commented 3 years ago

Thanks for your report, and sorry for the delay. Can you explain a bit more about your requirements? Why do you need to use jinja2_native=True in general?

xoxys commented 3 years ago

Thanks again for your suggestion. I've added your suggestion to the linked PR, together with some other small adjustments for jinaj2_native hope you don't mind. I would still be interested to learn about real world use cases for jinaj2_native :)

max-frank commented 3 years ago

Thanks for your report, and sorry for the delay. Can you explain a bit more about your requirements? Why do you need to use jinja2_native=True in general?

Thanks again for your suggestion. I've added your suggestion to the linked PR, together with some other small adjustments for jinaj2_native hope you don't mind. I would still be interested to learn about real world use cases for jinaj2_native :)

This original issue is a good example of some of the limitations one might encounter with the normal jinja2 everything is a string behavior. https://github.com/ansible/ansible/issues/9362

In our use case the Owncloud roles are used as a part of a larger deployment of multiple software configurations. For one of those configurations we use multiple layers of template variables that get serialized to YAML using the filter. Similar how its described in the issue we experienced type failures for this serialization that could not be fixed by adding e.g., | int before the final conversion.

Thanks for adding the changes to your upstream repositories.

xoxys commented 3 years ago

Ouch... Interesting read, thanks a lot for the insides!