Refactor MasterUserRecordController and tests to use only propagated claims #943
Conversation
sbryzak
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19 and
mfrancisc
as code owners
| IdentityClaims: toolchainv1alpha1.IdentityClaimsEmbedded{ | ||
| PropagatedClaims: toolchainv1alpha1.PropagatedClaims{ | ||
| Email: "[email protected]", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Why do we actually need to add the email when:
- there was no email set before
- in the toolchain-common PR you mentioned that you want to modify MUR and UserAccount only, not UserSignup
There was a problem hiding this comment.
I know that without the email address being set here, the black box mystery that is the metrics tests will fail. I've spent some time trying to work out why this is the case, and my only suspicion is that it's related to the email domain function.
There was a problem hiding this comment.
Talking about notifications - I see that the e2e tests are failing for the idling notifications. My guess is that it's because the idler controller is still using the email annotation for sending the notification. You need to update that part first before dropping it from the MUR:
https://github.com/codeready-toolchain/member-operator/blob/f4220407cbae99fa7d3713504fef5f7dbd196ace/controllers/idler/idler_controller.go#L270-L271
There was a problem hiding this comment.
@MatousJobanek thanks for pointing this out, I wasn't aware of the idler controller or what it did. I've updated it to now use the propagated claims.
| func GetEmailDomain(obj RuntimeObject) Domain { | ||
| emailAddress, exists := obj.GetAnnotations()[toolchainv1alpha1.UserSignupUserEmailAnnotationKey] | ||
| if !exists { | ||
| log.Error(nil, "no email address found in annotations", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName()) | ||
| emailAddress := "" | ||
|
|
||
| switch obj := obj.(type) { | ||
| case *toolchainv1alpha1.MasterUserRecord: | ||
| emailAddress = obj.Spec.PropagatedClaims.Email | ||
| case *toolchainv1alpha1.UserSignup: | ||
| emailAddress = obj.Spec.IdentityClaims.Email | ||
| } |
There was a problem hiding this comment.
if you change the type of the parameter of the function to take the PropagatedClaims, then it would simplify the logic and it would be also clear what the function does - where the value takes from. There is no need for checking the types.
func GetEmailDomain(claims toolchainv1alpha1.PropagatedClaims) Domain {
emailAddress := claims.Email
There was a problem hiding this comment.
This is a little simpler yes, however it would also mean that we lose details in the logs if the email address is not found. We cannot determine the object type or the name from just the PropagatedClaims.
log.Error(nil, "no email address found in object", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName())
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #943 +/- ##
==========================================
- Coverage 83.81% 83.71% -0.10%
==========================================
Files 53 53
Lines 6159 6128 -31
==========================================
- Hits 5162 5130 -32
- Misses 814 815 +1
Partials 183 183
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeykazakov, MatousJobanek, rajivnathan, sbryzak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR refactors the MasterUserRecordController to only use propagated claims when creating UserAccount etc.
Related toolchain-common PR: codeready-toolchain/toolchain-common#345
Fixes https://issues.redhat.com/browse/SANDBOX-482
e2e tests: codeready-toolchain/toolchain-e2e#862