refactor: extract RunContext Executor in JobExecutor #984
Conversation
This splits the executor from the RunContext into its own function called newJobExecutor. We defined an interface called jobInfo which is implemented by the RunContext. This enables better unit testing because only a small interface needs to be mocked. This is a preparation for implementing pre and post actions. Co-authored-by: Björn Brauer <[email protected]> Co-authored-by: Marcus Noll <[email protected]> Co-authored-by: Jonas Holland <[email protected]> Co-authored-by: Robert Kowalski <[email protected]> Co-authored-by: Markus Wolf <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #984 +/- ##
==========================================
+ Coverage 57.50% 57.58% +0.07%
==========================================
Files 32 33 +1
Lines 4594 4612 +18
==========================================
+ Hits 2642 2656 +14
- Misses 1729 1732 +3
- Partials 223 224 +1
Continue to review full report at Codecov.
|
| jpm.On("stopContainer").Return(func(ctx context.Context) error { | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
JFI @catthehacker You silently approved a behavior change, in december 2021 PR
This new Test for case stepWithFailure is incompatible with your comment
#694 (comment)
However I like this behavior change, but the next act version should probably be v0.3.0 and not v0.2.26.
Summary:
Container are always stopped and deleted even if the job failed ( since #840, current master and this PR ), they only keep running with cli flag --reuse.
It is possible that the old behavior still happens for failure in setup steps after container creation
During our earlier refactoring in nektos#984 we accidentally changed the behaviour in such a way that the `closeContainer()` executor was never called. This commit restores the earlier behaviour. Ref: * https://github.com/nektos/act/pull/984/files#diff-c057d66dc9657d8428e290c69871596e2b567bb8fecad62a99cab54398131a84L294 * https://github.com/nektos/act/pull/984/files#diff-ea9d5c93d769ef9b268932dd9990363e97fc3bec8a00114979d049bead5dd718R68
* fix: always execute closeContainer() executor During our earlier refactoring in #984 we accidentally changed the behaviour in such a way that the `closeContainer()` executor was never called. This commit restores the earlier behaviour. Ref: * https://github.com/nektos/act/pull/984/files#diff-c057d66dc9657d8428e290c69871596e2b567bb8fecad62a99cab54398131a84L294 * https://github.com/nektos/act/pull/984/files#diff-ea9d5c93d769ef9b268932dd9990363e97fc3bec8a00114979d049bead5dd718R68 * test: add testcases to ensure job containers are started/stopped This commit adds tests to ensure that the executors of `startContainer`, `stopContainer`, `interpolateOutputs` and `closeContainer` are always called in the correct order.
|
|
||
| return common.NewPipelineExecutor(steps...).Finally(rc.interpolateOutputs()).Finally(func(ctx context.Context) error { |
There was a problem hiding this comment.
This issue #758 was closed on November 2021, and from the comments it could be understood that the issue was actually fixed here #894. It seems like the changes introduced in this file might have caused the issue to respawn. I reproduced it in my local environment by creating a simple job with two steps, and it was only possible to set output variables in the first one.
@Poltergeist I would really appreciate it if you could verify that the fix from November 2021 was affected by these changes.
There was a problem hiding this comment.
Hi @cweckesser, can you create a new issue with a reproducible workflow to show this case?
This splits the executor from the
RunContextinto its own functioncalled
newJobExecutor.We defined an interface called
jobInfowhich is implemented by theRunContext.This enables better unit testing because only a small interface needs to
be mocked.
This is a preparation for implementing pre and post actions.