Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor deploy package #474

Merged
merged 6 commits into from
Oct 5, 2020
Merged

Refactor deploy package #474

merged 6 commits into from
Oct 5, 2020

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Sep 25, 2020

What does this PR do?

During today's Hack 'n' Hustle I tried quickly contribute to che-operator to move dashboard out of che deployment.
I found it's not easy for a stranger like me to orientate in 34 go files in one folder, so I tried to refactor that in a way where component-specific code(che, postgres, keycloak, ...) live in a separate folder while /deploy package is for common stuff.

The second nightmare I've faced: endpoint exposure logic with copied/pasted pretty many ifs and complicated stuff inside like:

if OpenShift {
  if gateway  
     ...
  else ...
else {
 if gateway
    ...
  else
}

see https://github.com/eclipse/che-operator/blob/master/pkg/deploy/devfile_registry.go#L53
I was close to losing consciousness 馃槰 when I thought that I need to write the same complex code for dashboard deployment exposing.

Screenshot_20200925_141153
-100 lines of redundant complex code 馃樅

I'll try my best to finalize that stuff today, if I don't manage to finish - I hope you'll be able to continue.

I've tested multihost on crc and it seems to works just fine but the thing that definitely needs to be tested - gateway, and I'm going to do it.
Early feedback is welcome.

@openshift-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@tolusha
Copy link
Contributor

tolusha commented Sep 28, 2020

single-host mode doesn't work correctly

chectl server:start --platform openshift --che-operator-cr-patch-yaml patch.yaml --che-operator-image <operator_image> --installer operator

patch.yaml:

apiVersion: org.eclipse.che/v1
kind: CheCluster
metadata:
  name: eclipse-che
spec:
  server:
    serverExposureStrategy: 'single-host'
    singleHostExposureType: 'gateway'

@metlos
Copy link
Contributor

metlos commented Sep 28, 2020

My personal biggest gripe with deploy package is the mixture of "per use-case" files like devfile_registry.go, gateway.go, plugin_registry.go with files containing "common" logic for individual k8s object types, e.g. ingress.go, deployment.go, etc. We seem to share that sentiment :)

The thing is that the per-object-type files are often "poisoned" with logic specific for concrete objects in che with that specific object type, e.g. https://github.com/eclipse/che-operator/blob/master/pkg/deploy/ingress.go#L146.

As you've correctly noticed, there are many places that seem just copy pasted, but if you look closely, there are usually minute details that are different. These might be intentional or just accidental and I personally can't decide which one it is. E.g. I was able to write a common sync logic for many object types in https://github.com/eclipse/che-operator/blob/master/pkg/deploy/gateway.go#L54 which all use the same sync function. In other places of the codebase, that is not the case and sync is implemented for each object type separately.

For example https://github.com/eclipse/che-operator/blob/master/pkg/deploy/deployment.go#L47 and https://github.com/eclipse/che-operator/blob/master/pkg/deploy/role.go#L26 are almost, but not quite, entirely unlike each other, while https://github.com/eclipse/che-operator/blob/master/pkg/deploy/ingress.go#L39 is quite the same, yet different (the sentence is confusing intentionally just to illustrate my mental state while trying to understand those files, bonus points for noticing the literary reference ;) ).

I personally don't know why we approach syncing different object types so drastically differently if the end-goal is the same and the k8s API offers the same capabilities for cruding and diffing all object types. The end result is that the codebase is large, inconsistent and, at least for me, confusing.

Now I don't want to cast blame in any direction, after all I am the author of the sad example you point out in the description. I just want to point out that the operator code, IMHO, has organically outgrown itself and we should think about simplifying/rethinking the code quite radically, learning from our experience writing this iteration.

And a HUGE 馃憤 to you for embarking on that path...

sleshchenko and others added 5 commits October 1, 2020 16:27
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha tolusha marked this pull request as ready for review October 1, 2020 13:29
@sleshchenko
Copy link
Member Author

@tolusha Thanks a lot for finalizing this PR!

@metlos Good job on common sync func for all objects.
Seems if go a bit further in that area it could be reused for almost all objects. Can't guarantee anything about che operator but I'll keep it in my during working on devworkspace controller, if I'm not mistaken we have pretty the same issue there. 馃憤

@@ -0,0 +1,118 @@
//
// Copyright (c) 2012-2019 Red Hat, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, year 2020

@@ -0,0 +1,9 @@
package deploy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add licence header, please.

@@ -0,0 +1,54 @@
//
// Copyright (c) 2012-2019 Red Hat, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor 2020 year

@AndrienkoAleksandr
Copy link
Contributor

@sleshchenko Impressive work!

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndrienkoAleksandr, sleshchenko, tolusha
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@@ -0,0 +1,139 @@
package registry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add licence header, please.

Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@tolusha tolusha merged commit cc93735 into eclipse-che:master Oct 5, 2020
@che-bot che-bot added this to the 7.20 milestone Oct 5, 2020
@sleshchenko sleshchenko deleted the refactor branch February 19, 2021 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants