search

AAROC / discourse-sso

If you're going to deploy discourse, best you deploy it with federated login
Apache License 2.0
2 stars 2 forks source link

How is user expected to customize content in vars/main.yml? #14

Closed gwarf closed 5 years ago

gwarf commented 5 years ago

How are we supposed to customize variables declared in vars/main.yml? Shouldn't things requiring configuration like the discourse_secret_key found in https://github.com/AAROC/discourse-sso/blob/272c9381fa9b107843741940eb15819f15d65645/vars/main.yml#L21 be moved to the default/main.yml file?

gwarf commented 5 years ago

In fact all configurable variable should probably be moved to default/main.yml otherwise it's more difficult to override them. https://docs.ansible.com/ansible/latest/user_guide/playbooks_variables.html#defining-variables-in-files Or am I missing something?

brucellino commented 5 years ago

Yes, all variables should be declared in defaults/main.yml so that they can be overwritten by the variable hierarchy as described in the Ansible docs you linked to However, in #16 there are some specific values (i.e. definitely not defaults) set, so I'm not comfortable with that.

To answer the question :

How are we supposed to customize variables declared in vars/main.yml?

The defaults are the ones shipped with the role, and should not be used in any instance, only for testing. The variables for your deployment should be set in vars/main.yml -- but this would result in change in git, and would be irritating to deal with. Perhaps the best thing to do would be to remove vars/main.yml from change tracking, and have only vanilla vars in defaults/main.yml. This would allow people using the role to write whatever they want in vars/main.yml.

I do not propose moving specific values to defaults though.

What do you think @gwarf ? would this be ok?

gwarf commented 5 years ago

There is something I don't get, if using a role you shouldn't set anything in vars/main.yml in the role file hierarchy, you just include the role and declare the variable you want to set wherever suits you the best. I don't understand why it would be required to change stuff in the role checkout...

brucellino commented 5 years ago

This may be an effect from the Ansible "moyens âges", but having things in vars makes the role less re-usable. A role should be re-usable off the shelf by default, that means having all the variables it needs declared somewhere -- like defaults. If the role is in change control and someone has forked it, in order to use it in their own instances, they want to maintain their own vars in change control too - this means that the vars/main.yml will keep conflicting with the origin repo every time you make a pull request (barring git-ignore gymnastics).

So:

  1. Either the people re-using the role and keeping their own vars put them in a different file (e.g. vars/egi.yml) -- we did that to keep site variables distinct in AAROC -- but then you need a task to include_vars for that specific file
  2. vars/main.yml is not part of the role, and everyone re-using the role has main/vars.yml with their own variables (and vars/main.yml in `.gitignore).

This is not specific to this role of course, the reasoning is applicable to all roles.

Of course, if you don't fork the role, but clone it into your own org, then we don't have a problem. That's plain re-use, not contributing.

gwarf commented 5 years ago

Yes I too think it's some legacy stuff, sdtill for me, to use a role you install it using ansible-galaxy and you don't have to add anything into the role folder, you had all the conf in your own repo/folder to be used with your playbook. so for me this vars/main.yml makes no sense and is just not needed in the role documentation.

brucellino commented 5 years ago

I can see your point, so removing contents from vars I think is a good idea. However removing reference to it from the README goes against the Ansible documentation of where and how to write variables.