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

SARIF: level using incorrect value notice #513

Closed
travbale opened this issue Dec 5, 2023 · 6 comments · Fixed by #516
Closed

SARIF: level using incorrect value notice #513

travbale opened this issue Dec 5, 2023 · 6 comments · Fixed by #516

Comments

@travbale
Copy link

travbale commented Dec 5, 2023

Per SARIF §3.27.10 the value of level must be warning, error, note, or none.

The implict-future-keywords rule is setting to value to notice. Other rules may be impacted but I have observed it on that one specfically.

@travbale travbale changed the title SARIF: level using incorrect value notice SARIF: level using incorrect value notice Dec 5, 2023
@anderseknert
Copy link
Member

Thanks @travbale 👍 Looks like you're right, and we should translate our "notice" level to "note" for SARIF. Any tooling that breaks because of this?

@travbale
Copy link
Author

travbale commented Dec 5, 2023

SARIF Viewer for VsCode flags it as an error in the SARIF document.

anderseknert added a commit that referenced this issue Dec 5, 2023
And don't include notices on deprecated/obsolete rules at all in report.

Fixes #513

Signed-off-by: Anders Eknert <anders@styra.com>
@anderseknert anderseknert linked a pull request Dec 5, 2023 that will close this issue
@anderseknert
Copy link
Member

Thanks again @travbale 👍 PRs up for both issues. That the implicit-future-keywords notice was included at all was a mistake of its own. There are essentially two types of notices at this point in time: 1) rules that have been excluded because some new capability or other rule made them obsolete and 2) rules that have been excluded because the targeted OPA version does not come with some more modern capability required by the rule.

Only the latter is interesting to most users, so that's the type we'll want to include in the reports (SARIF or not).

@travbale
Copy link
Author

travbale commented Dec 5, 2023

Thanks for the quick turn around! If I see anything else I will let you know.

@anderseknert
Copy link
Member

Thanks @travbale! You could go build from main once these PRs are merged if you'd want to test it with the known issues resolved before the next release (~2 weeks or so from now).

charlieegan3 pushed a commit that referenced this issue Dec 7, 2023
And don't include notices on deprecated/obsolete rules at all in report.

Fixes #513

Signed-off-by: Anders Eknert <anders@styra.com>
charlieegan3 pushed a commit that referenced this issue Dec 7, 2023
And don't include notices on deprecated/obsolete rules at all in report.

Fixes #513

Signed-off-by: Anders Eknert <anders@styra.com>
@anderseknert
Copy link
Member

Fixed with #516

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 a pull request may close this issue.

2 participants