chore: controller refactoring #963

tolusha · 2021-07-22T12:14:16Z

Signed-off-by: Anatolii Bazko abazko@redhat.com

What does this PR do?

Refactor checluster controller to easier update its status.

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

eclipse-che/che#19978

How to test this PR?

N/A

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

tolusha · 2021-07-22T14:20:27Z

/retest

tolusha · 2021-07-23T09:03:16Z

/retest

tolusha · 2021-07-23T13:11:52Z

/test v7-devworkspace-happy-path

tolusha · 2021-07-29T12:33:23Z

/retest

codecov · 2021-07-29T14:07:04Z

Codecov Report

Merging #963 (2f9b708) into main (a3abf33) will increase coverage by 0.83%.
The diff coverage is 38.56%.

❗ Current head 2f9b708 differs from pull request most recent head 0cd3dd1. Consider uploading reports for the commit 0cd3dd1 to get more accurate results

@@            Coverage Diff             @@
##             main     #963      +/-   ##
==========================================
+ Coverage   50.02%   50.86%   +0.83%     
==========================================
  Files          56       56              
  Lines        6806     6800       -6     
==========================================
+ Hits         3405     3459      +54     
+ Misses       3036     2990      -46     
+ Partials      365      351      -14

Impacted Files	Coverage Δ
pkg/deploy/identity-provider/identity_provider.go	`10.57% <0.00%> (-0.06%)`	⬇️
pkg/deploy/kubernetes_image_puller.go	`0.00% <0.00%> (ø)`
pkg/deploy/tls.go	`12.33% <0.00%> (ø)`
controllers/che/checluster_controller.go	`31.05% <45.73%> (+11.41%)`	⬆️
controllers/che/gateway_permission.go	`21.62% <50.00%> (ø)`
controllers/che/proxy.go	`61.11% <50.00%> (ø)`
controllers/che/workspace_namespace_permission.go	`69.94% <57.14%> (ø)`
controllers/che/create.go	`45.03% <100.00%> (ø)`
pkg/deploy/server/server_configmap.go	`85.50% <0.00%> (-0.22%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3abf33...0cd3dd1. Read the comment docs.

mmorhun · 2021-08-03T08:16:48Z

controllers/che/checluster_controller.go

+// Check Che CR correctness
+func validateCheClusterCR(deployContext *deploy.DeployContext) (*ctrl.Result, error) {
+	if !util.IsTestMode() {
+		if err := ValidateCheCR(deployContext.CheCluster); err != nil {


Can we use this function instead of the wrapper?

mmorhun · 2021-08-03T08:22:17Z

controllers/che/checluster_controller.go

+	return nil, nil
+}
+
+func doneToResult(done bool, forceRequeueIfNotDone bool, err error) (*ctrl.Result, error) {


Do we really need this method?
Please consider alternatives:

Detect reconcile status based on done and error as, for example, here

Return ctrl.Result in place

mmorhun · 2021-08-03T08:23:07Z

controllers/che/checluster_controller.go

+	}
+
+	if forceRequeueIfNotDone {
+		return &ctrl.Result{RequeueAfter: time.Second}, err


ctrl usually stands for control, but here you probably meant controller. Please change the short name.

mmorhun · 2021-08-03T08:23:19Z

controllers/che/checluster_controller.go

+	return &ctrl.Result{}, err
+}
+
+func errToResult(err error) (*ctrl.Result, error) {


The same as above

mmorhun · 2021-08-03T08:28:48Z

pkg/deploy/kubernetes_image_puller.go

@@ -42,19 +42,19 @@ type ImageAndName struct {

 // Reconcile the imagePuller section of the CheCluster CR.  If imagePuller.enable is set to true, install the Kubernetes Image Puller operator and create
 // a KubernetesImagePuller CR.  Add a finalizer to the CheCluster CR.  If false, remove the KubernetesImagePuller CR, uninstall the operator, and remove the finalizer.
-func ReconcileImagePuller(ctx *DeployContext) (reconcile.Result, error) {
+func ReconcileImagePuller(ctx *DeployContext) (*reconcile.Result, error) {


Why do we have to change to pointer for reconcile.Result? In main controller object is passed directly. Moreover it will cause a lot of edit below.

openshift-ci · 2021-08-03T10:40:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndrienkoAleksandr, tolusha

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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: Anatolii Bazko <abazko@redhat.com>

openshift-ci · 2021-08-04T07:11:40Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2021-08-08T04:20:44Z

@tolusha: PR needs rebase.

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.

openshift-ci · 2021-12-17T09:38:42Z

@tolusha: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/v7-stable-to-nightly	`0cd3dd1`	link	`/test v7-stable-to-nightly`
ci/prow/v7-devworkspace-happy-path	`0cd3dd1`	link	`/test v7-devworkspace-happy-path`
ci/prow/v8-e2e-all-namespaces-deployment	`0cd3dd1`	link	true	`/test v8-e2e-all-namespaces-deployment`
ci/prow/v7-e2e-all-namespaces-deployment	`0cd3dd1`	link	true	`/test v7-e2e-all-namespaces-deployment`
ci/prow/v9-single-host-nightly-deployment	`0cd3dd1`	link	true	`/test v9-single-host-nightly-deployment`
ci/prow/v9-multi-host-next-deployment	`0cd3dd1`	link	true	`/test v9-multi-host-next-deployment`
ci/prow/v9-devworkspace-happy-path	`0cd3dd1`	link	true	`/test v9-devworkspace-happy-path`
ci/prow/v9-stable-to-nightly	`0cd3dd1`	link	true	`/test v9-stable-to-nightly`
ci/prow/v9-images	`0cd3dd1`	link	true	`/test v9-images`
ci/prow/v9-ci-index	`0cd3dd1`	link	true	`/test v9-ci-index`
ci/prow/v9-upgrade-stable-to-next	`0cd3dd1`	link	true	`/test v9-upgrade-stable-to-next`
ci/prow/v9-operator-test	`0cd3dd1`	link	true	`/test v9-operator-test`
ci/prow/v8-operator-test	`0cd3dd1`	link	true	`/test v8-operator-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

