[WIP] Modify, Delete Pipeline feature (#30) #52

bidhan-a · 2018-07-22T06:43:08Z

I have started working on this.

I've added a new tab pane in the Settings page for managing pipelines. In the backend, I've also added a delete handler which basically does the following: (a) Remove binary, (b) Remove from store, and (c) Remove from the active pipelines list. Is this good enough?

Also, how should we handle the modify/rename thing? Would renaming the binary in the pipelines folder and updating the pipeline in the store & the active pipelines list suffice? Or did you have something else in mind?

Please let me know.

Thanks!

codecov-io · 2018-07-22T06:45:49Z

Codecov Report

Merging #52 into master will increase coverage by 1.09%.
The diff coverage is 68.29%.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
+ Coverage    52.5%   53.59%   +1.09%     
==========================================
  Files          14       14              
  Lines        1118     1196      +78     
==========================================
+ Hits          587      641      +54     
- Misses        472      491      +19     
- Partials       59       64       +5

Impacted Files	Coverage Δ
pipeline/pipeline.go	`90.52% <100%> (+3.38%)`	⬆️
handlers/handler.go	`83.58% <100%> (+0.5%)`	⬆️
store/pipeline.go	`75.96% <30%> (-2.9%)`	⬇️
handlers/pipeline.go	`26.77% <57.77%> (+17.01%)`	⬆️

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 17b7873...d08125e. Read the comment docs.

michelvocks · 2018-07-22T07:35:37Z

Also, how should we handle the modify/rename thing? Would renaming the binary in the pipelines folder and updating the pipeline in the store & the active pipelines list suffice? Or did you have something else in mind?

Yes, I think that's enough for now. I was also thinking about changing the source git repository but then you also have to update credentials etc. So for now renaming should be fine! 🤗

Skarlso

Had some nits about the Go part of this PR. :)

Skarlso · 2018-07-22T08:31:58Z

handlers/pipeline.go

+	}
+
+	// Look up pipeline for the given id
+	var foundPipeline *gaia.Pipeline


There is no need for this to be a reference since you are passing in a copy anyways. Let's keep this on the stack with removing the pointer and the address passing.

I changed it to a reference type as per @michelvocks suggestion.

You can make foundPipeline a pointer. Then you can check here: if foundPipeline == nil

Thoughts @Skarlso ?

You can check if it is initialized by comparing it to an empty struct of the same type. Rather than a pointer which might be stored on the heap.

I guess it doesn't matter much but avoids a nil pointer expedition.

@bidhan-a Sorry. My mistake here. You implementation was actually right. 😅

Skarlso · 2018-07-22T08:36:32Z

handlers/pipeline.go

