feat: Refactor GitHub outputter to support locations from Rego policy

output/github.go

@@ -1,12 +1,21 @@
 package output
 
 import (
+	"encoding/json"
 	"fmt"
 	"io"
 
 	"github.com/open-policy-agent/opa/v1/tester"
 )
 
+type githubLevel string
+
+const (
+	githubInfo  githubLevel = "notice"
+	githubWarn  githubLevel = "warning"
+	githubError githubLevel = "error"
+)
+
 // GitHub represents an Outputter that outputs
 // results in GitHub workflow format.
 // https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions
@@ -24,78 +33,85 @@ func NewGitHub(w io.Writer) *GitHub {
 }
 
 // Output outputs the results.
-func (t *GitHub) Output(checkResults CheckResults) error {
+func (g *GitHub) Output(checkResults CheckResults) error {
 	var totalFailures int
 	var totalExceptions int
 	var totalWarnings int
 	var totalSuccesses int
 	var totalSkipped int
 	for _, result := range checkResults {
-		totalPolicies := result.Successes + len(result.Failures) + len(result.Warnings) + len(result.Exceptions) + len(result.Skipped)
+		totalFailures += len(result.Failures)
+		totalExceptions += len(result.Exceptions)
+		totalWarnings += len(result.Warnings)
+		totalSkipped += len(result.Skipped)
+		totalSuccesses += result.Successes
+
+		numPolicies := result.Successes + len(result.Failures) + len(result.Warnings) + len(result.Exceptions) + len(result.Skipped)
 
-		fmt.Fprintf(t.writer, "::group::Testing '%v' against %v policies in namespace '%v'\n", result.FileName, totalPolicies, result.Namespace)
+		fileLoc := &Location{File: result.FileName, Line: json.Number("1")}
+
+		g.writeLn("::group::Testing %q against %d policies in namespace %q", result.FileName, numPolicies, result.Namespace)
 		for _, failure := range result.Failures {
-			fmt.Fprintf(t.writer, "::error file=%v,line=1::%v\n", result.FileName, failure.Message)
+			g.writeLocs(githubError, fileLoc, failure.Location, failure.Message)
 		}
-
 		for _, warning := range result.Warnings {
-			fmt.Fprintf(t.writer, "::warning file=%v,line=1::%v\n", result.FileName, warning.Message)
+			g.writeLocs(githubWarn, fileLoc, warning.Location, warning.Message)
 		}
-
 		for _, exception := range result.Exceptions {
-			fmt.Fprintf(t.writer, "::notice file=%v,line=1::%v\n", result.FileName, exception.Message)
+			g.writeLocs(githubInfo, fileLoc, exception.Location, exception.Message)
 		}
-
 		for _, skipped := range result.Skipped {
-			fmt.Fprintf(t.writer, "skipped file=%v %v\n", result.FileName, skipped.Message)
+			g.writeLocs(githubInfo, fileLoc, skipped.Location, "Test was skipped: %s", skipped.Message)
 		}
-
-		if result.Successes > 0 {
-			fmt.Fprintf(t.writer, "success file=%v %v\n", result.FileName, result.Successes)
-		}
-
-		totalFailures += len(result.Failures)
-		totalExceptions += len(result.Exceptions)
-		totalWarnings += len(result.Warnings)
-		totalSkipped += len(result.Skipped)
-		totalSuccesses += result.Successes
-		fmt.Fprintf(t.writer, "::endgroup::\n")
+		g.writeLoc(githubInfo, fileLoc, "Number of successful checks: %d", result.Successes)
+		g.writeLn("::endgroup::")
 	}
 
 	totalTests := totalFailures + totalExceptions + totalWarnings + totalSuccesses + totalSkipped
 
-	var pluralSuffixTests string
-	if totalTests != 1 {
-		pluralSuffixTests = "s"
-	}
+	g.writeLn("%d %s, %d passed, %d %s, %d %s, %d %s",
+		totalTests, plural("test", totalTests),
+		totalSuccesses,
+		totalWarnings, plural("warning", totalWarnings),
+		totalFailures, plural("failure", totalFailures),
+		totalExceptions, plural("exception", totalExceptions),
+	)
 
-	var pluralSuffixWarnings string
-	if totalWarnings != 1 {
-		pluralSuffixWarnings = "s"
-	}
+	return nil
+}
 
-	var pluralSuffixFailures string
-	if totalFailures != 1 {
-		pluralSuffixFailures = "s"
-	}
+func (g *GitHub) writeLn(msg string, args ...any) {
+	fmt.Fprintf(g.writer, msg+"\n", args...)
+}
 
-	var pluralSuffixExceptions string
-	if totalExceptions != 1 {
-		pluralSuffixExceptions = "s"
+func (g *GitHub) writeLoc(level githubLevel, loc *Location, msg string, args ...any) {
+	msg = fmt.Sprintf("::%s file=%s,line=%s::%s", level, loc.File, loc.Line, msg)
+	g.writeLn(msg, args...)
+}
+
+func (g *GitHub) writeLocs(level githubLevel, fileLoc, ogLoc *Location, msg string, args ...any) {
+	// If no location was specified by the policy, default to the file location.
+	if ogLoc == nil {
+		g.writeLoc(level, fileLoc, msg, args...)
+		return
 	}
 
-	outputText := fmt.Sprintf("%v test%s, %v passed, %v warning%s, %v failure%s, %v exception%s",
-		totalTests, pluralSuffixTests,
-		totalSuccesses,
-		totalWarnings, pluralSuffixWarnings,
-		totalFailures, pluralSuffixFailures,
-		totalExceptions, pluralSuffixExceptions,
-	)
-	fmt.Fprintln(t.writer, outputText)
+	// If in the same file, prefer the location produced by the Rego policy.
+	if ogLoc.File == fileLoc.File {
+		g.writeLoc(level, ogLoc, msg, args...)
+		return
+	}
 
-	return nil
+	// If different files, produce messages for both locations.
+	// Always produce a relattive path as some inputs may be a long absolute path.
+	og := &Location{
+		File: relPath(ogLoc.File),
+		Line: ogLoc.Line,
+	}
+	g.writeLoc(level, og, msg, args...)
+	g.writeLoc(level, fileLoc, fmt.Sprintf("(ORIGINATING FROM %s) %s", og, msg), args...)
 }
 
-func (t *GitHub) Report(_ []*tester.Result, _ string) error {
+func (g *GitHub) Report(_ []*tester.Result, _ string) error {
 	return fmt.Errorf("report is not supported in GitHub output")
 }

output/github_test.go

@@ -2,8 +2,11 @@ package output
 
 import (
 	"bytes"
+	"encoding/json"
 	"strings"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 func TestGitHub(t *testing.T) {
@@ -21,7 +24,8 @@ func TestGitHub(t *testing.T) {
 				},
 			},
 			expected: []string{
-				"::group::Testing 'examples/kubernetes/service.yaml' against 0 policies in namespace 'namespace'",
+				"::group::Testing \"examples/kubernetes/service.yaml\" against 0 policies in namespace \"namespace\"",
+				"::notice file=examples/kubernetes/service.yaml,line=1::Number of successful checks: 0",
 				"::endgroup::",
 				"0 tests, 0 passed, 0 warnings, 0 failures, 0 exceptions",
 				"",
@@ -38,30 +42,33 @@ func TestGitHub(t *testing.T) {
 				},
 			},
 			expected: []string{
-				"::group::Testing 'examples/kubernetes/service.yaml' against 2 policies in namespace 'namespace'",
+				"::group::Testing \"examples/kubernetes/service.yaml\" against 2 policies in namespace \"namespace\"",
 				"::error file=examples/kubernetes/service.yaml,line=1::first failure",
 				"::warning file=examples/kubernetes/service.yaml,line=1::first warning",
+				"::notice file=examples/kubernetes/service.yaml,line=1::Number of successful checks: 0",
 				"::endgroup::",
 				"2 tests, 0 passed, 1 warning, 1 failure, 0 exceptions",
 				"",
 			},
 		},
 		{
-			name: "mixed failure, warnings and skipped",
+			name: "mixed failure, warnings, successes and skipped",
 			input: CheckResults{
 				{
 					FileName:  "examples/kubernetes/service.yaml",
 					Namespace: "namespace",
 					Failures:  []Result{{Message: "first failure"}},
 					Skipped:   []Result{{Message: "first skipped"}},
+					Successes: 10,
 				},
 			},
 			expected: []string{
-				"::group::Testing 'examples/kubernetes/service.yaml' against 2 policies in namespace 'namespace'",
+				"::group::Testing \"examples/kubernetes/service.yaml\" against 12 policies in namespace \"namespace\"",
 				"::error file=examples/kubernetes/service.yaml,line=1::first failure",
-				"skipped file=examples/kubernetes/service.yaml first skipped",
+				"::notice file=examples/kubernetes/service.yaml,line=1::Test was skipped: first skipped",
+				"::notice file=examples/kubernetes/service.yaml,line=1::Number of successful checks: 10",
 				"::endgroup::",
-				"2 tests, 0 passed, 0 warnings, 1 failure, 0 exceptions",
+				"12 tests, 10 passed, 0 warnings, 1 failure, 0 exceptions",
 				"",
 			},
 		},
@@ -75,8 +82,58 @@ func TestGitHub(t *testing.T) {
 				},
 			},
 			expected: []string{
-				"::group::Testing '-' against 1 policies in namespace 'namespace'",
+				"::group::Testing \"-\" against 1 policies in namespace \"namespace\"",
 				"::error file=-,line=1::first failure",
+				"::notice file=-,line=1::Number of successful checks: 0",
+				"::endgroup::",
+				"1 test, 0 passed, 0 warnings, 1 failure, 0 exceptions",
+				"",
+			},
+		},
+		{
+			name: "handles location in the same file",
+			input: CheckResults{
+				{
+					FileName:  "foo.json",
+					Namespace: "main",
+					Failures: []Result{{
+						Message: "first failure",
+						Location: &Location{
+							File: "foo.json",
+							Line: json.Number("10"),
+						},
+					}},
+				},
+			},
+			expected: []string{
+				"::group::Testing \"foo.json\" against 1 policies in namespace \"main\"",
+				"::error file=foo.json,line=10::first failure",
+				"::notice file=foo.json,line=1::Number of successful checks: 0",
+				"::endgroup::",
+				"1 test, 0 passed, 0 warnings, 1 failure, 0 exceptions",
+				"",
+			},
+		},
+		{
+			name: "handles location outside of the file",
+			input: CheckResults{
+				{
+					FileName:  "foo.json",
+					Namespace: "main",
+					Failures: []Result{{
+						Message: "first failure",
+						Location: &Location{
+							File: "../other_file.json",
+							Line: json.Number("10"),
+						},
+					}},
+				},
+			},
+			expected: []string{
+				"::group::Testing \"foo.json\" against 1 policies in namespace \"main\"",
+				"::error file=../other_file.json,line=10::first failure",
+				"::error file=foo.json,line=1::(ORIGINATING FROM ../other_file.json L10) first failure",
+				"::notice file=foo.json,line=1::Number of successful checks: 0",
 				"::endgroup::",
 				"1 test, 0 passed, 0 warnings, 1 failure, 0 exceptions",
 				"",
@@ -92,11 +149,8 @@ func TestGitHub(t *testing.T) {
 			if err := NewGitHub(buf).Output(tt.input); err != nil {
 				t.Fatal("output GitHub:", err)
 			}
-
-			actual := buf.String()
-
-			if expected != actual {
-				t.Errorf("unexpected output. expected %v actual %v", expected, actual)
+			if diff := cmp.Diff(buf.String(), expected); diff != "" {
+				t.Errorf("GitHub.Output() produced an unexpected diff:\n%s", diff)
 			}
 		})
 	}

output/output.go

@@ -2,6 +2,7 @@ package output
 
 import (
 	"os"
+	"path/filepath"
 
 	"github.com/open-policy-agent/opa/v1/tester"
 )
@@ -133,3 +134,25 @@ func Outputs() []string {
 		OutputSARIF,
 	}
 }
+
+func plural(msg string, n int) string {
+	if n != 1 {
+		return msg + "s"
+	}
+	return msg
+}
+
+func relPath(path string) string {
+	if !filepath.IsAbs(path) {
+		return path
+	}
+	cwd, err := os.Getwd()
+	if err != nil {
+		return path
+	}
+	rel, err := filepath.Rel(cwd, path)
+	if err != nil {
+		return path
+	}
+	return rel
+}