Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat(enterprise/coderd): allow system users to be added to groups#18341

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
SasSwart merged 14 commits intomainfromjjs/prebuilds-user-group-membership
Aug 8, 2025

Conversation

SasSwart
Copy link
Contributor

@SasSwartSasSwart commentedJun 12, 2025•
edited
Loading

closes#18274

This pull request makes system users visible in various group related queries so that they can be added to and removed from groups. This allows system user quotas to be configured. System users are still ignored in certain queries, such as when license seat consumption is determined.

This pull request further ensures the existence of a "coder_prebuilt_workspaces" group in any organization that needs prebuilt workspaces

Summary by CodeRabbit

  • New Features
    • Organization and group member listings now include system users.
  • Bug Fixes
    • Updated tests to reflect the inclusion of system users in member and group queries.

@SasSwartSasSwart changed the titlefeat: allow system users to be added to groupsfeat(enterprise/coderd): allow system users to be added to groupsJun 12, 2025
@github-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelJun 20, 2025
@johnstcnjohnstcn reopened thisJun 23, 2025
@johnstcnjohnstcn removed the staleThis issue is like stale bread. labelJun 23, 2025
@github-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelJul 1, 2025
@SasSwartSasSwart reopened thisJul 14, 2025
@SasSwartSasSwart removed the staleThis issue is like stale bread. labelJul 14, 2025
@SasSwartSasSwart marked this pull request as ready for reviewJuly 14, 2025 13:55
Copy link
Contributor

@ssncferreirassncferreira left a comment

Choose a reason for hiding this comment

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

I'm wondering about this approach. Do we need to addIncludeSystem: true to all these queries just to support the prebuilds system user?
This means someone will need to manually create a group for the prebuilds user and assign it a quota, right?

Wouldn't it be more robust to automatically create a group for the prebuilds system user with a default quota, and allow users to override it if needed? That way, we avoid having to include system users in these queries and reduce the risk of system users leaking into user-facing features.

@SasSwart
Copy link
ContributorAuthor

The prebuilds user has already leaked into user facing features. Administrators search for prebuilds by specifying the prebuilds user on the workspace list page. For consistency and clarity, I think we should embrace listing and showing the prebuilds user.

I like the idea of automatically creating a prebuilds group, but we'll need to decide what a sensible default quota would be if there is such a value that makes sense.

@ssncferreira
Copy link
Contributor

ssncferreira commentedJul 16, 2025•
edited
Loading

The prebuilds user has already leaked into user facing features. Administrators search for prebuilds by specifying the prebuilds user on the workspace list page. For consistency and clarity, I think we should embrace listing and showing the prebuilds user.

It’s true that the prebuilds user is already exposed in some user-facing features, like showing prebuilt workspaces, but that might not be the ideal approach 😕 The issue isn’t just about the prebuilds user, it extends to listing all current and future system users.

I like the idea of automatically creating a prebuilds group, but we'll need to decide what a sensible default quota would be if there is such a value that makes sense.

We already have a precedent with theEveryone group:

-- name: InsertAllUsersGroup :one
INSERT INTO groups (
id,
name,
organization_id
)
VALUES
(sqlc.arg(organization_id),'Everyone',sqlc.arg(organization_id)) RETURNING*;

We could apply the same pattern here: automatically create the prebuilds group with a default quota of 0, and clearly document this behavior along with instructions on how users can update the quota if needed.

@github-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelJul 24, 2025
@coderabbitaicoderabbitai
Copy link

coderabbitaibot commentedJul 24, 2025•
edited
Loading

Caution

Review failed

The head commit changed during the review from1cd68e3 to4c0c4d4.

Walkthrough

This change updates the logic for retrieving organization and group members to consistently include system users, such as the prebuilds system user, by default. It introduces a new boolean parameter to control this behavior in both SQL queries and Go code, and updates related tests to expect the presence of system users in member lists.

Changes

Files / AreasChange Summary
coderd/database/queries.sql.go, coderd/database/queries/organizationmembers.sqlAddedIncludeSystem boolean parameter to queries and Go structs/methods; updated SQL to filter system users.
coderd/members.goModified queries to include system users by settingIncludeSystem: true.
enterprise/coderd/groups.goUpdated group member queries to include system users (IncludeSystem: true).
coderd/members_test.go, enterprise/coderd/groups_test.go, enterprise/coderd/roles_test.goUpdated tests to expect system users in member lists and adjusted assertions accordingly.

Sequence Diagram(s)

sequenceDiagram    participant API    participant MembersService    participant Database    API->>MembersService: Request organization/group members    MembersService->>Database: Query members (IncludeSystem: true)    Database-->>MembersService: Return all members (including system users)    MembersService-->>API: Return member list with system users
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

In the warren of code, a new rule appears,
System users hop in, dispelling old fears.
Member lists grow, with prebuilds in tow,
The queries now know who’s included below.
Tests count the bunnies, one more in the sun—
With every new member, the job is well done!
🐇✨

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branchjjs/prebuilds-user-group-membership
🪧 Tips

Chat

There are 3 ways to chat withCodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag@coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag@coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on oursupport page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings togenerate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add@coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add@coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add@coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a.coderabbit.yaml file to the root of your repository.
  • Please see theconfiguration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit ourDocumentation for detailed information on how to use CodeRabbit.
  • Join ourDiscord Community to get help, request features, and share feedback.
  • Follow us onX/Twitter for updates and announcements.

@github-actionsgithub-actionsbot removed the staleThis issue is like stale bread. labelJul 25, 2025
Copy link
Contributor

@ssncferreirassncferreira left a comment

Choose a reason for hiding this comment

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

Could we also add a test case that verifies the behavior when the prebuilds quota is actually used? I'm curious about whatQuotaAllowance: 0 means in practice, does this mean prebuild creation will always fail due to no quota, or does 0 represent unlimited/no enforcement?
It would be helpful to have a test that actually attempts to create prebuilds after the group is set up to clarify this behavior and ensure the default quota setting works as intended.

@SasSwart
Copy link
ContributorAuthor

@ssncferreira I've added unit tests to verify the scope of quotas. You were right. Quotas are org scoped. Prebuilds might reach it's quota in one organization while still having some budget left in another. The added tests prove the desired behaviour for all users, including prebuilds.

I'm planning a follow up where the prebuilds user checks its quota before it attempts to create a new workspace and fails early if it would exceed the quota.

ssncferreira reacted with rocket emoji

@SasSwartSasSwart requested a review fromjohnstcnJuly 30, 2025 13:27
// ZeroQuota tests that a user with a zero quota allowance can't create a workspace.
// Although relevant for all users, this test ensures that the prebuilds system user
// cannot create workspaces in an organization for which it has exhausted its quota.
t.Run("ZeroQuota",func(t*testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

While it's good to have this test, it would be best to explicitly test this scenario with the prebuilds reconciler.

@SasSwartSasSwart requested a review frommtojekAugust 4, 2025 13:50
@SasSwart
Copy link
ContributorAuthor

SasSwart commentedAug 4, 2025•
edited
Loading

Internal consensus:
We're going to keep system users hidden for now, but show the prebuilds group and document how to use it. Further UX refinement might follow.

Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

I want to highlight that this PR is significantly delayed compared to its original estimate (complexity = 1), and we’re still pending the final unit test.

At this point, we need to make a decision:
@SasSwart if the test proves too complex or time-consuming, let’s proceed without it and merge the PR as-is. We cannot afford further delays, as this is already impacting remaining roadmap work.

@SasSwartSasSwart merged commitb200fc8 intomainAug 8, 2025
28 checks passed
@SasSwartSasSwart deleted the jjs/prebuilds-user-group-membership branchAugust 8, 2025 09:03
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsAug 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn left review comments

@ssncferreirassncferreirassncferreira left review comments

@mtojekmtojekmtojek approved these changes

Assignees

@SasSwartSasSwart

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: Workspace prebuilds conform to the site quota, leading many to fail
4 participants
@SasSwart@ssncferreira@johnstcn@mtojek
[8]ページ先頭
©2009-2025 Movatter.jp