From db55dea129e3ecb144a05b32713b0972969dc0c8 Mon Sep 17 00:00:00 2001 From: Mark Vulfson Date: Thu, 4 Apr 2019 14:42:25 -0700 Subject: [PATCH] fix(expressions): make sure trigger is a map Some places uses `ContextParameterProcessor.buildExecutionContext` which (correctly) converts the `trigger` in the pipeline to an object (instead of keeping it as `Trigger` class). However, when tasks run, they use `.withMergedContext` which opts to build it's own SpEL executionContext (in `.augmentContext`). This is needed so that fancy look up in ancestor stages works. Fix `.augmentContext` to convert the trigger to an object/map. This addresses issues evaluating expressions like `${trigger.buildNumber ?: #stage('s1').context.buildNumber}`` when no `buildNumber` is present on the `trigger` --- .../pipeline/tasks/EvaluateVariablesTask.java | 5 +++ .../tasks/EvaluateVariablesTaskSpec.groovy | 2 +- .../orca/q/handler/ExpressionAware.kt | 10 ++++- .../orca/q/handler/RunTaskHandlerTest.kt | 42 ++++++++++++++++++- 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/EvaluateVariablesTask.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/EvaluateVariablesTask.java index 6d600a87ef..e7e8a3efb3 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/EvaluateVariablesTask.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/EvaluateVariablesTask.java @@ -36,6 +36,11 @@ public class EvaluateVariablesTask implements Task { public EvaluateVariablesTask() { } + /** + * Copies previously evaluated expressions to the outputs map for consumption by subsequent stages. + * The variables aren't evaluated here because would've been evaluated already by a call to + * e.g. ExpressionAware.Stage#withMergedContext + */ @Nonnull @Override public TaskResult execute(@Nonnull Stage stage) { diff --git a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/tasks/EvaluateVariablesTaskSpec.groovy b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/tasks/EvaluateVariablesTaskSpec.groovy index 94aa7a0e9d..d325fd2b42 100644 --- a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/tasks/EvaluateVariablesTaskSpec.groovy +++ b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/tasks/EvaluateVariablesTaskSpec.groovy @@ -26,7 +26,7 @@ class EvaluateVariablesTaskSpec extends Specification { @Subject task = new EvaluateVariablesTask() - void "Should correctly evaulate variables"() { + void "Should correctly copy evaluated variables"() { setup: def stage = stage { refId = "1" diff --git a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ExpressionAware.kt b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ExpressionAware.kt index a1d2d6f1a8..d8ce10b44c 100644 --- a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ExpressionAware.kt +++ b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ExpressionAware.kt @@ -16,7 +16,10 @@ package com.netflix.spinnaker.orca.q.handler +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.kotlin.convertValue import com.netflix.spinnaker.orca.exceptions.ExceptionHandler +import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper import com.netflix.spinnaker.orca.pipeline.expressions.ExpressionEvaluationSummary import com.netflix.spinnaker.orca.pipeline.expressions.PipelineExpressionEvaluator import com.netflix.spinnaker.orca.pipeline.model.Execution @@ -35,6 +38,9 @@ interface ExpressionAware { val contextParameterProcessor: ContextParameterProcessor val log: Logger get() = LoggerFactory.getLogger(javaClass) + private val mapper: ObjectMapper + get() = OrcaObjectMapper.newInstance() + fun Stage.withMergedContext(): Stage { val evalSummary = ExpressionEvaluationSummary() @@ -130,7 +136,9 @@ interface ExpressionAware { private fun StageContext.augmentContext(execution: Execution): StageContext = if (execution.type == PIPELINE) { - this + mapOf("trigger" to execution.trigger, "execution" to execution) + this + mapOf( + "trigger" to mapper.convertValue>(execution.trigger), + "execution" to execution) } else { this } diff --git a/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/RunTaskHandlerTest.kt b/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/RunTaskHandlerTest.kt index abaa7683f1..a933d6c998 100644 --- a/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/RunTaskHandlerTest.kt +++ b/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/RunTaskHandlerTest.kt @@ -44,7 +44,6 @@ import org.jetbrains.spek.api.dsl.on import org.jetbrains.spek.api.lifecycle.CachingMode.GROUP import org.jetbrains.spek.subject.SubjectSpek import org.threeten.extra.Minutes -import java.lang.RuntimeException import java.time.Duration import kotlin.reflect.jvm.jvmName @@ -1123,6 +1122,47 @@ object RunTaskHandlerTest : SubjectSpek({ } } + describe("can reference non-existent trigger props") { + mapOf( + "\${trigger.type == 'manual'}" to true, + "\${trigger.buildNumber == null}" to true, + "\${trigger.quax ?: 'no quax'}" to "no quax" + ).forEach { expression, expected -> + given("an expression $expression in the stage context") { + val pipeline = pipeline { + stage { + refId = "1" + type = "whatever" + context["expr"] = expression + trigger = DefaultTrigger ("manual") + task { + id = "1" + startTime = clock.instant().toEpochMilli() + } + } + } + val message = RunTask(pipeline.type, pipeline.id, "foo", pipeline.stageByRef("1").id, "1", DummyTask::class.java) + + beforeGroup { + whenever(task.execute(any())) doReturn TaskResult.SUCCEEDED + whenever(repository.retrieve(PIPELINE, message.executionId)) doReturn pipeline + } + + afterGroup(::resetMocks) + + action("the handler receives a message") { + subject.handle(message) + } + + it("evaluates the expression") { + verify(task).execute(check { + assertThat(it.context["expr"]).isEqualTo(expected) + }) + } + } + } + } + given("a reference to deployedServerGroups in the stage context") { val pipeline = pipeline { stage {