fix(cancel): consistently cancel both pipelines and orchestrations. (s…

…pinnaker#2572)
rradkris · Jan 2, 2019 · 1931bd1 · 1931bd1
1 parent edbd7d0
commit 1931bd1
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 14 deletions.
diff --git a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/TaskController.groovy b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/TaskController.groovy
@@ -165,17 +165,15 @@ class TaskController {
   @RequestMapping(value = "/tasks/{id}/cancel", method = RequestMethod.PUT)
   @ResponseStatus(HttpStatus.ACCEPTED)
   void cancelTask(@PathVariable String id) {
-    executionRepository.cancel(ORCHESTRATION, id, AuthenticatedRequest.getSpinnakerUser().orElse("anonymous"), null)
-    executionRepository.updateStatus(ORCHESTRATION, id, ExecutionStatus.CANCELED)
+    cancelExecution(ORCHESTRATION, id)
   }
 
   @PreFilter("hasPermission(this.getOrchestration(filterObject)?.application, 'APPLICATION', 'WRITE')")
   @RequestMapping(value = "/tasks/cancel", method = RequestMethod.PUT)
   @ResponseStatus(HttpStatus.ACCEPTED)
   void cancelTasks(@RequestBody List<String> taskIds) {
     taskIds.each {
-      executionRepository.cancel(ORCHESTRATION, it, AuthenticatedRequest.getSpinnakerUser().orElse("anonymous"), null)
-      executionRepository.updateStatus(ORCHESTRATION, it, ExecutionStatus.CANCELED)
+      cancelExecution(ORCHESTRATION, it)
     }
   }
 
@@ -406,14 +404,7 @@ class TaskController {
   void cancel(
     @PathVariable String id, @RequestParam(required = false) String reason,
     @RequestParam(defaultValue = "false") boolean force) {
-    executionRepository.retrieve(PIPELINE, id).with { pipeline ->
-      executionRunner.cancel(
-        pipeline,
-        AuthenticatedRequest.getSpinnakerUser().orElse("anonymous"),
-        reason
-      )
-    }
-    executionRepository.updateStatus(PIPELINE, id, ExecutionStatus.CANCELED)
+    cancelExecution(PIPELINE, id, reason)
   }
 
   @PreAuthorize("hasPermission(this.getPipeline(#id)?.application, 'APPLICATION', 'WRITE')")
@@ -556,6 +547,21 @@ class TaskController {
     return filterPipelinesByHistoryCutoff(allPipelines, limit)
   }
 
+  private void cancelExecution(ExecutionType executionType, String id) {
+    cancelExecution(executionType, id, null)
+  }
+
+  private void cancelExecution(ExecutionType executionType, String id, String reason) {
+    executionRepository.retrieve(executionType, id).with { execution ->
+      executionRunner.cancel(
+              execution,
+              AuthenticatedRequest.getSpinnakerUser().orElse("anonymous"),
+              reason
+      )
+    }
+    executionRepository.updateStatus(executionType, id, ExecutionStatus.CANCELED)
+  }
+
   private static void validateSearchForPipelinesByTriggerParameters(long triggerTimeStartBoundary, long triggerTimeEndBoundary, int startIndex, int size) {
     if (triggerTimeStartBoundary < 0) {
       throw new IllegalArgumentException(String.format("triggerTimeStartBoundary must be >= 0: triggerTimeStartBoundary=%s", triggerTimeStartBoundary))

diff --git a/orca-web/src/test/groovy/com/netflix/spinnaker/orca/controllers/TaskControllerSpec.groovy b/orca-web/src/test/groovy/com/netflix/spinnaker/orca/controllers/TaskControllerSpec.groovy
@@ -19,6 +19,7 @@ package com.netflix.spinnaker.orca.controllers
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.google.common.collect.Collections2
 import com.netflix.spectator.api.NoopRegistry
+import com.netflix.spinnaker.orca.ExecutionStatus
 import com.netflix.spinnaker.orca.front50.Front50Service
 import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper
 import com.netflix.spinnaker.orca.pipeline.ExecutionRunner
@@ -84,15 +85,23 @@ class TaskControllerSpec extends Specification {
   }
 
   void 'should cancel a list of tasks by id'() {
+    given:
+    def orc1 = Stub(Execution)
+    def orc2 = Stub(Execution)
     when:
     def response = mockMvc.perform(
       put('/tasks/cancel').contentType(MediaType.APPLICATION_JSON).content(objectMapper.writeValueAsString(["id1", "id2"]))
     )
 
     then:
     response.andExpect(status().isAccepted())
-    1 * executionRepository.cancel(ORCHESTRATION, 'id2', _, null)
-    1 * executionRepository.cancel(ORCHESTRATION, 'id1', _, null)
+    1 * executionRepository.retrieve(ORCHESTRATION, 'id2') >> orc2
+    1 * executionRunner.cancel(orc2, 'anonymous', null)
+    1 * executionRepository.updateStatus(ORCHESTRATION, 'id2', ExecutionStatus.CANCELED)
+    1 * executionRepository.retrieve(ORCHESTRATION, 'id1') >> orc1
+    1 * executionRunner.cancel(orc1, 'anonymous', null)
+    1 * executionRepository.updateStatus(ORCHESTRATION, 'id1', ExecutionStatus.CANCELED)
+    0 * _
   }
 
   void 'step names are properly translated'() {