fix: continue jobs + steps after failure

To allow proper if expression handling on jobs and steps (like always, success, failure, ...) we need to continue running all executors in the prepared chain. To keep the error handling intact we add an occurred error to the go context and handle it later in the pipeline/chain. Also we add the job result to the needs context to give expressions access to it. The needs object, failure and success functions are split between run context (on jobs) and step context. Closes nektos#442 Co-authored-by: Björn Brauer <[email protected]>
xing · Oct 14, 2021 · 5aead8b · 5aead8b
1 parent a0d3602
commit 5aead8b
Show file tree

Hide file tree

Showing 6 changed files with 190 additions and 62 deletions.
diff --git a/pkg/common/executor.go b/pkg/common/executor.go
@@ -7,6 +7,10 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
+type errorContextKey string
+
+const ErrorContextKeyVal = errorContextKey("error")
+
 // Warning that implements `error` but safe to ignore
 type Warning struct {
 	Message string
@@ -136,13 +140,12 @@ func (e Executor) Then(then Executor) Executor {
 			case Warning:
 				log.Warning(err.Error())
 			default:
-				log.Debugf("%+v", err)
-				return err
+				ctx = context.WithValue(ctx, ErrorContextKeyVal, err)
 			}
+		} else if ctx.Err() != nil {
+			ctx = context.WithValue(ctx, ErrorContextKeyVal, ctx.Err())
 		}
-		if ctx.Err() != nil {
-			return ctx.Err()
-		}
+
 		return then(ctx)
 	}
 }

diff --git a/pkg/model/workflow.go b/pkg/model/workflow.go
@@ -69,6 +69,7 @@ type Job struct {
 	RawContainer   yaml.Node                 `yaml:"container"`
 	Defaults       Defaults                  `yaml:"defaults"`
 	Outputs        map[string]string         `yaml:"outputs"`
+	Result         string
 }
 
 // Strategy for the job

diff --git a/pkg/runner/expression.go b/pkg/runner/expression.go
@@ -4,6 +4,8 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"errors"
+	"fmt"
 	"io"
 	"os"
 	"path/filepath"
@@ -25,6 +27,7 @@ func init() {
 // NewExpressionEvaluator creates a new evaluator
 func (rc *RunContext) NewExpressionEvaluator() ExpressionEvaluator {
 	vm := rc.newVM()
+
 	return &expressionEvaluator{
 		vm,
 	}
@@ -36,6 +39,10 @@ func (sc *StepContext) NewExpressionEvaluator() ExpressionEvaluator {
 	configers := []func(*otto.Otto){
 		sc.vmEnv(),
 		sc.vmInputs(),
+
+		sc.vmNeeds(),
+		sc.vmSuccess(),
+		sc.vmFailure(),
 	}
 	for _, configer := range configers {
 		configer(vm)
@@ -373,14 +380,33 @@ func (rc *RunContext) vmHashFiles() func(*otto.Otto) {
 func (rc *RunContext) vmSuccess() func(*otto.Otto) {
 	return func(vm *otto.Otto) {
 		_ = vm.Set("success", func() bool {
-			return rc.getJobContext().Status == "success"
+			jobs := rc.Run.Workflow.Jobs
+			jobNeeds := rc.Run.Job().Needs()
+
+			for _, needs := range jobNeeds {
+				if jobs[needs].Result != "success" {
+					return false
+				}
+			}
+
+			return true
 		})
 	}
 }
+
 func (rc *RunContext) vmFailure() func(*otto.Otto) {
 	return func(vm *otto.Otto) {
 		_ = vm.Set("failure", func() bool {
-			return rc.getJobContext().Status == "failure"
+			jobs := rc.Run.Workflow.Jobs
+			jobNeeds := rc.Run.Job().Needs()
+
+			for _, needs := range jobNeeds {
+				if jobs[needs].Result == "failure" {
+					return true
+				}
+			}
+
+			return false
 		})
 	}
 }
@@ -440,9 +466,9 @@ func (sc *StepContext) vmInputs() func(*otto.Otto) {
 	}
 }
 
-func (rc *RunContext) vmNeeds() func(*otto.Otto) {
-	jobs := rc.Run.Workflow.Jobs
-	jobNeeds := rc.Run.Job().Needs()
+func (sc *StepContext) vmNeeds() func(*otto.Otto) {
+	jobs := sc.RunContext.Run.Workflow.Jobs
+	jobNeeds := sc.RunContext.Run.Job().Needs()
 
 	using := make(map[string]map[string]map[string]string)
 	for _, needs := range jobNeeds {
@@ -457,6 +483,70 @@ func (rc *RunContext) vmNeeds() func(*otto.Otto) {
 	}
 }
 
+func (sc *StepContext) vmSuccess() func(*otto.Otto) {
+	return func(vm *otto.Otto) {
+		_ = vm.Set("success", func() bool {
+			return sc.RunContext.getJobContext().Status == "success"
+		})
+	}
+}
+
+func (sc *StepContext) vmFailure() func(*otto.Otto) {
+	return func(vm *otto.Otto) {
+		_ = vm.Set("failure", func() bool {
+			return sc.RunContext.getJobContext().Status == "failure"
+		})
+	}
+}
+
+type vmNeedsStruct struct {
+	Outputs map[string]string `json:"outputs"`
+	Result  string            `json:"result"`
+}
+
+func (rc *RunContext) vmNeeds() func(*otto.Otto) {
+	return func(vm *otto.Otto) {
+		needsFunc := func() otto.Value {
+			jobs := rc.Run.Workflow.Jobs
+			jobNeeds := rc.Run.Job().Needs()
+
+			using := make(map[string]vmNeedsStruct)
+			for _, needs := range jobNeeds {
+				using[needs] = vmNeedsStruct{
+					Outputs: jobs[needs].Outputs,
+					Result:  jobs[needs].Result,
+				}
+			}
+
+			log.Debugf("context needs => %+v", using)
+
+			value, err := vm.ToValue(using)
+			if err != nil {
+				return vm.MakeTypeError(err.Error())
+			}
+
+			return value
+		}
+
+		// Results might change after the Otto VM was created
+		// and initialized. To access the current state
+		// we can't just pass a copy to Otto - instead we
+		// created a 'live-binding'.
+		// Technical Note: We don't want to pollute the global
+		// js namespace (and add things github actions hasn't)
+		// we delete the helper function after installing it
+		// as a getter.
+		global, _ := vm.Run("this")
+		_ = global.Object().Set("__needs__", needsFunc)
+		_, _ = vm.Run(`
+			(function (global) {
+				Object.defineProperty(global, 'needs', { get: global.__needs__ });
+				delete global.__needs__;
+			})(this)
+		`)
+	}
+}
+
 func (rc *RunContext) vmJob() func(*otto.Otto) {
 	job := rc.getJobContext()
 
@@ -508,3 +598,48 @@ func (rc *RunContext) vmMatrix() func(*otto.Otto) {
 		_ = vm.Set("matrix", rc.Matrix)
 	}
 }
+
+// EvalBool evaluates an expression against given evaluator
+func EvalBool(evaluator ExpressionEvaluator, expr string) (bool, error) {
+	if splitPattern == nil {
+		splitPattern = regexp.MustCompile(fmt.Sprintf(`%s|%s|\S+`, expressionPattern.String(), operatorPattern.String()))
+	}
+	if strings.HasPrefix(strings.TrimSpace(expr), "!") {
+		return false, errors.New("expressions starting with ! must be wrapped in ${{ }}")
+	}
+	if expr != "" {
+		parts := splitPattern.FindAllString(expr, -1)
+		var evaluatedParts []string
+		for i, part := range parts {
+			if operatorPattern.MatchString(part) {
+				evaluatedParts = append(evaluatedParts, part)
+				continue
+			}
+
+			interpolatedPart, isString := evaluator.InterpolateWithStringCheck(part)
+
+			// This peculiar transformation has to be done because the GitHub parser
+			// treats false returned from contexts as a string, not a boolean.
+			// Hence env.SOMETHING will be evaluated to true in an if: expression
+			// regardless if SOMETHING is set to false, true or any other string.
+			// It also handles some other weirdness that I found by trial and error.
+			if (expressionPattern.MatchString(part) && // it is an expression
+				!strings.Contains(part, "!")) && // but it's not negated
+				interpolatedPart == "false" && // and the interpolated string is false
+				(isString || previousOrNextPartIsAnOperator(i, parts)) { // and it's of type string or has an logical operator before or after
+				interpolatedPart = fmt.Sprintf("'%s'", interpolatedPart) // then we have to quote the false expression
+			}
+
+			evaluatedParts = append(evaluatedParts, interpolatedPart)
+		}
+
+		joined := strings.Join(evaluatedParts, " ")
+		v, _, err := evaluator.Evaluate(fmt.Sprintf("Boolean(%s)", joined))
+		if err != nil {
+			return false, err
+		}
+		log.Debugf("expression '%s' evaluated to '%s'", expr, v)
+		return v == "true", nil
+	}
+	return true, nil
+}
diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go
@@ -3,7 +3,6 @@ package runner
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -229,7 +228,20 @@ func (rc *RunContext) Executor() common.Executor {
 		}
 		steps = append(steps, rc.newStepExecutor(step))
 	}
-	steps = append(steps, rc.stopJobContainer())
+	steps = append(steps, func(ctx context.Context) error {
+		err := rc.stopJobContainer()(ctx)
+		if err != nil {
+			return err
+		}
+
+		rc.Run.Job().Result = "success"
+		_, ok := ctx.Value(common.ErrorContextKeyVal).(error)
+		if ok {
+			rc.Run.Job().Result = "failure"
+		}
+
+		return nil
+	})
 
 	return common.NewPipelineExecutor(steps...).If(rc.isEnabled)
 }
@@ -245,7 +257,7 @@ func (rc *RunContext) newStepExecutor(step *model.Step) common.Executor {
 			Success: true,
 			Outputs: make(map[string]string),
 		}
-		runStep, err := rc.EvalBool(sc.Step.If.Value)
+		runStep, err := EvalBool(sc.NewExpressionEvaluator(), sc.Step.If.Value)
 
 		if err != nil {
 			common.Logger(ctx).Errorf("  \u274C  Error in if: expression - %s", sc.Step)
@@ -270,7 +282,19 @@ func (rc *RunContext) newStepExecutor(step *model.Step) common.Executor {
 		rc.ExprEval = exprEval
 
 		common.Logger(ctx).Infof("\u2B50  Run %s", sc.Step)
-		err = sc.Executor().Then(sc.interpolateOutputs())(ctx)
+		err = sc.Executor().Then(func(ctx context.Context) error {
+			err := sc.interpolateOutputs()(ctx)
+			if err != nil {
+				return err
+			}
+
+			err, ok := ctx.Value(common.ErrorContextKeyVal).(error)
+			if !ok {
+				return nil
+			}
+
+			return err
+		})(ctx)
 		if err == nil {
 			common.Logger(ctx).Infof("  \u2705  Success - %s", sc.Step)
 		} else {
@@ -336,7 +360,7 @@ func (rc *RunContext) hostname() string {
 func (rc *RunContext) isEnabled(ctx context.Context) bool {
 	job := rc.Run.Job()
 	l := common.Logger(ctx)
-	runJob, err := rc.EvalBool(job.If.Value)
+	runJob, err := EvalBool(rc.ExprEval, job.If.Value)
 	if err != nil {
 		common.Logger(ctx).Errorf("  \u274C  Error in if: expression - %s", job.Name)
 		return false
@@ -363,51 +387,6 @@ func (rc *RunContext) isEnabled(ctx context.Context) bool {
 
 var splitPattern *regexp.Regexp
 
-// EvalBool evaluates an expression against current run context
-func (rc *RunContext) EvalBool(expr string) (bool, error) {
-	if splitPattern == nil {
-		splitPattern = regexp.MustCompile(fmt.Sprintf(`%s|%s|\S+`, expressionPattern.String(), operatorPattern.String()))
-	}
-	if strings.HasPrefix(strings.TrimSpace(expr), "!") {
-		return false, errors.New("expressions starting with ! must be wrapped in ${{ }}")
-	}
-	if expr != "" {
-		parts := splitPattern.FindAllString(expr, -1)
-		var evaluatedParts []string
-		for i, part := range parts {
-			if operatorPattern.MatchString(part) {
-				evaluatedParts = append(evaluatedParts, part)
-				continue
-			}
-
-			interpolatedPart, isString := rc.ExprEval.InterpolateWithStringCheck(part)
-
-			// This peculiar transformation has to be done because the GitHub parser
-			// treats false returned from contexts as a string, not a boolean.
-			// Hence env.SOMETHING will be evaluated to true in an if: expression
-			// regardless if SOMETHING is set to false, true or any other string.
-			// It also handles some other weirdness that I found by trial and error.
-			if (expressionPattern.MatchString(part) && // it is an expression
-				!strings.Contains(part, "!")) && // but it's not negated
-				interpolatedPart == "false" && // and the interpolated string is false
-				(isString || previousOrNextPartIsAnOperator(i, parts)) { // and it's of type string or has an logical operator before or after
-				interpolatedPart = fmt.Sprintf("'%s'", interpolatedPart) // then we have to quote the false expression
-			}
-
-			evaluatedParts = append(evaluatedParts, interpolatedPart)
-		}
-
-		joined := strings.Join(evaluatedParts, " ")
-		v, _, err := rc.ExprEval.Evaluate(fmt.Sprintf("Boolean(%s)", joined))
-		if err != nil {
-			return false, err
-		}
-		log.Debugf("expression '%s' evaluated to '%s'", expr, v)
-		return v == "true", nil
-	}
-	return true, nil
-}
-
 func previousOrNextPartIsAnOperator(i int, parts []string) bool {
 	operator := false
 	if i > 0 {

diff --git a/pkg/runner/run_context_test.go b/pkg/runner/run_context_test.go
@@ -148,7 +148,7 @@ func TestRunContext_EvalBool(t *testing.T) {
 		t.Run(table.in, func(t *testing.T) {
 			assertObject := assert.New(t)
 			defer hook.Reset()
-			b, err := rc.EvalBool(table.in)
+			b, err := EvalBool(rc.ExprEval, table.in)
 			if table.wantErr {
 				assertObject.Error(err)
 			}

diff --git a/pkg/runner/runner.go b/pkg/runner/runner.go
@@ -2,6 +2,7 @@ package runner
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"path/filepath"
@@ -163,7 +164,16 @@ func (runner *runnerImpl) NewPlanExecutor(plan *model.Plan) common.Executor {
 		}
 	}
 
-	return common.NewPipelineExecutor(pipeline...)
+	return common.NewPipelineExecutor(pipeline...).Then(func(ctx context.Context) error {
+		for _, stage := range plan.Stages {
+			for _, run := range stage.Runs {
+				if run.Job().Result == "failure" {
+					return errors.New(fmt.Sprintf("Job '%s' failed", run.String()))
+				}
+			}
+		}
+		return nil
+	})
 }
 
 func (runner *runnerImpl) newRunContext(run *model.Run, matrix map[string]interface{}) *RunContext {