From c24cfc72f40cb336927296dac30e38d77d8b48d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Brauer?= Date: Fri, 25 Feb 2022 19:39:50 +0100 Subject: [PATCH] Expression evaluator fixes (#1009) * refactor: remove debug error output Errors should always be logged with an error level and not debug level. Since the error is returned here, it will be logged later as an error. Presumably this was a leftover from debugging the executor chain in: PR: #971 * refactor: debug log wich expression is going to be evaluated * fix: handle nil in EvalBool We've seen this issue when the env map is not set-up properly, i.e. when the env map is nil, EvalBool might return nil, which should be handled as a falsy value. * fix: fail on error in if expression and return the evaluation error Stop running the workflow in case an expression cannot be evaluated. Fixes: #1008 * fix: remove quotes from inside expression syntax in test It looks like having an expression inside double quotes inside the expression syntax is not valid: https://github.com/ZauberNerd/act-test/actions/runs/1881986429 The workflow is not valid. .github/workflows/test.yml (Line: 10, Col: 13): Unexpected symbol: '"endsWith'. Located at position 1 within expression: "endsWith('Hello world', 'ld')" * refactor: export IsTruthy function from exprparser package * refactor: use IsTruthy function in EvalBool * refactor: move debug log for expression rewrite to rewrite function Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> --- pkg/common/executor.go | 1 - pkg/exprparser/interpreter.go | 14 +++---- pkg/runner/expression.go | 44 ++++----------------- pkg/runner/run_context.go | 24 +++++++---- pkg/runner/step_context.go | 8 +--- pkg/runner/testdata/issue-597/spelling.yaml | 6 +-- pkg/runner/testdata/uses-composite/push.yml | 4 +- 7 files changed, 37 insertions(+), 64 deletions(-) diff --git a/pkg/common/executor.go b/pkg/common/executor.go index cd92a6c..5e9b28d 100644 --- a/pkg/common/executor.go +++ b/pkg/common/executor.go @@ -136,7 +136,6 @@ func (e Executor) Then(then Executor) Executor { case Warning: log.Warning(err.Error()) default: - log.Debugf("%+v", err) return err } } diff --git a/pkg/exprparser/interpreter.go b/pkg/exprparser/interpreter.go index 584999b..df73fb4 100644 --- a/pkg/exprparser/interpreter.go +++ b/pkg/exprparser/interpreter.go @@ -274,7 +274,7 @@ func (impl *interperterImpl) evaluateNot(notNode *actionlint.NotOpNode) (interfa return nil, err } - return !impl.isTruthy(reflect.ValueOf(operand)), nil + return !IsTruthy(operand), nil } func (impl *interperterImpl) evaluateCompare(compareNode *actionlint.CompareOpNode) (interface{}, error) { @@ -434,7 +434,8 @@ func (impl *interperterImpl) compareNumber(left float64, right float64, kind act } } -func (impl *interperterImpl) isTruthy(value reflect.Value) bool { +func IsTruthy(input interface{}) bool { + value := reflect.ValueOf(input) switch value.Kind() { case reflect.Bool: return value.Bool() @@ -452,10 +453,7 @@ func (impl *interperterImpl) isTruthy(value reflect.Value) bool { return value.Float() != 0 - case reflect.Map: - return true - - case reflect.Slice: + case reflect.Map, reflect.Slice: return true default: @@ -503,14 +501,14 @@ func (impl *interperterImpl) evaluateLogicalCompare(compareNode *actionlint.Logi switch compareNode.Kind { case actionlint.LogicalOpNodeKindAnd: - if impl.isTruthy(leftValue) { + if IsTruthy(left) { return impl.getSafeValue(rightValue), nil } return impl.getSafeValue(leftValue), nil case actionlint.LogicalOpNodeKindOr: - if impl.isTruthy(leftValue) { + if IsTruthy(left) { return impl.getSafeValue(leftValue), nil } diff --git a/pkg/runner/expression.go b/pkg/runner/expression.go index b19e8bd..5be4f9e 100644 --- a/pkg/runner/expression.go +++ b/pkg/runner/expression.go @@ -2,7 +2,6 @@ package runner import ( "fmt" - "math" "regexp" "strings" @@ -128,7 +127,9 @@ type expressionEvaluator struct { } func (ee expressionEvaluator) evaluate(in string, isIfExpression bool) (interface{}, error) { + log.Debugf("evaluating expression '%s'", in) evaluated, err := ee.interpreter.Evaluate(in, isIfExpression) + log.Debugf("expression '%s' evaluated to '%t'", in, evaluated) return evaluated, err } @@ -141,9 +142,6 @@ func (ee expressionEvaluator) evaluateScalarYamlNode(node *yaml.Node) error { 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 @@ -214,18 +212,12 @@ func (ee expressionEvaluator) Interpolate(in string) string { } expr, _ := rewriteSubExpression(in, true) - if in != expr { - log.Debugf("expression '%s' rewritten to '%s'", in, expr) - } - evaluated, err := ee.evaluate(expr, false) if err != nil { log.Errorf("Unable to interpolate expression '%s': %s", expr, err) return "" } - log.Debugf("expression '%s' evaluated to '%s'", expr, evaluated) - value, ok := evaluated.(string) if !ok { panic(fmt.Sprintf("Expression %s did not evaluate to a string", expr)) @@ -237,37 +229,13 @@ func (ee expressionEvaluator) Interpolate(in string) string { // EvalBool evaluates an expression against given evaluator func EvalBool(evaluator ExpressionEvaluator, expr string) (bool, error) { nextExpr, _ := rewriteSubExpression(expr, false) - if expr != nextExpr { - log.Debugf("expression '%s' rewritten to '%s'", expr, nextExpr) - } evaluated, err := evaluator.evaluate(nextExpr, true) if err != nil { return false, err } - var result bool - - switch t := evaluated.(type) { - case bool: - result = t - case string: - result = t != "" - case int: - result = t != 0 - case float64: - if math.IsNaN(t) { - result = false - } else { - result = t != 0 - } - default: - return false, fmt.Errorf("Unable to map return type to boolean for '%s'", expr) - } - - log.Debugf("expression '%s' evaluated to '%t'", nextExpr, result) - - return result, nil + return exprparser.IsTruthy(evaluated), nil } func escapeFormatString(in string) string { @@ -334,5 +302,9 @@ func rewriteSubExpression(in string, forceFormat bool) (string, error) { return in, nil } - return fmt.Sprintf("format('%s', %s)", strings.ReplaceAll(formatOut, "'", "''"), strings.Join(results, ", ")), nil + out := fmt.Sprintf("format('%s', %s)", strings.ReplaceAll(formatOut, "'", "''"), strings.Join(results, ", ")) + if in != out { + log.Debugf("expression '%s' rewritten to '%s'", in, out) + } + return out, nil } diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index abe0716..2131e96 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -275,7 +275,18 @@ func (rc *RunContext) steps() []*model.Step { // Executor returns a pipeline executor for all the steps in the job func (rc *RunContext) Executor() common.Executor { - return newJobExecutor(rc).If(rc.isEnabled) + return func(ctx context.Context) error { + isEnabled, err := rc.isEnabled(ctx) + if err != nil { + return err + } + + if isEnabled { + return newJobExecutor(rc)(ctx) + } + + return nil + } } // Executor returns a pipeline executor for all the steps in the job @@ -405,17 +416,16 @@ func (rc *RunContext) hostname() string { return *hostname } -func (rc *RunContext) isEnabled(ctx context.Context) bool { +func (rc *RunContext) isEnabled(ctx context.Context) (bool, error) { job := rc.Run.Job() l := common.Logger(ctx) 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 + return false, fmt.Errorf(" \u274C Error in if-expression: \"if: %s\" (%s)", job.If.Value, err) } if !runJob { l.Debugf("Skipping job '%s' due to '%s'", job.Name, job.If.Value) - return false + return false, nil } img := rc.platformImage() @@ -428,9 +438,9 @@ func (rc *RunContext) isEnabled(ctx context.Context) bool { platformName := rc.ExprEval.Interpolate(runnerLabel) l.Infof("\U0001F6A7 Skipping unsupported platform -- Try running with `-P %+v=...`", platformName) } - return false + return false, nil } - return true + return true, nil } func mergeMaps(maps ...map[string]string) map[string]string { diff --git a/pkg/runner/step_context.go b/pkg/runner/step_context.go index b9de160..51ce945 100644 --- a/pkg/runner/step_context.go +++ b/pkg/runner/step_context.go @@ -171,13 +171,7 @@ func (sc *StepContext) interpolateEnv(exprEval ExpressionEvaluator) { func (sc *StepContext) isEnabled(ctx context.Context) (bool, error) { runStep, err := EvalBool(sc.NewExpressionEvaluator(), sc.Step.If.Value) if err != nil { - common.Logger(ctx).Errorf(" \u274C Error in if: expression - %s", sc.Step) - exprEval, err := sc.setupEnv(ctx) - if err != nil { - return false, err - } - sc.RunContext.ExprEval = exprEval - return false, err + return false, fmt.Errorf(" \u274C Error in if-expression: \"if: %s\" (%s)", sc.Step.If.Value, err) } return runStep, nil diff --git a/pkg/runner/testdata/issue-597/spelling.yaml b/pkg/runner/testdata/issue-597/spelling.yaml index 280bdbf..d594dcb 100644 --- a/pkg/runner/testdata/issue-597/spelling.yaml +++ b/pkg/runner/testdata/issue-597/spelling.yaml @@ -1,10 +1,10 @@ name: issue-597 on: push - + jobs: my_first_job: - + runs-on: ubuntu-latest steps: - name: My first false step @@ -14,7 +14,7 @@ jobs: ref: refs/pull/${{github.event.pull_request.number}}/merge fetch-depth: 5 - name: My first true step - if: ${{"endsWith('Hello world', 'ld')"}} + if: ${{endsWith('Hello world', 'ld')}} uses: actions/hello-world-javascript-action@main with: who-to-greet: "Renst the Octocat" diff --git a/pkg/runner/testdata/uses-composite/push.yml b/pkg/runner/testdata/uses-composite/push.yml index 598c3b4..bc66aff 100755 --- a/pkg/runner/testdata/uses-composite/push.yml +++ b/pkg/runner/testdata/uses-composite/push.yml @@ -17,7 +17,7 @@ jobs: secret_input: ${{secrets.test_input_optional || 'NO SUCH SECRET'}} env: secret_input: ${{secrets.test_input_optional || 'NO SUCH SECRET'}} - - if: steps.composite.outputs.test_output != "test_output_value" + - if: steps.composite.outputs.test_output != 'test_output_value' run: | echo "steps.composite.outputs.test_output=${{ steps.composite.outputs.test_output }}" exit 1 @@ -38,7 +38,7 @@ jobs: test_input_optional_with_default_overriden: 'test_input_optional_with_default_overriden' test_input_required_with_default_overriden: 'test_input_required_with_default_overriden' - - if: steps.composite2.outputs.test_output != "test_output_value" + - if: steps.composite2.outputs.test_output != 'test_output_value' run: | echo "steps.composite.outputs.test_output=${{ steps.composite2.outputs.test_output }}" exit 1