feat: add prebuilt workspaces to non-default organizations #18010
Conversation
| rbac.ResourceOrganizationMember.Type: { | ||
| policy.ActionCreate, | ||
| }, |
There was a problem hiding this comment.
I think you also need read here to determine which organizations the prebuild system user is a member of.
user a better db call, avoid an inner loop.
| Roles: []string{}, | ||
| }) | ||
| if err != nil { | ||
| membershipInsertionErrors = errors.Join(membershipInsertionErrors, xerrors.Errorf("insert membership for prebuilt workspaces: %w", err)) |
There was a problem hiding this comment.
nit: maybe it would be useful to add the preset OrganizationID to the error message for further debugging?
| // 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. |
There was a problem hiding this comment.
Perfect 🥇
Just a small typo:
| // 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. | |
| // If the prebuilds system user is a member of an organization that doesn't currently 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{ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
closes coder/internal#527