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

Replace Sarif JSON structs with owenrumney/go-sarif/sarif #460

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

atombrella
Copy link
Contributor

RELNOTE: Replace Sarif JSON structs with owenrumney/go-sarif/sarif

This addresses #447 It doesn't seem to require more than simply replacing the import, and deleting the go file with the structs. The library in favor contains a more elaborate definition of the structs for the Sarif standard.

Note that I haven't checked the code coverage for the --output-format sarif flag.

@atombrella atombrella force-pushed the feature/sarif_library branch from 79ce4ed to 7934df8 Compare April 16, 2022 19:56
@zegl
Copy link
Owner

zegl commented Apr 20, 2022

Looks like the code doesn't compile anymore, I'm guessing that we need to make some alterations to our sarif.go to make it fit how the library is supposed to be used.

# github.com/zegl/kube-score/renderer/sarif
renderer/sarif/sarif.go:15: undefined: sarif.Results
renderer/sarif/sarif.go:16: undefined: sarif.Rules
renderer/sarif/sarif.go:25: undefined: sarif.Rules
renderer/sarif/sarif.go:50: undefined: sarif.Results
renderer/sarif/sarif.go:79: undefined: sarif.Driver
renderer/sarif/sarif.go:86: undefined: sarif.Sarif

@atombrella
Copy link
Contributor Author

Looks like the code doesn't compile anymore, I'm guessing that we need to make some alterations to our sarif.go to make it fit how the library is supposed to be used.

# github.com/zegl/kube-score/renderer/sarif
renderer/sarif/sarif.go:15: undefined: sarif.Results
renderer/sarif/sarif.go:16: undefined: sarif.Rules
renderer/sarif/sarif.go:25: undefined: sarif.Rules
renderer/sarif/sarif.go:50: undefined: sarif.Results
renderer/sarif/sarif.go:79: undefined: sarif.Driver
renderer/sarif/sarif.go:86: undefined: sarif.Sarif

Yes. I'll have a look during the weekend to fix it. I've been a bit occupied the last few days to fix everything, and should have waited to open the PR.

@zegl
Copy link
Owner

zegl commented Apr 21, 2022

Ah, ok, no worries! :-D

@atombrella
Copy link
Contributor Author

atombrella commented Apr 23, 2022

I have something working locally 🎉 https://sarifweb.azurewebsites.net/Validation complains that the output doesn't contain the version property. Is this available somehow? Note the addition of the URL for the tool 👍

https://gist.github.com/atombrella/8371135f661e48b465d666a01801a31d is the generated output. I used

./kube-score score --output-format=sarif score/testdata/pod-probes-all-missing.yaml > errors.sarif

Looking at this, it'd be nice to include some tests for the generated output, but perhaps it's a bit out of scope for this PR. It also looks a bit funky with the line numbers that appear to be always 1.

sarif.NewSimpleArtifactLocation("file://" + v.FileLocation.Name),
).WithRegion(
sarif.NewSimpleRegion(
v.FileLocation.Line,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These appear to be 1 regardless of the test file I try with.

@atombrella atombrella force-pushed the feature/sarif_library branch from 455eb5e to 0dcbaf3 Compare June 4, 2022 06:48
@atombrella
Copy link
Contributor Author

@zegl Do you have time to give some feedback? There's an unanswered question for v.FileLocation.Line. Tests can be added if needed.

Regarding bors, I think it'd be nice to add use_squash_merge = true to the config-file to avoid loads of intermediate commits for this work. I can also squash before you merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants