From 3f4998a4ede1327d85fe19ff7f02c79ca4cd441b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torbj=C3=B8rn=20Vatn?= Date: Thu, 12 Nov 2020 17:15:09 +0100 Subject: [PATCH] envs in if: - take 2 (#412) * Test more if env variants * The correct negation syntax is != * Make the Interpolate function support negated booleans from envs * Move assert := a.New(t) into t.Run This uncovered that some of the test premisses was wrong and the Eval Bool function also had flaws * Remove a stray logrus import --- go.mod | 2 +- pkg/runner/expression.go | 30 +++++++++++-------- pkg/runner/expression_test.go | 27 ++++++++++------- pkg/runner/run_context.go | 54 +++++++++++++++++++++++++++++++--- pkg/runner/run_context_test.go | 27 +++++++++++++---- 5 files changed, 107 insertions(+), 33 deletions(-) diff --git a/go.mod b/go.mod index 357c914..3d7d321 100644 --- a/go.mod +++ b/go.mod @@ -30,7 +30,7 @@ require ( github.com/morikuni/aec v1.0.0 // indirect github.com/opencontainers/go-digest v1.0.0-rc1 // indirect github.com/opencontainers/image-spec v1.0.1 // indirect - github.com/opencontainers/runc v0.1.1 // indirect + github.com/opencontainers/runc v0.1.1 github.com/pkg/errors v0.8.1 github.com/robertkrimen/otto v0.0.0-20191219234010-c382bd3c16ff github.com/sabhiram/go-gitignore v0.0.0-20180611051255-d3107576ba94 diff --git a/pkg/runner/expression.go b/pkg/runner/expression.go index eccfddd..bf19265 100644 --- a/pkg/runner/expression.go +++ b/pkg/runner/expression.go @@ -12,7 +12,6 @@ import ( "strings" "github.com/robertkrimen/otto" - "github.com/sirupsen/logrus" log "github.com/sirupsen/logrus" "gopkg.in/godo.v2/glob" ) @@ -20,7 +19,7 @@ import ( var contextPattern, expressionPattern *regexp.Regexp func init() { - contextPattern = regexp.MustCompile(`^(\w+(?:\[.+\])*)(?:\.([\w-]+))?(.*)$`) + contextPattern = regexp.MustCompile(`^(\w+(?:\[.+])*)(?:\.([\w-]+))?(.*)$`) expressionPattern = regexp.MustCompile(`\${{\s*(.+?)\s*}}`) } @@ -62,7 +61,7 @@ type expressionEvaluator struct { func (ee *expressionEvaluator) Evaluate(in string) (string, error) { re := ee.Rewrite(in) if re != in { - logrus.Debugf("Evaluating '%s' instead of '%s'", re, in) + log.Debugf("Evaluating '%s' instead of '%s'", re, in) } val, err := ee.vm.Run(re) @@ -83,15 +82,20 @@ func (ee *expressionEvaluator) Interpolate(in string) string { out = expressionPattern.ReplaceAllStringFunc(in, func(match string) string { // Extract and trim the actual expression inside ${{...}} delimiters expression := expressionPattern.ReplaceAllString(match, "$1") + // Evaluate the expression and retrieve errors if any - evaluated, err := ee.Evaluate(expression) + negatedExpression := strings.HasPrefix(expression, "!") + evaluated, err := ee.Evaluate(strings.ReplaceAll(expression, "!", "")) if err != nil { errList = append(errList, err) } + if negatedExpression { + evaluated = fmt.Sprintf("!%s", evaluated) + } return evaluated }) if len(errList) > 0 { - logrus.Errorf("Unable to interpolate string '%s' - %v", in, errList) + log.Errorf("Unable to interpolate string '%s' - %v", in, errList) break } if out == in { @@ -211,7 +215,7 @@ func vmToJSON(vm *otto.Otto) { toJSON := func(o interface{}) string { rtn, err := json.MarshalIndent(o, "", " ") if err != nil { - logrus.Errorf("Unable to marshal: %v", err) + log.Errorf("Unable to marshal: %v", err) return "" } return string(rtn) @@ -225,7 +229,7 @@ func vmFromJSON(vm *otto.Otto) { var dat map[string]interface{} err := json.Unmarshal([]byte(str), &dat) if err != nil { - logrus.Errorf("Unable to unmarshal: %v", err) + log.Errorf("Unable to unmarshal: %v", err) return dat } return dat @@ -237,11 +241,11 @@ func vmFromJSON(vm *otto.Otto) { func (rc *RunContext) vmHashFiles() func(*otto.Otto) { return func(vm *otto.Otto) { _ = vm.Set("hashFiles", func(paths ...string) string { - files := []*glob.FileAsset{} + var files []*glob.FileAsset for i := range paths { newFiles, _, err := glob.Glob([]string{filepath.Join(rc.Config.Workdir, paths[i])}) if err != nil { - logrus.Errorf("Unable to glob.Glob: %v", err) + log.Errorf("Unable to glob.Glob: %v", err) return "" } files = append(files, newFiles...) @@ -250,11 +254,13 @@ func (rc *RunContext) vmHashFiles() func(*otto.Otto) { for _, file := range files { f, err := os.Open(file.Path) if err != nil { - logrus.Errorf("Unable to os.Open: %v", err) + log.Errorf("Unable to os.Open: %v", err) } - defer f.Close() if _, err := io.Copy(hasher, f); err != nil { - logrus.Errorf("Unable to io.Copy: %v", err) + log.Errorf("Unable to io.Copy: %v", err) + } + if err := f.Close(); err != nil { + log.Errorf("Unable to Close file: %v", err) } } return hex.EncodeToString(hasher.Sum(nil)) diff --git a/pkg/runner/expression_test.go b/pkg/runner/expression_test.go index 817a8d1..5f841ca 100644 --- a/pkg/runner/expression_test.go +++ b/pkg/runner/expression_test.go @@ -8,7 +8,6 @@ import ( ) func TestEvaluate(t *testing.T) { - assert := a.New(t) rc := &RunContext{ Config: &Config{ Workdir: ".", @@ -105,6 +104,7 @@ func TestEvaluate(t *testing.T) { for _, table := range tables { table := table t.Run(table.in, func(t *testing.T) { + assert := a.New(t) out, err := ee.Evaluate(table.in) if table.errMesg == "" { assert.NoError(err, table.in) @@ -118,15 +118,16 @@ func TestEvaluate(t *testing.T) { } func TestInterpolate(t *testing.T) { - assert := a.New(t) rc := &RunContext{ Config: &Config{ Workdir: ".", }, Env: map[string]string{ - "keywithnothing": "valuewithnothing", - "key-with-hyphens": "value-with-hyphens", - "key_with_underscores": "value_with_underscores", + "KEYWITHNOTHING": "valuewithnothing", + "KEY-WITH-HYPHENS": "value-with-hyphens", + "KEY_WITH_UNDERSCORES": "value_with_underscores", + "TRUE": "true", + "FALSE": "false", }, Run: &model.Run{ JobID: "job1", @@ -144,15 +145,20 @@ func TestInterpolate(t *testing.T) { out string }{ {" ${{1}} to ${{2}} ", " 1 to 2 "}, - {" ${{ env.keywithnothing }} ", " valuewithnothing "}, - {" ${{ env.key-with-hyphens }} ", " value-with-hyphens "}, - {" ${{ env.key_with_underscores }} ", " value_with_underscores "}, - {"${{ env.unknown }}", ""}, + {" ${{ env.KEYWITHNOTHING }} ", " valuewithnothing "}, + {" ${{ env.KEY-WITH-HYPHENS }} ", " value-with-hyphens "}, + {" ${{ env.KEY_WITH_UNDERSCORES }} ", " value_with_underscores "}, + {"${{ env.UNKNOWN }}", ""}, + {"${{ env.TRUE }}", "true"}, + {"${{ env.FALSE }}", "false"}, + {"${{ !env.TRUE }}", "!true"}, + {"${{ !env.FALSE }}", "!false"}, } for _, table := range tables { table := table t.Run(table.in, func(t *testing.T) { + assert := a.New(t) out := ee.Interpolate(table.in) assert.Equal(table.out, out, table.in) }) @@ -160,8 +166,6 @@ func TestInterpolate(t *testing.T) { } func TestRewrite(t *testing.T) { - assert := a.New(t) - rc := &RunContext{ Config: &Config{}, Run: &model.Run{ @@ -195,6 +199,7 @@ func TestRewrite(t *testing.T) { for _, table := range tables { table := table t.Run(table.in, func(t *testing.T) { + assert := a.New(t) re := ee.Rewrite(table.in) assert.Equal(table.re, re, table.in) }) diff --git a/pkg/runner/run_context.go b/pkg/runner/run_context.go index 89a61e6..0da5228 100644 --- a/pkg/runner/run_context.go +++ b/pkg/runner/run_context.go @@ -14,7 +14,6 @@ import ( "github.com/nektos/act/pkg/common" "github.com/nektos/act/pkg/model" - "github.com/sirupsen/logrus" log "github.com/sirupsen/logrus" ) @@ -255,8 +254,43 @@ func (rc *RunContext) isEnabled(ctx context.Context) bool { // EvalBool evaluates an expression against current run context func (rc *RunContext) EvalBool(expr string) bool { if expr != "" { - expr = fmt.Sprintf("Boolean(%s)", rc.ExprEval.Interpolate(expr)) - v, err := rc.ExprEval.Evaluate(expr) + interpolated := rc.ExprEval.Interpolate(expr) + parts := strings.Split(interpolated, " ") + + operatorRe := regexp.MustCompile("^[!=><|&]+$") + var evaluatedParts []string + for _, part := range parts { + part = fixNegation(part) + + if operatorRe.MatchString(part) { + evaluatedParts = append(evaluatedParts, part) + continue + } + + if strings.HasPrefix(part, "!") { + withoutNegation, err := rc.ExprEval.Evaluate(strings.ReplaceAll(part, "!", "")) + if err != nil { + return false + } + evaluatedParts = append(evaluatedParts, fmt.Sprintf("!%s", withoutNegation)) + continue + } + // strings with / are misinterpreted as a file path by otto + if strings.Contains(part, "/") { + evaluatedParts = append(evaluatedParts, part) + continue + } + evaluatedPart, err := rc.ExprEval.Evaluate(part) + if err != nil { + log.Errorf("Unable to evaluate part: %s: %v", part, err) + return false + } + evaluatedPart = fixQuotingForStrings(evaluatedPart) + evaluatedParts = append(evaluatedParts, evaluatedPart) + } + + boolExpr := fmt.Sprintf("Boolean(%s)", strings.Join(evaluatedParts, " ")) + v, err := rc.ExprEval.Evaluate(boolExpr) if err != nil { return false } @@ -266,6 +300,18 @@ func (rc *RunContext) EvalBool(expr string) bool { return true } +func fixNegation(s string) string { + re := regexp.MustCompile("![ ]+") + return re.ReplaceAllString(s, "!") +} + +func fixQuotingForStrings(s string) string { + if s == "true" || s == "false" { + return s + } + return fmt.Sprintf("'%s'", s) +} + func mergeMaps(maps ...map[string]string) map[string]string { rtnMap := make(map[string]string) for _, m := range maps { @@ -403,7 +449,7 @@ func (rc *RunContext) getGithubContext() *githubContext { if rc.EventJSON != "" { err = json.Unmarshal([]byte(rc.EventJSON), &ghc.Event) if err != nil { - logrus.Errorf("Unable to Unmarshal event '%s': %v", rc.EventJSON, err) + log.Errorf("Unable to Unmarshal event '%s': %v", rc.EventJSON, err) } } diff --git a/pkg/runner/run_context_test.go b/pkg/runner/run_context_test.go index 759f54c..9b9c945 100644 --- a/pkg/runner/run_context_test.go +++ b/pkg/runner/run_context_test.go @@ -1,6 +1,7 @@ package runner import ( + "fmt" "github.com/nektos/act/pkg/model" a "github.com/stretchr/testify/assert" "testing" @@ -10,7 +11,6 @@ import ( func TestRunContext_EvalBool(t *testing.T) { hook := test.NewGlobal() - assert := a.New(t) rc := &RunContext{ Config: &Config{ Workdir: ".", @@ -58,29 +58,45 @@ func TestRunContext_EvalBool(t *testing.T) { // The basic ones {"true", true}, {"false", false}, - {"1 !== 0", true}, - {"1 !== 1", false}, + {"1 != 0", true}, + {"1 != 1", false}, {"1 == 0", false}, {"1 == 1", true}, {"1 > 2", false}, {"1 < 2", true}, {"success()", true}, {"failure()", false}, + {"always()", true}, + {"failure()", false}, // And or {"true && false", false}, {"true && 1 < 2", true}, {"false || 1 < 2", true}, {"false || false", false}, // None boolable - {"env.SOME_TEXT", true}, {"env.UNKNOWN == 'true'", false}, {"env.UNKNOWN", false}, // Inline expressions + {"env.SOME_TEXT", true}, // this is because Boolean('text') is true in Javascript + {"env.SOME_TEXT == 'text'", true}, {"env.TRUE == 'true'", true}, {"env.FALSE == 'true'", false}, + {"env.TRUE", true}, + {"env.FALSE", false}, + {"!env.TRUE", false}, + {"!env.FALSE", true}, + {"${{ env.TRUE }}", true}, + {"${{ env.FALSE }}", false}, + {"${{ !env.TRUE }}", false}, + {"${{ !env.FALSE }}", true}, + {"!env.TRUE && true", false}, + {"!env.FALSE && true", true}, + {"!env.TRUE || true", true}, + {"!env.FALSE || false", true}, {"${{env.TRUE == 'true'}}", true}, {"${{env.FALSE == 'true'}}", false}, {"${{env.FALSE == 'false'}}", true}, + // All together now {"false || env.TRUE == 'true'", true}, {"true || env.FALSE == 'true'", true}, @@ -97,10 +113,11 @@ func TestRunContext_EvalBool(t *testing.T) { for _, table := range tables { table := table t.Run(table.in, func(t *testing.T) { + assert := a.New(t) defer hook.Reset() b := rc.EvalBool(table.in) - assert.Equal(table.out, b, table.in) + assert.Equal(table.out, b, fmt.Sprintf("Expected %s to be %v, was %v", table.in, table.out, b)) assert.Empty(hook.LastEntry(), table.in) }) }