deletes pipelines on space removal #2352

hrishin · 2018-11-21T06:22:34Z

Pipelines don't get deleted while removing a space
which leaves dangling pipelines in OpenShift.

This patch deletes the pipelines while removing a
space

Patch adds

REST API to delete pipeline by space label
Test cases for delete pipeline(buildconfig) method
Delete BuildConfig API in OpenShift client
Test cases for delete BuildConfig API
Refactors kubernetes_client to get client objects
Fixes delete OpenShift resources tests

Fixes
-openshiftio/openshift.io#3802

hrishin · 2018-11-21T07:38:07Z

force pushed by mistake :/

nurali-techie · 2018-11-21T10:49:24Z

controller/space.go

+
+	//delete pipelines from the space
+	spaceLabel := space.Data.Attributes.Name
+	log.Debug(ctx, map[string]interface{}{"label": spaceLabel}, "deleting pipelines in")


seems error msg is not complete ??

nurali-techie · 2018-11-21T10:58:39Z

controller/space.go

+
+	if err != nil {
+		log.Error(ctx, nil, fmt.Sprintf("error occurred while deleting pipelines from space : %s", spaceLabel))
+	}


we need to return error here .. right ??

Initially, I assumed it a failure of this call should not fail delete space considering .

I will fix this.

design/pipelines.go

controller/pipelines.go

sbose78 · 2018-11-21T12:58:37Z

controller/platform_clients.go

+
+	// The deployments API communicates with the rest of WIT via the stnadard WIT API.
+	// This environment variable is used for local development of the deployments API, to point ot a remote WIT.
+	witURLStr := os.Getenv("FABRIC8_WIT_API_URL")


Shouldn't read directly from env, should come from the viper config object ( configuration/configuration.go )

sbose78 · 2018-11-21T13:03:48Z

controller/platform_clients.go

+// ClientGetter creates an instances of clients used by this controller
+type ClientGetter interface {
+	GetKubeClient(ctx context.Context) (kubernetes.KubeClientInterface, error)
+	GetAndCheckOSIOClient(ctx context.Context) (OpenshiftIOClient, error)


Should we really be using osio further in our identifiers given that there's gonna be changes :)

sbose78 · 2018-11-21T13:09:46Z

controller/platform_clients.go

+)
+
+// ClientGetter creates an instances of clients used by this controller
+type ClientGetter interface {


Could you use a more meaningful name please?

This is not something that should be part of the controller layer. This should be a part of the service layer. Could you maybe put in a different new/existing package?

Yes, we tried to decouple it from the controller layer but stumbled into the extensive refactoring. Though it's better to do progressive refactoring.

GetAndCheckOSIOClient is tightly coupled to the controller layer.

sbose78 · 2018-11-21T13:11:01Z

controller/platform_clients.go

+				"FABRIC8_WIT_API_URL": witURLStr,
+				"err": err,
+			}, "cannot parse FABRIC8_WIT_API_URL: %s", witURLStr)
+			return nil, errs.Wrapf(err, "cannot parse FABRIC8_WIT_API_URL: %s", witURLStr)


Could you please use a more specific error from the errors package. That way, jsonapi.JSONErrorResponse(ctx, err) will convert it into the right status code.

sbose78 · 2018-11-21T13:14:15Z

Btw, in follow-up PRs, do you plan to remove 'apps' from -stage and -run as well while deleting a space :) ?

hrishin · 2018-11-21T16:37:18Z

Thank you 👍 @nurali-techie @sbose78 for initial review.

Btw, in follow-up PRs, do you plan to remove 'apps' from -stage and -run as well while deleting a space

Its already taken care by existing code https://github.com/fabric8-services/fabric8-wit/blob/master/controller/space.go#L359
Is that what you mean @sbose78 ?

controller/space.go

pipelines doesnt get deleted while removing a space which leaves dangling pipelines in OpenShift. This patch deletes the pipelines while removing a space Patch adds - REST API to delete pipeline by space label - Test cases for delete pipeline(buildconfig) function - Delete BuildConfig API in OpenShift client - Test cases for delete BuildConfig API - Refactors kubernetes_client to get client objects - Fixes delete OpenShift resources tests Fixes -openshiftio/openshift.io#3802

- updates an API to delete by space id - updates an API URL - fixes test cases

codecov · 2018-11-29T03:05:45Z

Codecov Report

