- Notifications
You must be signed in to change notification settings - Fork905
fix: improve error message when deleting organization with resources#17049
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
fix: improve error message when deleting organization with resources#17049
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Also I'm not sure what permissions I should be checking for for reading from the |
| workspace_count int; | ||
| template_count int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
classic tabs vs spaces moment (you've got 4 spaces on one line and a tab on the next)
| -- Fail the deletion if one of the following: | ||
| -- * the organization has 1 or more workspaces | ||
| -- * the organization has 1 or more templates | ||
| -- * the organization has 1 or more groups other than "Everyone" group | ||
| -- * the organization has 1 or more members other than the organization owner | ||
| -- * the organization has 1 or more provisioner keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
goes back and forth again here, and actually a lot throughout the file 😅 I usually normalize to tabs, but I don't think we really have a consistent indentation style in sql. it is nice to pick one and stick with it within a file at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it looks like right now your down migration is just a copy of your up migration.
- up migrations should add your new things
- down migrations should revert them
I think your down migration really only needs to be
droptrigger if exists protect_deleting_organizationson organizations;dropfunction if exists protect_deleting_organizations;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry, I think there were some copy paste errors as I changed from another branch to this one (long story). Probably also why there is the tabs vs spaces issue.
The down migration can't just be dropping the function since the function already exists, as far as I understand I'd have to re-declare the original function from a previous migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually taking a closer look now the down migration is correct, or at least what I meant it to be. In it we drop the existing functions and triggers and re-create the old one frommigration 296
| @@ -0,0 +1,96 @@ | |||
| DROP TRIGGER IF EXISTS protect_deleting_organizations ON organizations; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
seems a bit over defensive, but also doesn't really hurt anything.
mostly I'm just wondering if you think this needs to stay or if this is just some weird thing that your little buddy dreamt up 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and this file also has inconsistent indentation throughout. we're really not usually picky about sql formatting, but sticking with a consistent indentation character/width is nice.
Uh oh!
There was an error while loading.Please reload this page.
| SELECT | ||
| (SELECT COUNT(*) FROM workspaces WHERE workspaces.organization_id = $1 AND workspaces.deleted = false) AS workspace_count, | ||
| (SELECT COUNT(*) FROM groups WHERE groups.organization_id = $1) AS group_count, | ||
| (SELECT COUNT(*) FROM templates WHERE templates.organization_id = $1 AND templates.deleted = false) AS template_count, | ||
| (SELECT COUNT(*) FROM organization_members WHERE organization_members.organization_id = $1) AS member_count, | ||
| (SELECT COUNT(*) FROM provisioner_keys WHERE provisioner_keys.organization_id = $1) AS provisioner_key_count; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder if we should declare this as a function so that it could be used inprotect_deleting_organizations. this is super nice, readable, and concise, but the code for accomplishing the same counting inprotect_deleting_organizations is like 20 lines longer. 🤔
| // There will always be one member and group so instead we need to check that | ||
| // the count is greater than one. | ||
| addDetailPart("member", orgResourcesRow.MemberCount-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder if members should actually prevent deletion. it's not quite as serious as deleting the others which aren't recoverable. workspaces, templates, and groups are all more work to configure, but removing a member doesn't delete the user, and adding them back is quite simple.
I think@Emyrk and I talked about this forever ago and I convinced him that members shouldn't block deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yea, it is a tough one. If we keep members, their roles in that org I think are still fetched on every auth. Maybe we should omit them in that call?
I'd rather be strict today, and loosen it up later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we could still delete all the members from the database tho, and probably even should so that the admin who deletes the org isn't the only one paying that penalty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this something we wan to do in this PR or should we make an issue for it and get to it later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@brettkolodny let's not delete things for the user at this time.
I think we will want to add some "cleanup" button later that the user clicks to explictly delete all the resources required to cleanup an org.
aslilac commentedMar 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
coder/coderd/rbac/object_gen.go Lines 215 to 223 ine40ea25
good guess! we use the same resource type for provisioners and their keys right now |
cf10d98 intomainUh oh!
There was an error while loading.Please reload this page.
Closescoder/internal#477
I'm solving this issue in two parts: