Updates per review

Updates per @anderseknert's comments -- changes the --fail-defined behavior to match that seen in eval, and adds testing to cover the undefined case Signed-off-by: Byron Lagrone <byron.lagrone@seqster.com>
byronic · Oct 17, 2022 · e39b1ef · e39b1ef
1 parent ec9d85b
commit e39b1ef
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 15 deletions.
diff --git a/cmd/exec.go b/cmd/exec.go
@@ -68,7 +68,7 @@ e.g., opa exec --decision /foo/bar/baz ...`,
 	addConfigOverrides(cmd.Flags(), &params.ConfigOverrides)
 	addConfigOverrideFiles(cmd.Flags(), &params.ConfigOverrideFiles)
 	cmd.Flags().StringVarP(&params.Decision, "decision", "", "", "set decision to evaluate")
-	cmd.Flags().BoolVarP(&params.FailDefined, "fail-defined", "", false, "exits with non-zero exit code on false/non-empty result and errors")
+	cmd.Flags().BoolVarP(&params.FailDefined, "fail-defined", "", false, "exits with non-zero exit code on defined/non-empty result and errors")
 	cmd.Flags().VarP(params.LogLevel, "log-level", "l", "set log level")
 	cmd.Flags().Var(params.LogFormat, "log-format", "set log format")
 	cmd.Flags().StringVar(&params.LogTimestampFormat, "log-timestamp-format", "", "set log timestamp format (OPA_LOG_TIMESTAMP_FORMAT environment variable)")

diff --git a/cmd/exec_test.go b/cmd/exec_test.go
@@ -220,8 +220,8 @@ func TestFailDefinedFlagWithTrueBoolean(t *testing.T) {
 		params.FailDefined = true
 
 		err := runExec(params)
-		if err != nil {
-			t.Fatal(err)
+		if err == nil {
+			t.Fatal("expected error with failDefined, but none occurred")
 		}
 
 		output := util.MustUnmarshalJSON(bytes.ReplaceAll(buf.Bytes(), []byte(dir), nil))
@@ -274,3 +274,50 @@ func TestFailDefinedFlagWithPopulatedResult(t *testing.T) {
 
 	})
 }
+
+func TestFailDefinedFlagWithUndefinedResult(t *testing.T) {
+
+	files := map[string]string{
+		"files/test.json": `{"foo": 7}`,
+		"bundle/x.rego": `package system
+
+		test_fun := x {
+			x = false
+			x
+		}
+
+		undefined_test {
+			test_fun
+		}`,
+	}
+
+	test.WithTempFS(files, func(dir string) {
+
+		var buf bytes.Buffer
+		params := exec.NewParams(&buf)
+		_ = params.OutputFormat.Set("json")
+		params.BundlePaths = []string{dir + "/bundle/"}
+		params.Paths = append(params.Paths, dir+"/files/")
+		params.FailDefined = true
+
+		err := runExec(params)
+		if err != nil {
+			t.Fatal("undefined result should not fail")
+		}
+
+		output := util.MustUnmarshalJSON(bytes.ReplaceAll(buf.Bytes(), []byte(dir), nil))
+
+		exp := util.MustUnmarshalJSON([]byte(`{"result": [{
+			"path": "/files/test.json",
+			"error": {
+				"code": "opa_undefined_error",
+				"message": "/system/main decision was undefined"
+			  }
+		}]}`))
+
+		if !reflect.DeepEqual(output, exp) {
+			t.Fatal("Expected:", exp, "Got:", output)
+		}
+
+	})
+}
diff --git a/cmd/internal/exec/exec.go b/cmd/internal/exec/exec.go
@@ -27,7 +27,7 @@ type Params struct {
 	LogTimestampFormat  string         // log timestamp format for plugins
 	BundlePaths         []string       // explicit paths of bundles to inject into the configuration
 	Decision            string         // decision to evaluate (overrides default decision set by configuration)
-	FailDefined         bool           // exits with non-zero exit code on non-True result and errors
+	FailDefined         bool           // exits with non-zero exit code on defined/non-empty result and errors
 }
 
 func NewParams(w io.Writer) *Params {
@@ -83,7 +83,7 @@ func Exec(ctx context.Context, opa *sdk.OPA, params *Params) error {
 			if err2 := r.Report(result{Path: item.Path, Error: err}); err2 != nil {
 				return err2
 			}
-			if params.FailDefined {
+			if params.FailDefined && !sdk.IsUndefinedErr(err) {
 				errorCount++
 			}
 			continue
@@ -94,15 +94,7 @@ func Exec(ctx context.Context, opa *sdk.OPA, params *Params) error {
 		}
 
 		if params.FailDefined && rs.Result != nil {
-			// counts all non-boolean results as failures as well as boolean results that are false
-			switch typedResult := rs.Result.(type) {
-			case bool:
-				if !typedResult {
-					failCount++
-				}
-			default:
-				failCount++
-			}
+			failCount++
 		}
 	}
 

diff --git a/docs/content/cli.md b/docs/content/cli.md
@@ -590,7 +590,7 @@ opa exec <path> [<path> [...]] [flags]
   -c, --config-file string                   set path of configuration file
       --decision string                      set decision to evaluate
   -f, --format {pretty,json}                 set output format (default pretty)
-      --fail-defined                         exits with non-zero exit code on false/non-empty result and errors
+      --fail-defined                         exits with non-zero exit code on defined/non-empty result and errors
   -h, --help                                 help for exec
       --log-format {text,json,json-pretty}   set log format (default json)
   -l, --log-level {debug,info,error}         set log level (default error)