Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add JUnit output format #929

Merged
merged 1 commit into from
Jul 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -599,12 +599,13 @@ are:
from the linter report
- `sarif` - [SARIF](https://sarifweb.azurewebsites.net/) JSON output, for consumption by tools processing code analysis
reports
- `junit` - JUnit XML output, e.g. for CI servers like GitLab that show these results in a merge request.

## OPA Check and Strict Mode

Linting with Regal assumes syntactically correct Rego. If there are errors parsing any files during linting, the
process is aborted and any parser errors are logged similarly to OPA. OPA itself provides a "linter" of sorts,
via the `opa check` comand and its `--strict` flag. This checks the provided Rego files not only for syntax errors,
via the `opa check` command and its `--strict` flag. This checks the provided Rego files not only for syntax errors,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

but also for OPA [strict mode](https://www.openpolicyagent.org/docs/latest/policy-language/#strict-mode) violations.

> **Note** It is recommended to run `opa check --strict` as part of your policy build process, and address any violations
Expand Down
2 changes: 2 additions & 0 deletions cmd/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ const (
formatFestive = "festive"
// formatSarif is the SARIF format value for the --format flag in various commands.
formatSarif = "sarif"
// formatJunit is the JUnit format value for the --format flag in various commands.
formatJunit = "junit"
)
27 changes: 27 additions & 0 deletions cmd/lint.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cmd

import (
"bytes"
"encoding/json"
"errors"
"fmt"
Expand All @@ -12,6 +13,7 @@ import (
"time"

"github.com/fatih/color"
"github.com/jstemmer/go-junit-report/v2/junit"
"github.com/spf13/cobra"
"gopkg.in/yaml.v3"

Expand Down Expand Up @@ -372,6 +374,8 @@ func getReporter(format string, outputWriter io.Writer) (reporter.Reporter, erro
return reporter.NewFestiveReporter(outputWriter), nil
case formatSarif:
return reporter.NewSarifReporter(outputWriter), nil
case formatJunit:
return reporter.NewJUnitReporter(outputWriter), nil
default:
return nil, fmt.Errorf("unknown format %s", format)
}
Expand Down Expand Up @@ -406,6 +410,29 @@ func formatError(format string, err error) error {
}

return fmt.Errorf("%s", string(bs))
} else if format == formatJunit {
testSuites := junit.Testsuites{
Name: "regal",
}
testsuite := junit.Testsuite{
Name: "lint",
}
testsuite.AddTestcase(junit.Testcase{
Name: "Command execution failed",
Error: &junit.Result{
Message: err.Error(),
},
})
testSuites.AddSuite(testsuite)

buf := &bytes.Buffer{}

err := testSuites.WriteXML(buf)
if err != nil {
return fmt.Errorf("failed to format errors for output: %w", err)
}

return fmt.Errorf("%s", buf.String())
}

return err
Expand Down
9 changes: 7 additions & 2 deletions docs/cicd.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,14 @@ regal_lint_policies:
name: ghcr.io/styrainc/regal:latest
entrypoint: ['/bin/sh', '-c']
script:
- regal lint ./policy
- regal lint ./policy --format junit > regal-results.xml
artifacts:
reports:
junit: regal-results.xml
when: always
rules:
- if: '$CI_PIPELINE_SOURCE == "merge_request_event"'
```

The above will run Regal on the `policy` directory when a merge request is created or updated.
The above will run Regal on the `policy` directory when a merge request is created or updated and will show linting
violations as part of the merge request.
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ require (
github.com/fsnotify/fsnotify v1.7.0
github.com/gobwas/glob v0.2.3
github.com/google/go-cmp v0.6.0
github.com/jstemmer/go-junit-report/v2 v2.1.0
github.com/mitchellh/mapstructure v1.5.0
github.com/olekukonko/tablewriter v0.0.5
github.com/open-policy-agent/opa v0.66.0
Expand Down
3 changes: 3 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ github.com/golang/snappy v0.0.4/go.mod h1:/XxbfmMg8lxefKM7IXC3fBNl/7bRcc72aCRzEW
github.com/google/flatbuffers v24.3.25+incompatible h1:CX395cjN9Kke9mmalRoL3d81AtFUxJM+yDthflgJGkI=
github.com/google/flatbuffers v24.3.25+incompatible/go.mod h1:1AeVuKshWv4vARoZatz6mlQ0JxURH0Kv5+zNeJKJCa8=
github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU=
github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
Expand All @@ -90,6 +91,8 @@ github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0/go.mod h1:YN5jB8ie0yfIUg6VvR9K
github.com/ianlancetaylor/demangle v0.0.0-20210905161508-09a460cdf81d/go.mod h1:aYm2/VgdVmcIU8iMfdMvDMsRAQjcfZSKFby6HOFvi/w=
github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8=
github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw=
github.com/jstemmer/go-junit-report/v2 v2.1.0 h1:X3+hPYlSczH9IMIpSC9CQSZA0L+BipYafciZUWHEmsc=
github.com/jstemmer/go-junit-report/v2 v2.1.0/go.mod h1:mgHVr7VUo5Tn8OLVr1cKnLuEy0M92wdRntM99h7RkgQ=
github.com/klauspost/compress v1.17.0 h1:Rnbp4K9EjcDuVuHtd0dgA4qNuv9yKDYKK1ulpJwgrqM=
github.com/klauspost/compress v1.17.0/go.mod h1:ntbaceVETuRiXiv4DpjP66DpAtAGkEQskQzEyD//IeE=
github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
Expand Down
59 changes: 59 additions & 0 deletions pkg/reporter/reporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import (
"fmt"
"io"
"os"
"sort"
"strings"

"github.com/fatih/color"
"github.com/jstemmer/go-junit-report/v2/junit"
"github.com/olekukonko/tablewriter"
"github.com/owenrumney/go-sarif/v2/sarif"

Expand Down Expand Up @@ -53,6 +55,12 @@ type SarifReporter struct {
out io.Writer
}

// JUnitReporter reports violations in the JUnit XML format
// (https://github.com/junit-team/junit5/blob/main/platform-tests/src/test/resources/jenkins-junit.xsd).
type JUnitReporter struct {
out io.Writer
}

// NewPrettyReporter creates a new PrettyReporter.
func NewPrettyReporter(out io.Writer) PrettyReporter {
return PrettyReporter{out: out}
Expand Down Expand Up @@ -83,6 +91,11 @@ func NewSarifReporter(out io.Writer) SarifReporter {
return SarifReporter{out: out}
}

// NewJUnitReporter creates a new JUnitReporter.
func NewJUnitReporter(out io.Writer) JUnitReporter {
return JUnitReporter{out: out}
}

// Publish prints a pretty report to the configured output.
func (tr PrettyReporter) Publish(_ context.Context, r report.Report) error {
table := buildPrettyViolationsTable(r.Violations)
Expand Down Expand Up @@ -423,3 +436,49 @@ func getUniqueViolationURLs(violations []report.Violation) map[string]string {

return urls
}

// Publish prints a JUnit XML report to the configured output.
func (tr JUnitReporter) Publish(_ context.Context, r report.Report) error {
testSuites := junit.Testsuites{
Name: "regal",
}

// group by file & sort by file
files := make([]string, 0)
violationsPerFile := map[string][]report.Violation{}

for _, violation := range r.Violations {
files = append(files, violation.Location.File)
violationsPerFile[violation.Location.File] = append(violationsPerFile[violation.Location.File], violation)
}

sort.Strings(files)

for _, file := range files {
testsuite := junit.Testsuite{
Name: file,
}

for _, violation := range violationsPerFile[file] {
testsuite.AddTestcase(junit.Testcase{
Name: fmt.Sprintf("%s/%s: %s", violation.Category, violation.Title, violation.Description),
Classname: violation.Location.String(),
Failure: &junit.Result{
Message: fmt.Sprintf("%s. To learn more, see: %s", violation.Description, getDocumentationURL(violation)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most notable omission here would be the category... and understandably so given that it's a test result format after all. If there isn't some clever meta attribute we can use for that, perhaps we could format the Name like $category/$title: $message, or perhaps have the Message include it?

Knowing the category helps with prioritization of fixing the issues reported, so it'd be good to have it in there somewhere, but again, if there's some other place to put it, I'm open to suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah good idea! What would $message be though? Maybe just the description of a violation and have the Message contain both description and link to the docs?

I think we can add any kind of additional metadata to the Data field of a junit.Result. It's currently not used in the code but we could write all the violation details in there. I'll try it out on our instance and post screenshots to further the discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shows the test report widget inside a merge request:

2024-07-22T10:15:42,541965099+02:00

that's the popup you get when clicking on View details:

2024-07-22T10:15:59,187202303+02:00

and this is what the Full report looks like

2024-07-22T10:16:15,244480379+02:00

when you click on View details in the full report, you get the same popup as in the merge request

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Where does the category come from there? You've updated your code to show that? I don't see it in the PR, but that looks great to me, so please do include it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

er yeah I pushed it to another branch so not to pollute this PR. I also just realized that GitLab no longer shows the failure message which included the link to the documentation. So the latest version now includes the documentation link and its output is aligned with the pretty printer. It looks like this:

2024-07-22T12:50:15,296173773+02:00

It's kinda sad that GitLab formats this output as a code block so links are not clickable but it think there is no way around that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but not much to do about it I suppose. GitHub doesn't make it a link either, even if it's not in a codeblock. In that context we can at least display real markdown links in the summary though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great though!

Type: violation.Level,
Data: fmt.Sprintf("Rule: %s\nDescription: %s\nCategory: %s\nLocation: %s\nText: %s\nDocumentation: %s",
violation.Title,
violation.Description,
violation.Category,
violation.Location.String(),
strings.TrimSpace(*violation.Location.Text),
getDocumentationURL(violation)),
},
})
}

testSuites.AddSuite(testsuite)
}

return testSuites.WriteXML(tr.out)
}
62 changes: 62 additions & 0 deletions pkg/reporter/reporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -595,3 +595,65 @@ func TestSarifReporterPublishNoViolations(t *testing.T) {
t.Errorf("expected %s, got %s", expect, buf.String())
}
}

//nolint:lll // the expected output is unfortunately longer than the allowed max line length
func TestJUnitReporterPublish(t *testing.T) {
t.Parallel()

var buf bytes.Buffer

sr := NewJUnitReporter(&buf)

err := sr.Publish(context.Background(), rep)
if err != nil {
t.Fatal(err)
}

expect := `<testsuites name="regal" tests="2" failures="2">
<testsuite name="a.rego" tests="1" failures="1" errors="0" id="0" time="">
<testcase name="legal/breaking-the-law: Rego must not break the law!" classname="a.rego:1:1">
<failure message="Rego must not break the law!. To learn more, see: https://example.com/illegal" type="error"><![CDATA[Rule: breaking-the-law
Description: Rego must not break the law!
Category: legal
Location: a.rego:1:1
Text: package illegal
Documentation: https://example.com/illegal]]></failure>
</testcase>
</testsuite>
<testsuite name="b.rego" tests="1" failures="1" errors="0" id="0" time="">
<testcase name="really?/questionable-decision: Questionable decision found" classname="b.rego:22:18">
<failure message="Questionable decision found. To learn more, see: https://example.com/questionable" type="warning"><![CDATA[Rule: questionable-decision
Description: Questionable decision found
Category: really?
Location: b.rego:22:18
Text: default allow = true
Documentation: https://example.com/questionable]]></failure>
</testcase>
</testsuite>
</testsuites>
`

if buf.String() != expect {
t.Errorf("expected \n%s, got \n%s", expect, buf.String())
}
}

func TestJUnitReporterPublishNoViolations(t *testing.T) {
t.Parallel()

var buf bytes.Buffer

sr := NewJUnitReporter(&buf)

err := sr.Publish(context.Background(), report.Report{})
if err != nil {
t.Fatal(err)
}

expect := `<testsuites name="regal"></testsuites>
`

if buf.String() != expect {
t.Errorf("expected \n%s, got \n%s", expect, buf.String())
}
}