fix(errors): Better error messages

orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/pipeline/BakeStage.groovy

@@ -81,7 +81,7 @@ class BakeStage implements StageDefinitionBuilder {
 
   @CompileDynamic
   Collection<Map<String, Object>> parallelContexts(Stage stage) {
-    Set<String> deployRegions = stage.context.region ? [stage.context.region] as Set<String> : []
+    Set<String> deployRegions = (stage.context.region ? [stage.context.region] : []) as Set<String>
     deployRegions.addAll(stage.context.regions as Set<String> ?: [])
 
     if (!deployRegions.contains("global")) {

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/FindImageFromClusterTask.groovy

@@ -125,10 +125,12 @@ class FindImageFromClusterTask extends AbstractCloudProviderAwareTask implements
 
     // Supplement config with regions from subsequent deploy/canary stages:
     def deployRegions = regionCollector.getRegionsFromChildStages(stage)
+    Set<String> inferredRegions = new HashSet<>()
 
     deployRegions.forEach {
       if (!config.regions.contains(it)) {
         config.regions.add(it)
+        inferredRegions.add(it)
         log.info("Inferred and added region ($it) from deploy stage to FindImageFromClusterTask (executionId: ${stage.execution.id})")
       }
     }
@@ -170,7 +172,7 @@ class FindImageFromClusterTask extends AbstractCloudProviderAwareTask implements
             return [(location): null]
           }
 
-          throw new IllegalStateException("Could not find cluster '$config.cluster' for '$account' in '$location.value'.")
+          throw new IllegalStateException("Could not find cluster '${config.cluster}' for '$account' in '${location.value}'.")
         }
         throw e
       }
@@ -205,14 +207,19 @@ class FindImageFromClusterTask extends AbstractCloudProviderAwareTask implements
           // the image into the target account
           List<Map> defaultImages = oortService.findImage(cloudProvider, searchNames[0] + '*', defaultBakeAccount, null, null)
           resolveFromBaseImageName(defaultImages, missingLocations, imageSummaries, deploymentDetailTemplate, config, stage.execution.id)
-          def stillUnresolved = imageSummaries.findResults { it.value == null ? it.key : null }
-          if (stillUnresolved) {
-            throw new IllegalStateException("Missing images in $stillUnresolved.value")
-          }
-        } else {
-          throw new IllegalStateException("Missing images in $unresolved.value")
+          unresolved = imageSummaries.findResults { it.value == null ? it.key : null }
         }
       }
