fix(echo events/gate): Echo events will be generated when the pipeline is deleted. #1483
Conversation
| @@ -60,10 +60,39 @@ class PipelineController { | |||
| @Autowired | |||
| ObjectMapper objectMapper | |||
|
|
|||
| @CompileDynamic | |||
There was a problem hiding this comment.
This method has some dynamic properties added in the result at line no. 84....we will be checking the result properties for exception, hence the method has to be made CompileDynamic.
Same will be the case for savePipeline() method.
| @ApiOperation(value = "Delete a pipeline definition") | ||
| @DeleteMapping("/{application}/{pipelineName:.+}") | ||
| void deletePipeline(@PathVariable String application, @PathVariable String pipelineName) { | ||
| pipelineService.deleteForApplication(application, pipelineName) | ||
| List<Map> pipelineConfigs = front50Service.getPipelineConfigsForApplication(application, true) | ||
| if (pipelineConfigs!=null && !pipelineConfigs.isEmpty()){ |
There was a problem hiding this comment.
Please run spotless:apply to fix the formatting
There was a problem hiding this comment.
This has been run already. Still ran this task, but no formatting changes seen.
There was a problem hiding this comment.
I'm not sure you need the isEmpty - it'll just be an empty stream. Agree with Dan though - fetching all pipelines seems odd to only delete one.
| def result = taskService.createAndWaitForCompletion(operation) | ||
| String resultStatus = result.get("status") | ||
|
|
||
| if (!"SUCCEEDED".equalsIgnoreCase(resultStatus)) { |
There was a problem hiding this comment.
This way around is null safe as in if resultStatus is null the check still works. If you write resultStatus.equalsIgnoreCase("SUCCEEDED") you need to check if resultStatus is null first.
|
@Mergifyio update |
|
Command
|
| @ApiOperation(value = "Delete a pipeline definition") | ||
| @DeleteMapping("/{application}/{pipelineName:.+}") | ||
| void deletePipeline(@PathVariable String application, @PathVariable String pipelineName) { | ||
| pipelineService.deleteForApplication(application, pipelineName) | ||
| List<Map> pipelineConfigs = front50Service.getPipelineConfigsForApplication(application, true) |
There was a problem hiding this comment.
Isn't this a potentially expensive operation if there are lots of pipeline configs in an application? Is there a way to ask front50 only for the pipeline config we care about instead of asking for them all and then filtering here?
There was a problem hiding this comment.
Not an expensive operation...there is no front50 api only for the pipeline config we care about.
There was a problem hiding this comment.
I'm still pretty convinced that with a large number of pipelines in an application that this is expensive. Isn't https://github.com/spinnaker/front50/blob/master/front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/PipelineController.java#L136-L141 a way to get the config for one pipeline?
There was a problem hiding this comment.
I am getting the application name and pipeline name from the deck. I am not getting the pipeline id from deck. If we want to use the pipeline id , then the deck api also needs to be changed...
I am pretty much sure and convinced that even if a application has ~ 1000 pipelines, the list scales well enough to get the fast response.
Therefore, i cannot change this api.
|
I'd like to see some tests for the new functionality. |
| @ApiOperation(value = "Delete a pipeline definition") | ||
| @DeleteMapping("/{application}/{pipelineName:.+}") | ||
| void deletePipeline(@PathVariable String application, @PathVariable String pipelineName) { | ||
| pipelineService.deleteForApplication(application, pipelineName) | ||
| List<Map> pipelineConfigs = front50Service.getPipelineConfigsForApplication(application, true) | ||
| if (pipelineConfigs!=null && !pipelineConfigs.isEmpty()){ |
There was a problem hiding this comment.
I'm not sure you need the isEmpty - it'll just be an empty stream. Agree with Dan though - fetching all pipelines seems odd to only delete one.
There are no test cases for save, cancel, resume, etc.... pipeline.... Testcases are added in orca service for this new functionality |
This is part of: spinnaker/spinnaker#6530.
Existing design:
When a delete pipeline request is received in GATE service. GATE authenticates the request and routes it to the front50 service.
This will only delete the pipeline from the database but, events will not be generated.
New design:
==GATE service changes:==
When a delete pipeline request is received in GATE service. GATE authenticates the request and form a task object.
This task object is passed as a parameter to /ops API in ORCA service.
Once submitted GATE service will keep polling the ORCA service to track the task status.
This is exactly the same process followed for other operations like, savePipeline, updatePipeline etc.
==ORCA service changes:==
A new task type "deletePipeline" is created and registered in ORCA.
ORCA service already have the support to process , monitor the task and generate events by routing it to Echo service.
Only changes needed was to register the new task "deletePipeline"
Once the task is registered , ORCA service takes up the responsibility of generating events by routing it to Echo service.
The request will also be routed to Front50 service to actually delete the pipeline.
Rest API:
==GATE service:==
endpoint : /pipelines/{application}/{pipelineName:.+} - DELETE API
==ORCA service:==
endpoint : /ops - POST API
sample request payload:
{ "description": "Delete pipeline pc", "application": "demopc", "job": [{ "type": "deletePipeline", "pipeline": { "keepWaitingPipelines": false, "limitConcurrent": true, "application": "demopc", "spelEvaluator": "v4", "lastModifiedBy": "user2", "name": "pc", "stages": [{ "name": "Wait", "refId": "1", "requisiteStageRefIds": [], "type": "wait", "waitTime": 5.0 }, { "name": "testgate", "parameters": { "connectors": [{ "connectorType": "PRISMACLOUD", "helpText": "PrismaCloud", "isMultiSupported": false, "label": "PrismaCloud", "supportedParams": [{ "helpText": "ImageID", "label": "ImageID", "name": "imageId" }], "values": [{ "imageId": "sha256:390cc7609d3bd3ab1fe8620fbf28d3e9c912c69a901f99e93af7d3b081ff6de2,sha25,sha256:ddbd686f8b5e3d3744443ebbdb0d91284da61f30f78604eabf193e065870f0726:6fed5fb61064c25e91e8afad7e199f20fd1422893bac05fdd2cae274790319fb" }] }], "gateUrl": "https://example.com/xxx/xxx/xxx/xxx/trigger", "imageIds": "nginx:1.14.2" }, "refId": "2", "requisiteStageRefIds": ["1"], "type": "approval" }, { "account": "kubeacc", "cloudProvider": "kubernetes", "manifests": [{ "apiVersion": "apps/v1", "kind": "Deployment", "metadata": { "labels": { "app": "nginx" }, "name": "nginx-deployment" }, "spec": { "replicas": 3.0, "selector": { "matchLabels": { "app": "nginx" } }, "template": { "metadata": { "labels": { "app": "nginx" } }, "spec": { "containers": [{ "image": "nginx:1.14.2", "name": "nginx", "ports": [{ "containerPort": 80.0 }] }] } } } }], "moniker": { "app": "demopc" }, "name": "Deploy (Manifest)", "namespaceOverride": "oes-agent", "refId": "3", "requisiteStageRefIds": ["2"], "skipExpressionEvaluation": false, "source": "text", "trafficManagement": { "enabled": false, "options": { "enableTraffic": false } }, "type": "deployManifest" }], "index": 0.0, "id": "4a5bd769-1887-42ed-a1e1-277417a8e366", "triggers": [], "updateTs": "1628769454000" }, "user": "user2" }] }