curationexperts-deprecated / ansible-hydra

Ansible playbook & roles to build a production-style Hydra Head.
http://www.curationexperts.com
Other
13 stars 13 forks source link

Refactor to remove use of role-level sudo/become #44

Closed mark-dce closed 8 years ago

mark-dce commented 8 years ago

Consider which roles if any should continue to run with elevated privileges for all tasks.

Advantage of making the entire role run with elevated privileges - code is DRYer. Disadvantage of this approach - may be confusing when looking at the task list, may lead to new tasks running with elevated privileges when they should not, makes debugging by recreating each task at the command line trickier (the tasks themselves don't show that they are being run / need to be run as root). Also, when writing new playbooks with existing roles, you need to know that the role needs to be called with become at the role-level - probably worth a comment in the role's main.yml if we keep this structure.

mark-dce commented 8 years ago

There's a good discussion of DAMP not DRY here: http://stackoverflow.com/questions/6453235/what-does-damp-not-dry-mean-when-talking-about-unit-tests - see the top voted answer. I think Descriptive and Meaningful is probably the goal I was going for when I suggested we remove the role-level sudo.

Since I think we're hoping for folks new to ansible and hydra to use this repo, I think we should aim to balance of clarity and concision. I struggled a little bit to understand that some of the roles had sudo listed at the task level and some had the whole role run with sudo - so I'm definitely projecting my personal experience onto the rest of the world and am willing to retract the proposal if folks don't agree.

acozine commented 8 years ago

If the aim is to encourage people to understand and perhaps extend or contribute to the repo, then I think eliminating role-level sudo is good. It also aligns with the strategy of storing top-level playbooks in other codebases (e.g. in vagrant projects) - because it will eliminate the possibility that an outside playbook will omit sudo on a role that requires it.