ROX-11070: Update group service and datastore to use ID instead of composite key.#2063
ROX-11070: Update group service and datastore to use ID instead of composite key.#2063
Conversation
|
Skipping CI for Draft Pull Request. |
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
|
Tag for build #729986 is 💻 For deploying this image using the dev scripts, run the following first: export MAIN_IMAGE_TAG='3.70.x-573-gd2b50aecca'🕹️ A |
3b92aac to
0dfa619
Compare
7239011 to
f4894c6
Compare
0dfa619 to
048c22e
Compare
f4894c6 to
cf7585b
Compare
048c22e to
6d122bb
Compare
cfccb5a to
9ac311b
Compare
6d122bb to
25526b3
Compare
9ac311b to
e4f75d9
Compare
25526b3 to
deee13b
Compare
e4f75d9 to
f642da0
Compare
rukletsov
left a comment
There was a problem hiding this comment.
Looks good to me modulo some questions for diffGroups(). Left a couple of comments on the cosmetics.
central/group/datastore/validate.go
Outdated
| return errors.Wrap(err, "invalid group properties") | ||
| } | ||
| if group.GetRoleName() == "" { | ||
| return errox.InvalidArgs.New("groups must match to roles") |
There was a problem hiding this comment.
I highly suggest not using error classes in validation for two reasons:
- At the point of the validation you can't really know the class because you don't know the origin of the validated object. Imagine we have some default groups we add upon startup — this clearly won't be
400. - It's much easier to follow error class conversions if we standardize on one level. Datastore (here probably Store) is usually a good candidate: it serves both to GraphQL and gRPC and usually possesses knowledge of objects' origin.
@0x656b694d maybe we can update the errox guidelines with this?
deee13b to
3fa2c16
Compare
f642da0 to
6d061fe
Compare
rukletsov
left a comment
There was a problem hiding this comment.
One last change and we're good to go. Thank you, Daniel!
…ID to be set. Fix failing tests.
…ne before setting values on properties.
…ore, minor comments).
|
Images are ready for the commit at d2b50ae. To use with deploy scripts, first |
6d061fe to
c5828e5
Compare
|
/retest |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(grps) == 0 { |
There was a problem hiding this comment.
You previously had an error case if more than one group has been found. I think you want to put it back here.
if len(grps) > 1 {
return nil, errox.InvalidArgs.Newf("multiple groups found for properties (auth provider id=%s, key=%s, "+
"value=%s), provide an ID to retrieve a group unambiguously",
props.GetAuthProviderId(), props.GetKey(), props.GetValue())
|
/retest |
|
@dhaus67: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
Will merge this now, the UI related tests are also sadly failing within nightlies. |
Description
This is the second PR in a series of PRs regarding removing the composite key from the groups datastore, allowing users to specify different roles for the same group properties.
For full context, you can navigate to the previous PRs by using the comment created below.
This PR will cover the following:
storeinterface and its implementations to use IDs instead ofGroupProperties, as well as changing the key that is used within the bucket from a composite key to the ID.datastoreinterface and its implementations to use IDs instead ofGroupProperties, as well as moving the validation within the datastore instead of the service layer (similar to how other datastores behave currently).servicelayer to use IDs instead of a composite key created out of theGroupProperties.Checklist
If any of these don't apply, please comment below.
Testing Performed