Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

initial clean-up of deployment API #2395

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

corinnekrych
Copy link
Contributor

@corinnekrych corinnekrych commented Jan 18, 2019

fixes https://openshift.io/openshiftio/Openshift_io/plan/detail/1315

  • remove deployment api
  • use latest dep (v0.5.0)

@corinnekrych corinnekrych changed the title initial clean-up of deployment API WIP initial clean-up of deployment API Jan 18, 2019
@corinnekrych corinnekrych force-pushed the osio1315.removal.of.deployement.api branch from c633a4e to 9c5c0cb Compare January 21, 2019 15:46
@corinnekrych corinnekrych changed the title WIP initial clean-up of deployment API initial clean-up of deployment API Jan 22, 2019
Copy link
Contributor

@xcoulon xcoulon left a comment

Choose a reason for hiding this comment

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

this was a huge "chainsaw" operation, @corinnekrych!

@corinnekrych
Copy link
Contributor Author

this was a huge "chainsaw" operation, @corinnekrych!

Indeed! ✂️ 💇

// now delete the OpenShift resources associated with this space on an
// OpenShift cluster, unless otherwise specified
if ctx.SkipCluster == nil || !*ctx.SkipCluster {
err = deleteOpenShiftResource(c.DeploymentsClient, config, ctx.Context, spaceID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How are deployments deleted if not through the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, indeed another services will have to deal with deployment resources.
This is the purpose of this PR remove deployment out of wit.
In an event-driven world, an event sent for delete space would triggered a subscribed deployment service.

Copy link
Member

Choose a reason for hiding this comment

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

The deleteOpenshiftResource method fails a lot of times and we already have an issue to fix it openshiftio/openshift.io#4657. Since @kwk will be implementing an improved version of space deletion (in future), I'm in favour of deleting this method.

Copy link
Collaborator

@kwk kwk left a comment

Choose a reason for hiding this comment

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

That is impressive: bildschirmfoto von 2019-01-22 15-07-02

@sbose78
Copy link
Member

sbose78 commented Jan 22, 2019

@corinnekrych , could you please verify that on the UI side, this API is really not used ( experimental / non experimental views ) ?

@corinnekrych
Copy link
Contributor Author

corinnekrych commented Jan 24, 2019

to be merged after fabric8-ui/fabric8-ui#3502
@sbose78 ^^ here is the PR to remove any call to WIT deployment API

@corinnekrych
Copy link
Contributor Author

@sbose78 @jarifibrahim since UI PR is merged 🎉 could we have another review on this one and potentially merge it?

Copy link
Member

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning up the code :)

// now delete the OpenShift resources associated with this space on an
// OpenShift cluster, unless otherwise specified
if ctx.SkipCluster == nil || !*ctx.SkipCluster {
err = deleteOpenShiftResource(c.DeploymentsClient, config, ctx.Context, spaceID)
Copy link
Member

Choose a reason for hiding this comment

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

The deleteOpenshiftResource method fails a lot of times and we already have an issue to fix it openshiftio/openshift.io#4657. Since @kwk will be implementing an improved version of space deletion (in future), I'm in favour of deleting this method.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants