Expression evaluator fixes

[ZauberNerd]

This PR contains a few fixes and improvements for the new expression evaluator:

• Adds more debug output about which expression is going to be evaluated
• Use exported IsTruthy() function from the exprparser package inside EvalBool()
• Fail act when an expression contains errors and cannot be evaluated

Fixes: #1008
Ref: #971

[ZauberNerd]

@ChristopherHX do you remember why you added this debug log in #971 7dbf3fc#diff-fa62a470d83900f015861d603c59c5e371354fb0cdf36089a274d40ee409029eR139?
Since the error is being returned it will be printed eventually.

[mergify]

@ZauberNerd this pull request has failed checks 🛠

[ChristopherHX]

I restored the function 1:1 as it was prior running steps after job failure

https://github.com/nektos/act/blob/v0.2.25/pkg/common/executor.go#L139

I'm not the original author of this debug print

Since the error is being returned it will be printed eventually.

Yes it will eventually get printed. I think it good for debugging to always print any error of the then executor.

[ZauberNerd]

Yes it will eventually get printed. I think it good for debugging to always print any error of the then executor.

In my opinion debug logging should be used to provide more information that otherwise isn't visible in order to be able to understand what is happening.
In this case the error is already being printed, so logging the error into the debug log doesn't make much sense.

Below is the test run of https://github.com/nektos/act/pull/971/files#diff-4526efc54b2af950bff29a71c13adadb9f22132374291f264c843cc3cfd51d21

$ ./dist/local/act -W ./pkg/runner/testdata/non-existent-action/push.yml 
[non-existent-action/nopanic] 🚀  Start image=ghcr.io/catthehacker/ubuntu:act-latest
[non-existent-action/nopanic]   🐳  docker pull image=ghcr.io/catthehacker/ubuntu:act-latest platform= username= forcePull=false
[non-existent-action/nopanic]   🐳  docker create image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[non-existent-action/nopanic]   🐳  docker run image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[non-existent-action/nopanic]   🐳  docker exec cmd=[mkdir -m 0777 -p /var/run/act] user=root workdir=
[non-existent-action/nopanic] ⭐  Run ./path/to/non-existent-action
[non-existent-action/nopanic]   ❌  Failure - ./path/to/non-existent-action
[non-existent-action/nopanic] file does not exist
Error: Job 'nopanic' failed

[codecov]

Codecov Report

Merging #1009 (4114460) into master (4f8da0a) will increase coverage by 1.04%.
The diff coverage is 81.41%.

@@            Coverage Diff             @@
##           master    #1009      +/-   ##
==========================================
+ Coverage   57.50%   58.55%   +1.04%     
==========================================
  Files          32       34       +2     
  Lines        4594     4625      +31     
==========================================
+ Hits         2642     2708      +66     
+ Misses       1729     1690      -39     
- Partials      223      227       +4     

Impacted Files	Coverage Δ
pkg/common/executor.go	46.90% <ø> (+0.41%)	⬆️
pkg/model/action.go	0.00% <ø> (ø)
pkg/model/planner.go	50.73% <ø> (+0.32%)	⬆️
pkg/exprparser/interpreter.go	72.79% <66.66%> (-0.61%)	⬇️
pkg/runner/expression.go	89.36% <77.61%> (-1.46%)	⬇️
pkg/runner/runner.go	75.89% <78.00%> (-0.58%)	⬇️
pkg/runner/job_executor.go	81.57% <81.57%> (ø)
pkg/runner/step_context.go	85.71% <83.33%> (+4.08%)	⬆️
pkg/runner/action.go	84.21% <84.21%> (ø)
pkg/runner/run_context.go	79.96% <95.83%> (+0.31%)	⬆️
... and 4 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d43396...4114460. Read the comment docs.

[catthehacker]

In my opinion debug logging should be used to provide more information that otherwise isn't visible in order to be able to understand what is happening. In this case the error is already being printed, so logging the error into the debug log doesn't make much sense.

you could make it a trace instead debug

[ZauberNerd]

@ChristopherHX @catthehacker so what's the status here? Should I:

• restore the old behaviour and log the error as debug?
• log the error as trace?
• leave it as is?

In case you'd like to keep a log in the Then executor, I'd probably change the log to include a prefix, such as Error in Then executor: %+v or similar, because @KnisterPeter and me had a hard time figuring out where the log came from.

[catthehacker]

I don't really have any preference, I'll leave it up to you
Just wanted to give another option due to #928

[ChristopherHX]

Works: I'm ok with removing the debug print. You won't need to address my review comment, which is just an example ( and a bug in act ) why I prefer to use actions/runner locally.

In a later change, we might want to connect this error to an actual job.

