Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: deep evaluate matrix strategy #964

Merged
merged 7 commits into from
Feb 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 78 additions & 0 deletions pkg/runner/expression.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
ChristopherHX marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
104 changes: 58 additions & 46 deletions pkg/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not for s, stage in range plan.Stages as it was before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, stage are always assigned to the last stage.

I also believed that I can safely reference loop variables in func(){...}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}
}
matrixes := job.GetMatrixes()
maxParallel := 4
if job.Strategy != nil {
maxParallel = job.Strategy.MaxParallel
}
if len(rc.String()) > maxJobNameLen {
maxJobNameLen = len(rc.String())

if len(matrixes) < maxParallel {
maxParallel = len(matrixes)
}
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)

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
}

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 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 {
Expand Down
5 changes: 5 additions & 0 deletions pkg/runner/runner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
12 changes: 12 additions & 0 deletions pkg/runner/testdata/evalmatrix-merge-array/push.yml
Original file line number Diff line number Diff line change
@@ -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' }}
14 changes: 14 additions & 0 deletions pkg/runner/testdata/evalmatrix-merge-map/push.yml
Original file line number Diff line number Diff line change
@@ -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' }}
18 changes: 18 additions & 0 deletions pkg/runner/testdata/evalmatrix/push.yml
Original file line number Diff line number Diff line change
@@ -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)}}
24 changes: 24 additions & 0 deletions pkg/runner/testdata/evalmatrixneeds/push.yml
Original file line number Diff line number Diff line change
@@ -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)}}
29 changes: 29 additions & 0 deletions pkg/runner/testdata/evalmatrixneeds2/push.yml
Original file line number Diff line number Diff line change
@@ -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'}}