Skip to content

Conversation

@ChristopherHX
Copy link
Contributor

Restore the old behavior of the then executor. A lot of code in act relies on the fact that the pipeline executor fails fast instead of failing later.

Here a list of regressions

Also print the error of the step, since it is useful to know why my action failed. E.g. if you used continue-on-error in a composite action.

Resolves #946

@KnisterPeter @ZauberNerd You introduced the regression in 1891c72

The observation of the edit of this comment #948 (comment) is caused by the same regression, job failure without reason.

@ChristopherHX ChristopherHX requested a review from a team as a code owner January 27, 2022 14:30
@codecov
Copy link

codecov bot commented Jan 27, 2022

Codecov Report

Merging #971 (67dbd23) into master (0f04942) will increase coverage by 8.23%.
The diff coverage is 66.99%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #971      +/-   ##
==========================================
+ Coverage   49.27%   57.50%   +8.23%     
==========================================
  Files          23       32       +9     
  Lines        2401     4594    +2193     
==========================================
+ Hits         1183     2642    +1459     
- Misses       1090     1729     +639     
- Partials      128      223      +95     
Impacted Files Coverage Δ
pkg/common/job_error.go 0.00% <0.00%> (ø)
pkg/common/outbound_ip.go 0.00% <0.00%> (ø)
pkg/common/testflag.go 0.00% <0.00%> (ø)
pkg/container/docker_volume.go 0.00% <0.00%> (ø)
pkg/model/action.go 0.00% <0.00%> (ø)
pkg/model/step_result.go 0.00% <0.00%> (ø)
pkg/container/docker_run.go 5.54% <14.15%> (+3.61%) ⬆️
pkg/common/git.go 49.82% <31.81%> (-9.97%) ⬇️
pkg/runner/logger.go 65.43% <37.50%> (+1.28%) ⬆️
pkg/container/docker_auth.go 45.00% <45.00%> (ø)
... and 38 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 557dc75...67dbd23. Read the comment docs.

@mergify mergify bot requested a review from a team January 27, 2022 15:48
@KnisterPeter
Copy link
Member

@ChristopherHX Thanks for taking care. We where not fully aware that our change was breaking the cleanup.

@mergify mergify bot merged commit 7dbf3fc into nektos:master Jan 27, 2022
ZauberNerd added a commit to ZauberNerd/act that referenced this pull request Feb 22, 2022
Errors should always be logged with an error level and not debug level.
Since the error is returned here, it will be logged later as an error.
Presumably this was a leftover from debugging the executor chain in:
PR: nektos#971
mergify bot added a commit that referenced this pull request Feb 25, 2022
* refactor: remove debug error output

Errors should always be logged with an error level and not debug level.
Since the error is returned here, it will be logged later as an error.
Presumably this was a leftover from debugging the executor chain in:
PR: #971

* refactor: debug log wich expression is going to be evaluated

* fix: handle nil in EvalBool

We've seen this issue when the env map is not set-up properly,
i.e. when the env map is nil, EvalBool might return nil, which should
be handled as a falsy value.

* fix: fail on error in if expression and return the evaluation error

Stop running the workflow in case an expression cannot be evaluated.

Fixes: #1008

* fix: remove quotes from inside expression syntax in test

It looks like having an expression inside double quotes inside the
expression syntax is not valid: https://github.com/ZauberNerd/act-test/actions/runs/1881986429
The workflow is not valid. .github/workflows/test.yml (Line: 10, Col: 13): Unexpected symbol: '"endsWith'. Located at position 1 within expression: "endsWith('Hello world', 'ld')"

* refactor: export IsTruthy function from exprparser package

* refactor: use IsTruthy function in EvalBool

* refactor: move debug log for expression rewrite to rewrite function

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue: Panic after failing to load an action

4 participants