Skip to content

feat: expose Everyone group through UI #9117

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

Merged
merged 13 commits into from
Aug 17, 2023
Merged

feat: expose Everyone group through UI #9117

merged 13 commits into from
Aug 17, 2023

Conversation

sreya
Copy link
Collaborator

@sreya sreya commented Aug 15, 2023

Frontend Behavior Changes:

  • Disables modifying name and display in Group Settings page when group is Everyone.
  • Disables adding/removing users in the Group page when group is Everyone.
  • Disables deleting group in Group page when group is Everyone.

Backend Behavior Changes;

  • Calculating quota now takes into account the Everyone group.
  • GET /groups endpoint now returns the Everyone group.
  • Disabled modifying name, display_name in backend for Everyone. Allows updating quota_allowance and avatar_url (why not)

fixes #6518

@sreya sreya force-pushed the jon/everyonegroup branch from 7ad3520 to 2c7b98d Compare August 16, 2023 02:31
@sreya sreya requested a review from Emyrk August 16, 2023 02:37
@sreya sreya marked this pull request as ready for review August 16, 2023 14:37
@ammario
Copy link
Member

ammario commented Aug 16, 2023

PR title should be about exposing everyone group through the UI right?

@sreya sreya changed the title feat: allow setting quotas on Everyone group feat: expose Everyone group through UI Aug 16, 2023
@sreya sreya requested a review from Emyrk August 16, 2023 20:51
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

Even ported the tests, nice!

Copy link
Member

@ammario ammario left a comment

Choose a reason for hiding this comment

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

good stuff

func ReadModifyUpdate(db Store, f func(tx Store) error,
) error {
var err error
for retries := 0; retries < maxRetries; retries++ {
Copy link
Member

Choose a reason for hiding this comment

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

Curious about the decision to not use retry logic and spin. Is there a thundering herd risk?

Copy link
Member

Choose a reason for hiding this comment

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

Inspired by this function, I added Jitter to our retry package: https://pkg.go.dev/github.com/coder/retry#Retrier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly I just copied this from v1. In the interest of getting this PR merged I'd like to leave it as-is.

Copy link
Member

Choose a reason for hiding this comment

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

sure thing, didn't know that

@sreya sreya requested a review from ammario August 17, 2023 17:31
@sreya sreya merged commit 2f6687a into main Aug 17, 2023
@sreya sreya deleted the jon/everyonegroup branch August 17, 2023 18:25
@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: a quota budget should be able to be specified for the Everyone group
4 participants