add JUnit output format #929
Conversation
| Name: violation.Title, | ||
| Classname: violation.Location.String(), | ||
| Failure: &junit.Result{ | ||
| Message: fmt.Sprintf("%s. To learn more, see: %s", violation.Description, getDocumentationURL(violation)), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This shows the test report widget inside a merge request:
that's the popup you get when clicking on View details:
and this is what the Full report looks like
when you click on View details in the full report, you get the same popup as in the merge request
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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:
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
There was a problem hiding this comment.
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.
cmd/lint.go
Outdated
| // currently, JSON and SARIF will get the same generic JSON error format | ||
| if format == formatJSON || format == formatSarif { | ||
| // currently, JSON, SARIF, and JUnit will get the same generic JSON error format | ||
| if format == formatJSON || format == formatSarif || format == formatJunit { |
There was a problem hiding this comment.
I would probably have expected the errors to be reported as "JUnit errors"... but I guess we'd need to update the reporters to allow for that. We'll deal with that later.
There was a problem hiding this comment.
OK thanks for pinging me about this - I wasn't really sure why this code exists. In my understanding, this function is only called if the lint command itself failed. As a user I would not expect to get any kind of formatted report for that but rather a harsh error message telling me to fix my setup before regal can work properly.
That said, I could add proper JUnit XML output here as well. Besides Failure it does contain an Error field that could be used here..
There was a problem hiding this comment.
Yeah, but the lint command can fail for reasons like parser errors too. Ideally, other tools like opa test would have failed the build before linting in that case, but still. In most of the contexts where e.g. SARIF is used, like in CI/CD pipelines, users would likely want to find the report in the same place regardless of what made it fail, but yeah, there's a reason we've not exactly made this our highest priority too.
There was a problem hiding this comment.
OK OK sounds good - will add JUnit output here as well!
There was a problem hiding this comment.
If non-trivial to pull off, it's alright to defer this to later. We should look into better error reporting altogether sometime soon anyway.
There was a problem hiding this comment.
I think the latest version is a reasonable start - it basically just creates a single testsuite with a single testcase that contains the given error string.
|
Sorry, I didn't think of this before, but can you please update this section in the README too? And I suppose this guide could use an update too so that GitLab users can benefit from this. |
|
|
||
| ## 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, |
There was a problem hiding this comment.
Thanks a lot!
@charlieegan3 take a look if you want, or feel free to merge directly.
Signed-off-by: Sebastian Hoß <seb@xn--ho-hia.de>
|
OK I rebased on latest main, fixed all linting errors, and squashed all commits. Good to go from my side ^^ |
Signed-off-by: Sebastian Hoß <seb@xn--ho-hia.de>
fixes #928
The output groups violations by files in a testsuite and expresses each violation inside a file as a testcase. The files are sorted so that we have stable output during tests and each failure message contains the documentation link for a failed rule.
I'm not entirely sure whether notices should be part of the output as well. I've skipped them because most other reporters do not output them either.