Repository navigation
feat(deleter): delete disabled projects and groups with the org - #1976
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Direct issue #1978 requires organization deletion to include disabled projects and groups. DeleteOrganization now lists both child types with disabled records included and sends each child through t… |
| Out of Scope Changes check | |
The changes stay within issue #1978. IncludeDisabled list behavior enables the cascade to find disabled children. The group service and membership lookup changes enable the existing DeleteGroup pa… |
✨ Finishing Touches 💡 1
-
🛠️ Fix failing CI checks 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Coverage Report for CI Build 38363489979Coverage increased (+0.03%) to 54.79%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
rohilsurana
changed the title
e777a73 to
6b1fbfd
Compare
17ac65f to
4da8268
Compare
4da8268 to
8ff2e7c
Compare
Local sandbox test report for the organization soft delete stackAn independent run against a real local stack (Postgres, SpiceDB, the Frontier server). Nothing was fixed during the run, so it shows the stack as it is. Code under test: the tip of this stack (#1971, #1972, #1973, #1976) merged with #1974. Summary
After every delete these were checked: the API answers, the org row and all child rows, live policies and relations, SpiceDB tuples for every id of the org, the audit records, and the org lists (owner, admin, search, KYC). What passed
Name reuse after a soft delete
Failures and surprises
Found along the way and not related to deletes: Control on
|
| Case | What happened |
|---|---|
| Org with a disabled project | The delete fails with a foreign key error and internal. The owner policy is already gone, so the org is left ownerless and stuck: the owner gets permission_denied, a retry fails |
| Normal org | Hard delete. The row is gone, the database cascade removes domains and PATs |
| Policy on a custom resource | Left behind, same as F1 |
Not tested
| Item | Why |
|---|---|
| Billing views and billing blockers | Billing is not configured in the sandbox |
| DNS domain verification | Needs real DNS. One domain was set to verified with SQL instead |
| KYC list skipping deleted orgs (#1974) beyond the trivial check | The deleter removes the KYC row before the org row, so no API path leaves a deleted org with a KYC row |
| A failure injected exactly inside the domain delete step | Not possible without code changes. The failure before that step shows the domains survive and go on the retry |
8ff2e7c to
7799eef
Compare
6b1fbfd to
144374a
Compare
144374a to
614bec4
Compare
7799eef to
194537b
Compare
…omains and policies
194537b to
d288a28
Compare
614bec4 to
4b13c1e
Compare
| grp, err := s.groupService.Get(ctx, groupID) | ||
| if !errors.Is(err, group.ErrNotExist) { | ||
| return grp, err | ||
| } | ||
| disabled, listErr := s.groupService.List(ctx, group.Filter{GroupIDs: []string{groupID}, State: group.Disabled}) |
There was a problem hiding this comment.
Can be replaced by GetByIDs. It accepts filter Filter{IncludeDisabled: true}
| enabled, err := d.projService.List(ctx, project.Filter{OrgID: orgID}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| disabled, err := d.projService.List(ctx, project.Filter{OrgID: orgID, State: project.Disabled}) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Groups already have Filter.IncludeDisabled; GetByIDs honors it, but List ignores it. Can we make group List honor it, add the same field to project.Filter, and have the deleter make one list call per type instead of two? One query also means a project whose state changes between the two reads can't be missed or listed twice.
|
This fixes #1978 (an org delete with a disabled project fails and leaves the org without owners). Can we add |
…es-disabled-children
What changes
DeleteOrganizationnow also deletes the org's disabled projects and disabled groups. It lists them in one query that includes disabled ones, and runs the same per-project and per-group delete on each.not_found, because two places read the group through a lookup that skips disabled groups. Both now find disabled groups: the group deleted hook in membership (oneGetByIDscall withIncludeDisabled), and the group service delete. Group and projectListnow honorIncludeDisabled. The shared lookups used by other code are unchanged.DeleteGroupcall too. Deleting a disabled group used to answernot_found. It now deletes the group, the same as a disabled project.Why
The deleter listed projects and groups without the disabled ones, so it never visited them. Until now this was caught by accident. The final org row delete failed on the foreign key from
projectsorgroups, and left the org with no owner.The earlier PR in this stack makes the org row delete soft, so that foreign key never fires again. Without this change the org delete would succeed and leave the disabled projects and groups live, with their policies, under a deleted org.
Depends on
This branch is stacked on the PR that makes the org row delete soft. Ship them together.
Tests
Closes #1978