drop CRTClient abstraction #473

MatousJobanek · 2024-10-02T08:36:22Z

drops the CRTClient abstraction and all the related types.
It uses only the InformerService as the ResourceProvider for getting ifnromation about UserSignups. The InformerService will go away as well, it's just getting harder to make incremental changes 🤯
- This also simplifies some of the unit-tests as we don't have to test two different types of implementation of the ResourceProvider
I introduced a new type called namespaced.Client which holds the Client itself plus the host operator namespace. With this type it's much easier to propagate the Client and the namespace in the services.
It now calls the client directly (instead of relying on the abstraction & types), in those cases where the CRTClient was used.

MatousJobanek · 2024-10-02T08:40:24Z

test/util/service_context.go

+func NewMemberClusterServiceContext(_ *testing.T, cl client.Client) fake.MemberClusterServiceContext {
 	return fake.MemberClusterServiceContext{
-		CrtClient: crtClient,
-		Svcs:      application,
+		NamespacedClient: namespaced.NewClient(cl, commontest.HostOperatorNs),
+		Svcs:             server.NewInClusterApplication(cl, commontest.HostOperatorNs),
 	}
 }


This will go away in one of the following PRs. Just wanted to minimize the number of changes so I left it here for now.

MatousJobanek · 2024-10-02T08:46:46Z

pkg/signup/service/signup_service.go

+	labelSelector := client.MatchingLabels{
+		toolchainv1alpha1.UserSignupStateLabelKey:           toolchainv1alpha1.UserSignupStateLabelValueApproved,
+		toolchainv1alpha1.BannedUserPhoneNumberHashLabelKey: labelValue,
+	}
+	userSignups := &toolchainv1alpha1.UserSignupList{}
+	if err := s.List(gocontext.TODO(), userSignups, client.InNamespace(s.Namespace), labelSelector); err != nil {


Originally, we listed UserSignups that were NOT deactivated nor not-ready.
see:

registration-service/pkg/kubeclient/signup.go

Lines 83 to 87 in 6d3e720

selector := labels.NewSelector()

stateRequirement, err := labels.NewRequirement(crtapi.UserSignupStateLabelKey, selection.NotIn, []string{crtapi.UserSignupStateLabelValueDeactivated, crtapi.UserSignupStateLabelValueNotReady})

if err != nil {

return nil, err

}

This means usersignups that were either approved or banned. Banned user are already covered by the BannedUser CRs, so I decided to simplify the logic and list only the approved ones.

codecov · 2024-10-02T10:38:49Z

Codecov Report

Attention: Patch coverage is 74.19355% with 8 lines in your changes missing coverage. Please review.

Project coverage is 85.43%. Comparing base (d0b1f7d) to head (cff0b63).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/signup/service/signup_service.go	80.00%	1 Missing and 4 partials ⚠️
pkg/server/in_cluster_application.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
+ Coverage   82.47%   85.43%   +2.96%     
==========================================
  Files          42       33       -9     
  Lines        2687     2541     -146     
==========================================
- Hits         2216     2171      -45     
+ Misses        385      283     -102     
- Partials       86       87       +1

Flag	Coverage Δ
unittests	`85.43% <74.19%> (+2.96%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mfrancisc

Looks Good 👍

I have only few minor comments.

pkg/signup/service/signup_service.go

pkg/namespaced/client.go

xcoulon · 2024-10-02T14:30:12Z

pkg/signup/service/signup_service.go

-		if signup.Spec.IdentityClaims.Sub != userID && signup.Spec.IdentityClaims.PreferredUsername != username && !states.Deactivated(signup) { // nolint:gosec
+
+	for _, signup := range userSignups.Items {
+		userSignup := signup


is it needed? IIRC, signup is not a pointer

no, it's not a pointer, but I create a pointer to the userSignup in the next line and since we still use go 1.20 we have to do this re-assignment

added a comment 8df7f52

great, thanks!

xcoulon

thanks for taking my comment into account, @MatousJobanek !

openshift-ci · 2024-10-03T07:36:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc, xcoulon

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:

~~OWNERS~~ [MatousJobanek,alexeykazakov,mfrancisc,xcoulon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sonarcloud · 2024-10-03T08:07:33Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

drop CRT Client abstraction

08d1705

MatousJobanek requested review from xcoulon, alexeykazakov, rajivnathan, ranakan19 and mfrancisc as code owners October 2, 2024 08:36

openshift-ci bot added the approved label Oct 2, 2024

MatousJobanek commented Oct 2, 2024

View reviewed changes

mfrancisc reviewed Oct 2, 2024

View reviewed changes

pkg/signup/service/signup_service.go Show resolved Hide resolved

pkg/namespaced/client.go Show resolved Hide resolved

xcoulon approved these changes Oct 2, 2024

View reviewed changes

mfrancisc approved these changes Oct 2, 2024

View reviewed changes

alexeykazakov approved these changes Oct 2, 2024

View reviewed changes

add comment

8df7f52

xcoulon approved these changes Oct 3, 2024

View reviewed changes

Merge branch 'master' into drop-crtclient

cff0b63

MatousJobanek merged commit c11f3a8 into codeready-toolchain:master Oct 3, 2024
9 of 11 checks passed

MatousJobanek added a commit to MatousJobanek/registration-service that referenced this pull request Oct 3, 2024

drop CRTClient abstraction (codeready-toolchain#473)

3857ef6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop CRTClient abstraction #473

drop CRTClient abstraction #473

MatousJobanek commented Oct 2, 2024 •

edited

Loading

MatousJobanek Oct 2, 2024

MatousJobanek Oct 2, 2024

codecov bot commented Oct 2, 2024 •

edited

Loading

mfrancisc left a comment

xcoulon Oct 2, 2024

MatousJobanek Oct 2, 2024 •

edited

Loading

MatousJobanek Oct 3, 2024

xcoulon Oct 3, 2024

xcoulon left a comment

openshift-ci bot commented Oct 3, 2024

sonarcloud bot commented Oct 3, 2024

	selector := labels.NewSelector()
	stateRequirement, err := labels.NewRequirement(crtapi.UserSignupStateLabelKey, selection.NotIn, []string{crtapi.UserSignupStateLabelValueDeactivated, crtapi.UserSignupStateLabelValueNotReady})
	if err != nil {
	return nil, err
	}

drop CRTClient abstraction #473

drop CRTClient abstraction #473

Conversation

MatousJobanek commented Oct 2, 2024 • edited Loading

MatousJobanek Oct 2, 2024

Choose a reason for hiding this comment

MatousJobanek Oct 2, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

mfrancisc left a comment

Choose a reason for hiding this comment

xcoulon Oct 2, 2024

Choose a reason for hiding this comment

MatousJobanek Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

MatousJobanek Oct 3, 2024

Choose a reason for hiding this comment

xcoulon Oct 3, 2024

Choose a reason for hiding this comment

xcoulon left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 3, 2024

sonarcloud bot commented Oct 3, 2024

Quality Gate passed

MatousJobanek commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

MatousJobanek Oct 2, 2024 •

edited

Loading