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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Improve chectl KubeHelper class and context #1085

Merged
merged 14 commits into from
Feb 25, 2021
Merged

fix: Improve chectl KubeHelper class and context #1085

merged 14 commits into from
Feb 25, 2021

Conversation

flacatus
Copy link
Collaborator

@flacatus flacatus commented Feb 8, 2021

Signed-off-by: Flavius Lacatusu flacatus@redhat.com

What does this PR do?

Improve chectl context and improve kubeHelper class to not initialize kube context in every command.

Ref issue: eclipse-che/che#18684

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

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.

@flacatus flacatus changed the title fix: Create 2 separated context for chectl: global context and k8s co… fix: Create 2 separated context for chectl Feb 8, 2021
…ntext

Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@mmorhun
Copy link
Contributor

mmorhun commented Feb 8, 2021

Having two contexts might be very dengeros, especially with missing typings (ctx has any type)

@flacatus
Copy link
Collaborator Author

flacatus commented Feb 8, 2021

We have 2 methods in k8sCTX
ctx[IS_OPENSHIFT] = await kube.isOpenShift()
ctx[IS_OPENSHIFT4] = await kube.isOpenShift4()
Maybe instead of using them in ctx we can call this method without context

@mmorhun
Copy link
Contributor

mmorhun commented Feb 8, 2021

It is not ideal solution, but maybe we can have sort of inheritance with contexts: initK8SCtx includes usual chectl context.
Also, it would be nice to have some typings that defines common fields and allows arbitrary values too. For extended (K8s) conntext the fields have to be of type | undefined. @flacatus @tolusha WDYT?

@mmorhun
Copy link
Contributor

mmorhun commented Feb 8, 2021

Maybe instead of using them in ctx we can call this method without context

Ok for me, but the call might change from sync to async that is not gonna work in some places (I didn't check, just a concern)

Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@flacatus flacatus changed the title fix: Create 2 separated context for chectl fix: Improve chectl KubeHelper class and context Feb 9, 2021
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@che-incubator che-incubator deleted a comment from openshift-ci bot Feb 9, 2021
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
src/tasks/platforms/api.ts Outdated Show resolved Hide resolved
src/tasks/platforms/api.ts Outdated Show resolved Hide resolved
src/api/kube.ts Outdated Show resolved Hide resolved
src/commands/auth/login.ts Outdated Show resolved Hide resolved
src/api/kube.ts Outdated Show resolved Hide resolved
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@che-incubator che-incubator deleted a comment from openshift-ci bot Feb 23, 2021
src/tasks/platforms/api.ts Outdated Show resolved Hide resolved
src/commands/dashboard/open.ts Outdated Show resolved Hide resolved
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@flacatus
Copy link
Collaborator Author

/retest

1 similar comment
@tolusha
Copy link
Collaborator

tolusha commented Feb 24, 2021

/retest

task: async (ctx: any, task: any) => {
try {
await kube.checkKubeApi()
cli.info(`› Current Kubernetes context: '${await kube.currentContext()}'`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not all the commands use these API tasks. Is it planned to print the message on a subset of commands only?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Basically this task is run only when k8s api is used.

Copy link
Contributor

@mmorhun mmorhun left a comment

Choose a reason for hiding this comment

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

I added a few suggestions for improvements, other than that no more questions from my side.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AndrienkoAleksandr, flacatus, mmorhun, 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

@flacatus flacatus merged commit 6c95e9d into master Feb 25, 2021
@flacatus flacatus deleted the context branch February 25, 2021 07:51
@che-bot che-bot added this to the 7.27 milestone Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants