From a65d34f986d97f7a9a22aca4edfd898b694cbd2a Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Mon, 9 Oct 2023 23:55:52 +0000
Subject: [PATCH] fix(front50): teach MonitorPipelineTask to handle
 missing/null execution ids (#4555) (#4556)

give the task terminal status in this case since that feels closer to the truth than
succeeded even though the error is likely elsewhere (e.g. in a StartPipelineTask).

(cherry picked from commit 7523f5dc539cabb9e1da290b1a43f081da95ddb9)

Co-authored-by: David Byron <82477955+dbyron-sf@users.noreply.github.com>
---
 .../front50/tasks/MonitorPipelineTask.groovy  | 12 +++++-
 .../tasks/MonitorPipelineTaskSpec.groovy      | 40 ++++++++++++++++++-
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTask.groovy b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTask.groovy
index 2e42af5466..574e23d363 100644
--- a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTask.groovy
+++ b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTask.groovy
@@ -54,12 +54,20 @@ class MonitorPipelineTask implements OverridableTimeoutRetryableTask {
     MonitorPipelineStage.StageParameters stageData = stage.mapTo(MonitorPipelineStage.StageParameters.class)
 
     if (stage.type == MonitorPipelineStage.PIPELINE_CONFIG_TYPE) {
-      pipelineIds = stageData.executionIds
+      // Use - null to remove null elements
+      pipelineIds = stageData.executionIds ? stageData.executionIds - null : Collections.emptyList()
     } else {
-      pipelineIds = Collections.singletonList(stageData.executionId)
+      pipelineIds = stageData.executionId ? Collections.singletonList(stageData.executionId) : Collections.emptyList()
       isLegacyStage = true
     }
 
+    if (pipelineIds.size() == 0) {
+      String message = "no pipeline execution ids to monitor";
+      log.info(message)
+      stage.appendErrorMessage(message)
+      return TaskResult.ofStatus(ExecutionStatus.TERMINAL)
+    }
+
     HashMap<String, MonitorPipelineStage.ChildPipelineStatusDetails> pipelineStatuses = new HashMap<>(pipelineIds.size())
     PipelineExecution firstPipeline
 
diff --git a/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTaskSpec.groovy b/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTaskSpec.groovy
index d830b2fb3e..c6d39800ae 100644
--- a/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTaskSpec.groovy
+++ b/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/tasks/MonitorPipelineTaskSpec.groovy
@@ -28,6 +28,7 @@ import spock.lang.Specification
 import spock.lang.Subject
 import spock.lang.Unroll
 
+import static com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType.PIPELINE
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.pipeline
@@ -36,13 +37,14 @@ import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.stage
 class MonitorPipelineTaskSpec extends Specification {
 
   ExecutionRepository repo = Mock(ExecutionRepository)
-  StageExecutionImpl stage = new StageExecutionImpl(type: "whatever")
+  StageExecutionImpl stage = new StageExecutionImpl()
 
   @Subject
   MonitorPipelineTask task = new MonitorPipelineTask(repo, new ObjectMapper())
 
   def setup() {
     stage.context.executionId = 'abc'
+    stage.type = 'whatever'
   }
 
   @Unroll
@@ -283,4 +285,40 @@ class MonitorPipelineTaskSpec extends Specification {
     MonitorPipelineStage.MonitorBehavior.FailFast               || ExecutionStatus.TERMINAL
     MonitorPipelineStage.MonitorBehavior.WaitForAllToComplete   || ExecutionStatus.RUNNING
   }
+
+  @Unroll
+  def "handles a missing execution id (stage type: #stageType)"() {
+    given:
+    stage.context.remove('executionId')
+    stage.type = stageType
+
+    when:
+    task.execute(stage)
+
+    then:
+    noExceptionThrown()
+    0 * repo.retrieve(PIPELINE, _)
+
+    where:
+    stageType << [ MonitorPipelineStage.PIPELINE_CONFIG_TYPE, PipelineStage.PIPELINE_CONFIG_TYPE ]
+  }
+
+  @Unroll
+  def "handles a null execution id (stage type: #stageType)"() {
+    given:
+    stage.context.executionId = null
+    stage.context.executionIds = [null]
+    stage.type = stageType
+
+    when:
+    task.execute(stage)
+
+    then:
+    noExceptionThrown()
+    0 * repo.retrieve(PIPELINE, _)
+
+    where:
+    stageType << [ MonitorPipelineStage.PIPELINE_CONFIG_TYPE, PipelineStage.PIPELINE_CONFIG_TYPE ]
+  }
+
 }