Skip to content

Conversation

@edlerd
Copy link
Contributor

@edlerd edlerd commented Mar 19, 2025

Done

  • ensure admin group is created on install and update
  • add server admin permission to the group

This simplifies the UI onboarding, because we can rely on the presence of this group.

@edlerd edlerd force-pushed the init-admin-group branch from 6fb4ce4 to 7d0f89b Compare March 19, 2025 17:40
@tomponline tomponline requested a review from markylaing March 19, 2025 21:08
@edlerd edlerd force-pushed the init-admin-group branch 3 times, most recently from 4abc9ab to a1430a8 Compare March 19, 2025 22:06
@edlerd edlerd changed the title create admin group with server admin permission on install and update create admin group with server admin permission on install Mar 19, 2025
@edlerd edlerd force-pushed the init-admin-group branch 5 times, most recently from 0b6a1d6 to 3b901b2 Compare March 20, 2025 11:52
@edlerd edlerd force-pushed the init-admin-group branch from 3b901b2 to 0e65343 Compare March 24, 2025 12:36
@edlerd edlerd requested a review from tomponline March 24, 2025 12:38
@edlerd edlerd force-pushed the init-admin-group branch 2 times, most recently from 7529182 to 7707522 Compare March 24, 2025 13:11
@markylaing
Copy link
Contributor

I'm not opposed to creating an admin group by default but just want to clarify a little more. Why did we decide to just add an admin group by default rather than making this a lxd init option?

Also, can the permissions of this group be modified? Can the group be deleted? Who relies on the admin group being present?

@edlerd
Copy link
Contributor Author

edlerd commented Mar 24, 2025

I'm not opposed to creating an admin group by default but just want to clarify a little more. Why did we decide to just add an admin group by default rather than making this a lxd init option?

I think it is simpler to create the group with permissions, we can avoid adding another question on the lxd init flow.

Also, can the permissions of this group be modified? Can the group be deleted?

Currently, yes, I don't think we need to protect it, see below.

Who relies on the admin group being present?

The UI onboarding will reference the group. Currently, we suggest to 1. create the group 2. add the permission and 3. create the user with the group. We hope to simplify the onboarding to be a single step with creation of the user and referencing the group in a single command. The UI will show a note that if the group is not existing, this is how to create it. The note might be a solution for the question above to let users delete or change the group as they like and still have reliable onboarding instructions.

@edlerd edlerd force-pushed the init-admin-group branch from 7707522 to 2262b90 Compare March 25, 2025 09:19
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Just the one comment and then LGTM :)

@edlerd edlerd force-pushed the init-admin-group branch from 2262b90 to bbefd3f Compare March 26, 2025 13:00
@edlerd edlerd requested a review from markylaing March 26, 2025 13:00
@edlerd
Copy link
Contributor Author

edlerd commented Mar 26, 2025

Comments should be resolved. Please have another pass at this one @markylaing

Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Ok last thing then as per #15215 (comment)

@edlerd edlerd force-pushed the init-admin-group branch from bbefd3f to 710cf34 Compare March 26, 2025 15:08
@edlerd edlerd requested a review from markylaing March 26, 2025 15:08
@edlerd edlerd force-pushed the init-admin-group branch from 710cf34 to dab8b84 Compare March 26, 2025 16:09
@edlerd edlerd requested a review from markylaing March 26, 2025 16:10
@edlerd edlerd force-pushed the init-admin-group branch 2 times, most recently from 5ef058e to 649353a Compare March 26, 2025 16:42
@edlerd edlerd force-pushed the init-admin-group branch from 649353a to 2e18e59 Compare March 26, 2025 16:45
@edlerd
Copy link
Contributor Author

edlerd commented Mar 26, 2025

I misunderstood what you meant previously @markylaing Please have another pass at this one now.

Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Ta!

@tomponline tomponline merged commit c2a2764 into canonical:main Mar 26, 2025
29 checks passed
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