fix(manifest): fixes deploy manifest SpEL evaluation toggle

[danielpeach]

Addresses parts of spinnaker/spinnaker#5910

[edgarulg]

LGTM

[link108]

Thank you! LGTM

[dbyron-sf]

This looks great to me too. Is it worth noting that it doesn't do everything that spinnaker/spinnaker#5910 mentions? The base64-encoding and removing the manifest from pipeline context still remain, yes?

I don't remember all the various pieces, but is there some potential followup work either in deck or in the deploy manifest stage (to remove the skip SpEL evaluation checkbox)? Or maybe that's fine and this PR only affects deploy manifest stages?

EDIT: I mean, the PR title does say "deploy manifest" so maybe I'm supposed to figure that last part out on my own...

[danielpeach]

Hi @dbyron-sf, thanks for the feedback.

This PR fixes the SpEL expression toggle in Deck, which doesn't work but did work at some point. There are legitimate cases where you can want SpEL expressions to be evaluated and cases where you don't want them to be evaluated. If we were to start over and re-do this feature it might have been better to use a different SpEL-start character (i.e., not $) when you're inside a manifest.

As far as I know, there aren't any updates to Deck. I actually don't have a ton of context around Maggie's last bullet point regarding Helm overrides. We can keep the bug open if you'd like for that bullet point.

In terms of base64 encoding - that seems to just have been a suggested implementation rather than a requirement. The SpEL v4 changes that Maggie mentions are now in place and are what I'm using to implement this.

It's also true that Maggie mentions spinnaker/spinnaker#5909. I think we should just close spinnaker/spinnaker#5910 and fix 5909 separately. It's true that the manifest is dumped in the stage context a few different times (I think 4 total times) and that in some cases pipeline expressions in those manifests will still be evaluated, but the key fix here is that the manifest that's sent to Clouddriver isn't evaluated if the skipPipelineExpressions flag is on.

[dbyron-sf]

This PR fixes the SpEL expression toggle in Deck, which doesn't work but did work at some point.

Just to be brutally clear, you're talking about the toggle specific to the deploy manifest stage, right?

I'm torn, but leaning towards keeping spinnaker/spinnaker#5910 open because of the helm overrides issue. I don't remember the details of v3 vs v4 expression evaluation, so maybe ignore me and do what you think is right...Maybe the bas64-encoding idea is something that more applies to spinnaker/spinnaker#5909 anyway?

[danielpeach]

Just to be brutally clear, you're talking about the toggle specific to the deploy manifest stage, right?

Yes, the "Expression Evaluation" toggle in the Deploy Manifest stage below:

I've updated the top PR description to reflect that this change doesn't fix all of the issues mentioned in spinnaker/spinnaker#5910.

[danielpeach]

@Mergifyio backport release-1.24.x release-1.25.x release-1.26.x

[mergify]

Command backport release-1.24.x release-1.25.x release-1.26.x: success

Backports have been created

• #4129 fix(manifest): fixes deploy manifest SpEL evaluation toggle (backport #4128) has been created for branch release-1.24.x
• #4130 fix(manifest): fixes deploy manifest SpEL evaluation toggle (backport #4128) has been created for branch release-1.25.x
• #4131 fix(manifest): fixes deploy manifest SpEL evaluation toggle (backport #4128) has been created for branch release-1.26.x

[dbyron-sf]

I've updated the top PR description to reflect that this change doesn't fix all of the issues mentioned in spinnaker/spinnaker#5910.

Thank you!