fix(apps): Report task result status properly for app ops. (spinnaker…

…#2387)
fkyc · Aug 22, 2018 · 745f78f · 745f78f
1 parent 25d84fa
commit 745f78f
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 24 deletions.
diff --git a/...rc/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy b/...rc/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy
@@ -16,15 +16,19 @@
 
 package com.netflix.spinnaker.orca.applications.tasks
 
+import com.netflix.spinnaker.orca.ExecutionStatus
+import com.netflix.spinnaker.orca.TaskResult
 import com.netflix.spinnaker.orca.front50.model.Application
 import com.netflix.spinnaker.orca.front50.tasks.AbstractFront50Task
+import groovy.util.logging.Slf4j
 import org.springframework.stereotype.Component
 import retrofit.RetrofitError
 
+@Slf4j
 @Component
 class DeleteApplicationTask extends AbstractFront50Task {
   @Override
-  Map<String, Object> performRequest(Application application) {
+  TaskResult performRequest(Application application) {
     Map<String, Object> outputs = [:]
 
     try {
@@ -36,18 +40,20 @@ class DeleteApplicationTask extends AbstractFront50Task {
           front50Service.deletePermission(application.name)
         } catch (RetrofitError re) {
           if (re.response?.status == 404) {
-            return [:]
+            return new TaskResult(ExecutionStatus.SUCCEEDED, [:], [:])
           }
-          throw re
+          log.error("Could not create or update application permission", re)
+          return new TaskResult(ExecutionStatus.TERMINAL, [:], outputs)
         }
       }
     } catch (RetrofitError e) {
       if (e.response?.status == 404) {
-        return [:]
+        return new TaskResult(ExecutionStatus.SUCCEEDED, [:], [:])
       }
-      throw e
+      log.error("Could not create or update application permission", e)
+      return new TaskResult(ExecutionStatus.TERMINAL, [:], outputs)
     }
-    return outputs
+    return new TaskResult(ExecutionStatus.SUCCEEDED, [:], outputs)
   }
 
   @Override

diff --git a/...rc/main/groovy/com/netflix/spinnaker/orca/applications/tasks/UpsertApplicationTask.groovy b/...rc/main/groovy/com/netflix/spinnaker/orca/applications/tasks/UpsertApplicationTask.groovy
@@ -17,6 +17,8 @@
 package com.netflix.spinnaker.orca.applications.tasks
 
 import com.netflix.spinnaker.fiat.model.resources.Permissions
+import com.netflix.spinnaker.orca.ExecutionStatus
+import com.netflix.spinnaker.orca.TaskResult
 import com.netflix.spinnaker.orca.front50.model.Application
 import com.netflix.spinnaker.orca.front50.tasks.AbstractFront50Task
 import groovy.transform.CompileStatic
@@ -29,7 +31,7 @@ import retrofit.RetrofitError
 @CompileStatic
 class UpsertApplicationTask extends AbstractFront50Task {
   @Override
-  Map<String, Object> performRequest(Application application) {
+  TaskResult performRequest(Application application) {
     Map<String, Object> outputs = [:]
     outputs.previousState = [:]
 
@@ -38,28 +40,29 @@ class UpsertApplicationTask extends AbstractFront50Task {
      */
 
     def existingApplication = fetchApplication(application.name)
-    if (existingApplication) {
-      outputs.previousState = existingApplication
-      log.info("Updating application (name: ${application.name})")
-      front50Service.update(application.name, application)
-    } else {
-      log.info("Creating application (name: ${application.name})")
-      front50Service.create(application)
-      if (application.permission?.permissions == null) {
-        application.setPermissions(Permissions.EMPTY)
+    try {
+      if (existingApplication) {
+        outputs.previousState = existingApplication
+        log.info("Updating application (name: ${application.name})")
+        front50Service.update(application.name, application)
+      } else {
+        log.info("Creating application (name: ${application.name})")
+        front50Service.create(application)
+        if (application.permission?.permissions == null) {
+          application.setPermissions(Permissions.EMPTY)
+        }
       }
-    }
 
-    try {
       if (application.permission?.permissions != null) {
         front50Service.updatePermission(application.name, application.permission)
       }
     } catch (RetrofitError re) {
       log.error("Could not create or update application permission", re)
+      return new TaskResult(ExecutionStatus.TERMINAL, [:], outputs)
     }
 
     outputs.newState = application ?: [:]
-    outputs
+    return new TaskResult(ExecutionStatus.SUCCEEDED, [:], outputs)
   }
 
   @Override

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/ExecutionStatus.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/ExecutionStatus.java
@@ -19,6 +19,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Optional;
 
 public enum ExecutionStatus {
   /**
@@ -105,7 +106,6 @@ public final boolean isHalt() {
 
   private static final Collection<ExecutionStatus> SUCCESSFUL = Collections.unmodifiableList(Arrays.asList(SUCCEEDED, STOPPED, SKIPPED));
   private static final Collection<ExecutionStatus> FAILURE = Collections.unmodifiableList(Arrays.asList(TERMINAL, STOPPED, FAILED_CONTINUE));
-
   private final boolean complete;
   private final boolean halt;
 

diff --git a/...ont50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/AbstractFront50Task.groovy b/...ont50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/AbstractFront50Task.groovy
@@ -34,7 +34,7 @@ abstract class AbstractFront50Task implements Task {
   @Autowired
   ObjectMapper mapper
 
-  abstract  Map<String, Object> performRequest(Application application)
+  abstract  TaskResult performRequest(Application application)
   abstract String getNotificationType()
 
   @Override
@@ -62,10 +62,10 @@ abstract class AbstractFront50Task implements Task {
       "notification.type": getNotificationType(),
       "application.name": application.name
     ]
-    def executionStatus = ExecutionStatus.SUCCEEDED
+    TaskResult taskResult = performRequest(application)
+    outputs << taskResult.outputs
 
-    Map<String, Object> results = performRequest(application)
-    return new TaskResult(executionStatus, outputs + (results ?: [:]))
+    return new TaskResult(taskResult.status, outputs)
   }
 
   Application fetchApplication(String applicationName) {