github-workflows-kt icon indicating copy to clipboard operation
github-workflows-kt copied to clipboard

Env vars in type-safe expressions are serialized incorrectly

Open krzema12 opened this issue 1 year ago • 9 comments

See https://github.com/krzema12/github-actions-kotlin-dsl/actions/workflows/Integration%20tests%20-%20type-safe%20expressions.yaml

The workflow is not valid. .github/workflows/Integration tests - type-safe expressions.yaml (Line: 26, Col: 14): Unexpected symbol: '$GREETING'. Located at position 1 within expression: $GREETING

There wasn't a single successful run. I noticed it by accident. It doesn't produce a failed run within e. g. a PR which is a real bummer.

FYI @jmfayard

krzema12 avatar Aug 11 '22 08:08 krzema12

See https://github.com/krzema12/github-actions-kotlin-dsl/blob/1832f83159d5b4896e313610312e342dbba8525a/.github/workflows/Integration%20tests%20-%20type-safe%20expressions.yaml#L26

Shouldn't it be something like

run: echo ${{ env.GREETING }} ${{ env.FIRST_NAME }}

? (I didn't run it, just the nested dollar sign seems suspicious.)

krzema12 avatar Aug 11 '22 08:08 krzema12

It's an error to use command = "echo " + expr { GREETING } It should be simply command = "echo $GREETING"

See documentation https://krzema12.github.io/github-actions-kotlin-dsl/user-guide/type-safe-expressions/#custom-environment-variables

jmfayard avatar Aug 11 '22 10:08 jmfayard

Actually Contexts.env should be removed to prevent this error https://github.com/krzema12/github-actions-kotlin-dsl/blob/main/library/src/main/kotlin/it/krzeminski/githubactions/dsl/expressions/Contexts.kt#L16

jmfayard avatar Aug 11 '22 10:08 jmfayard

Actually Contexts.env should be removed to prevent this error https://github.com/krzema12/github-actions-kotlin-dsl/blob/main/library/src/main/kotlin/it/krzeminski/githubactions/dsl/expressions/Contexts.kt#L16

But then the documented usage will be broken right?

krzema12 avatar Aug 11 '22 11:08 krzema12

I did a GitHub code search. The syntax ${{ secrets.GITHUB_TOKEN }} or ${{ env.MILESTONE }} is correct. What I have done is broken in this case then, the addition $ shouldn't be there https://github.com/search?q=GITHUB_TOKEN&type=code

Only command = "echo $GREETING" currently works.

jmfayard avatar Aug 11 '22 12:08 jmfayard

Let me adjust the test :point_up:, and as far as I understand, we'd need a change in the lib to fix the {{ [env/secrets].FOO_BAR }} usage right?

krzema12 avatar Aug 11 '22 13:08 krzema12

yep but I fear that would be a breaking change

jmfayard avatar Aug 11 '22 15:08 jmfayard

Not a problem, we're still 0.x :) It's more important to have it done right.

krzema12 avatar Aug 11 '22 15:08 krzema12

@krzema12 can't contribute, going to Germany tomorrow, but I've thought about it.

I have a more fancy solution where EnvContext generate a EnvironmentVariable, where we use a proper type for data class EnvironmentVariable : CharSequence(val name: String) and use a Map<CharSequence, String> instead of Map<String, String>

But there is an obvious solution that does the job

-fun expr(value: String) = "\${{ $value }}"
+fun expr(value: String) = "\${{ ${value.removePrefix("$")} }}"

No documentation or api change necessary then

jmfayard avatar Aug 11 '22 17:08 jmfayard

Done via #455

jmfayard avatar Sep 21 '22 07:09 jmfayard