+
+      if (unresolved) {
+        def errorMessage = "Missing image '${searchNames[0]}' in regions: ${unresolved.value}"
+
+        if (unresolved.value.any {it -> inferredRegions.contains(it)}) {
+          errorMessage = "Missing image '${searchNames[0]}' in regions: ${unresolved.value}; ${inferredRegions} were inferred from subsequent deploy stages"
+        }
+
+        throw new IllegalStateException(errorMessage)
+      }
     }
 
     List<Map> deploymentDetails = imageSummaries.collect { location, summaries ->
@@ -302,7 +309,7 @@ class FindImageFromClusterTask extends AbstractCloudProviderAwareTask implements
     for (Map image : images) {
       for (Location location : missingLocations) {
         if (imageSummaries[location] == null && image.amis && image.amis[location.value]) {
-          log.info("Resolved missing image in '$location.value' with '$image.imageName' (executionId: $executionId)")
+          log.info("Resolved missing image in '${location.value}' with '${image.imageName}' (executionId: $executionId)")
 
           imageSummaries[location] = [
             mkDeploymentDetail((String) image.imageName, (String) image.amis[location.value][0], deploymentDetailTemplate, config)

orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/expressions/ExpressionEvaluationSummary.java

@@ -80,6 +80,14 @@ public String toString() {
     );
   }
 
+  public boolean wasAttempted(String expression) {
+    return attempts.contains(expression);
+  }
+
+  public boolean hasFailed(String expression) {
+    return expressionResult.containsKey(expression);
+  }
+
   public static class Result {
     private String description;
     private Class<?> exceptionType;

orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessor.java

@@ -62,11 +62,21 @@ public ContextParameterProcessor(ContextFunctionConfiguration contextFunctionCon
   }
 
   public Map<String, Object> process(Map<String, Object> source, Map<String, Object> context, boolean allowUnknownKeys) {
+    ExpressionEvaluationSummary summary = new ExpressionEvaluationSummary();
+
+    return process(source, context, allowUnknownKeys, summary);
+  }
+
+  public Map<String, Object> process(
+    Map<String, Object> source,
+    Map<String, Object> context,
+    boolean allowUnknownKeys,
+    ExpressionEvaluationSummary summary) {
+
     if (source.isEmpty()) {
       return new HashMap<>();
     }
 
-    ExpressionEvaluationSummary summary = new ExpressionEvaluationSummary();
     Map<String, Object> result = expressionEvaluator.evaluate(
       source,
       precomputeValues(context),

orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ExpressionAware.kt

@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.orca.q.handler
 
 import com.netflix.spinnaker.orca.exceptions.ExceptionHandler
+import com.netflix.spinnaker.orca.pipeline.expressions.ExpressionEvaluationSummary
 import com.netflix.spinnaker.orca.pipeline.expressions.PipelineExpressionEvaluator
 import com.netflix.spinnaker.orca.pipeline.model.Execution
 import com.netflix.spinnaker.orca.pipeline.model.Execution.ExecutionType.PIPELINE
@@ -36,7 +37,8 @@ interface ExpressionAware {
     get() = LoggerFactory.getLogger(javaClass)
 
   fun Stage.withMergedContext(): Stage {
-    val processed = processEntries(this)
+    val evalSummary = ExpressionEvaluationSummary()
+    val processed = processEntries(this, evalSummary)
     val execution = execution
     this.context = object : MutableMap<String, Any?> by processed {
       override fun get(key: String): Any? {
@@ -60,12 +62,28 @@ interface ExpressionAware {
         return result
       }
     }
+
+    // Clean up errors: since expressions are evaluated multiple times, it's possible that when
+    // they were evaluated before the execution started not all data was available and the evaluation failed for
+    // some property. If that evaluation subsequently succeeds, make sure to remove past error messages from the
+    // context. Otherwise, it's very confusing in the UI because the value is clearly correctly evaluated but
+    // the error is still shown
+    if (hasFailedExpressions()) {
+      val failedExpressions = this.context[PipelineExpressionEvaluator.SUMMARY] as MutableMap<String, *>
+
+      failedExpressions.keys.forEach { expressionKey ->
+        if (evalSummary.wasAttempted(expressionKey) && !evalSummary.hasFailed(expressionKey)) {
+          failedExpressions.remove(expressionKey)
+        }
+      }
+    }
+
     return this
   }
 
   fun Stage.includeExpressionEvaluationSummary() {
     when {
-      PipelineExpressionEvaluator.SUMMARY in this.context ->
+      hasFailedExpressions() ->
         try {
           val expressionEvaluationSummary = this.context[PipelineExpressionEvaluator.SUMMARY] as Map<*, *>
           val evaluationErrors: List<String> = expressionEvaluationSummary.values.flatMap { (it as List<*>).map { (it as Map<*, *>)["description"] as String } }
@@ -76,7 +94,9 @@ interface ExpressionAware {
     }
   }
 
-  fun Stage.hasFailedExpressions(): Boolean = PipelineExpressionEvaluator.SUMMARY in this.context
+  fun Stage.hasFailedExpressions(): Boolean =
+    (PipelineExpressionEvaluator.SUMMARY in this.context) &&
+    ((this.context[PipelineExpressionEvaluator.SUMMARY] as Map<*, *>).size > 0)
 
   fun Stage.shouldFailOnFailedExpressionEvaluation(): Boolean {
     return this.hasFailedExpressions() && this.context.containsKey("failOnFailedExpressions")
@@ -92,11 +112,12 @@ interface ExpressionAware {
       mapOf("details" to mapOf("errors" to mergedErrors))
     }
 
-  private fun processEntries(stage: Stage): StageContext =
+  private fun processEntries(stage: Stage, summary: ExpressionEvaluationSummary): StageContext =
     StageContext(stage, contextParameterProcessor.process(
       stage.context,
       (stage.context as StageContext).augmentContext(stage.execution),
-      true
+      true,
+      summary
     )
     )