oksanagit / deployAD

0 stars 1 forks source link

Use with_items where possible. #2

Open danielballan opened 4 years ago

danielballan commented 4 years ago

Use "{{ item }}" and with_items: rather than {{ packages }} and vars:.

https://github.com/oksanagit/deployAD/blob/fb35cd5913917ef4b163adae60966cc018c63adf/roles/install_system_packages/tasks/Debian.yml#L3

Why? The with_items: approach is more constrained---less flexible in terms of what it can do. As it reader, it's easier to understand what it's being used for (because can really only be used for one thing). The vars: approach is more powerful and thus should only be used when it's needed. It's easier to misuse, as in fact happened here: it disguised the bug reported in #1 in both occurrences. Something that was intended as a parameter ended up being defined as a variable and thus the fact that it was misspelled was never noticed.

I believe that all of these occurrences can and should be replaced by with_items:.

$ git grep "vars:"
roles/ADEiger/tasks/main.yml:  vars: 
roles/ADEiger/tasks/main.yml:  vars: 
roles/install_system_packages/tasks/Debian.yml:  vars: 
roles/install_system_packages/tasks/RedHat.yml:  vars: 
oksanagit commented 4 years ago

I have changed to looping over {item} for the list of packages in Debian.yml and RedHat.yml, however, left vars in the Eiger module. If I change to looping over {item} in Eiger module, I get this warning: [WARNING]: The loop variable 'item' is already in use. You should set the loop_var value in the loop_control option for the task to something else to avoid variable collisions and unexpected behavior. " Given that there is only one package is needed, it is OK to use var in this case, and it seems to be inline with "good practices" . I am open to discuss this.

danielballan commented 4 years ago

I suspect that's because the variable, be it {{ packages }} or {{ items }}, is included in the name: field.

https://github.com/oksanagit/deployAD/blob/fb35cd5913917ef4b163adae60966cc018c63adf/roles/ADEiger/tasks/main.yml#L1

That is unusual. A static name: is usually sufficient. In this case, you don't necessarily need Ansible to report

The following packages zeromq-devel is installed

or, on Debian,

The following packages libzmq5-dev is installed

A generic and more human-friendly message like "Install ZeroMQ support libraries" might be better, and it would avoid this problem.