feat(doNoEval): default doNotEval SPeL expression to On #1028

ciurescuraul · 2023-03-21T08:48:57Z

This turns "doNotEval" expression to ON by default.

References PR #978

ovidiupopa07

lgtm

...ssions/src/main/java/com/netflix/spinnaker/kork/expressions/config/ExpressionProperties.java

dbyron-sf

Please update the PR description and commit message to feat(doNoEval) since this is a behavior change. But then also, what's the reasoning behind this change? Please include that as well in the PR description + commit message.

And then finally, I don't think it's quite time to do this. This release note says the flag is new for 1.30, but 1.30 hasn't been released yet. Let's wait until it has before merging this.

ovidiupopa07 · 2023-03-21T16:25:07Z

Please update the PR description and commit message to feat(doNoEval) since this is a behavior change. But then also, what's the reasoning behind this change? Please include that as well in the PR description + commit message.

And then finally, I don't think it's quite time to do this. This release note says the flag is new for 1.30, but 1.30 hasn't been released yet. Let's wait until it has before merging this.

hi @dbyron-sf . The initial PR was back-ported to 1.28 and 1.29. The intention here is to enable this feature by default in 1.30 instead of manually opting in for it. As per the RFC we should enable a feature in the next LTS (since it was back-ported to previous LTS releases)

dbyron-sf · 2023-03-21T17:55:43Z

Please update the PR description and commit message to feat(doNoEval) since this is a behavior change. But then also, what's the reasoning behind this change? Please include that as well in the PR description + commit message.
And then finally, I don't think it's quite time to do this. This release note says the flag is new for 1.30, but 1.30 hasn't been released yet. Let's wait until it has before merging this.

hi @dbyron-sf . The initial PR was back-ported to 1.28 and 1.29. The intention here is to enable this feature by default in 1.30 instead of manually opting in for it. As per the RFC we should enable a feature in the next LTS (since it was back-ported to previous LTS releases)

OK, it helps to know the feature has already been released. I don't see where spinnaker/governance#303 says "we should enable a feature in the next LTS", but even if it does, I'd still like to know the specific reason for doing it for this flag. Like, what are the consequences for changing this flag on existing spinnaker users/installations/pipelines/etc. ? Who is it going to break vs. who is it going to help. Or perhaps put another way, I'd like to see the release note describing this change.

As a bit of an aside, spinnaker/governance#303 does say

We propose that this change take effect immediately in the backport
policy and relevant contributor documentation be updated to describe
the facilities for feature flagging in the project.

but I don't think https://spinnaker.io/docs/community/contributing/code/releasing/#release-branch-patch-criteria has been updated.

I would like the “doNotEval” SPeL expression feature flag on, so that I can more accurately control the processing of SPeL expression (particularly when processing Terraform output data).

...ssions/src/main/java/com/netflix/spinnaker/kork/expressions/config/ExpressionProperties.java

