redhat-cop / automation-good-practices

Recommended practices for all elements of automation using Ansible, starting with collections and roles, continuing with playbooks, inventories and plug-ins... These good practices are planned to be used by all Red Hat teams interested but can of course be used by others.
268 stars 66 forks source link

Recommendations for Jinja and roles #68

Closed jillr closed 10 months ago

jillr commented 2 years ago

Add Jinja recommendations and some role text that was missed in previous PR.

This will likely conflict with 64,65; my expectation would be that those need to land first so I can rebase and clean this one up.

ericzolf commented 2 years ago

You should now be un-blocked!

jillr commented 2 years ago

@ericzolf I know I'm too late for the call today but when folks have a chance this should be ready for review

jillr commented 2 years ago

Hi @ericzolf, would it be possible to get a re-review on this before the CoP meeting next week?

ericzolf commented 2 years ago

I'm fine with almost everything but still struggling with Design the interface focused on the functionality, not on the software implementation behind it. The message isn't either the issue, rather the example chosen. As much as I recommend not using tasks_from and rather splitting roles to make them more granular and fit for purpose (how many times do I see include_role: something_create/tasks_from: delete.yml which basically does the contrary of what the role's name says it does?), I still wouldn't recommend to have a front-end role with such a complex variables structure. I don't have a better example but perhaps the crowd has? If it needs be, I would prefer no example than this example, but we can discuss.

gravesm commented 2 years ago

Why wouldn't you just use a simple variable structure? For example:

- hosts: all
  gather_facts: false
  tasks:
  - name: Perform several actions
    include_role: mycollection.run
    vars:
      var_1: 1
      var_2: 2
      var_3: 3
      var_4: 4

If the point of the role is to hide the underlying implementation of the included roles, it seems like that should also extend to the variables. The example as it exists makes it look like the variables are just being passed through, which would expose the user to changes in those roles. You might need a bit more explanatory text since this example doesn't make it clear what the connection is to the underlying variables, but that kind of seems like what you'd want in this pattern.

jillr commented 2 years ago

@ericzolf Do you think gravesm's suggestion^ would be better? Or should we just get rid of the contrived examples entirely?

ericzolf commented 10 months ago

I resolved all outdated comments, hopefully everything is clear now (only two small formatting fixes). Sorry for the big delay, this MR review got swamped in daily life. Thanks for supporting this effort!

ericzolf commented 10 months ago

Approved yesterday in the CoP meeting, merging. Thanks for the effort (and the patience!).