Skip to content

Add role for configuring sudoers groups #709

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

MoteHue
Copy link
Contributor

@MoteHue MoteHue commented Jun 18, 2025

No description provided.

@MoteHue MoteHue requested a review from a team as a code owner June 18, 2025 10:34
@MoteHue MoteHue requested a review from technowhizz June 18, 2025 10:34
@priteau
Copy link
Member

priteau commented Jun 18, 2025

Why keep it limited to sudoers groups? It could easily support both users and groups.

@MoteHue
Copy link
Contributor Author

MoteHue commented Jun 18, 2025

Why keep it limited to sudoers groups? It could easily support both users and groups.

No reason to keep it limited, this is just all we needed for the customer site.
I'll mark as draft and come back to propose handling users too.

@MoteHue MoteHue marked this pull request as draft June 18, 2025 12:54
@sjpb
Copy link
Collaborator

sjpb commented Jun 18, 2025

I'll mark as draft and come back to propose handling users too.

I definitely don't think we should expand if we don't need it. I think current scope/role/PR title is OK TBH.

It does definitely need to note in ansible/roles/compute_init/README.md the level of support it provides for running via compute-init. And ideally that needs to not be none.

Comment on lines +8 to +10
- `group`: Required string. The group name to grant sudo privileges to.
- `commands`: Required string. The sudo commands specification (e.g., "ALL=(ALL) ALL").
- `state`: Optional string. Either "present" (default) or "absent" to remove the configuration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add more indentation to these lines.

@priteau
Copy link
Member

priteau commented Jun 19, 2025

My issue with keeping it as it is now: if we want to add users later, we might want to change the way the parameters are defined. It is easier to do now while there are no users of the role.

Also, much of what you are doing can be done with https://docs.ansible.com/ansible/latest/collections/community/general/sudoers_module.html. Maybe it could just be a thin wrapper around it?

@sjpb
Copy link
Collaborator

sjpb commented Jun 20, 2025

We also have support for sudo rules in basic_users. If we merge this [edit: + user support], should we remove that in this PR too (which implies some effort required to migrate clients using it)?

@sjpb
Copy link
Collaborator

sjpb commented Jun 20, 2025

Also, much of what you are doing can be done with https://docs.ansible.com/ansible/latest/collections/community/general/sudoers_module.html. Maybe it could just be a thin wrapper around it?

Ok so if we have "native" Ansible support and a single client needing this, I'm strongly in favor of just leaving this as a hook for the moment. We can't wrap every Ansible module in a role. Is there a reason for not leaving it like that?

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.

3 participants