Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only include users.sudo when a user with sudouser is declared. #42

Merged
merged 1 commit into from
Jul 28, 2014

Conversation

TimJones
Copy link
Contributor

Using users-formula with another formula that manages sudo can cause clashes.
This patch stops users-formula from including the users.sudo state unless a user is specifically defined as a sudouser in the pillar data.

@gravyboat
Copy link
Contributor

@TimJones I feel like the edge case of having a formula that manages sudo, as well as some users that are sudo users might be problematic. Were you able to test that by chance?

@TimJones
Copy link
Contributor Author

When used along side the sudoers-formula, the result is users being managed in the /etc/sudoers file as well as /etc/suders.d/<user>.
In addition, if the sudoers.included state is invoked, it can result in both sudoers.included and users.sudo trying to manage /etc/sudoers.d/<user>.
As a third, minor, point is that if sudo is not wanted at all, users cannot be used to manage accounts.

@gravyboat
Copy link
Contributor

Cool, I'm going to merge this for the time being. I feel like the include is out of place, but there isn't much that can be done about that due to the way this formula is structure. Thanks for the addition!

gravyboat added a commit that referenced this pull request Jul 28, 2014
Only include users.sudo when a user with sudouser is declared.
@gravyboat gravyboat merged commit 93e3c15 into saltstack-formulas:master Jul 28, 2014
@TimJones
Copy link
Contributor Author

Agreed. Another option would be to move the entire {% if 'sudouser' in user and user['sudouser'] %} block to the top of the state file for better visibility, but I felt that it would be a disproportionally large diff for such a functionally small change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants