-
Notifications
You must be signed in to change notification settings - Fork 408
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
feat: warn if a vulnerability is ignored multiple times in the same config #1377
Conversation
2ca09c8
to
c67b7d5
Compare
{ | ||
name: "config files should not have multiple ignores with the same id", | ||
args: []string{"", "--config=./fixtures/osv-scanner-duplicate-config.toml", "./fixtures/locks-many"}, | ||
exit: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exit code 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only results in a warning which doesn't cause a non-zero exit code - I raised that as a thought in the description:
I've not had it error as it doesn't feel like a big enough deal to be worth erroring, though maybe that would be better as I can't think of an actual reason to have duplicate entries 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wait maybe it does, according to the tests? let me double check 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok not sure what happened there, but I've updated the fixture to ignore the vulnerability so now the exit code is zero, which I think is best to showcase that warnings don't result in a non-zero exit code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1377 +/- ##
==========================================
- Coverage 68.99% 68.99% -0.01%
==========================================
Files 186 186
Lines 18253 18262 +9
==========================================
+ Hits 12593 12599 +6
- Misses 4977 4979 +2
- Partials 683 684 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Currently if you have multiple ignores for a vulnerability, we just silently use the first one; this has us instead print a warning to make it more obvious when that occurs.
I've not had it error as it doesn't feel like a big enough deal to be worth erroring, though maybe that would be better as I can't think of an actual reason to have duplicate entries 🤔
Resolves #1367