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 support for verbosity levels #727

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Conversation

kemzeb
Copy link
Contributor

@kemzeb kemzeb commented Dec 28, 2023

Inspired by what was discussed in #698, this PR makes the necessary (breaking) changes to the reporter package such that the Reporter interface now provides methods for printing at certain levels (i.e. ErrorLevel, WarnLevel, InfoLevel, and VerboseLevel). It also adds a --verbosity option so that users/automated tools can take advantage of this too.

@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2023

Codecov Report

Attention: 50 lines in your changes are missing coverage. Please review.

Comparison is base (f1412ee) 78.35% compared to head (53808a2) 78.65%.

Files Patch % Lines
pkg/osvscanner/osvscanner.go 32.25% 21 Missing ⚠️
cmd/osv-reporter/main.go 0.00% 9 Missing ⚠️
internal/sourceanalysis/rust.go 14.28% 6 Missing ⚠️
pkg/reporter/void_reporter.go 14.28% 6 Missing ⚠️
internal/local/check.go 25.00% 3 Missing ⚠️
cmd/osv-scanner/main.go 83.33% 1 Missing and 1 partial ⚠️
internal/sourceanalysis/go.go 0.00% 2 Missing ⚠️
pkg/osvscanner/vulnerability_result.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #727      +/-   ##
==========================================
+ Coverage   78.35%   78.65%   +0.29%     
==========================================
  Files          85       86       +1     
  Lines        6044     6109      +65     
==========================================
+ Hits         4736     4805      +69     
+ Misses       1103     1098       -5     
- Partials      205      206       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kemzeb kemzeb force-pushed the verbosity-levels branch 3 times, most recently from 7bb2465 to 251ccc9 Compare December 29, 2023 01:21
@kemzeb
Copy link
Contributor Author

kemzeb commented Dec 29, 2023

Not sure if I resolved all the problem lines that Codecov complained about as it has been silent with my recent force pushes; making this PR ready for review

@kemzeb kemzeb marked this pull request as ready for review December 29, 2023 01:32
@another-rex another-rex self-requested a review December 29, 2023 01:33
Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Thanks! Overall looks good to me.

pkg/reporter/table_reporter.go Outdated Show resolved Hide resolved
pkg/reporter/table_reporter.go Outdated Show resolved Hide resolved
cmd/osv-scanner/main.go Outdated Show resolved Hide resolved
@another-rex
Copy link
Collaborator

I just merged #722 which would have caused some conflicts, should be straightforward to resolve as it only removes unused print functions.

@kemzeb kemzeb force-pushed the verbosity-levels branch 4 times, most recently from b392107 to acf6f30 Compare January 2, 2024 09:13
@kemzeb
Copy link
Contributor Author

kemzeb commented Jan 2, 2024

I believe I made all the necessary changes. I also changed some of the Reporter documentation because I felt Infof()'s comment would apply to all the other verbosity-related methods (well besides Errorf()). Please let me know if there are any further problems!

@kemzeb
Copy link
Contributor Author

kemzeb commented Jan 2, 2024

Recent change has pkg/reporter/gh-annotations_reporter_test.go create the correct reporter (was creating JSONReporter). It does seem like there is a lot of test code duplication; let me know if this may be a problem

@kemzeb
Copy link
Contributor Author

kemzeb commented Jan 2, 2024

Also just realized that the CodeCov bot just modifies its comment when I make changes to my branch instead of creating a new comment.

Since the changes I made to the reporter has also changed older areas of the codebase, it is complaining about these areas not being covered. Not sure what I should do; should I try to write coverage tests for these older areas?

@another-rex
Copy link
Collaborator

Also just realized that the CodeCov bot just modifies its comment when I make changes to my branch instead of creating a new comment.

Since the changes I made to the reporter has also changed older areas of the codebase, it is complaining about these areas not being covered. Not sure what I should do; should I try to write coverage tests for these older areas?

No need, as long as the new code has good coverage you're good, and it looks like it does :).

@another-rex
Copy link
Collaborator

Recent change has pkg/reporter/gh-annotations_reporter_test.go create the correct reporter (was creating JSONReporter). It does seem like there is a lot of test code duplication; let me know if this may be a problem

Yeah it is quite a bit of duplication, I think eventually we want to decouple the results reporting from the general info/warn/err printing, but that is something for V2.

For now I don't have any ideas for reducing this without over complicating the tests.

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

ooh love getting a new bikeshed for me to lure people into through follow up PRs :D

Just got a bug to fix and a recommended change, and then this should be good to go - I'm also pretty sure I know a clean way to deduplicate the tests, but it's not that important

pkg/reporter/gh-annotations_reporter.go Outdated Show resolved Hide resolved
pkg/reporter/reporter.go Outdated Show resolved Hide resolved
@kemzeb kemzeb force-pushed the verbosity-levels branch 2 times, most recently from 2e4e9e0 to daff216 Compare January 8, 2024 02:33
@kemzeb
Copy link
Contributor Author

kemzeb commented Jan 8, 2024

(Recent change updates some of my test error messages that are out-of-date)

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Looks good to me, though it'd be great if you could chuck in a couple of basic CLI tests (cmd/osv-scanner/main_test.go) with different verbosity levels - among other things, that'll catch if setting HasErrored is ever mucked up

@kemzeb
Copy link
Contributor Author

kemzeb commented Jan 9, 2024

Looks good to me, though it'd be great if you could chuck in a couple of basic CLI tests (cmd/osv-scanner/main_test.go) with different verbosity levels - among other things, that'll catch if setting HasErrored is ever mucked up

Sure thing, making the changes now

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Thanks!

@another-rex another-rex enabled auto-merge (squash) January 9, 2024 05:19
@another-rex another-rex merged commit 4090b04 into google:main Jan 9, 2024
11 checks passed
@kemzeb kemzeb deleted the verbosity-levels branch January 9, 2024 05:30
another-rex added a commit that referenced this pull request Jan 24, 2024
Update govet printf settings for #727
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.

4 participants