Merging #2352 into master will decrease coverage by 0.31%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master    #2352      +/-   ##
==========================================
- Coverage   70.18%   69.86%   -0.32%     
==========================================
  Files         171      173       +2     
  Lines       16919    17056     +137     
==========================================
+ Hits        11874    11916      +42     
- Misses       3893     3980      +87     
- Partials     1152     1160       +8

Impacted Files	Coverage Δ
controller/deployments.go	`70.2% <ø> (+14.35%)`	⬆️
controller/platform_clients.go	`0% <0%> (ø)`
controller/pipelines.go	`0% <0%> (ø)`
controller/space.go	`69.88% <100%> (+0.23%)`	⬆️
kubernetes/deployments_kubeclient.go	`71.81% <66.66%> (-0.2%)`	⬇️
workitem/workitem_repository.go	`68.02% <0%> (+0.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7fb836...0b1e507. Read the comment docs.

nurali-techie · 2018-11-29T06:53:03Z

controller/pipelines.go

+	}
+	if k8sSpace == nil {
+		return jsonapi.JSONErrorResponse(ctx, errors.NewNotFoundError("namespace", "user"))
+	}


should be 500 rather 404 ??

if resource not found then it should be 404 right?

My thinking is like .. user ask to delete_space .. if space not found then 404 is fine but here it's namespace and if that not found then returning 404 gives the impression that space_not_found. so to me namesapce_not_found it's internal_error when one is trying to deleting_space. This is feedback comment.

@hrishin I am fine with either 404 or 500.

nurali-techie · 2018-11-29T06:54:53Z

controller/pipelines.go

+		return jsonapi.JSONErrorResponse(ctx, errors.NewNotFoundError("namespace", "user"))
+	}
+
+	osc, err := c.GetOSClient(ctx)


For OSIO client, it's osioClient.. While for OS client, it's osc.. we can have consistency between these names here :-)

nurali-techie · 2018-11-29T06:56:57Z

controller/pipelines.go

+	}
+
+	userNS := *k8sSpace.Name
+	spacename, err := c.getSpaceNameFromSpaceID(ctx.SpaceID)


we can reuse osioClient by passing it to getSpaceNameFromSpaceID().

nurali-techie · 2018-11-29T06:59:41Z

controller/pipelines.go

+
+func (c *PipelinesController) getSpaceNameFromSpaceID(spaceID uuid.UUID) (*string, error) {
+	// use WIT API to convert Space UUID to Space name
+	osioclient, err := c.GetAndCheckOSIOClient(c.Context)


not sure c.Context will do, but it's always better to use request context (here it's DeletePipelinesContext)

nurali-techie · 2018-11-29T07:01:10Z

controller/pipelines.go

+	}
+
+	osioSpace, err := osioclient.GetSpaceByID(c.Context, spaceID)
+	fmt.Printf("spcae %#v", osioSpace)


plz remove/replace fmt; check space text.

nurali-techie · 2018-11-29T07:23:22Z

controller/pipelines.go

+}
+
+// Delete a pipelines from given space
+func (c *PipelinesController) Delete(ctx *app.DeletePipelinesContext) error {


plz add tests for Delete().

nurali-techie · 2018-11-29T07:40:41Z

controller/space.go

 	if len(errorsList) != 0 {
 		var errString string
 		for _, err = range errorsList {
 			errString += fmt.Sprintf("%s\n", err)
 		}
 		return errors.NewInternalErrorFromString(errString)
 	}
+
+	//delete pipelines from the space


I guess it's better to extract delete_pipleline logic out from deleteOpenShiftResource() ..

.. because it will make delete_pipeline test clean test_link. These tests has nothing to do with deployment.

It can be refactor like:

deleteOpenShiftResource() { deleteDeployments() deletePipelines() } deleteDeployments() { } deletePipelines() { }

nurali-techie · 2018-11-29T07:42:24Z

controller/space_test.go

-		require.NoError(t, err)
+		t.Run("with delete pipeline success", func(t *testing.T) {
+			// given
+			r, err := recorder.New("../test/data/deployments/deployments_delete_space.ok")


it's better to have separate dir for pipeline rather using deployments dir to keep test files.

nurali-techie · 2018-11-29T09:48:00Z

controller/platform_clients.go

+}
+
+// GetOSClient creates a OpenShift client for the appropriate cluster assigned to the current user
+func (g *defaultClientGetter) GetOSClient(ctx context.Context) (kubernetes.OpenShiftRESTAPI, error) {


impl of GetOSClient() share lot of common impl from GetKubeClient() above. Can we refactor and have one common method to do common stuff.

nurali-techie

@hrishin it would be good if you can do some refactoring and add tests.

ppitonak · 2019-01-04T12:25:13Z

@hrishin @michaelkleinhenz what is the next step with this PR?

michaelkleinhenz · 2019-01-04T12:29:45Z

@ppitonak let's coordinate with @kwk on Monday. He was also working on delete spaces.

hrishin · 2019-01-04T12:31:42Z

@ppitonak will update this PR soon, got sidetrack from this work.

Thanks, @michaelkleinhenz for heads up, will sync with @kwk.

hrishin requested a review from ebaron November 21, 2018 06:22

hrishin force-pushed the delete-pipeline branch from 97c79c9 to 165ab7a Compare November 21, 2018 06:23

hrishin requested review from xcoulon, baijum and nurali-techie November 21, 2018 06:23

nurali-techie reviewed Nov 21, 2018

View reviewed changes

design/pipelines.go Outdated Show resolved Hide resolved

sbose78 reviewed Nov 21, 2018

View reviewed changes

controller/pipelines.go Outdated Show resolved Hide resolved

sbose78 reviewed Nov 21, 2018

View reviewed changes

hrishin added the work-in-progress label Nov 22, 2018

alien-ike removed the work-in-progress label Nov 22, 2018

kwk reviewed Nov 22, 2018

View reviewed changes

controller/space.go Outdated Show resolved Hide resolved

hrishin added 4 commits November 29, 2018 08:25

fixes gofmt

41dd555

Fixes review comments

cc41ea4

- updates an API to delete by space id - updates an API URL - fixes test cases

fixes gofmt

0b1e507

hrishin force-pushed the delete-pipeline branch from 3e1585a to 0b1e507 Compare November 29, 2018 02:57

nurali-techie reviewed Nov 29, 2018

View reviewed changes

nurali-techie suggested changes Nov 29, 2018

View reviewed changes

michaelkleinhenz mentioned this pull request Jan 4, 2019

Space Deletion Function is not reliably deleting OpenShift resources openshiftio/openshift.io#4657

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deletes pipelines on space removal #2352

deletes pipelines on space removal #2352

hrishin commented Nov 21, 2018 •

edited

Loading

hrishin commented Nov 21, 2018

nurali-techie Nov 21, 2018

nurali-techie Nov 21, 2018

hrishin Nov 29, 2018

sbose78 Nov 21, 2018

sbose78 Nov 21, 2018

hrishin Nov 21, 2018

sbose78 Nov 21, 2018

hrishin Nov 21, 2018

hrishin Nov 22, 2018

sbose78 Nov 21, 2018

sbose78 commented Nov 21, 2018

hrishin commented Nov 21, 2018 •

edited

Loading

codecov bot commented Nov 29, 2018 •

edited

Loading

nurali-techie Nov 29, 2018

hrishin Jan 10, 2019

nurali-techie Jan 11, 2019

nurali-techie Jan 11, 2019

nurali-techie Nov 29, 2018

nurali-techie Nov 29, 2018

nurali-techie Nov 29, 2018

nurali-techie Nov 29, 2018

nurali-techie Nov 29, 2018

nurali-techie Nov 29, 2018

nurali-techie Nov 29, 2018

nurali-techie Nov 29, 2018

nurali-techie left a comment

ppitonak commented Jan 4, 2019

michaelkleinhenz commented Jan 4, 2019

hrishin commented Jan 4, 2019

deletes pipelines on space removal #2352

Are you sure you want to change the base?

deletes pipelines on space removal #2352

Conversation

hrishin commented Nov 21, 2018 • edited Loading

hrishin commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 commented Nov 21, 2018

hrishin commented Nov 21, 2018 • edited Loading

codecov bot commented Nov 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nurali-techie left a comment

Choose a reason for hiding this comment

ppitonak commented Jan 4, 2019

michaelkleinhenz commented Jan 4, 2019

hrishin commented Jan 4, 2019

hrishin commented Nov 21, 2018 •

edited

Loading

hrishin commented Nov 21, 2018 •

edited

Loading

codecov bot commented Nov 29, 2018 •

edited

Loading