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

Unable to interpolate string - hyphenated variables #104

Closed
mced opened this issue Feb 25, 2020 · 17 comments · Fixed by #287
Closed

Unable to interpolate string - hyphenated variables #104

mced opened this issue Feb 25, 2020 · 17 comments · Fixed by #287
Labels
area/workflow Relating to workflow definitions kind/bug Something isn't working meta/workaround A workaround has been identified. needs-work Extra attention is needed

Comments

@mced
Copy link

mced commented Feb 25, 2020

I'm facing an issue when I'm trying to reference a steps.<id>.outputs.<variable> with an id composed with -.

Run echo ${{ steps.login-ecr.outputs.registry }}
ERRO[0005] Unable to interpolate string 'echo ${{ steps.login-ecr.outputs.registry }}' - [ReferenceError: 'ecr' is not defined] 

Workaround; remove all - in id names.

@cplee
Copy link
Contributor

cplee commented Feb 25, 2020

i was afraid of this :(

Implementing the expression logic in act was interesting. I noticed that if i squinted enough that the syntax looked like javascript, so i used robertkrimen/otto to process it as javascript. Unfortunately, javascript doesn't allow - in variable names, but GH expressions does.

Any ideas on how to fix?

@mced
Copy link
Author

mced commented Feb 25, 2020

From the blogpost I've just read [1] and the releases history, you actually used a parser released by github. Unfortunately actions/workflow-parser is dead. Can you tell me what happened ?

[1] https://github.blog/2019-02-07-an-open-source-parser-for-github-actions/

@cplee
Copy link
Contributor

cplee commented Feb 25, 2020

The first cut of actions was completely different from the GA version. Seems like it was a rewrite to be based on Azure DevOps.

@mced
Copy link
Author

mced commented Feb 25, 2020

You're right, it was based on HashiCorp HCL. Now GA uses yaml syntax.
Thus I'd say to replace otto by an yaml parser, but I won't :)

@cplee
Copy link
Contributor

cplee commented Feb 25, 2020

The problem here isn't the parsing of the workflow files. I am using yaml parser for those (and previously the lib for HCL from github). However, some of the values in the yaml have a custom GitHub expression: https://help.github.com/en/actions/reference/contexts-and-expression-syntax-for-github-actions

That expression is what needs an evaluator, and what I use Otto for

@cplee cplee added kind/bug Something isn't working kind/question Further information is requested labels Feb 27, 2020
@icarcal
Copy link

icarcal commented Feb 27, 2020

Hey guys, I'm having the same problem here :(

As @mced pointed out,for a workaround to this, you can switch all variables with - to _ for example.

The problem is when you have another action that implement output variables, for example: actions/cache

When I check for the steps.yarn_cache.outputs.cache-hit != 'true', I receive the following error:
sh ERRO[0017] Error evaluating expression 'steps.yarn_cache.outputs.cache-hit != 'true'' - ReferenceError: 'hit' is not defined

Great work in this package btw.

@cplee cplee added area/workflow Relating to workflow definitions needs-work Extra attention is needed and removed kind/question Further information is requested labels Feb 28, 2020
@icarcal
Copy link

icarcal commented Feb 29, 2020

Hey guys.
I found a new "workaround" for this.

Looking further into the GA expression docs, there are two ways of writing an expression.
From the GA expression docs:

As part of an expression, you may access context information using one of two syntaxes.
Index syntax: github['sha']
Property dereference syntax: github.sha

Since act uses otto to evaluate all expressions, if we write:
steps.yarn_cache.outputs['cache-hit'] != 'true'
instead of
steps.yarn_cache.outputs.cache-hit != 'true'

GA runner accepts it, otto evaluates it correctly, and we can run it both with act and GA

@cplee
Copy link
Contributor

cplee commented Mar 2, 2020

Thanks @icarcal for calling out this workaround! Unfortunately, I think this is as good as it gets for now.

@mced - are you ok closing this given the workaround?

@cplee cplee added the meta/workaround A workaround has been identified. label Mar 2, 2020
@mced
Copy link
Author

