-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix entity group associations #10085
Conversation
@@ -830,6 +847,14 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit | |||
return nil, err | |||
} | |||
|
|||
for _, group := range fromEntityGroups { | |||
group.MemberEntityIDs = append(group.MemberEntityIDs, toEntity.ID) | |||
err = i.UpsertGroupInTxn(ctx, txn, group, persist && !isPerfSecondaryOrStandby) |
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 the usage of persist && !isPerfSecondaryOrStandby
correct here?
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.
Yep!
Don't want to be subscribe d |
Are there any edge cases that need to be handled for "external" groups? |
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.
Hey @mdgreenfield. Thanks for catching the bug and for working on a fix. Apologies for getting back late on this. Would you be willing to take this forward?
Assuming you are, would you mind adding this behavior as part of the API docs as well?
@@ -830,6 +847,14 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit | |||
return nil, err | |||
} | |||
|
|||
for _, group := range fromEntityGroups { | |||
group.MemberEntityIDs = append(group.MemberEntityIDs, toEntity.ID) | |||
err = i.UpsertGroupInTxn(ctx, txn, group, persist && !isPerfSecondaryOrStandby) |
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.
Yep!
Hi @vishalnayak, Not a problem. I can look at getting this PR in order next week. |
58fa3fa
to
34ac5ea
Compare
- When two entities are merged, remove the from entity ID in any associated groups. - When two entities are merged, also merge their associated group memberships. Fixes hashicorp#10084
34ac5ea
to
b2d88d8
Compare
PR has been rebased. API docs have been updated. Changelog file has been added. @vishalnayak, very much related to this PR is #10101. Does it make sense to get that one in too? |
@mdgreenfield would you please work on this #10101 as well. Some tests in there are failing |
associated groups.
memberships.
Fixes #10084