From d1e0216039d92a4253fecc2b11a775a37ba43ceb Mon Sep 17 00:00:00 2001 From: ChristopherHX Date: Tue, 15 Feb 2022 17:35:02 +0100 Subject: [PATCH] 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 Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- pkg/runner/expression.go | 78 ++++++++++++++ pkg/runner/runner.go | 100 ++++++++++-------- pkg/runner/runner_test.go | 5 + .../testdata/evalmatrix-merge-array/push.yml | 12 +++ .../testdata/evalmatrix-merge-map/push.yml | 14 +++ pkg/runner/testdata/evalmatrix/push.yml | 18 ++++ pkg/runner/testdata/evalmatrixneeds/push.yml | 24 +++++ pkg/runner/testdata/evalmatrixneeds2/push.yml | 29 +++++ 8 files changed, 236 insertions(+), 44 deletions(-) create mode 100644 pkg/runner/testdata/evalmatrix-merge-array/push.yml create mode 100644 pkg/runner/testdata/evalmatrix-merge-map/push.yml create mode 100644 pkg/runner/testdata/evalmatrix/push.yml create mode 100644 pkg/runner/testdata/evalmatrixneeds/push.yml create mode 100644 pkg/runner/testdata/evalmatrixneeds2/push.yml diff --git a/pkg/runner/expression.go b/pkg/runner/expression.go index cc27f41..b19e8bd 100644 --- a/pkg/runner/expression.go +++ b/pkg/runner/expression.go @@ -8,11 +8,13 @@ import ( "github.com/nektos/act/pkg/exprparser" log "github.com/sirupsen/logrus" + "gopkg.in/yaml.v3" ) // ExpressionEvaluator is the interface for evaluating expressions type ExpressionEvaluator interface { evaluate(string, bool) (interface{}, error) + EvaluateYamlNode(node *yaml.Node) error Interpolate(string) string } @@ -130,6 +132,82 @@ func (ee expressionEvaluator) evaluate(in string, isIfExpression bool) (interfac return evaluated, err } +func (ee expressionEvaluator) evaluateScalarYamlNode(node *yaml.Node) error { + var in string + if err := node.Decode(&in); err != nil { + return err + } + if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") { + return nil + } + expr, _ := rewriteSubExpression(in, false) + if in != expr { + log.Debugf("expression '%s' rewritten to '%s'", in, expr) + } + res, err := ee.evaluate(expr, false) + if err != nil { + return err + } + return node.Encode(res) +} + +func (ee expressionEvaluator) evaluateMappingYamlNode(node *yaml.Node) error { + // GitHub has this undocumented feature to merge maps, called insert directive + insertDirective := regexp.MustCompile(`\${{\s*insert\s*}}`) + for i := 0; i < len(node.Content)/2; { + k := node.Content[i*2] + v := node.Content[i*2+1] + if err := ee.EvaluateYamlNode(v); err != nil { + return err + } + var sk string + // Merge the nested map of the insert directive + if k.Decode(&sk) == nil && insertDirective.MatchString(sk) { + node.Content = append(append(node.Content[:i*2], v.Content...), node.Content[(i+1)*2:]...) + i += len(v.Content) / 2 + } else { + if err := ee.EvaluateYamlNode(k); err != nil { + return err + } + i++ + } + } + return nil +} + +func (ee expressionEvaluator) evaluateSequenceYamlNode(node *yaml.Node) error { + for i := 0; i < len(node.Content); { + v := node.Content[i] + // Preserve nested sequences + wasseq := v.Kind == yaml.SequenceNode + if err := ee.EvaluateYamlNode(v); err != nil { + return err + } + // GitHub has this undocumented feature to merge sequences / arrays + // We have a nested sequence via evaluation, merge the arrays + if v.Kind == yaml.SequenceNode && !wasseq { + node.Content = append(append(node.Content[:i], v.Content...), node.Content[i+1:]...) + i += len(v.Content) + } else { + i++ + } + } + return nil +} + +func (ee expressionEvaluator) EvaluateYamlNode(node *yaml.Node) error { + switch node.Kind { + case yaml.ScalarNode: + return ee.evaluateScalarYamlNode(node) + case yaml.MappingNode: + return ee.evaluateMappingYamlNode(node) + case yaml.SequenceNode: + return ee.evaluateSequenceYamlNode(node) + default: + return nil + } +} + func (ee expressionEvaluator) Interpolate(in string) string { if !strings.Contains(in, "${{") || !strings.Contains(in, "}}") { return in diff --git a/pkg/runner/runner.go b/pkg/runner/runner.go index 8a43623..7b26922 100644 --- a/pkg/runner/runner.go +++ b/pkg/runner/runner.go @@ -115,59 +115,71 @@ func New(runnerConfig *Config) (Runner, error) { func (runner *runnerImpl) NewPlanExecutor(plan *model.Plan) common.Executor { maxJobNameLen := 0 - pipeline := make([]common.Executor, 0) - for s, stage := range plan.Stages { - stageExecutor := make([]common.Executor, 0) - for r, run := range stage.Runs { - job := run.Job() - matrixes := job.GetMatrixes() - maxParallel := 4 - if job.Strategy != nil { - maxParallel = job.Strategy.MaxParallel - } - - if len(matrixes) < maxParallel { - maxParallel = len(matrixes) - } - - b := 0 - for i, matrix := range matrixes { - rc := runner.newRunContext(run, matrix) - rc.JobName = rc.Name - if len(matrixes) > 1 { - rc.Name = fmt.Sprintf("%s-%d", rc.Name, i+1) + stagePipeline := make([]common.Executor, 0) + for i := range plan.Stages { + s := i + stage := plan.Stages[i] + stagePipeline = append(stagePipeline, func(ctx context.Context) error { + pipeline := make([]common.Executor, 0) + stageExecutor := make([]common.Executor, 0) + for r, run := range stage.Runs { + job := run.Job() + if job.Strategy != nil { + strategyRc := runner.newRunContext(run, nil) + if err := strategyRc.NewExpressionEvaluator().EvaluateYamlNode(&job.Strategy.RawMatrix); err != nil { + log.Errorf("Error while evaluating matrix: %v", err) + } } - if len(rc.String()) > maxJobNameLen { - maxJobNameLen = len(rc.String()) + matrixes := job.GetMatrixes() + maxParallel := 4 + if job.Strategy != nil { + maxParallel = job.Strategy.MaxParallel } - stageExecutor = append(stageExecutor, func(ctx context.Context) error { - jobName := fmt.Sprintf("%-*s", maxJobNameLen, rc.String()) - return rc.Executor().Finally(func(ctx context.Context) error { - isLastRunningContainer := func(currentStage int, currentRun int) bool { - return currentStage == len(plan.Stages)-1 && currentRun == len(stage.Runs)-1 - } - if runner.config.AutoRemove && isLastRunningContainer(s, r) { - log.Infof("Cleaning up container for job %s", rc.JobName) - if err := rc.stopJobContainer()(ctx); err != nil { - log.Errorf("Error while cleaning container: %v", err) + if len(matrixes) < maxParallel { + maxParallel = len(matrixes) + } + + b := 0 + for i, matrix := range matrixes { + rc := runner.newRunContext(run, matrix) + rc.JobName = rc.Name + if len(matrixes) > 1 { + rc.Name = fmt.Sprintf("%s-%d", rc.Name, i+1) + } + if len(rc.String()) > maxJobNameLen { + maxJobNameLen = len(rc.String()) + } + stageExecutor = append(stageExecutor, func(ctx context.Context) error { + jobName := fmt.Sprintf("%-*s", maxJobNameLen, rc.String()) + return rc.Executor().Finally(func(ctx context.Context) error { + isLastRunningContainer := func(currentStage int, currentRun int) bool { + return currentStage == len(plan.Stages)-1 && currentRun == len(stage.Runs)-1 } - } - return nil - })(common.WithJobErrorContainer(WithJobLogger(ctx, jobName, rc.Config.Secrets, rc.Config.InsecureSecrets))) - }) - b++ - if b == maxParallel { - pipeline = append(pipeline, common.NewParallelExecutor(stageExecutor...)) - stageExecutor = make([]common.Executor, 0) - b = 0 + if runner.config.AutoRemove && isLastRunningContainer(s, r) { + log.Infof("Cleaning up container for job %s", rc.JobName) + if err := rc.stopJobContainer()(ctx); err != nil { + log.Errorf("Error while cleaning container: %v", err) + } + } + + return nil + })(common.WithJobErrorContainer(WithJobLogger(ctx, jobName, rc.Config.Secrets, rc.Config.InsecureSecrets))) + }) + b++ + if b == maxParallel { + pipeline = append(pipeline, common.NewParallelExecutor(stageExecutor...)) + stageExecutor = make([]common.Executor, 0) + b = 0 + } } } - } + return common.NewPipelineExecutor(pipeline...)(ctx) + }) } - return common.NewPipelineExecutor(pipeline...).Then(handleFailure(plan)) + return common.NewPipelineExecutor(stagePipeline...).Then(handleFailure(plan)) } func handleFailure(plan *model.Plan) common.Executor { diff --git a/pkg/runner/runner_test.go b/pkg/runner/runner_test.go index ca0d19e..2094837 100644 --- a/pkg/runner/runner_test.go +++ b/pkg/runner/runner_test.go @@ -133,6 +133,11 @@ func TestRunEvent(t *testing.T) { {"testdata", "steps-context/outcome", "push", "", platforms, ""}, {"testdata", "job-status-check", "push", "job 'fail' failed", platforms, ""}, {"testdata", "if-expressions", "push", "Job 'mytest' failed", platforms, ""}, + {"testdata", "evalmatrix", "push", "", platforms, ""}, + {"testdata", "evalmatrixneeds", "push", "", platforms, ""}, + {"testdata", "evalmatrixneeds2", "push", "", platforms, ""}, + {"testdata", "evalmatrix-merge-map", "push", "", platforms, ""}, + {"testdata", "evalmatrix-merge-array", "push", "", platforms, ""}, {"../model/testdata", "strategy", "push", "", platforms, ""}, // TODO: move all testdata into pkg so we can validate it with planner and runner // {"testdata", "issue-228", "push", "", platforms, ""}, // TODO [igni]: Remove this once everything passes diff --git a/pkg/runner/testdata/evalmatrix-merge-array/push.yml b/pkg/runner/testdata/evalmatrix-merge-array/push.yml new file mode 100644 index 0000000..6a07e31 --- /dev/null +++ b/pkg/runner/testdata/evalmatrix-merge-array/push.yml @@ -0,0 +1,12 @@ +on: push +jobs: + a: + strategy: + matrix: + a: + - env: + key1: ${{'val'}}1 + - ${{fromJSON('[{"env":{"key2":"val2"}},{"env":{"key3":"val3"}}]')}} + runs-on: ubuntu-latest + steps: + - run: exit ${{ (matrix.a.env.key2 == 'val2' || matrix.a.env.key1 == 'val1' || matrix.a.env.key3 == 'val3' ) && '0' || '1' }} \ No newline at end of file diff --git a/pkg/runner/testdata/evalmatrix-merge-map/push.yml b/pkg/runner/testdata/evalmatrix-merge-map/push.yml new file mode 100644 index 0000000..41d35b6 --- /dev/null +++ b/pkg/runner/testdata/evalmatrix-merge-map/push.yml @@ -0,0 +1,14 @@ +on: push +jobs: + a: + strategy: + matrix: + a: + - env: + key1: val1 + ${{insert}}: + key2: val2 + ${{ insert }}: ${{fromJSON('{"key3":"val3"}')}} + runs-on: ubuntu-latest + steps: + - run: exit ${{ (matrix.a.env.key2 == 'val2' && matrix.a.env.key1 == 'val1' && matrix.a.env.key3 == 'val3' ) && '0' || '1' }} \ No newline at end of file diff --git a/pkg/runner/testdata/evalmatrix/push.yml b/pkg/runner/testdata/evalmatrix/push.yml new file mode 100644 index 0000000..94e9741 --- /dev/null +++ b/pkg/runner/testdata/evalmatrix/push.yml @@ -0,0 +1,18 @@ +on: push +jobs: + evalm: + strategy: + matrix: |- + ${{fromJson(' + { + "A": [ "A", "B" ] + } + ')}} + runs-on: ubuntu-latest + steps: + - name: Check if the matrix key A exists + run: | + echo $MATRIX + exit ${{matrix.A && '0' || '1'}} + env: + MATRIX: ${{toJSON(matrix)}} \ No newline at end of file diff --git a/pkg/runner/testdata/evalmatrixneeds/push.yml b/pkg/runner/testdata/evalmatrixneeds/push.yml new file mode 100644 index 0000000..30f52ac --- /dev/null +++ b/pkg/runner/testdata/evalmatrixneeds/push.yml @@ -0,0 +1,24 @@ +on: push +jobs: + prepare: + runs-on: ubuntu-latest + steps: + - run: | + echo '::set-output name=matrix::{"package": ["a", "b"]}' + id: r1 + outputs: + matrix: ${{steps.r1.outputs.matrix}} + evalm: + needs: + - prepare + strategy: + matrix: |- + ${{fromJson(needs.prepare.outputs.matrix)}} + runs-on: ubuntu-latest + steps: + - name: Check if the matrix key package exists + run: | + echo $MATRIX + exit ${{matrix.package && '0' || '1'}} + env: + MATRIX: ${{toJSON(matrix)}} \ No newline at end of file diff --git a/pkg/runner/testdata/evalmatrixneeds2/push.yml b/pkg/runner/testdata/evalmatrixneeds2/push.yml new file mode 100644 index 0000000..225b518 --- /dev/null +++ b/pkg/runner/testdata/evalmatrixneeds2/push.yml @@ -0,0 +1,29 @@ +on: push +jobs: + prepare: + runs-on: ubuntu-latest + steps: + - run: | + echo '::set-output name=matrix::["a", "b"]' + id: r1 + outputs: + matrix: ${{steps.r1.outputs.matrix}} + helix: steady + evalm: + needs: + - prepare + strategy: + matrix: + ${{needs.prepare.outputs.helix}}: |- + ${{fromJson(needs.prepare.outputs.matrix)}} + runs-on: ubuntu-latest + steps: + - name: Check if the matrix key doesn't ends up unevaluated + run: | + echo $MATRIX + exit ${{matrix['${{needs.prepare.outputs.helix}}'] && '1' || '0'}} + env: + MATRIX: ${{toJSON(matrix)}} + - name: Check if the evaluated matrix key contains a value + run: | + exit ${{matrix[needs.prepare.outputs.helix] && '0' || '1'}}