fix: deep evaluate matrix strategy #964
Conversation
|
@ChristopherHX this pull request has failed checks 🛠 |
Codecov Report
@@ Coverage Diff @@
## master #964 +/- ##
==========================================
+ Coverage 57.50% 58.50% +0.99%
==========================================
Files 32 34 +2
Lines 4594 4687 +93
==========================================
+ Hits 2642 2742 +100
+ Misses 1729 1715 -14
- Partials 223 230 +7
Continue to review full report at Codecov.
|
|
@ChristopherHX this pull request has failed checks 🛠 |
|
I noticed that my first map approuch of evaluateMappingYamlNode failed with yaml validation errors, if I used
Edit 2022/2/8 Footnotes
|
There was a problem hiding this comment.
So if I understand it correctly, you take the parsed yaml node, turn it back into its string representation, run Evaluate on that and then encode it as a yaml.Node again?
Feels a bit hack-y to me, but I think it is fine for now until we have refactored the whole workflow parsing to use actionlint and then we can remove this again.
| if len(matrixes) > 1 { | ||
| rc.Name = fmt.Sprintf("%s-%d", rc.Name, i+1) | ||
| stagePipeline := make([]common.Executor, 0) | ||
| for i := range plan.Stages { |
There was a problem hiding this comment.
Why not for s, stage in range plan.Stages as it was before?
There was a problem hiding this comment.
golang reuses the loop variables s, stage, the linter warns you if you do that after my changes.
So what happens if you reference s
- you capture the loop variable in a function
- the loop finishes
- the callbacks with references to the loop variables get called
s,stageare always assigned to the last stage.
I also believed that I can safely reference loop variables in func(){...}
There was a problem hiding this comment.
Ah, yes. We ran into the same issue here: https://github.com/nektos/act/blob/master/pkg/runner/job_executor_test.go#L147-L155 ;)
I only look for expressions in scalar values could be boolean, number or string. Maybe we could read the Value field directly like in if statements. I just find it myself safer to use the decode function.
Everything which isn't an expression is skipped and not encoded again For actionslint, we may need to replace all yaml types. I rely much on the current act api with yaml.Node, with a lot of convertion functions to translate job requests of the github actions service into act's layout.
I doubt we can remove this later without breaking my test cases. |
We need to wait for the previous stage to finish, otherwise we don't know how the final strategy will look like.
Then we need to evaluate all expression nodes of the strategy.matrix property, even the mapping keys.
I also added support for the insert directive and array merge operations, both are undocumented features of the github/actions which are available since 2019.
My tests aren't cover all possibilities, but the fixed issue has a test.
Most likely this change collides with @catthehacker plans of rewriting the yaml parser to actionslint, since I use the current yaml.Node types.
I have no idea how big the regressions regarding parallelism are with this change.
Resolves #927