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: Remove completelly Devworkspace Che-operator when server:delete is executed #1261

Merged
merged 7 commits into from
Jun 1, 2021

Conversation

flacatus
Copy link
Collaborator

@flacatus flacatus commented May 26, 2021

What does this PR do?

This PR remove remove Chemanager objects after executing server:delete command.

Screenshot/screencast of this PR

image

What issues does this PR fix or reference?

eclipse-che/che#19758

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.

@flacatus flacatus changed the title Remove completelly Devworkspace Che-operator when server:delete is ex… fix: Remove completelly Devworkspace Che-operator when server:delete is executed May 26, 2021
@flacatus
Copy link
Collaborator Author

flacatus commented May 26, 2021

@tolusha @mmorhun @AndrienkoAleksandr
Wdyt about have a common methods for Custom Resources like I have in my PR?(patchCustomResource)?
Methods like getCustomResource, deleteCustomResource... and pass the resourceName, resourceVersion, resourceGroup... In this way we will avoid to have methods for every CR in part

@tolusha
Copy link
Collaborator

tolusha commented May 26, 2021

I don't mind.

@flacatus flacatus force-pushed the remove_dche branch 2 times, most recently from 5e280d8 to c3dcdde Compare June 1, 2021 08:13
src/api/che.ts Outdated Show resolved Hide resolved
src/tasks/installers/operator.ts Outdated Show resolved Hide resolved
flacatus added 3 commits June 1, 2021 10:53
Signed-off-by: Flavius Lacatusu <[email protected]>
Signed-off-by: Flavius Lacatusu <[email protected]>
@flacatus flacatus requested a review from tolusha June 1, 2021 09:02
Signed-off-by: Flavius Lacatusu <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm label Jun 1, 2021
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.

@flacatus I like the generalization in getCustomResource, however I would keep old getCheCluster too with the following implementation:

async getCheCluster(cheNamespace: string) {
  return this.kube.getCustomResource(cheNamespace, CHE_CLUSTER_API_GROUP, CHE_CLUSTER_API_VERSION, CHE_CLUSTER_KIND_PLURAL)
}

That way we won't need to change a lot of places where the old method already used and, more important, the old method is more convenient to use (and it is used relatively often from what I've seen in the changes).
The same is for other methods like getAllCheClusters that are used often (however I see no point in such method for, say, deleteCheCluster as it is used only once).

Signed-off-by: Flavius Lacatusu <[email protected]>
@openshift-ci
Copy link

openshift-ci bot commented Jun 1, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flacatus, mmorhun, tolusha

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 b1a1ca3 into main Jun 1, 2021
@flacatus flacatus deleted the remove_dche branch June 1, 2021 13:58
@che-bot che-bot added this to the 7.32 milestone Jun 14, 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.

4 participants