Refactor chectl to make tasks reusable for different commands #257

sleshchenko · 2019-08-21T09:29:42Z

What does this PR do?

This PR has two major purposed:

Refactor chectl to make tasks reusable for different commands;
Implement an ability to start already deployed but stopped Che.

The main idea of refactoring is the following:
Have the following different packages:

api <- holds everything that might be reused while working with different APIs BUT not tasks, cli stuff here
tasks <- holds tasks that are devided into different classes by components
| - installers
| - platforms
| che
commands <- holds commands of chectl, command should reuse tasks from tasks package or it can declare it's own unique small tasks

The main pros of this refactoring: encapsulate logic related to some component to one class,
like currently, all commands read flags['che-namespace'] everytime when they need to do something with che components (find ingress, scale deployment) but it could be encapsulated in one che-tasks class.

Note that this PR also contains changes how chectl check if there is already deployed Che, after that - with them it's possible to start already deployed but stopped Che.

This PR also contains a lot of small fixes but I did no manage to highlight them and move to separate commits, like:

platform parameter for server:start command is required only when there is no already Deployed Che;
server:stop now stops devfile and plugin registry as well as Che, Postgres and Keycloak;
print a warning that multiuser is automatically deployed when che-operator is used;
minishift addon is updated to use Deployment instead of DeploymentConfig, so chectl get rid of calling DC methods;

Is this PR tested?

I made sure that E2E tests are not broken but there are not much covered cases yet.

I tried to cover start-stop-start scenario with E2E tests https://github.com/sleshchenko/chectl/compare/startStopStartScenario...sleshchenko:startStopStartScenarioE2E?expand=1
but they failed because of the current implementation of api/kube.ts#waitUntilPodIsDeleted method, it says that it waits until the pod is deleted but in fact, it waits until pod is not ready. It causes an issue that server:start gets two Che pods intead of expected 1. I'll register a separate issue for it.

I did manual testing and here I'm posting detailed results. It may be useful to review them as well since it shows what is the actual output of chectl:

No Platform Specified

Screenshot

Minishift

Minishift Addon

server:start --platform=minishift

server:stop

server:start

server:delete

Che-operator

server:start --platform=minishift --installer=operator

server:stop