tolusha requested review from AndrienkoAleksandr, flacatus, mmorhun and nickboldt as code owners July 22, 2021 12:14

tolusha mentioned this pull request Jul 22, 2021

Che Deploy Sprint #204 eclipse-che/che#20085

Closed

24 tasks

tolusha force-pushed the 19978 branch from 6c5c370 to b9e7ef2 Compare July 23, 2021 07:16

tolusha mentioned this pull request Jul 26, 2021

Che Deploy Sprint #205 eclipse-che/che#20194

Closed

31 tasks

tolusha force-pushed the 19978 branch 2 times, most recently from 74b28a6 to 45920a4 Compare July 28, 2021 14:27

mmorhun reviewed Aug 3, 2021

View reviewed changes

AndrienkoAleksandr approved these changes Aug 3, 2021

View reviewed changes

openshift-ci bot assigned AndrienkoAleksandr Aug 3, 2021

openshift-ci bot added the lgtm label Aug 3, 2021

tolusha added 5 commits August 4, 2021 09:23

chore: controller refactoring

3eacfe5

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

fixes

9f4cb9a

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

Fix tests

a3f9168

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

Fixes

6cf0d1b

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

Fix remarks

0cd3dd1

Signed-off-by: Anatolii Bazko <abazko@redhat.com>

tolusha force-pushed the 19978 branch from 2f9b708 to 0cd3dd1 Compare August 4, 2021 07:11

openshift-ci bot removed the lgtm label Aug 4, 2021

openshift-ci bot added the needs-rebase label Aug 8, 2021

tolusha mentioned this pull request Aug 16, 2021

Che Deploy Sprint #206 eclipse-che/che#20302

Closed

19 tasks

tolusha closed this Dec 17, 2021

tolusha deleted the 19978 branch December 17, 2021 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: controller refactoring #963

chore: controller refactoring #963

tolusha commented Jul 22, 2021

tolusha commented Jul 22, 2021

tolusha commented Jul 23, 2021

tolusha commented Jul 23, 2021

tolusha commented Jul 29, 2021

codecov bot commented Jul 29, 2021 •

edited

Loading

mmorhun Aug 3, 2021

mmorhun Aug 3, 2021

mmorhun Aug 3, 2021

mmorhun Aug 3, 2021

mmorhun Aug 3, 2021

openshift-ci bot commented Aug 3, 2021

openshift-ci bot commented Aug 4, 2021

openshift-ci bot commented Aug 8, 2021

openshift-ci bot commented Dec 17, 2021

chore: controller refactoring #963

chore: controller refactoring #963

Conversation

tolusha commented Jul 22, 2021

What does this PR do?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

tolusha commented Jul 22, 2021

tolusha commented Jul 23, 2021

tolusha commented Jul 23, 2021

tolusha commented Jul 29, 2021

codecov bot commented Jul 29, 2021 • edited Loading

Codecov Report

mmorhun Aug 3, 2021

Choose a reason for hiding this comment

mmorhun Aug 3, 2021

Choose a reason for hiding this comment

mmorhun Aug 3, 2021

Choose a reason for hiding this comment

mmorhun Aug 3, 2021

Choose a reason for hiding this comment

mmorhun Aug 3, 2021

Choose a reason for hiding this comment

openshift-ci bot commented Aug 3, 2021

openshift-ci bot commented Aug 4, 2021

openshift-ci bot commented Aug 8, 2021

openshift-ci bot commented Dec 17, 2021

codecov bot commented Jul 29, 2021 •

edited

Loading