search

OSC / ood-ansible

An ansible role for Open Ondemand
MIT License
30 stars 31 forks source link

suggestion: use a common prefix for every variable in the role #238

Open pescobar opened 8 months ago

pescobar commented 8 months ago

Hi,

it's a recommended practice in ansible that every variable in a role uses a common prefix so it's easier to map a variable to the role defining it

You can find some docs here https://ansible.readthedocs.io/projects/lint/rules/var-naming/

And and example here where where mysql role uses prefix mysql_ for every variable https://github.com/geerlingguy/ansible-role-mysql/blob/master/defaults/main.yml

I think it would make sense that every variable in this role would use preix ood_ (or something similar). I can work on this refactor in case there is interest to get it merged upstream.

I understand this would be a breaking change and maybe you prefer to keep things as they are and that's totally fine and understandable. Just let me know your opinion.

johrstrom commented 8 months ago

I understand this would be a breaking change and maybe you prefer to keep things as they are and that's totally fine and understandable. Just let me know your opinion.

I think I agree, but the change would likely be very disruptive. The next release is going to be 4.0 with some breaking changes in OOD, so maybe that's a good time to introduce breaking changes here...

pescobar commented 8 months ago

What you suggest makes total sense.

Please feel free to contact me in case you want contributions for this task in the future. I have some experience with ansible and I would be happy to help

Feel free to close this issue.

johrstrom commented 8 months ago

Please feel free to contact me in case you want contributions for this task in the future. I have some experience with ansible and I would be happy to help

Thanks! I figure it'll be a slog where it's not all done in one go, but in many smaller PRs.

Feel free to close this issue.

I'll keep it open and actually pin it in case others want to chime in.

sjpb commented 6 months ago

+1 on this, it really needs it especially for variables which are really likely to clash like servername. Maybe some of those could be tackled first to reduce the scale of the issue?