spicedb icon indicating copy to clipboard operation
spicedb copied to clipboard

Enable all linters and resolve all outstanding lint issues (Resolves issue 2386)

Open James9074 opened this issue 8 months ago • 11 comments

As per https://github.com/authzed/spicedb/issues/2386, this PR uncomments gocritic, govet, and testifylint, and resolves all outstanding lint findings. Note that govet did not actually return anything, but gocritic and testifylint did. Those were resolved, along with a number of stubborn yaml comment spacing complaints within the github workflows yamls.

I generally resolved each finding in its own commit, for clarity. Additionally, I ran the full suite of unit tests after each fix and ensured no bloat in test execution time. The test suite took ~2.5 minutes before and after all changes on my machine.

Before:

See this for the raw output of all findings as of main/HEAD

After:

mage lint:all                                                                                                                                                    ✔  at 04:25:57 PM  
generating buf
generating go
Generating options for common.SchemaInformation...
Generated 1 options
Generating options for graph.ConcurrencyLimits...
Generated 1 options
Generating options for options.ExperimentalServerOptions...
Generated 1 options
Generating options for datastore.ConnPoolConfig...
Generated 1 options
Generating options for datastore.Config...
Generated 1 options
Generating options for datastore.RelIntegrityKey...
Generated 1 options
Generating options for server.CacheConfig...
Generated 1 options
Generating options for server.MiddlewareOption...
Generated 1 options
Generating options for server.Config...
Generated 1 options
Generating options for testserver.Config...
Generated 1 options
Generating options for util.GRPCServerConfig, HTTPServerConfig...
Generated 1 options
Generating options for options.QueryOptions, ReverseQueryOptions, RWTOptions...
Generated 1 options
Generating options for options.DeleteOptions...
Generated 1 options
running vulncheck
formatting go
running golangci-lint
running analyzers
0 issues.

James9074 avatar Jun 17 '25 14:06 James9074

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

github-actions[bot] avatar Jun 17 '25 14:06 github-actions[bot]

I have read the CLA Document and I hereby sign the CLA

James9074 avatar Jun 17 '25 14:06 James9074

@vroldanbet It's a wall of changes, but ready for you to look! I took great care in validating each one, ensuring it logically made sense and wouldn't change the spirit of each test or function. Feel free to suggest any changes - most are cut and dry but a few (mostly the Requires within a goroutine issues) could have been written a number of different ways.

James9074 avatar Jun 17 '25 20:06 James9074

Codecov Report

Attention: Patch coverage is 70.05988% with 50 lines in your changes missing coverage. Please review.

Project coverage is 77.35%. Comparing base (a97e9c3) to head (b1ee733).

Files with missing lines Patch % Lines
pkg/datastore/test/revisions.go 25.00% 8 Missing and 4 partials :warning:
pkg/development/validation.go 0.00% 10 Missing and 1 partial :warning:
internal/datastore/postgres/watch.go 30.00% 7 Missing :warning:
internal/datastore/mysql/watch.go 0.00% 4 Missing :warning:
pkg/datastore/test/watch.go 80.96% 4 Missing :warning:
internal/datastore/memdb/memdb.go 75.00% 3 Missing :warning:
pkg/cmd/server/server.go 50.00% 2 Missing :warning:
internal/datastore/postgres/xid8.go 0.00% 1 Missing :warning:
internal/testfixtures/datastore.go 0.00% 1 Missing :warning:
pkg/caveats/types/ipaddress.go 0.00% 1 Missing :warning:
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2455      +/-   ##
==========================================
- Coverage   77.39%   77.35%   -0.03%     
==========================================
  Files         414      414              
  Lines       50718    50733      +15     
==========================================
- Hits        39246    39238       -8     
- Misses       9002     9026      +24     
+ Partials     2470     2469       -1     

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

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 17 '25 22:06 codecov[bot]

@tstirrat15 Anything else I need to get this merged before it gets stale? Unable to merge it myself - not sure if @vroldanbet has to review or if he was auto-included.

James9074 avatar Jun 19 '25 16:06 James9074

@James9074 just wanted to update - this is still something we're interested in and we haven't had the bandwidth to look at it yet. Apologies for the delay.

tstirrat15 avatar Jul 07 '25 17:07 tstirrat15

@James9074 we're finally done with the push and should have some more bandwidth now. do you have the time and energy to get this rebased and ready? If not, we can take over the PR and make sure that you're attributed.

tstirrat15 avatar Aug 26 '25 16:08 tstirrat15

@James9074 we're finally done with the push and should have some more bandwidth now. do you have the time and energy to get this rebased and ready? If not, we can take over the PR and make sure that you're attributed.

Awesome! No worries, I'll rebase this week and resubmit for review.

James9074 avatar Aug 26 '25 21:08 James9074

@tstirrat15 Feel free to take this one over or close it out, unfortunately I'm short on time this quarter and don't think I'll be able to wrap this up before it's too stale.

James9074 avatar Oct 09 '25 13:10 James9074

Sounds good. Thank you so much for the work you put into it!

tstirrat15 avatar Oct 09 '25 22:10 tstirrat15

Thank you @James9074! Your hard work doesn't go unnoticed. We'll take this over the finish line :)

miparnisari avatar Oct 21 '25 17:10 miparnisari

All of the individual pieces of this have been brought in. Thank you for putting this up!

tstirrat15 avatar Dec 15 '25 18:12 tstirrat15