Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pipeline): allow mid-task spel evaluation to reference prior stage outputs #3220

Merged
merged 3 commits into from
Oct 9, 2019

Conversation

maggieneterval
Copy link
Contributor

@maggieneterval maggieneterval commented Oct 7, 2019

Closes spinnaker/spinnaker#4932

  • chore(pipeline): add buildExecutionContext test to ContextParameterProcessorSpec

    Adds test to demonstrate that currently, the context returned by ContextParameterProcessor.buildExecutionContext, when used as the context for ContextParameterProcessor.process, does not allow for the evaluation of variables that refer to the outputs of a prior stage.

  • fix(pipeline): allow mid-task SpEL evaluation to reference prior stage outputs

    Changes ContextParameterProcessor.buildExecutionContext to return StageContext, which extends a ForwardingMap in order to override get to search the stage's ancestors' outputs if a given key does not exist on the delegate object. Uses the Map previously returned by this method as the delegate. Removes the includeStageContext argument since all existing consumers want stage context included. Updates ExpressionAware interface to use updated buildExecutionContext per todo.

To do: Investigate whether tasks like ExpressionPreconditionTask even need to explicitly take another pass at expression evaluation, since presumably any resolvable variables were evaluated in the StartTaskHandler. I think only if the task itself fetches an artifact that may contain SpEL should we need another evaluation pass?

true,
summary
)
)

// TODO (mvulfson): Ideally, we opt out of this method and use ContextParameterProcessor.buildExecutionContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ You are brave soul, @maggieneterval, for tackling this :)

Copy link
Contributor

@marchello2000 marchello2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Contributor

@ezimanyi ezimanyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔣🎭📤🎉 💯

@maggieneterval maggieneterval merged commit a5bd525 into spinnaker:master Oct 9, 2019
@maggieneterval maggieneterval deleted the spel-sadness branch October 9, 2019 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants