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: add prebuilt workspaces to non-default organizations#18010

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 8 commits intomainfromjjs/internal-527
Jun 4, 2025

Conversation

SasSwart
Copy link
Contributor

@SasSwartSasSwart commentedMay 23, 2025•
edited
Loading

Comment on lines +394 to +396
rbac.ResourceOrganizationMember.Type: {
policy.ActionCreate,
},
Copy link
Member

Choose a reason for hiding this comment

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

I think you also need read here todetermine which organizations the prebuild system user is a member of.

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.

Overall looks good to me 👍 just a few questions, mainly because I'm not too familiar with the RBAC code.
Also, I agree on using a map to store memberships, that seems like a cleaner approach.

@SasSwartSasSwart marked this pull request as ready for reviewMay 27, 2025 13:30
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.

LGTM, just a couple of minor suggestions to improve the readability of the tests (mostly around naming and comments). I’m not very familiar with the RBAC code, so I’ll defer formal approval to someone with more experience in this area.

Roles: []string{},
})
if err != nil {
membershipInsertionErrors = errors.Join(membershipInsertionErrors, xerrors.Errorf("insert membership for prebuilt workspaces: %w", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe it would be useful to add the presetOrganizationID to the error message for further debugging?

@github-actionsgithub-actionsbot added the staleThis issue is like stale bread. labelJun 4, 2025
@SasSwartSasSwart removed the staleThis issue is like stale bread. labelJun 4, 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.

LGTM 🚀 Just some small non-blocking comments

Comment on lines +52 to +55
// If the prebuilds system user is a member of an organization that doesn't have need any prebuilds,
// then it must have required prebuilds in the past. The membership is not currently necessary, but
// the reconciler won't remove it, because there's little cost to keeping it and prebuilds might be
// enabled again.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect 🥇
Just a small typo:

Suggested change
// If the prebuilds system user is a member of an organization that doesn'thave need any prebuilds,
// then it must have required prebuilds in the past. The membership is not currently necessary, but
// the reconciler won't remove it, because there's little cost to keeping it and prebuilds might be
// enabled again.
// If the prebuilds system user is a member of an organization that doesn'tcurrently need any prebuilds,
// then it must have required prebuilds in the past. The membership is not currently necessary, but
// the reconciler won't remove it, because there's little cost to keeping it and prebuilds might be
// enabled again.

func (s StoreMembershipReconciler) ReconcileAll(ctx context.Context, userID uuid.UUID, presets []database.GetTemplatePresetsWithPrebuildsRow) error {
organizationMemberships, err := s.store.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{
UserID: userID,
Deleted: sql.NullBool{
Copy link
Contributor

Choose a reason for hiding this comment

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

I might have missed it, but do we have tests covering the case where a user is deleted?
Non-blocking, just something that might be worth considering in a future PR if not already covered.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

covering the case where a user is deleted
We don't. Although we allow the userID here is a parameter, it is guaranteed to be the prebuilds system user, so the risk of deletion is low.

coder/internal#526 will ensure that the prebuilds system user is not deleted whenever the reconciliation loop runs.

@SasSwartSasSwart merged commit5f7e5d7 intomainJun 4, 2025
34 checks passed
@SasSwartSasSwart deleted the jjs/internal-527 branchJune 4, 2025 12:20
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 4, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@EmyrkEmyrkEmyrk left review comments

@evgeniy-scherbinaevgeniy-scherbinaevgeniy-scherbina left review comments

@ssncferreirassncferreirassncferreira approved these changes

Assignees

@SasSwartSasSwart

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prebuilt workspaces should work in any organization
4 participants
@SasSwart@ssncferreira@Emyrk@evgeniy-scherbina
[8]ページ先頭
©2009-2025 Movatter.jp