…on, so that we can more accurately control the processing of SPeL expression (particularly when processing Terraform output data). * This feature is disabled only when the flag is set explicitly to false. *The "doNotEval" SpEL helper makes it possible to skip SpEL evaluation in other SpEL helpers e.g. toJson * One reason is that a SpEL expression failure appears while using Terraformer to serialize data from a Terraform Plan execution. The execution creates a JSON object instead of a JSON string. * The cause is that the SpEL processor in Spinnaker throws a null value if the SpEL expression has ${ in it * Also, if the users are using an older Terraform version (<= 0.12), then it is also using the template provider from Terraform. * HashiCorp has deprecated the template provider since v0.12 * Users should use the template file function instead

...ssions/src/main/java/com/netflix/spinnaker/kork/expressions/config/ExpressionProperties.java

dbyron-sf · 2023-03-22T15:38:49Z

Thanks for all the updates. I'm still curious what, if anything, could break as a result of this default changing.

…on, so that we can more accurately control the processing of SPeL expression (particularly when processing Terraform output data). * This feature is disabled only when the flag is set explicitly to false. * The "doNotEval" SpEL helper makes it possible to skip SpEL evaluation in other SpEL helpers e.g. toJson * One reason is that a SpEL expression failure appears while using Terraformer to serialize data from a Terraform Plan execution. The execution creates a JSON object instead of a JSON string. * The cause is that the SpEL processor in Spinnaker throws a null value if the SpEL expression has ${ in it * Also, if the users are using an older Terraform version (<= 0.12), then it is also using the template provider from Terraform. * HashiCorp has deprecated the template provider since v0.12 * Users should use the template file function instead

ciurescuraul · 2023-03-23T09:37:19Z

Thanks for all the updates. I'm still curious what, if anything, could break as a result of this default changing.

@dbyron-sf it's complex to say what can be broken because it's a part of SpEL. Our customers may write on SpEL everything. My task was to enable this feature and I guess Armory product managers are fine with possible issues caused by this change. I guess the worst issue that could happen is the pipeline will not work as expected.

dbyron-sf · 2023-03-23T19:49:44Z

I guess the worst issue that could happen is the pipeline will not work as expected.

This is a bit of an unsatisfying answer, but with SpEL expressions it may be the best we can do. It does look like https://spinnaker.io/docs/releases/next-release-preview/#donoteval-spel-helper could use a tweak when this PR goes in. Would you mind putting up a PR with those changes? At that point I'm OK with this going in, though I still think it may not make 1.30.0.

ovidiupopa07 · 2023-03-24T08:39:26Z

I guess the worst issue that could happen is the pipeline will not work as expected.

This is a bit of an unsatisfying answer, but with SpEL expressions it may be the best we can do. It does look like https://spinnaker.io/docs/releases/next-release-preview/#donoteval-spel-helper could use a tweak when this PR goes in. Would you mind putting up a PR with those changes? At that point I'm OK with this going in, though I still think it may not make 1.30.0.

spinnaker/spinnaker.io#317 -> PR for the docs

Co-authored-by: root <root@09edb544a130>

dbyron-sf · 2023-03-30T13:19:25Z

Now that 1.30.0 is out, spinnaker/spinnaker.io#317 could use an update @ovidiupopa07 , and I think this is good to go.

ciurescuraul · 2023-03-30T16:46:12Z

Created new PR with updated docs for Release 1.31 -> spinnaker/spinnaker.io#324

dbyron-sf · 2023-03-30T17:27:50Z

@Mergifyio update

mergify · 2023-03-30T17:27:53Z

update

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@armory-io needs to authorize modification on its head branch.
err-code: AE990

dbyron-sf · 2023-03-30T17:28:49Z

Looks like this needs a rebase, but otherwise it looks good to go to me. You agree @ovidiupopa07 ? I pushed the button too fast and merged spinnaker/spinnaker.io#324 already...

I would like the “doNotEval” SPeL expression feature flag on, so that I can more accurately control the processing of SPeL expression (particularly when processing Terraform output data).

…on, so that we can more accurately control the processing of SPeL expression (particularly when processing Terraform output data). * This feature is disabled only when the flag is set explicitly to false. *The "doNotEval" SpEL helper makes it possible to skip SpEL evaluation in other SpEL helpers e.g. toJson * One reason is that a SpEL expression failure appears while using Terraformer to serialize data from a Terraform Plan execution. The execution creates a JSON object instead of a JSON string. * The cause is that the SpEL processor in Spinnaker throws a null value if the SpEL expression has ${ in it * Also, if the users are using an older Terraform version (<= 0.12), then it is also using the template provider from Terraform. * HashiCorp has deprecated the template provider since v0.12 * Users should use the template file function instead

…on, so that we can more accurately control the processing of SPeL expression (particularly when processing Terraform output data). * This feature is disabled only when the flag is set explicitly to false. * The "doNotEval" SpEL helper makes it possible to skip SpEL evaluation in other SpEL helpers e.g. toJson * One reason is that a SpEL expression failure appears while using Terraformer to serialize data from a Terraform Plan execution. The execution creates a JSON object instead of a JSON string. * The cause is that the SpEL processor in Spinnaker throws a null value if the SpEL expression has ${ in it * Also, if the users are using an older Terraform version (<= 0.12), then it is also using the template provider from Terraform. * HashiCorp has deprecated the template provider since v0.12 * Users should use the template file function instead

…able_spel_expressions

dbyron-sf · 2023-03-30T17:45:22Z

gradle.properties

@@ -1,5 +1,5 @@
 kotlinVersion=1.4.32
 org.gradle.parallel=true
-spinnakerGradleVersion=8.25.0
+spinnakerGradleVersion=8.26.0


This seems fishy as the HEAD of master is already using 8.26.0.

This seems fishy as the HEAD of master is already using 8.26.0.

Created new PR to eliminate any doubt -> #1030

Bummer...then we lose all the context. Sure there's no way to bring this back to life?

Didn't realize at the moment what the impact would be.
It doesn't seem to let me reopen this one, the new PR #1030 has a reference to the old PR #1028. I hope this helps

chore(feature-flag): default doNotEval SPeL expression to On

6bde3cf

ovidiupopa07 approved these changes Mar 21, 2023

View reviewed changes

dbyron-sf reviewed Mar 21, 2023

View reviewed changes

...ssions/src/main/java/com/netflix/spinnaker/kork/expressions/config/ExpressionProperties.java Outdated Show resolved Hide resolved

dbyron-sf requested changes Mar 21, 2023

View reviewed changes

ciurescuraul changed the title ~~chore(feature-flag): default doNotEval SPeL expression to On~~ feat(doNoEval): default doNotEval SPeL expression to On Mar 21, 2023

feat(doNoEval): default doNotEval SPeL expression to On

8e0c1ef

I would like the “doNotEval” SPeL expression feature flag on, so that I can more accurately control the processing of SPeL expression (particularly when processing Terraform output data).

ovidiupopa07 requested changes Mar 22, 2023

View reviewed changes

...ssions/src/main/java/com/netflix/spinnaker/kork/expressions/config/ExpressionProperties.java Outdated Show resolved Hide resolved

dbyron-sf reviewed Mar 22, 2023

View reviewed changes

...ssions/src/main/java/com/netflix/spinnaker/kork/expressions/config/ExpressionProperties.java Show resolved Hide resolved

ciurescuraul requested review from dbyron-sf and ovidiupopa07 and removed request for dbyron-sf and ovidiupopa07 March 27, 2023 08:54

chore(dependencies): Autobump spinnakerGradleVersion (spinnaker#1029)

7e6e0d4

Co-authored-by: root <root@09edb544a130>

dbyron-sf approved these changes Mar 30, 2023

View reviewed changes

ciurescuraul added 2 commits March 30, 2023 20:35

chore(feature-flag): default doNotEval SPeL expression to On

29bc523

feat(doNoEval): default doNotEval SPeL expression to On

bb29c3e

I would like the “doNotEval” SPeL expression feature flag on, so that I can more accurately control the processing of SPeL expression (particularly when processing Terraform output data).

ciurescuraul added 3 commits March 30, 2023 20:35

Merge remote-tracking branch 'origin/enable_spel_expressions' into en…

ca50177

…able_spel_expressions

dbyron-sf reviewed Mar 30, 2023

View reviewed changes

ciurescuraul closed this Mar 30, 2023

ciurescuraul mentioned this pull request Mar 31, 2023

feat(doNoEval): Default the “doNotEval” SPeL expression to ON #1030

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(doNoEval): default doNotEval SPeL expression to On #1028

feat(doNoEval): default doNotEval SPeL expression to On #1028

ciurescuraul commented Mar 21, 2023 •

edited

Loading

ovidiupopa07 left a comment

dbyron-sf left a comment

ovidiupopa07 commented Mar 21, 2023 •

edited

Loading

dbyron-sf commented Mar 21, 2023

dbyron-sf commented Mar 22, 2023

ciurescuraul commented Mar 23, 2023 •

edited

Loading

dbyron-sf commented Mar 23, 2023

ovidiupopa07 commented Mar 24, 2023

dbyron-sf commented Mar 30, 2023

ciurescuraul commented Mar 30, 2023

dbyron-sf commented Mar 30, 2023

mergify bot commented Mar 30, 2023

dbyron-sf commented Mar 30, 2023

dbyron-sf Mar 30, 2023

ciurescuraul Mar 30, 2023

dbyron-sf Mar 30, 2023

ciurescuraul Mar 31, 2023

feat(doNoEval): default doNotEval SPeL expression to On #1028

feat(doNoEval): default doNotEval SPeL expression to On #1028

Conversation

ciurescuraul commented Mar 21, 2023 • edited Loading

ovidiupopa07 left a comment

Choose a reason for hiding this comment

dbyron-sf left a comment

Choose a reason for hiding this comment

ovidiupopa07 commented Mar 21, 2023 • edited Loading

dbyron-sf commented Mar 21, 2023

dbyron-sf commented Mar 22, 2023

ciurescuraul commented Mar 23, 2023 • edited Loading

dbyron-sf commented Mar 23, 2023

ovidiupopa07 commented Mar 24, 2023

dbyron-sf commented Mar 30, 2023

ciurescuraul commented Mar 30, 2023

dbyron-sf commented Mar 30, 2023

mergify bot commented Mar 30, 2023

❌ Pull request can't be updated with latest base branch changes

dbyron-sf commented Mar 30, 2023

dbyron-sf Mar 30, 2023

Choose a reason for hiding this comment

ciurescuraul Mar 30, 2023

Choose a reason for hiding this comment

dbyron-sf Mar 30, 2023

Choose a reason for hiding this comment

ciurescuraul Mar 31, 2023

Choose a reason for hiding this comment

ciurescuraul commented Mar 21, 2023 •

edited

Loading

ovidiupopa07 commented Mar 21, 2023 •

edited

Loading

ciurescuraul commented Mar 23, 2023 •

edited

Loading