- Notifications
You must be signed in to change notification settings - Fork907
chore: handle deleted organizations in idp sync#17405
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
f1878af to9bdf31dCompare| deleted := false | ||
| q.data.organizationMembers = slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool { | ||
| match := member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID | ||
| deleted = deleted || match | ||
| return match | ||
| }) | ||
| iflen(deleted) == 0 { | ||
| if!deleted { |
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.
A dbmem bug I found
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.
oh this is nasty, took me a minute to realize what the bug was. 💀
if you want, rather than trackdeleted yourself, you could do something like
newMembers:=slices.DeleteFunc(q.data.organizationMembers,func(member database.OrganizationMember)bool {returnmember.OrganizationID==arg.OrganizationID&&member.UserID==arg.UserID})iflen(newMembers)==len(q.data.organizationMembers) {returnsql.ErrNoRows}q.data.organizationMembers=newMembers
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 had thought about that as well. That felt a bit less direct and less intuitive to me though, so I went with the crude and simple lol
| -- Optionally provide a filter for deleted organizations. | ||
| CASE WHEN | ||
| sqlc.narg('deleted') :: boolean IS NULL THEN | ||
| true | ||
| ELSE | ||
| deleted = sqlc.narg('deleted') | ||
| END AND |
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.
Adds the ability to query for all orgs, regardless of deletion
Deleted organizations are still attempting to sync members.This causes an error on inserting the member, and would likelycause issues later in the sync process even if that member isinserted. Deleted orgs should be skipped.
Idp sync should exclude deleted organizations and auto removemembers. They should not be members in the first place.
7b0cb6c to909c895Compare| // If you pass in an empty slice to the db arg, you get all orgs. So the slice | ||
| // has to be non-empty to get the expected set. Logically it also does not make | ||
| // sense to fetch an empty set from the db. |
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.
yeah but this turns it into our problem. wouldn't this usually be the result of a configuration error?
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 don't follow. You can configure it such that a user is in 0 orgs. Even if it's a configuration mistake on the admin, it is a valid configuration.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: ケイラ <mckayla@hey.com>
669e790 intomainUh oh!
There was an error while loading.Please reload this page.
/cherry-pick release/2.20 |
…17405)Deleted organizations are still attempting to sync members. This causesan error on inserting the member, and would likely cause issues later inthe sync process even if that member is inserted. Deleted orgs should beskipped.
/cherry-pick release/2.21 |
…17405)Deleted organizations are still attempting to sync members. This causesan error on inserting the member, and would likely cause issues later inthe sync process even if that member is inserted. Deleted orgs should beskipped.
Uh oh!
There was an error while loading.Please reload this page.
Deleted organizations are still attempting to sync members. This causes an error on inserting the member, and would likely cause issues later in the sync process even if that member is inserted. Deleted orgs should be skipped.
The error that arises: