Simplify Matrix decode, add defaults for fail-fast and max-parallel, add test (#763)

* fix[workflow]: multiple fixes for workflow/matrix

fix[workflow]: default `max-parallel`
fix[workflow]: default `fail-fast`, it's `true`, not `false`
fix[workflow]: skipping over the job when `strategy:` is defined but `matrix:` isn't (fixes #625)
fix[workflow]: skip non-existing includes keys and hard fail on non-existing excludes keys
fix[workflow]: simplify Matrix decode (because I "think" I know how `yaml` works) (fixes #760)
fix[tests]: add test for planner and runner

* fix(workflow): use yaml node for env key

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

pkg/model/testdata/strategy/push.yml

@@ -0,0 +1,50 @@
+---
+jobs:
+  strategy-all:
+    name: ${{ matrix.node-version }} | ${{ matrix.site }} | ${{ matrix.datacenter }}
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 'Hello!'
+    strategy:
+      fail-fast: false
+      matrix:
+        datacenter:
+          - site-c
+          - site-d
+        exclude:
+          - datacenter: site-d
+            node-version: 14.x
+            site: staging
+        include:
+          - php-version: 5.4
+          - datacenter: site-a
+            node-version: 10.x
+            site: prod
+          - datacenter: site-b
+            node-version: 12.x
+            site: dev
+        node-version: [14.x, 16.x]
+        site:
+          - staging
+      max-parallel: 2
+  strategy-no-matrix:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 'Hello!'
+    strategy:
+      fail-fast: false
+      max-parallel: 2
+  strategy-only-fail-fast:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 'Hello!'
+    strategy:
+      fail-fast: false
+  strategy-only-max-parallel:
+    runs-on: ubuntu-latest
+    steps:
+      - run: echo 'Hello!'
+    strategy:
+      max-parallel: 2
+'on':
+  push: null

pkg/model/workflow.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"reflect"
 	"regexp"
+	"strconv"
 	"strings"
 
 	"github.com/nektos/act/pkg/common"
@@ -58,7 +59,7 @@ type Job struct {
 	Name           string                    `yaml:"name"`
 	RawNeeds       yaml.Node                 `yaml:"needs"`
 	RawRunsOn      yaml.Node                 `yaml:"runs-on"`
-	Env            interface{}               `yaml:"env"`
+	Env            yaml.Node                 `yaml:"env"`
 	If             yaml.Node                 `yaml:"if"`
 	Steps          []*Step                   `yaml:"steps"`
 	TimeoutMinutes int64                     `yaml:"timeout-minutes"`
@@ -71,9 +72,11 @@ type Job struct {
 
 // Strategy for the job
 type Strategy struct {
-	FailFast    bool        `yaml:"fail-fast"`
-	MaxParallel int         `yaml:"max-parallel"`
-	Matrix      interface{} `yaml:"matrix"`
+	FailFast          bool
+	MaxParallel       int
+	FailFastString    string    `yaml:"fail-fast"`
+	MaxParallelString string    `yaml:"max-parallel"`
+	RawMatrix         yaml.Node `yaml:"matrix"`
 }
 
 // Default settings that will apply to all steps in the job or workflow
@@ -87,6 +90,37 @@ type RunDefaults struct {
 	WorkingDirectory string `yaml:"working-directory"`
 }
 
+// GetMaxParallel sets default and returns value for `max-parallel`
+func (s Strategy) GetMaxParallel() int {
+	// MaxParallel default value is `GitHub will maximize the number of jobs run in parallel depending on the available runners on GitHub-hosted virtual machines`
+	// So I take the liberty to hardcode default limit to 4 and this is because:
+	// 1: tl;dr: self-hosted does only 1 parallel job - https://github.com/actions/runner/issues/639#issuecomment-825212735
+	// 2: GH has 20 parallel job limit (for free tier) - https://github.com/github/docs/blob/3ae84420bd10997bb5f35f629ebb7160fe776eae/content/actions/reference/usage-limits-billing-and-administration.md?plain=1#L45
+	// 3: I want to add support for MaxParallel to act and 20! parallel jobs is a bit overkill IMHO
+	maxParallel := 4
+	if s.MaxParallelString != "" {
+		var err error
+		if maxParallel, err = strconv.Atoi(s.MaxParallelString); err != nil {
+			log.Errorf("Failed to parse 'max-parallel' option: %v", err)
+		}
+	}
+	return maxParallel
+}
+
+// GetFailFast sets default and returns value for `fail-fast`
+func (s Strategy) GetFailFast() bool {
+	// FailFast option is true by default: https://github.com/github/docs/blob/3ae84420bd10997bb5f35f629ebb7160fe776eae/content/actions/reference/workflow-syntax-for-github-actions.md?plain=1#L1107
+	failFast := true
+	log.Debug(s.FailFastString)
+	if s.FailFastString != "" {
+		var err error
+		if failFast, err = strconv.ParseBool(s.FailFastString); err != nil {
+			log.Errorf("Failed to parse 'fail-fast' option: %v", err)
+		}
+	}
+	return failFast
+}
+
 // Container details for the job
 func (j *Job) Container() *ContainerSpec {
 	var val *ContainerSpec
@@ -149,89 +183,99 @@ func (j *Job) RunsOn() []string {
 	return nil
 }
 
-func environment(e interface{}) map[string]string {
+func environment(yml yaml.Node) map[string]string {
 	env := make(map[string]string)
-	switch t := e.(type) {
-	case map[string]interface{}:
-		for k, v := range t {
-			switch t := v.(type) {
-			case string:
-				env[k] = t
-			case interface{}:
-				env[k] = ""
-			}
-		}
-	case map[string]string:
-		for k, v := range e.(map[string]string) {
-			env[k] = v
+	if yml.Kind == yaml.MappingNode {
+		if err := yml.Decode(&env); err != nil {
+			log.Fatal(err)
 		}
 	}
 	return env
 }
 
+// Environments returns string-based key=value map for a job
 func (j *Job) Environment() map[string]string {
 	return environment(j.Env)
 }
 
+// Matrix decodes RawMatrix YAML node
 func (j *Job) Matrix() map[string][]interface{} {
-	a := reflect.ValueOf(j.Strategy.Matrix)
-	if a.Type().Kind() == reflect.Map {
-		output := make(map[string][]interface{})
-		for _, e := range a.MapKeys() {
-			v := a.MapIndex(e)
-			switch t := v.Interface().(type) {
-			case []interface{}:
-				output[e.String()] = t
-			case interface{}:
-				var in []interface{}
-				in = append(in, t)
-				output[e.String()] = in
-			}
+	if j.Strategy.RawMatrix.Kind == yaml.MappingNode {
+		var val map[string][]interface{}
+		if err := j.Strategy.RawMatrix.Decode(&val); err != nil {
+			log.Fatal(err)
 		}
-		return output
+		return val
 	}
 	return nil
 }
 
 // GetMatrixes returns the matrix cross product
+// It skips includes and hard fails excludes for non-existing keys
+// nolint:gocyclo
 func (j *Job) GetMatrixes() []map[string]interface{} {
 	matrixes := make([]map[string]interface{}, 0)
 	if j.Strategy != nil {
-		m := j.Matrix()
-		includes := make([]map[string]interface{}, 0)
-		for _, v := range m["include"] {
-			switch t := v.(type) {
-			case []interface{}:
-				for _, i := range t {
-					includes = append(includes, i.(map[string]interface{}))
-				}
-			case interface{}:
-				includes = append(includes, v.(map[string]interface{}))
-			}
-		}
-		delete(m, "include")
+		j.Strategy.FailFast = j.Strategy.GetFailFast()
+		j.Strategy.MaxParallel = j.Strategy.GetMaxParallel()
 
-		excludes := make([]map[string]interface{}, 0)
-		for _, v := range m["exclude"] {
-			excludes = append(excludes, v.(map[string]interface{}))
-		}
-		delete(m, "exclude")
-
-		matrixProduct := common.CartesianProduct(m)
-
-	MATRIX:
-		for _, matrix := range matrixProduct {
-			for _, exclude := range excludes {
-				if commonKeysMatch(matrix, exclude) {
-					log.Debugf("Skipping matrix '%v' due to exclude '%v'", matrix, exclude)
-					continue MATRIX
+		if m := j.Matrix(); m != nil {
+			includes := make([]map[string]interface{}, 0)
+			for _, v := range m["include"] {
+				switch t := v.(type) {
+				case []interface{}:
+					for _, i := range t {
+						i := i.(map[string]interface{})
+						for k := range i {
+							if _, ok := m[k]; ok {
+								includes = append(includes, i)
+								break
+							}
+						}
+					}
+				case interface{}:
+					v := v.(map[string]interface{})
+					for k := range v {
+						if _, ok := m[k]; ok {
+							includes = append(includes, v)
+							break
+						}
+					}
 				}
 			}
-			matrixes = append(matrixes, matrix)
-		}
-		for _, include := range includes {
-			log.Debugf("Adding include '%v'", include)
-			matrixes = append(matrixes, include)
+			delete(m, "include")
+
+			excludes := make([]map[string]interface{}, 0)
+			for _, e := range m["exclude"] {
+				e := e.(map[string]interface{})
+				for k := range e {
+					if _, ok := m[k]; ok {
+						excludes = append(excludes, e)
+					} else {
+						// We fail completely here because that's what GitHub does for non-existing matrix keys, fail on exclude, silent skip on include
+						log.Fatalf("The workflow is not valid. Matrix exclude key '%s' does not match any key within the matrix", k)
+					}
+				}
+			}
+			delete(m, "exclude")
+
+			matrixProduct := common.CartesianProduct(m)
+		MATRIX:
+			for _, matrix := range matrixProduct {
+				for _, exclude := range excludes {
+					if commonKeysMatch(matrix, exclude) {
+						log.Debugf("Skipping matrix '%v' due to exclude '%v'", matrix, exclude)
+						continue MATRIX
+					}
+				}
+				matrixes = append(matrixes, matrix)
+			}
+			for _, include := range includes {
+				log.Debugf("Adding include '%v'", include)
+				matrixes = append(matrixes, include)
+			}
+		} else {
+			matrixes = append(matrixes, make(map[string]interface{}))
 		}
 	} else {
 		matrixes = append(matrixes, make(map[string]interface{}))
@@ -270,7 +314,7 @@ type Step struct {
 	Run              string            `yaml:"run"`
 	WorkingDirectory string            `yaml:"working-directory"`
 	Shell            string            `yaml:"shell"`
-	Env              interface{}       `yaml:"env"`
+	Env              yaml.Node         `yaml:"env"`
 	With             map[string]string `yaml:"with"`
 	ContinueOnError  bool              `yaml:"continue-on-error"`
 	TimeoutMinutes   int64             `yaml:"timeout-minutes"`
@@ -288,6 +332,7 @@ func (s *Step) String() string {
 	return s.ID
 }
 
+// Environments returns string-based key=value map for a step
 func (s *Step) Environment() map[string]string {
 	return environment(s.Env)
 }

pkg/model/workflow_test.go

@@ -173,6 +173,64 @@ jobs:
 	assert.Equal(t, "${{ steps.test1_1.outputs.b-key }}", workflow.Jobs["test1"].Outputs["some-b-key"])
 }
 
+func TestReadWorkflow_Strategy(t *testing.T) {
+	w, err := NewWorkflowPlanner("testdata/strategy/push.yml", true)
+	assert.NoError(t, err)
+
+	p := w.PlanJob("strategy-only-max-parallel")
+
+	assert.Equal(t, len(p.Stages), 1)
+	assert.Equal(t, len(p.Stages[0].Runs), 1)
+
+	wf := p.Stages[0].Runs[0].Workflow
+
+	job := wf.Jobs["strategy-only-max-parallel"]
+	assert.Equal(t, job.GetMatrixes(), []map[string]interface{}{{}})
+	assert.Equal(t, job.Matrix(), map[string][]interface{}(nil))
+	assert.Equal(t, job.Strategy.MaxParallel, 2)
+	assert.Equal(t, job.Strategy.FailFast, true)
+
+	job = wf.Jobs["strategy-only-fail-fast"]
+	assert.Equal(t, job.GetMatrixes(), []map[string]interface{}{{}})
+	assert.Equal(t, job.Matrix(), map[string][]interface{}(nil))
+	assert.Equal(t, job.Strategy.MaxParallel, 4)
+	assert.Equal(t, job.Strategy.FailFast, false)
+
+	job = wf.Jobs["strategy-no-matrix"]
+	assert.Equal(t, job.GetMatrixes(), []map[string]interface{}{{}})
+	assert.Equal(t, job.Matrix(), map[string][]interface{}(nil))
+	assert.Equal(t, job.Strategy.MaxParallel, 2)
+	assert.Equal(t, job.Strategy.FailFast, false)
+
+	job = wf.Jobs["strategy-all"]
+	assert.Equal(t, job.GetMatrixes(),
+		[]map[string]interface{}{
+			{"datacenter": "site-c", "node-version": "14.x", "site": "staging"},
+			{"datacenter": "site-c", "node-version": "16.x", "site": "staging"},
+			{"datacenter": "site-d", "node-version": "16.x", "site": "staging"},
+			{"datacenter": "site-a", "node-version": "10.x", "site": "prod"},
+			{"datacenter": "site-b", "node-version": "12.x", "site": "dev"},
+		},
+	)
+	assert.Equal(t, job.Matrix(),
+		map[string][]interface{}{
+			"datacenter": {"site-c", "site-d"},
+			"exclude": {
+				map[string]interface{}{"datacenter": "site-d", "node-version": "14.x", "site": "staging"},
+			},
+			"include": {
+				map[string]interface{}{"php-version": 5.4},
+				map[string]interface{}{"datacenter": "site-a", "node-version": "10.x", "site": "prod"},
+				map[string]interface{}{"datacenter": "site-b", "node-version": "12.x", "site": "dev"},
+			},
+			"node-version": {"14.x", "16.x"},
+			"site":         {"staging"},
+		},
+	)
+	assert.Equal(t, job.Strategy.MaxParallel, 2)
+	assert.Equal(t, job.Strategy.FailFast, false)
+}
+
 func TestStep_ShellCommand(t *testing.T) {
 	tests := []struct {
 		shell string

pkg/runner/expression_test.go

@@ -9,9 +9,17 @@ import (
 
 	"github.com/nektos/act/pkg/model"
 	assert "github.com/stretchr/testify/assert"
+	yaml "gopkg.in/yaml.v3"
 )
 
 func TestEvaluate(t *testing.T) {
+	var yml yaml.Node
+	err := yml.Encode(map[string][]interface{}{
+		"os":  {"Linux", "Windows"},
+		"foo": {"bar", "baz"},
+	})
+	assert.NoError(t, err)
+
 	rc := &RunContext{
 		Config: &Config{
 			Workdir: ".",
@@ -29,10 +37,7 @@ func TestEvaluate(t *testing.T) {
 				Jobs: map[string]*model.Job{
 					"job1": {
 						Strategy: &model.Strategy{
-							Matrix: map[string][]interface{}{
-								"os":  {"Linux", "Windows"},
-								"foo": {"bar", "baz"},
-							},
+							RawMatrix: yml,
 						},
 					},
 				},

pkg/runner/run_context_test.go

@@ -14,9 +14,17 @@ import (
 	log "github.com/sirupsen/logrus"
 	"github.com/sirupsen/logrus/hooks/test"
 	assert "github.com/stretchr/testify/assert"
+	yaml "gopkg.in/yaml.v3"
 )
 
 func TestRunContext_EvalBool(t *testing.T) {
+	var yml yaml.Node
+	err := yml.Encode(map[string][]interface{}{
+		"os":  {"Linux", "Windows"},
+		"foo": {"bar", "baz"},
+	})
+	assert.NoError(t, err)
+
 	hook := test.NewGlobal()
 	rc := &RunContext{
 		Config: &Config{
@@ -34,10 +42,7 @@ func TestRunContext_EvalBool(t *testing.T) {
 				Jobs: map[string]*model.Job{
 					"job1": {
 						Strategy: &model.Strategy{
-							Matrix: map[string][]interface{}{
-								"os":  {"Linux", "Windows"},
-								"foo": {"bar", "baz"},
-							},
+							RawMatrix: yml,
 						},
 					},
 				},

pkg/runner/runner_test.go

@@ -118,6 +118,7 @@ func TestRunEvent(t *testing.T) {
 		{"testdata", "issue-597", "push", "", platforms, ""},
 		{"testdata", "issue-598", "push", "", platforms, ""},
 		{"testdata", "env-and-path", "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
 
 		// single test for different architecture: linux/arm64