server:start (does not work correctly because of https://github.com/eclipse-che/che/issues/14445)

server:delete

Minikube

Helm Single User

server:start --platform=minikube

server:stop

server:start

server:delete

Helm multiuser

server:start --platform=minikube --multiuser

server:stop

server:start

server:delete

CRC

server:start --platform=openshift --installer=operator

server:stop

server:start (does not work correctly because of https://github.com/eclipse-che/che/issues/14445)

server:delete

What issues does this PR fix or reference?

It's done in the scope of the following issue eclipse-che/che#14465

It depends on minishift/minishift#3333

benoitf

I'm fine after 7.1 release

benoitf · 2019-09-02T08:09:19Z

but you need to fix CI build and semantic commits

sleshchenko · 2019-09-02T08:14:13Z

@benoitf Sure. This PR rare is not ready for merging. Hopefully, I'll continue testing and fixing this PR after finishing console link issue.

sleshchenko · 2019-09-06T08:45:09Z

Update, it's pretty ready to review. TODO list

// TODO Che Deployment depends on Postgres and Keycloak. TAKE CARE OF THIS https://github.com/che-incubator/chectl/pull/257/files#diff-0c0044e814885b268bec876f26c37a87R240
Clean up commits - separate unrelated changes to make review easier, make semantic commits
Make sure E2E tests works (maybe add new ones not sure that I'll have enough time to do it)

sleshchenko · 2019-09-10T14:09:56Z

@benoitf @l0rd Now I believe It's ready to review. Sorry that I did not manage to separate changes into different self-explaining commits, but it needs a lot of time because I made a lot of changes after actual refactoring.

benoitf · 2019-09-10T14:21:44Z

src/api/kube.ts

    const agent = new https.Agent({
      rejectUnauthorized: false
    })
    let endpoint = ''
    try {
      endpoint = `${currentCluster.server}/healthz`
-      let response = await axios.get(`${endpoint}`, { httpsAgent: agent, headers: { Authorization: 'bearer ' + token } })
+      let response = await axios.get(`${endpoint}`, { httpsAgent: agent })


why token is removed there ?

Otherwise, the following error appears on minishift when logged as developer

hum but if you remove it there are failures with some k8s clusters....

I can not guarantee.
I only discovered that it does not work on minishift when logged in as non-admin user.
And tested my changes (without token) that is works on minikube(without explicit specifying RBAC enabled) and minishift.

I can test it on like minikube with RBAC specified or OpenShift 4.1

Tested on CRC 4.1 as well

security is for remote, not local dev like minishift, minikube or crc.

eclipse-che/che#13800

@benoitf Thanks for sharing concrete implementation where it fails.
Because on one of OSIO cluster, healthz endpoint is public and it supposed by be secure
https://api.starter-us-east-2.openshift.com/healthz
I'm going to revert it and create a separate issue that there is an issue with healthz if user does not have cluster admin rights.

SDAdham · 2019-09-12T01:40:54Z

Hello everyone, pls let me know once this is released

benoitf

repository shouldn't use sleshchenko ("eclipse-che-minishift": "git://github.com/sleshchenko/minishift#updateChe", )

and token shouldn't be removed in Kube (or be removed only for minishift)

tasks reusable Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

…stry that is used by Che Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

… addon Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

sleshchenko · 2019-09-13T07:53:00Z

repository shouldn't use sleshchenko ("eclipse-che-minishift": "git://github.com/sleshchenko/minishift#updateChe", )

Since my PR to minishift repo is merged, I'm able to do it.
master from minishift repo is used now!

and token shouldn't be removed in Kube (or be removed only for minishift)

I believe this issue may be resolved out of the scope of current PR, reverted that changes (removing of using service account token) and created the corresponding issue eclipse-che/che#14534 to investigate it deeper and fix

sleshchenko requested review from l0rd and benoitf August 21, 2019 09:29

sleshchenko force-pushed the startStopStartScenario branch 3 times, most recently from 7d1e326 to 980c329 Compare August 27, 2019 15:25

benoitf approved these changes Sep 2, 2019

View reviewed changes

sleshchenko force-pushed the startStopStartScenario branch 3 times, most recently from 2c720df to f654110 Compare September 6, 2019 08:36

sleshchenko force-pushed the startStopStartScenario branch from f654110 to d784047 Compare September 6, 2019 10:40

sleshchenko mentioned this pull request Sep 6, 2019

Small fixes #301

Merged

sleshchenko force-pushed the startStopStartScenario branch 7 times, most recently from 8076cd0 to ab2d57b Compare September 9, 2019 14:59

sleshchenko marked this pull request as ready for review September 10, 2019 11:59

sleshchenko requested review from azatsarynnyy, rhopp and vparfonov as code owners September 10, 2019 11:59

sleshchenko force-pushed the startStopStartScenario branch from ab2d57b to a988cdb Compare September 10, 2019 12:01

benoitf reviewed Sep 10, 2019

View reviewed changes

benoitf requested changes Sep 13, 2019

View reviewed changes

sleshchenko added 4 commits September 13, 2019 10:46

feat & refactor: Add an ability to start Che after server:stop && Make

eea8978

tasks reusable Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

fix(minishift-addon): use the same version of plugin and devfile regi…

9a35233

…stry that is used by Che Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

fix: fix plugin registry selector

6b201d5

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

fix: fix init status of Che components after deploying with minishift…

51b8858

… addon Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

sleshchenko force-pushed the startStopStartScenario branch from d28f176 to 51b8858 Compare September 13, 2019 07:47

sleshchenko mentioned this pull request Sep 13, 2019

chectl: k8s health check fails if logged in user does not have cluster admin rights eclipse-che/che#14534

Closed

23 tasks

benoitf approved these changes Sep 13, 2019

View reviewed changes

l0rd approved these changes Sep 13, 2019

View reviewed changes

sleshchenko merged commit b9f6ac7 into che-incubator:master Sep 13, 2019

sleshchenko deleted the startStopStartScenario branch September 13, 2019 10:41

sleshchenko mentioned this pull request Oct 4, 2019

fix: do not fetch service account if it's unneeded #340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor chectl to make tasks reusable for different commands #257

Refactor chectl to make tasks reusable for different commands #257

sleshchenko commented Aug 21, 2019 •

edited

Loading

benoitf left a comment

benoitf commented Sep 2, 2019

sleshchenko commented Sep 2, 2019

sleshchenko commented Sep 6, 2019

sleshchenko commented Sep 10, 2019

benoitf Sep 10, 2019

sleshchenko Sep 10, 2019

benoitf Sep 10, 2019

sleshchenko Sep 11, 2019

sleshchenko Sep 11, 2019

benoitf Sep 13, 2019

sleshchenko Sep 13, 2019

SDAdham commented Sep 12, 2019

benoitf left a comment

sleshchenko commented Sep 13, 2019

Refactor chectl to make tasks reusable for different commands #257

Refactor chectl to make tasks reusable for different commands #257

Conversation

sleshchenko commented Aug 21, 2019 • edited Loading

What does this PR do?

Is this PR tested?

No Platform Specified

Minishift

Minishift Addon

Che-operator

Minikube

Helm Single User

Helm multiuser

CRC

What issues does this PR fix or reference?

benoitf left a comment

Choose a reason for hiding this comment

benoitf commented Sep 2, 2019

sleshchenko commented Sep 2, 2019

sleshchenko commented Sep 6, 2019

sleshchenko commented Sep 10, 2019

benoitf Sep 10, 2019

Choose a reason for hiding this comment

sleshchenko Sep 10, 2019

Choose a reason for hiding this comment

benoitf Sep 10, 2019

Choose a reason for hiding this comment

sleshchenko Sep 11, 2019

Choose a reason for hiding this comment

sleshchenko Sep 11, 2019

Choose a reason for hiding this comment

benoitf Sep 13, 2019

Choose a reason for hiding this comment

sleshchenko Sep 13, 2019

Choose a reason for hiding this comment

SDAdham commented Sep 12, 2019

benoitf left a comment

Choose a reason for hiding this comment

sleshchenko commented Sep 13, 2019

sleshchenko commented Aug 21, 2019 •

edited

Loading