google / osv-scanner

Vulnerability scanner written in Go which uses the data provided by https://osv.dev
https://google.github.io/osv-scanner/
Apache License 2.0
6.15k stars 347 forks source link

chore: make guided remediation follow revive's default lint rules #1259

Closed michaelkedar closed 1 week ago

michaelkedar commented 1 week ago

Helping with #1257 Changed the guided remediation code (cmd/osv-scanner/fix, and internal/resolution, remediation and tui) to fix lint errors found when using revive's default settings. All of this is internal, so there's no API breakages.

It was mostly unexported-return and stuttering complaints (e.g. resolution.ResolutionResult -> resolution.Result), so a bunch of structs have been renamed.

codecov-commenter commented 1 week ago

Codecov Report

Attention: Patch coverage is 64.18605% with 77 lines in your changes missing coverage. Please review.

Project coverage is 68.30%. Comparing base (1856add) to head (91f842d).

Files with missing lines Patch % Lines
internal/tui/vuln-list.go 0.00% 12 Missing :warning:
scripts/generate_mock_resolution_universe/main.go 0.00% 10 Missing :warning:
cmd/osv-scanner/fix/state-relock-result.go 0.00% 8 Missing :warning:
cmd/osv-scanner/fix/state-choose-strategy.go 0.00% 7 Missing :warning:
internal/tui/vuln-info.go 0.00% 7 Missing :warning:
cmd/osv-scanner/fix/state-in-place-result.go 0.00% 6 Missing :warning:
cmd/osv-scanner/fix/noninteractive.go 82.75% 2 Missing and 3 partials :warning:
cmd/osv-scanner/fix/model.go 0.00% 4 Missing :warning:
internal/resolution/datasource/npm_registry.go 62.50% 3 Missing :warning:
internal/tui/relock-info.go 0.00% 3 Missing :warning:
... and 8 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1259 +/- ## ========================================== + Coverage 68.29% 68.30% +0.01% ========================================== Files 175 175 Lines 16764 16764 ========================================== + Hits 11449 11451 +2 + Misses 4682 4681 -1 + Partials 633 632 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

G-Rath commented 1 week ago

fwiw I've written up my thoughts on this on the issue - overall my main comment here is that I think it would be best to see if you can get the now-not-violated revive rules enabled for these files/packages to prevent future regressions and to let us know what in these files still needs actioning via explicitly inline disables.

michaelkedar commented 1 week ago

overall my main comment here is that I think it would be best to see if you can get the now-not-violated revive rules enabled for these files/packages to prevent future regressions and to let us know what in these files still needs actioning via explicitly inline disables.

I can't seem to work out good a way to only enable revive rules for specific files (apparently you can add an exclude: do individual rules, but I can't get that to work.

Best I can do is add something like

issues:
  exclude-rules:
    - path-except: internal/(resolution|remediation|tui)/
      linters:
        - revive

But that's not great, and it'll disable the indent-error-flow lint that we currently have enabled for everything else...

I can go disable the violating rules inline everywhere else, but tbh it'd be easier to fix most of the violations instead (and I wanted to limit the scope of this PR)