+	for pipeline := range pipeline.GlobalActivePipelines.Iter() {
+		if pipeline.ID == pipelineID {
+			foundPipeline = &pipeline
+			deletedPipelineIndex = index


Add break here once the pipeline you are looking for is found.

Hey @Skarlso I tried adding break inside Iter() but after doing so the process just freezes. I assume this is why break isn't used here either? https://github.com/gaia-pipeline/gaia/blob/master/pipeline/pipeline.go#L102

Oh beasue iter is a channel. Never mind then. Carry on :-).

Skarlso · 2018-07-22T08:44:01Z

handlers/pipeline.go

+// PipelineDelete accepts a pipeline id and deletes it from the
+// store. It also removes the binary inside the pipeline folder.
+func PipelineDelete(c echo.Context) error {
+	pipelineIDStr := c.Param("pipelineid")


There is no need for this variable to exist. c.Param will return an empty string if the parameter is not found making Atoi fail gracefully. So this can be removed and inlined into Atoi.

I thought so too initially but after seeing this pattern being used in other handler functions, I decided to stick with it. 😄

Skarlso · 2018-07-22T08:45:42Z

handlers/pipeline.go

+	}
+
+	if foundPipeline == nil {
+		return c.String(http.StatusNotFound, err.Error())


There is a errPipelineNotFound error.

Skarlso · 2018-07-22T08:47:20Z

handlers/pipeline.go

+	// Delete pipeline binary
+	err = pipeline.DeleteBinary(*foundPipeline)
+	if err != nil {
+		return c.String(http.StatusInternalServerError, err.Error())


Instead of an internal server error I suggest creating an errPipelineDelete error. Something like, oops sorry, I couldn't delete the binary, maybe permissions are incorrect? Please fix permissions and try again or delete it by hand.. Thoughts?

Skarlso · 2018-07-22T08:49:42Z

handlers/pipeline.go

+	// Delete pipeline from store
+	err = storeService.PipelineDelete(pipelineID)
+	if err != nil {
+		return c.String(http.StatusNotFound, err.Error())


This shouldn't be a StatusNotFound not? Rather an internal error since it's a db operation.

…essages

bidhan-a · 2018-07-23T10:16:23Z

Hey guys @michelvocks @Skarlso

So I've added the implementation for renaming pipelines. Both renaming & deletion seem to be working properly. Please feel free to critique the code and let me know if I should add/remove anything. If all seems to be okay, I'll start writing test cases for the methods which I've added.

Thanks!

michelvocks · 2018-07-23T14:14:01Z

handlers/pipeline.go

 		if err != nil {
 			return c.String(http.StatusInternalServerError, errPipelineRename.Error())
 		}
+
+		// Update pipeline in store
+		err = storeService.PipelinePut(&p)


I think it's better to update the name in foundPipeline and store this object instead of p which we got from the client. For example, a hacker could potentially change something else than the name in the struct and that will be saved too.

michelvocks · 2018-07-23T14:15:03Z

handlers/pipeline.go

+		}
+
+		// Update active pipelines
+		pipeline.GlobalActivePipelines.ReplaceByName(foundPipeline.Name, p)


Same here. I think we should stick to the old object 😃

michelvocks · 2018-07-24T05:52:27Z

Looks really good now. Good job @bidhan-a 🤗 ❤️
Let me know when you are ready with your tests.

Skarlso

Looking good! :-)

bidhan-a · 2018-07-24T06:47:21Z

Thanks @michelvocks and @Skarlso . Will let you know once tests are added. 😃

…ssue-30

bidhan-a · 2018-07-24T11:58:17Z

Hey @michelvocks I've added test cases for all the newly added functions. Let me know if I've missed anything!

michelvocks

LGTM 🤗 Thanks a lot @bidhan-a ❤️

bidhan-a added 2 commits July 21, 2018 12:45

add UI components for pipeline settings

34c57db

implement handler for deleting pipelines

d6ef7e3

change foundPipeline to a pointer

72535cd

Skarlso requested changes Jul 22, 2018

View reviewed changes

bidhan-a added 2 commits July 22, 2018 17:20

add initial implementation for renaming pipelines + fix a few error m…

11d0998

…essages

add handler to update/rename pipeline

af8aed2

michelvocks reviewed Jul 23, 2018

View reviewed changes

update existing pipeline object instead of replacing it with a new one

0f10cd8

Skarlso reviewed Jul 24, 2018

View reviewed changes

bidhan-a added 5 commits July 24, 2018 13:14

Merge branch 'master' of https://github.com/gaia-pipeline/gaia into i…

3457391

…ssue-30

add pipeline tests

caf8060

fix test for GetExecPath

e11c829

update test for PipelinePut

8b15534

add tests for pipeline handlers

d08125e

Skarlso approved these changes Jul 24, 2018

View reviewed changes

michelvocks approved these changes Jul 25, 2018

View reviewed changes

michelvocks merged commit d6a51f1 into gaia-pipeline:master Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Modify, Delete Pipeline feature (#30) #52

[WIP] Modify, Delete Pipeline feature (#30) #52

bidhan-a commented Jul 22, 2018

codecov-io commented Jul 22, 2018 •

edited

Loading

michelvocks commented Jul 22, 2018

Skarlso left a comment

Skarlso Jul 22, 2018

bidhan-a Jul 22, 2018 •

edited

Loading

Skarlso Jul 22, 2018

Skarlso Jul 22, 2018

michelvocks Jul 22, 2018

Skarlso Jul 22, 2018

bidhan-a Jul 22, 2018 •

edited

Loading

Skarlso Jul 22, 2018 •

edited

Loading

Skarlso Jul 22, 2018

bidhan-a Jul 22, 2018 •

edited

Loading

Skarlso Jul 22, 2018

Skarlso Jul 22, 2018

Skarlso Jul 22, 2018

bidhan-a commented Jul 23, 2018

michelvocks Jul 23, 2018

michelvocks Jul 23, 2018 •

edited

Loading

michelvocks commented Jul 24, 2018

Skarlso left a comment

bidhan-a commented Jul 24, 2018 •

edited

Loading

bidhan-a commented Jul 24, 2018

michelvocks left a comment

[WIP] Modify, Delete Pipeline feature (#30) #52

[WIP] Modify, Delete Pipeline feature (#30) #52

Conversation

bidhan-a commented Jul 22, 2018

codecov-io commented Jul 22, 2018 • edited Loading

Codecov Report

michelvocks commented Jul 22, 2018

Skarlso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bidhan-a Jul 22, 2018 • edited Loading

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

bidhan-a Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Skarlso Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bidhan-a Jul 22, 2018 • edited Loading

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

bidhan-a commented Jul 23, 2018

Choose a reason for hiding this comment

michelvocks Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

michelvocks commented Jul 24, 2018

Skarlso left a comment

Choose a reason for hiding this comment

bidhan-a commented Jul 24, 2018 • edited Loading

bidhan-a commented Jul 24, 2018

michelvocks left a comment

Choose a reason for hiding this comment

codecov-io commented Jul 22, 2018 •

edited

Loading

bidhan-a Jul 22, 2018 •

edited

Loading

bidhan-a Jul 22, 2018 •

edited

Loading

Skarlso Jul 22, 2018 •

edited

Loading

bidhan-a Jul 22, 2018 •

edited

Loading

michelvocks Jul 23, 2018 •

edited

Loading

bidhan-a commented Jul 24, 2018 •

edited

Loading