search

scylladb / scylla-ansible-roles

Ansible roles for deploying and managing Scylla, Scylla-Manager and Scylla-Monitoring
44 stars 38 forks source link

Unsatisfiable conditional #46

Closed elcomtik closed 2 years ago

elcomtik commented 3 years ago

This condition cannot be satisfied because of contradiction. https://github.com/scylladb/scylla-ansible-roles/blob/087ccb655a7a54d431904e9fe27bcef132edebe2/ansible-scylla-manager/tasks/Debian.yml#L27-L35

By the way, the second part (line) in the condition is not reachable because items in the list are combined by AND logical operator https://docs.ansible.com/ansible/latest/user_guide/playbooks_conditionals.html. So it may be omitted entirelly

elcomtik commented 3 years ago

I'm thinking more about it. If enable_upgrade is defined in defaults, then the first line is useless and only the second one should be left there.

dyasny commented 3 years ago

First rule of ansible - never trust a variable is defined :)

I'm thinking maybe it would make more sense on a single line with the second conditional in brackets. In any case, these are the conditions. Or I could wrap it in a block that runs under one of the conditionals and then implement the other conditional on the task level...

elcomtik commented 3 years ago

Problem is that currently, it means 

(enable_upgrade is not defined) and (enable_upgrade is defined and enable_upgrade|bool == False)

However, I think it was meant to be 

(enable_upgrade is not defined) or (enable_upgrade is defined and enable_upgrade|bool == False)

This could be oneliner as above or like this

when: >
  (enable_upgrade is not defined) or
  (enable_upgrade is defined and enable_upgrade|bool == False)
dyasny commented 3 years ago

Looks good. Do you have a working cluster to test this change on?

elcomtik commented 3 years ago

Looks good. Do you have a working cluster to test this change on?

Yes, this one I tested personally.