Update common libraries to use PropagatedClaims #345
Conversation
sbryzak
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19 and
mfrancisc
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #345 +/- ##
=======================================
Coverage 76.41% 76.41%
=======================================
Files 43 43
Lines 1997 1997
=======================================
Hits 1526 1526
Misses 424 424
Partials 47 47 |
There was a problem hiding this comment.
the description still contains "WIP" so not sure if it's already ready for review or not.
Anyway, I got a bit lost in the changes you have made and you are planning to do. I'm also not sure when you want to drop the fields from the CRDs.
Apart from that, you missed a few cases:
- the
usersignup.gofile still uses theUserSignupUserEmailAnnotationKey - the same for
notification_builder.go - plus there is also one in the useraccount_test.go
| func UserID(userID string) MurModifier { | ||
| return func(mur *toolchainv1alpha1.MasterUserRecord) error { | ||
| mur.Spec.UserID = userID | ||
| mur.Spec.PropagatedClaims.UserID = userID | ||
| return nil |
There was a problem hiding this comment.
Could you please create corresponding PRs for this PR in:
- host-operator
- member-operator
- registration-service
because for example this function should be used in different cases. IIRC, the original mur.Spec.UserID is now stored as mur.Spec.PropagatedClaims.Sub, so it would be good to see the corresponding changes in the tests.
Also, shouldn't you:
- either create a new function for setting the
Sub - or rename this one including the var names?
There was a problem hiding this comment.
Host operator PR is already here: codeready-toolchain/host-operator#943
I've created two new PRs as requested, I'm not sure what value these have though:
Registration service: codeready-toolchain/registration-service#380
Member operator: codeready-toolchain/member-operator#512
I've also added a new function for setting the Sub
There was a problem hiding this comment.
I've created two new PRs as requested, I'm not sure what value these have though:
well the reason is that you are changing logic of functions that are used in these repositories, so it's good to make sure that the repos are compatible with the changes.
I've also added a new function for setting the Sub
I don't see anything like that in the PR, but still, do you want to keep the changes of the UserID function? See my comment in the host-operator PR - does it make sense to have the function there when changing it's logic doesn't have any impact on the tests. Or I'm maybe just missing some small detail.
| func (a *MasterUserRecordAssertion) HasOriginalSub(sub string) *MasterUserRecordAssertion { | ||
| err := a.loadMasterUserRecord() | ||
| require.NoError(a.t, err) | ||
| assert.Equal(a.t, sub, a.mur.Spec.OriginalSub) | ||
| return a | ||
| } | ||
|
|
There was a problem hiding this comment.
btw, why did you drop this function?
There was a problem hiding this comment.
the description still contains "WIP" so not sure if it's already ready for review or not.
Anyway, I got a bit lost in the changes you have made and you are planning to do. I'm also not sure when you want to drop the fields from the CRDs.
Apart from that, you missed a few cases:
the usersignup.go file still uses the UserSignupUserEmailAnnotationKey the same for notification_builder.go plus there is also one in the useraccount_test.go
The PR is indeed ready for review. Fields won't be dropped from the CRDs until everything has been refactored to only use the PropagatedClaims.
The cases mentioned in usersignup.go and notification_builder.go are not missed, they are not within the scope of this PR and will be updated when the UserSignupController is refactored (these changes are limited to only the MasterUserRecordController).
I've updated useraccount_test.go to perform an equality check of the entire PropagatedClaims instead.
There was a problem hiding this comment.
cool, thanks for the explanation 👍 I was missing the description in the PR - what the PR is changing and what are the next steps.
I've updated useraccount_test.go to perform an equality check of the entire PropagatedClaims instead.
I don't see any changes in the file. Apart from that, it's still using the UserId from spec
|
This is a companion PR to codeready-toolchain/host-operator#943 which refactors the MasterUserRecordController to only use the user's SSO claim values from PropagatedClaims and nowhere else.