Remove deleted pipelines from global active pipeline list #34
Conversation
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
==========================================
+ Coverage 33.5% 36.26% +2.76%
==========================================
Files 13 13
Lines 991 1012 +21
==========================================
+ Hits 332 367 +35
+ Misses 622 608 -14
Partials 37 37
Continue to review full report at Codecov.
|
|
Thanks a lot @bidhan-a for your contribution. 🤗 |
pipeline/pipeline.go
Outdated
| // RemoveDeletedPipelines removes the pipelines whose names are NOT | ||
| // present in `existingPipelineNames` from the given ActivePipelines instance. | ||
| func (ap *ActivePipelines) RemoveDeletedPipelines(existingPipelineNames []string) { | ||
| for i, pipeline := range ap.Pipelines { |
There was a problem hiding this comment.
I think it makes sense to use the iter function 🤓: https://github.com/gaia-pipeline/gaia/blob/master/pipeline/pipeline.go#L132
There was a problem hiding this comment.
@michelvocks The iter function doesn't provide a counter so we'd have to introduce a counter variable to get the pipeline index. Would that be okay?
There was a problem hiding this comment.
@bidhan-a Yes that is fine. 😄
There was a problem hiding this comment.
@michelvocks Cool! I'll implement that and let you know.
|
Hey @michelvocks I have encountered an issue. We're using an RLock inside the Iter() function: https://github.com/gaia-pipeline/gaia/blob/master/pipeline/pipeline.go#L136 Similarly, I have added a Lock inside the Remove() function: https://github.com/bidhan-a/gaia/blob/issue-29/pipeline/pipeline.go#L92 If we try to remove pipelines while Iter() is running (https://github.com/bidhan-a/gaia/blob/issue-29/pipeline/pipeline.go#L181), then Any way we can circumvent this? Thanks |
|
Good point @bidhan-a. I think the only viable way to work around this is to copy all elements which you want to delete into an intermediate slice. After you have left the |
|
Sounds good to me @michelvocks ! |
|
All done @michelvocks. I've also included tests for the functions which I've added. Thanks! |
Fixes #29
Let me know your thoughts @michelvocks
Thanks!