Remove unused code for devspace endpoints #476

baijum · 2023-10-04T11:32:48Z

Ref. https://issues.redhat.com/browse/SANDBOX-345

Ref. https://issues.redhat.com/browse/SANDBOX-345 Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

openshift-ci · 2023-10-04T11:32:59Z

Hi @baijum. Thanks for your PR.

I'm waiting for a codeready-toolchain member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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.

mfrancisc · 2023-10-04T13:07:23Z

/ok-to-test

alexeykazakov

Looks good. Thanks!
@rajivnathan could you please take a look?

codecov · 2023-10-04T17:46:58Z

Codecov Report

Merging #476 (2b427c4) into master (70d1c48) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
+ Coverage   82.02%   82.29%   +0.26%     
==========================================
  Files          30       28       -2     
  Lines        3444     3101     -343     
==========================================
- Hits         2825     2552     -273     
+ Misses        474      419      -55     
+ Partials      145      130      -15

Files	Coverage Δ
...ontrollers/memberstatus/memberstatus_controller.go	`94.27% <ø> (-2.35%)`	⬇️
controllers/useraccount/useraccount_controller.go	`83.08% <100.00%> (-1.40%)`	⬇️

(based on golangci-lint error report)

baijum · 2023-10-05T08:22:21Z

/test e2e

baijum · 2023-10-05T09:11:46Z

/test e2e

MatousJobanek · 2023-10-05T12:27:59Z

/retest
infra

MatousJobanek

Thanks a lot for your first contribution @baijum 🙏

@rajivnathan can correct me if I'm wrong but we can drop the entire pkg/che package.

together with that, we can also drop the MemberConfig fields that were there only for this purpose from https://github.com/codeready-toolchain/api/blob/master/api/v1alpha1/memberoperatorconfig_types.go#L77-L101 - this can be done as separate PRs though ;-)

EDIT: I updated the description of the story with additional information

MatousJobanek · 2023-10-05T12:35:48Z

controllers/useraccount/useraccount_controller.go

 		return nil
 	}

-	if config.Che().IsDevSpacesMode() {
-		return r.deleteDevSpacesUser(logger, userAcc)
-	}
-
 	userExists, err := r.CheClient.UserExists(userAcc.Name)
 	if err != nil {
 		return err


We should drop the whole lookupAndDeleteCheUser function, not only the subsections of it.
When you look at the PR that is linked from the story https://github.com/codeready-toolchain/sandbox-sre/pull/1046 it sets

userDeletionEnabled: false

which means that the none of these methods are executed anymore. The parameter is checked at the very beginning and when it's false, then it immediately returns from the method

member-operator/controllers/useraccount/useraccount_controller.go

Lines 667 to 674 in 00d55d9

func (r *Reconciler) lookupAndDeleteCheUser(logger logr.Logger, config membercfg.Configuration, userAcc *toolchainv1alpha1.UserAccount) error {

// If Che user deletion is not required then just return, this is a way to disable this Che user deletion logic since

// it's meant to be a temporary measure until Che is updated to handle user deletion on its own

if !config.Che().IsUserDeletionEnabled() {

logger.Info("Che user deletion is not enabled, skipping it")

return nil

}

Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

…s-unused-code

MatousJobanek · 2023-10-09T13:14:56Z

/ok-to-test

Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

baijum · 2023-10-09T16:25:52Z

/test e2e

rajivnathan

Nice work!

I guess that the following can be removed as well:

controllers/useraccount/useraccount_controller_test.go

cheRoute func
keycloackRoute func
testRoute func
newSecretWithCheAdminCreds func

rajivnathan

Looks good! Thanks for addressing the comments

Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

MatousJobanek · 2023-10-12T12:20:55Z

/ok-to-test

MatousJobanek

Hey, I'm so happy that we are getting rid of this legacy thing.
It's crazy how huge it became during the time.
Thanks a lot for taking care of it @baijum 👍 🥇

MatousJobanek · 2023-10-12T12:27:09Z

controllers/useraccount/useraccount_controller_test.go

@@ -424,14 +422,7 @@ func TestReconcile(t *testing.T) {
 		// given

 		// when the member operator secret exists and has a che admin user configured then che user deletion is enabled


Suggested change

// when the member operator secret exists and has a che admin user configured then che user deletion is enabled

// when

Done. 795fc70

MatousJobanek · 2023-10-12T12:27:43Z

controllers/useraccount/useraccount_controller_test.go

 		// given

 		// when the member operator secret exists and has a che admin user configured then che user deletion is enabled


you could remove this whole test

t.Run("delete che user fails because find che user request failed", func(t *testing.T) {

Done. 795fc70

MatousJobanek · 2023-10-12T12:30:43Z

controllers/useraccount/useraccount_controller_test.go

 func gockTokenSuccess(calls *int) {
 	gock.New(testKeycloakURL).
 		Post("auth/realms/codeready/protocol/openid-connect/token").


I guess that we can delete this gock mock and also the other ones in this file.

Done. 795fc70

openshift-ci · 2023-10-12T12:32:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, baijum, MatousJobanek, rajivnathan

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]

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

Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

sonarqubecloud · 2023-10-12T18:55:43Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
35.9% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

alexeykazakov · 2023-10-13T02:54:24Z

/lgtm

Remove unused code for devspace endpoints

2b4e06c

Ref. https://issues.redhat.com/browse/SANDBOX-345 Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

openshift-ci bot added do-not-merge/work-in-progress needs-ok-to-test labels Oct 4, 2023

baijum marked this pull request as ready for review October 4, 2023 11:47

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 4, 2023

openshift-ci bot requested review from MatousJobanek and michaelkleinhenz October 4, 2023 11:47

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Oct 4, 2023

alexeykazakov approved these changes Oct 4, 2023

View reviewed changes

openshift-ci bot added the approved label Oct 4, 2023

Remove unused functions

0b51cb7

(based on golangci-lint error report)

MatousJobanek reviewed Oct 5, 2023

View reviewed changes

baijum added 2 commits October 9, 2023 16:44

Removed che package

eb30e14

Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

Merge remote-tracking branch 'upstream/master' into devspace-endpoint…

7463db4

…s-unused-code

Fix lint errors

6db4533

Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

Merge branch 'master' into devspace-endpoints-unused-code

fb7601b

rajivnathan reviewed Oct 11, 2023

View reviewed changes

Remove unused code based on review comment

98eec49

rajivnathan approved these changes Oct 11, 2023

View reviewed changes

rajivnathan and others added 2 commits October 11, 2023 20:21

Merge branch 'master' into devspace-endpoints-unused-code

6dbee14

Remove leftover function

2393adf

Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

MatousJobanek approved these changes Oct 12, 2023

View reviewed changes

baijum added 2 commits October 12, 2023 21:32

Remove gock related code

795fc70

Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

Merge branch 'master' into devspace-endpoints-unused-code

2b427c4

openshift-ci bot assigned alexeykazakov Oct 13, 2023

openshift-ci bot added the lgtm label Oct 13, 2023

MatousJobanek merged commit 94559a0 into codeready-toolchain:master Oct 13, 2023
9 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unused code for devspace endpoints #476

Remove unused code for devspace endpoints #476

baijum commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

mfrancisc commented Oct 4, 2023

alexeykazakov left a comment

codecov bot commented Oct 4, 2023 •

edited

Loading

baijum commented Oct 5, 2023

baijum commented Oct 5, 2023

MatousJobanek commented Oct 5, 2023

MatousJobanek left a comment •

edited

Loading

MatousJobanek Oct 5, 2023

MatousJobanek commented Oct 9, 2023

baijum commented Oct 9, 2023

rajivnathan left a comment

rajivnathan left a comment

MatousJobanek commented Oct 12, 2023

MatousJobanek left a comment

MatousJobanek Oct 12, 2023

baijum Oct 12, 2023

MatousJobanek Oct 12, 2023

baijum Oct 12, 2023

MatousJobanek Oct 12, 2023

baijum Oct 12, 2023

openshift-ci bot commented Oct 12, 2023

sonarqubecloud bot commented Oct 12, 2023

alexeykazakov commented Oct 13, 2023

	func (r Reconciler) lookupAndDeleteCheUser(logger logr.Logger, config membercfg.Configuration, userAcc toolchainv1alpha1.UserAccount) error {

	// If Che user deletion is not required then just return, this is a way to disable this Che user deletion logic since
	// it's meant to be a temporary measure until Che is updated to handle user deletion on its own
	if !config.Che().IsUserDeletionEnabled() {
	logger.Info("Che user deletion is not enabled, skipping it")
	return nil
	}

		@@ -424,14 +422,7 @@ func TestReconcile(t *testing.T) {
		// given

		// when the member operator secret exists and has a che admin user configured then che user deletion is enabled

	// when the member operator secret exists and has a che admin user configured then che user deletion is enabled
	// when

Remove unused code for devspace endpoints #476

Remove unused code for devspace endpoints #476

Conversation

baijum commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

mfrancisc commented Oct 4, 2023

alexeykazakov left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

baijum commented Oct 5, 2023

baijum commented Oct 5, 2023

MatousJobanek commented Oct 5, 2023

MatousJobanek left a comment • edited Loading

Choose a reason for hiding this comment

MatousJobanek Oct 5, 2023

Choose a reason for hiding this comment

MatousJobanek commented Oct 9, 2023

baijum commented Oct 9, 2023

rajivnathan left a comment

Choose a reason for hiding this comment

rajivnathan left a comment

Choose a reason for hiding this comment

MatousJobanek commented Oct 12, 2023

MatousJobanek left a comment

Choose a reason for hiding this comment

MatousJobanek Oct 12, 2023

Choose a reason for hiding this comment

baijum Oct 12, 2023

Choose a reason for hiding this comment

MatousJobanek Oct 12, 2023

Choose a reason for hiding this comment

baijum Oct 12, 2023

Choose a reason for hiding this comment

MatousJobanek Oct 12, 2023

Choose a reason for hiding this comment

baijum Oct 12, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Oct 12, 2023

sonarqubecloud bot commented Oct 12, 2023

alexeykazakov commented Oct 13, 2023

codecov bot commented Oct 4, 2023 •

edited

Loading

MatousJobanek left a comment •

edited

Loading