Hey all,
I’ve spent the last couple days assembling a small Ansible project to manage a multi-user, multi-distribution cluster. I’d really appreciate some code review at this early stage of development! Please feel free to tear into the code and give me as much brutal feedback as possible. I want to make sure my code is clean, efficient, and idiomatic. Let me know if you have any questions. (Also note this code is under heavy churn so it might change rapidly from the time of this writing, especially as a result of feedback).
https://github.com/SegFaultAX/ansible-demo
Thanks,
Michael-Keith
Other feedback definitely welcome but here are my suggestions:
In your provisioning playbook, you only need one play versus the two.
gather_facts: yes never needs to be explicit, it’s the default.
It looks like you’re doing conditional includes rather than group_by so you could remove the group_by task. However, I’d be much more tempted to just rely on group_by if you have conditonal OSes because the output is much much cleaner (tasks not run don’t have to be printed, etc).
You have a lot of “when” statements in https://github.com/SegFaultAX/ansible-demo/blob/master/roles/common/tasks/packages.yml that I would take advantage of your conditional include
and just move them to the appropriate centos or ubuntu file to save the repeated conditions. Currently those files are blank. (Or do as above and use group_by).
If templating sudoers, use the validate option to avoid locking yourself out (see module docs)
Stick a creates= or something on your “shell: apt-key” line to avoid executing that step when you don’t need to, it’s just a nice thing to do when you are relying on bandwidth and
resources from others and are running playbooks repeatedly, and will also save you time.
If you’re going to break up lines (personal preference), instead of:
One very tiny nit-pick. Inventory files are not YAML files, but .ini files.
Spectacular feedback so far, thanks Michael! I 100% agree with everything you said, especially regarding group_by. When I started the project, group_by seemed like the correct solution. What tripped me up though was using group_by with roles. I wanted to have, for example, a single common role that knows how to correctly provision CentOS and Ubuntu hosts. Under that model, I don’t know how to efficiently use the additional group information provided by the group_by call without sprinkling conditions all over (which isn’t any better than what I already have). Is your suggestion to split out roles into (type, distro) pairs, eg: common-centos, common-ubuntu, java-centos, java-ubuntu? Please provide an example of what you’re describing (pseudo-code is enough, I can extrapolate from there once I understand your proposal).
I also have some feedback as a new Ansible user still acclimating from the Chef/Puppet world:
-
Roles feel very limited. I wish I had all the power of a normal playbook in a role instead of just lists of tasks + conditional includes. As in the group_by case, I feel it would allow me to DRY up my roles while still keeping them super flexible.
-
It’s entirely unclear to me how to include multiple vars files per role (is that even supported?). For example, I really wanted to split out my user account information into roles/common/vars/accounts.yml, but even after asking around in IRC I couldn’t figure out how to do it if it’s possible.
-
Some of the docs (modules I’m looking at you) have grown to such a size that they’re becoming hard to search and traverse quickly. Also, the structure of docs makes it really easy to miss important information.
Anyway, I’ll start incorporating your feedback into the project now and update the thread when it’s ready for another round of review.
Cheers,
Michael-Keith
Rather than saying something feels limited, please describe what you are actually looking for being able to do.
No, currently you can’t have more than one variable file in roles/foo/vars/ … that’s something we’ll likely do later.
Please offer alternatives about how you would like to see docs structured, they are already indexed in the left sidebar.