Error:   ❌  Error in if-expression: "if: fromJSON('{}')||github.event.pull_request || !github.event.pull_request
" (Unable to map type 'map[]' to boolean for 'fromJSON('{}')||github.event.pull_request || !github.event.pull_request
')

[ChristopherHX]

I thought the default case is impossible to reach for a conforming evaluator.
Any Object != null => true, except empty string

• if: github.event.pull_request ( true if PR )
• if: fromJSON('{}') ( always true )

This weird workflow only fails in act, the last step is expected to run

on:
- push
- pull_request
jobs:
  m:
    runs-on:
    - ubuntu-latest
    steps:
    - run: |
                echo PR!
      shell: bash
      if: |
                github.event.pull_request
    - run: |
                echo Push!
      shell: bash
      if: |
                !github.event.pull_request
    - run: |
                echo empty obj!
      shell: bash
      if: |
                fromJSON('{}')

[ZauberNerd]

Good catch, I'll update the function to handle maps.

[ZauberNerd]

@ChristopherHX @catthehacker I've updated this PR to use the IsTruthy() function from the exprparser package to turn the evaluated string in EvalBool() into a boolean.
Please have another look.

[KnisterPeter]

Nice

[cplee]

@ZauberNerd - is this a breaking change? looks like the tests had to be updated from " -> '...any concern with that having impact on users?

[ZauberNerd]

@ZauberNerd - is this a breaking change? looks like the tests had to be updated from " -> '...any concern with that having impact on users?

These workflows are invalid according to GitHub and strings can only use single quotes: https://docs.github.com/en/actions/learn-github-actions/expressions#literals

Here's a workflow run of the "issue-597" workflow: https://github.com/ZauberNerd/act-test/actions/runs/1889956547

[Invalid workflow file: .github/workflows/test.yml#L17](https://github.com/ZauberNerd/act-test/actions/runs/1889956547/workflow)
The workflow is not valid. .github/workflows/test.yml (Line: 17, Col: 13): Unexpected symbol: '"endsWith'. Located at position 1 within expression: "endsWith('Hello world', 'ld')"

Also, it doesn't look like they worked on master either, just the error while parsing the if condition didn't lead to a non-zero exit-code:

$ ./dist/local/act -W ./pkg/runner/testdata/issue-597/spelling.yaml 
[issue-597/my_first_job] 🚀  Start image=ghcr.io/catthehacker/ubuntu:act-latest
[issue-597/my_first_job]   🐳  docker pull image=ghcr.io/catthehacker/ubuntu:act-latest platform= username= forcePull=false
[issue-597/my_first_job]   🐳  docker create image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[issue-597/my_first_job]   🐳  docker run image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[issue-597/my_first_job]   🐳  docker exec cmd=[mkdir -m 0777 -p /var/run/act] user=root workdir=
[issue-597/my_first_job]   ❌  Error in if: expression - My first true step

With these changes it looks like this:

$ ~/act/dist/local/act -W ./issue-597/spelling.yaml 
[issue-597/my_first_job] 🚀  Start image=ghcr.io/catthehacker/ubuntu:act-latest
[issue-597/my_first_job]   🐳  docker pull image=ghcr.io/catthehacker/ubuntu:act-latest platform= username= forcePull=false
[issue-597/my_first_job]   🐳  docker create image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[issue-597/my_first_job]   🐳  docker run image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[issue-597/my_first_job]   🐳  docker exec cmd=[mkdir -m 0777 -p /var/run/act] user=root workdir=
[issue-597/my_first_job] ⭐  Run My first true step
[issue-597/my_first_job]   ☁  git clone 'https://github.com/actions/hello-world-javascript-action' # ref=main
[issue-597/my_first_job]   🐳  docker cp src=/home/ubuntu/.cache/act/actions-hello-world-javascript-action@main/ dst=/var/run/act/actions/actions-hello-world-javascript-action@main/
[issue-597/my_first_job]   🐳  docker exec cmd=[mkdir -p /var/run/act/actions/actions-hello-world-javascript-action@main/] user= workdir=
[issue-597/my_first_job]   🐳  docker exec cmd=[node /var/run/act/actions/actions-hello-world-javascript-action@main/index.js] user= workdir=
| Hello Renst the Octocat!
[issue-597/my_first_job]   ⚙  ::set-output:: time=22:26:32 GMT+0000 (GMT)
| The event payload: {}
[issue-597/my_first_job]   ✅  Success - My first true step

[ChristopherHX]

I also wouldn't classify the test workflow corrections as a breaking change, since it never used to work on github. I even blindly duplicated the invalid if in the composite test, in the past assuming these were valid.
I would say deleting container on workflow failure was a breaking change introduced after 0.2.25, no tests no ci failure.

[cplee]

Thanks @ZauberNerd - solid answer :)