Skip to content

Commit d1e0216

Browse files
ChristopherHXcpleemergify[bot]
authored
fix: deep evaluate matrix strategy (#964)
* fix: deep evaluate matrix strategy * Try to make linter happy. * Apply PR feedback, fix insert directive more tests * Fix: logic error Co-authored-by: Casey Lee <cplee@nektos.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
1 parent 0fae967 commit d1e0216

8 files changed

Lines changed: 238 additions & 46 deletions

File tree

‎pkg/runner/expression.go‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@ import (
88

99
"github.com/nektos/act/pkg/exprparser"
1010
log "github.com/sirupsen/logrus"
11+
"gopkg.in/yaml.v3"
1112
)
1213

1314
// ExpressionEvaluator is the interface for evaluating expressions
1415
type ExpressionEvaluator interface {
1516
evaluate(string, bool) (interface{}, error)
17+
EvaluateYamlNode(node *yaml.Node) error
1618
Interpolate(string) string
1719
}
1820

@@ -130,6 +132,82 @@ func (ee expressionEvaluator) evaluate(in string, isIfExpression bool) (interfac
130132
return evaluated, err
131133
}
132134

135+
func (ee expressionEvaluator) evaluateScalarYamlNode(node *yaml.Node) error {
136+
var in string
137+
if err := node.Decode(&in); err != nil {
138+
return err
139+
}
140+
if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") {
141+
return nil
142+
}
143+
expr, _ := rewriteSubExpression(in, false)
144+
if in != expr {
145+
log.Debugf("expression '%s' rewritten to '%s'", in, expr)
146+
}
147+
res, err := ee.evaluate(expr, false)
148+
if err != nil {
149+
return err
150+
}
151+
return node.Encode(res)
152+
}
153+
154+
func (ee expressionEvaluator) evaluateMappingYamlNode(node *yaml.Node) error {
155+
// GitHub has this undocumented feature to merge maps, called insert directive
156+
insertDirective := regexp.MustCompile(`\${{\s*insert\s*}}`)
157+
for i := 0; i < len(node.Content)/2; {
158+
k := node.Content[i*2]
159+
v := node.Content[i*2+1]
160+
if err := ee.EvaluateYamlNode(v); err != nil {
161+
return err
162+
}
163+
var sk string
164+
// Merge the nested map of the insert directive
165+
if k.Decode(&sk) == nil && insertDirective.MatchString(sk) {
166+
node.Content = append(append(node.Content[:i*2], v.Content...), node.Content[(i+1)*2:]...)
167+
i += len(v.Content) / 2
168+
} else {
169+
if err := ee.EvaluateYamlNode(k); err != nil {
170+
return err
171+
}
172+
i++
173+
}
174+
}
175+
return nil
176+
}
177+
178+
func (ee expressionEvaluator) evaluateSequenceYamlNode(node *yaml.Node) error {
179+
for i := 0; i < len(node.Content); {
180+
v := node.Content[i]
181+
// Preserve nested sequences
182+
wasseq := v.Kind == yaml.SequenceNode
183+
if err := ee.EvaluateYamlNode(v); err != nil {
184+
return err
185+
}
186+
// GitHub has this undocumented feature to merge sequences / arrays
187+
// We have a nested sequence via evaluation, merge the arrays
188+
if v.Kind == yaml.SequenceNode && !wasseq {
189+
node.Content = append(append(node.Content[:i], v.Content...), node.Content[i+1:]...)
190+
i += len(v.Content)
191+
} else {
192+
i++
193+
}
194+
}
195+
return nil
196+
}
197+
198+
func (ee expressionEvaluator) EvaluateYamlNode(node *yaml.Node) error {
199+
switch node.Kind {
200+
case yaml.ScalarNode:
201+
return ee.evaluateScalarYamlNode(node)
202+
case yaml.MappingNode:
203+
return ee.evaluateMappingYamlNode(node)
204+
case yaml.SequenceNode:
205+
return ee.evaluateSequenceYamlNode(node)
206+
default:
207+
return nil
208+
}
209+
}
210+
133211
func (ee expressionEvaluator) Interpolate(in string) string {
134212
if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") {
135213
return in

‎pkg/runner/runner.go‎

Lines changed: 58 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -115,59 +115,71 @@ func New(runnerConfig *Config) (Runner, error) {
115115
func (runner *runnerImpl) NewPlanExecutor(plan *model.Plan) common.Executor {
116116
maxJobNameLen := 0
117117

118-
pipeline := make([]common.Executor, 0)
119-
for s, stage := range plan.Stages {
120-
stageExecutor := make([]common.Executor, 0)
121-
for r, run := range stage.Runs {
122-
job := run.Job()
123-
matrixes := job.GetMatrixes()
124-
maxParallel := 4
125-
if job.Strategy != nil {
126-
maxParallel = job.Strategy.MaxParallel
127-
}
128-
129-
if len(matrixes) < maxParallel {
130-
maxParallel = len(matrixes)
131-
}
132-
133-
b := 0
134-
for i, matrix := range matrixes {
135-
rc := runner.newRunContext(run, matrix)
136-
rc.JobName = rc.Name
137-
if len(matrixes) > 1 {
138-
rc.Name = fmt.Sprintf("%s-%d", rc.Name, i+1)
118+
stagePipeline := make([]common.Executor, 0)
119+
for i := range plan.Stages {
120+
s := i
121+
stage := plan.Stages[i]
122+
stagePipeline = append(stagePipeline, func(ctx context.Context) error {
123+
pipeline := make([]common.Executor, 0)
124+
stageExecutor := make([]common.Executor, 0)
125+
for r, run := range stage.Runs {
126+
job := run.Job()
127+
if job.Strategy != nil {
128+
strategyRc := runner.newRunContext(run, nil)
129+
if err := strategyRc.NewExpressionEvaluator().EvaluateYamlNode(&job.Strategy.RawMatrix); err != nil {
130+
log.Errorf("Error while evaluating matrix: %v", err)
131+
}
132+
}
133+
matrixes := job.GetMatrixes()
134+
maxParallel := 4
135+
if job.Strategy != nil {
136+
maxParallel = job.Strategy.MaxParallel
139137
}
140-
if len(rc.String()) > maxJobNameLen {
141-
maxJobNameLen = len(rc.String())
138+
139+
if len(matrixes) < maxParallel {
140+
maxParallel = len(matrixes)
142141
}
143-
stageExecutor = append(stageExecutor, func(ctx context.Context) error {
144-
jobName := fmt.Sprintf("%-*s", maxJobNameLen, rc.String())
145-
return rc.Executor().Finally(func(ctx context.Context) error {
146-
isLastRunningContainer := func(currentStage int, currentRun int) bool {
147-
return currentStage == len(plan.Stages)-1 && currentRun == len(stage.Runs)-1
148-
}
149-
150-
if runner.config.AutoRemove && isLastRunningContainer(s, r) {
151-
log.Infof("Cleaning up container for job %s", rc.JobName)
152-
if err := rc.stopJobContainer()(ctx); err != nil {
153-
log.Errorf("Error while cleaning container: %v", err)
142+
143+
b := 0
144+
for i, matrix := range matrixes {
145+
rc := runner.newRunContext(run, matrix)
146+
rc.JobName = rc.Name
147+
if len(matrixes) > 1 {
148+
rc.Name = fmt.Sprintf("%s-%d", rc.Name, i+1)
149+
}
150+
if len(rc.String()) > maxJobNameLen {
151+
maxJobNameLen = len(rc.String())
152+
}
153+
stageExecutor = append(stageExecutor, func(ctx context.Context) error {
154+
jobName := fmt.Sprintf("%-*s", maxJobNameLen, rc.String())
155+
return rc.Executor().Finally(func(ctx context.Context) error {
156+
isLastRunningContainer := func(currentStage int, currentRun int) bool {
157+
return currentStage == len(plan.Stages)-1 && currentRun == len(stage.Runs)-1
158+
}
159+
160+
if runner.config.AutoRemove && isLastRunningContainer(s, r) {
161+
log.Infof("Cleaning up container for job %s", rc.JobName)
162+
if err := rc.stopJobContainer()(ctx); err != nil {
163+
log.Errorf("Error while cleaning container: %v", err)
164+
}
154165
}
155-
}
156-
157-
return nil
158-
})(common.WithJobErrorContainer(WithJobLogger(ctx, jobName, rc.Config.Secrets, rc.Config.InsecureSecrets)))
159-
})
160-
b++
161-
if b == maxParallel {
162-
pipeline = append(pipeline, common.NewParallelExecutor(stageExecutor...))
163-
stageExecutor = make([]common.Executor, 0)
164-
b = 0
166+
167+
return nil
168+
})(common.WithJobErrorContainer(WithJobLogger(ctx, jobName, rc.Config.Secrets, rc.Config.InsecureSecrets)))
169+
})
170+
b++
171+
if b == maxParallel {
172+
pipeline = append(pipeline, common.NewParallelExecutor(stageExecutor...))
173+
stageExecutor = make([]common.Executor, 0)
174+
b = 0
175+
}
165176
}
166177
}
167-
}
178+
return common.NewPipelineExecutor(pipeline...)(ctx)
179+
})
168180
}
169181

170-
return common.NewPipelineExecutor(pipeline...).Then(handleFailure(plan))
182+
return common.NewPipelineExecutor(stagePipeline...).Then(handleFailure(plan))
171183
}
172184

173185
func handleFailure(plan *model.Plan) common.Executor {

‎pkg/runner/runner_test.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,11 @@ func TestRunEvent(t *testing.T) {
133133
{"testdata", "steps-context/outcome", "push", "", platforms, ""},
134134
{"testdata", "job-status-check", "push", "job 'fail' failed", platforms, ""},
135135
{"testdata", "if-expressions", "push", "Job 'mytest' failed", platforms, ""},
136+
{"testdata", "evalmatrix", "push", "", platforms, ""},
137+
{"testdata", "evalmatrixneeds", "push", "", platforms, ""},
138+
{"testdata", "evalmatrixneeds2", "push", "", platforms, ""},
139+
{"testdata", "evalmatrix-merge-map", "push", "", platforms, ""},
140+
{"testdata", "evalmatrix-merge-array", "push", "", platforms, ""},
136141
{"../model/testdata", "strategy", "push", "", platforms, ""}, // TODO: move all testdata into pkg so we can validate it with planner and runner
137142
// {"testdata", "issue-228", "push", "", platforms, ""}, // TODO [igni]: Remove this once everything passes
138143

‎pkg/runner/testdata/evalmatrix-merge-array/push.yml‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
on: push
2+
jobs:
3+
a:
4+
strategy:
5+
matrix:
6+
a:
7+
- env:
8+
key1: ${{'val'}}1
9+
- ${{fromJSON('[{"env":{"key2":"val2"}},{"env":{"key3":"val3"}}]')}}
10+
runs-on: ubuntu-latest
11+
steps:
12+
- run: exit ${{ (matrix.a.env.key2 == 'val2' || matrix.a.env.key1 == 'val1' || matrix.a.env.key3 == 'val3' ) && '0' || '1' }}

‎pkg/runner/testdata/evalmatrix-merge-map/push.yml‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
on: push
2+
jobs:
3+
a:
4+
strategy:
5+
matrix:
6+
a:
7+
- env:
8+
key1: val1
9+
${{insert}}:
10+
key2: val2
11+
${{ insert }}: ${{fromJSON('{"key3":"val3"}')}}
12+
runs-on: ubuntu-latest
13+
steps:
14+
- run: exit ${{ (matrix.a.env.key2 == 'val2' && matrix.a.env.key1 == 'val1' && matrix.a.env.key3 == 'val3' ) && '0' || '1' }}

‎pkg/runner/testdata/evalmatrix/push.yml‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
on: push
2+
jobs:
3+
evalm:
4+
strategy:
5+
matrix: |-
6+
${{fromJson('
7+
{
8+
"A": [ "A", "B" ]
9+
}
10+
')}}
11+
runs-on: ubuntu-latest
12+
steps:
13+
- name: Check if the matrix key A exists
14+
run: |
15+
echo $MATRIX
16+
exit ${{matrix.A && '0' || '1'}}
17+
env:
18+
MATRIX: ${{toJSON(matrix)}}

‎pkg/runner/testdata/evalmatrixneeds/push.yml‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
on: push
2+
jobs:
3+
prepare:
4+
runs-on: ubuntu-latest
5+
steps:
6+
- run: |
7+
echo '::set-output name=matrix::{"package": ["a", "b"]}'
8+
id: r1
9+
outputs:
10+
matrix: ${{steps.r1.outputs.matrix}}
11+
evalm:
12+
needs:
13+
- prepare
14+
strategy:
15+
matrix: |-
16+
${{fromJson(needs.prepare.outputs.matrix)}}
17+
runs-on: ubuntu-latest
18+
steps:
19+
- name: Check if the matrix key package exists
20+
run: |
21+
echo $MATRIX
22+
exit ${{matrix.package && '0' || '1'}}
23+
env:
24+
MATRIX: ${{toJSON(matrix)}}

‎pkg/runner/testdata/evalmatrixneeds2/push.yml‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
on: push
2+
jobs:
3+
prepare:
4+
runs-on: ubuntu-latest
5+
steps:
6+
- run: |
7+
echo '::set-output name=matrix::["a", "b"]'
8+
id: r1
9+
outputs:
10+
matrix: ${{steps.r1.outputs.matrix}}
11+
helix: steady
12+
evalm:
13+
needs:
14+
- prepare
15+
strategy:
16+
matrix:
17+
${{needs.prepare.outputs.helix}}: |-
18+
${{fromJson(needs.prepare.outputs.matrix)}}
19+
runs-on: ubuntu-latest
20+
steps:
21+
- name: Check if the matrix key doesn't ends up unevaluated
22+
run: |
23+
echo $MATRIX
24+
exit ${{matrix['${{needs.prepare.outputs.helix}}'] && '1' || '0'}}
25+
env:
26+
MATRIX: ${{toJSON(matrix)}}
27+
- name: Check if the evaluated matrix key contains a value
28+
run: |
29+
exit ${{matrix[needs.prepare.outputs.helix] && '0' || '1'}}

0 commit comments

Comments
 (0)