mced commented Mar 2, 2020

I'm ok with the given workarounds, thanks @icarcal @cplee.

@torbjornvatn
Copy link
Contributor

@cplee I found another case where this is an issue and the workaround doesn't work;
When your trying to use someone else's action and it has input parameters with - in them.

name: Test stuff

on:
  - push

jobs:
  build:
    name: Testing Testing
    runs-on: ubuntu-latest
    steps:

    - name: hello
      uses: actions/hello-world-docker-action@master
      with:
        who-to-greet: "World"

which gives the error:

ERRO[0001] Unable to interpolate string '${{ inputs.who-to-greet }}' - [ReferenceError: 'to' is not defined]

@cplee cplee reopened this Mar 6, 2020
@cplee
Copy link
Contributor

cplee commented Mar 6, 2020

Confirmed @torbjornvatn :(

I'm really stumped on this one. I'd really rather not have to implement my own parser for GitHub Actions expression language. Open to suggestions.

@icarcal
Copy link

icarcal commented Mar 6, 2020

@cplee Replace the dot notation with the array notation before evaluate the expression is an option?

@cplee
Copy link
Contributor

cplee commented Mar 6, 2020

ya, that may work @icarcal ...would be a rather interesting regex

@eventualbuddha
Copy link

While I agree it would be somewhat annoying to port it to Golang, the expression parser for it is at least open source. You can find the lexer here, for example. It really doesn't look too bad.

@cplee
Copy link
Contributor

cplee commented Apr 16, 2020

Great find @eventualbuddha - would love some help with this one!

@cplee cplee changed the title Bug, reference to steps.<id>.outputs.<variable> Unable to interpolate string - hyphenated variables Apr 16, 2020
@Shadowfiend
Copy link

Dug a little more at that lexer, looks like the right regex for the actual token would be something like \.([a-zA-Z_][a-zA-Z0-9]*(?:-[a-zA-Z0-9]*)+) (see the legal keyword test for valid keyword contents). Replacing that with .["$1"] should do the trick.

I'll try and do a PR in the next couple of days.

@badouralix
Copy link
Contributor

#286 is quite interesting

Here is the code handling the evaluation of the if-expression at high-level :

// EvalBool evaluates an expression against current run context
func (rc *RunContext) EvalBool(expr string) bool {
if expr != "" {
expr = fmt.Sprintf("Boolean(%s)", rc.ExprEval.Interpolate(expr))
v, err := rc.ExprEval.Evaluate(expr)
if err != nil {
return false
}
log.Debugf("expression '%s' evaluated to '%s'", expr, v)
return v == "true"
}
return true
}


With this setup, we have expr == "steps.bincache.outputs.cache-hit != 'true'".

This is not properly parsed by Interpolate as it is missing the ${{...}} delimiters ( github actions support this feature though ). By default, Interpolate will return the same output as the input.

So we end up with expr == "Boolean(steps.bincache.outputs.cache-hit != 'true')" which is the input of Evaluate
And it fails evaluating because of the js reference error, so we enter this block :

if err != nil {
return false
}

So here EvalBool returns false.


But if we change just a little bit the if-expression with :

-        if: steps.bincache.outputs.cache-hit != 'true'
+        if: ${{ steps.bincache.outputs.cache-hit != 'true' }}

Then Interpolate can parse its input. It calls Evaluate, which fails for the same reason as before. But the error is caught by Interpolate which will return the empty string because of :

if err != nil {
return "", err
}

So here, we end up with expr == Boolean() ( this is different from the previous case ). It means that we reach :

log.Debugf("expression '%s' evaluated to '%s'", expr, v)
return v == "true"

In particular, we got this debug log line :

DEBU[0003] Expression 'Boolean()' evaluated to 'false'

And EvalBool also returns false.

But the reason is different from the previous case, and the fix will likely be sightly different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/workflow Relating to workflow definitions kind/bug Something isn't working meta/workaround A workaround has been identified. needs-work Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants