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 staticcheck as a presubmit check #10779

Open
julieqiu opened this issue Aug 28, 2024 · 6 comments
Open

add staticcheck as a presubmit check #10779

julieqiu opened this issue Aug 28, 2024 · 6 comments
Labels
type: process A process-related concern. May include testing, release, or the like.

Comments

@julieqiu
Copy link
Member

julieqiu commented Aug 28, 2024

staticcheck is a common linter for Go projects. We should consider adding it as a presubmit check for PRs. See googleapis/gapic-generator-go#1404 and googleapis/gapic-showcase#1529 for related issues.

This first requires fixing these issues in this repository:

$ staticcheck ./...                                                                         
debugger/apiv2/controller2_client.go:23:2: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.  (SA1019)
debugger/apiv2/controller2_client.go:53:3: internaloption.WithDefaultEndpoint is deprecated: WithDefaultEndpoint does not support setting the universe domain. Use WithDefaultEndpointTemplate and WithDefaultUniverseDomain to compose the default endpoint instead.  (SA1019)
debugger/apiv2/controller2_client.go:386:3: internaloption.WithDefaultEndpoint is deprecated: WithDefaultEndpoint does not support setting the universe domain. Use WithDefaultEndpointTemplate and WithDefaultUniverseDomain to compose the default endpoint instead.  (SA1019)
debugger/apiv2/debugger2_client.go:23:2: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.  (SA1019)
debugger/apiv2/debugger2_client.go:55:3: internaloption.WithDefaultEndpoint is deprecated: WithDefaultEndpoint does not support setting the universe domain. Use WithDefaultEndpointTemplate and WithDefaultUniverseDomain to compose the default endpoint instead.  (SA1019)
debugger/apiv2/debugger2_client.go:394:3: internaloption.WithDefaultEndpoint is deprecated: WithDefaultEndpoint does not support setting the universe domain. Use WithDefaultEndpointTemplate and WithDefaultUniverseDomain to compose the default endpoint instead.  (SA1019)
debugger/apiv2/debugger2_client.go:715:5: req.GetStripResults is deprecated: Do not use.  (SA1019)
debugger/apiv2/debugger2_client.go:716:48: req.GetStripResults is deprecated: Do not use.  (SA1019)
httpreplay/internal/proxy/debug.go:24:6: type debugTransport is unused (U1000)
httpreplay/internal/proxy/debug.go:29:25: func debugTransport.RoundTrip is unused (U1000)
httpreplay/internal/proxy/debug.go:46:6: func logHeaders is unused (U1000)
internal/btree/btree.go:579:33: field byte is unused (U1000)
internal/btree/btree_test.go:32:2: rand.Seed has been deprecated since Go 1.20 and an alternative has been available since Go 1.0: As of Go 1.20 there is no reason to call Seed with a random value. Programs that call Seed with a known value to get a specific sequence of results should use New(NewSource(seed)) to obtain a local random generator.  (SA1019)
internal/testutil/server_test.go:32:15: grpc.Dial is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (SA1019)
internal/testutil/server_test.go:32:35: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x.  (SA1019)
rpcreplay/example_test.go:34:15: grpc.Dial is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (SA1019)
rpcreplay/example_test.go:47:15: grpc.Dial is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay.go:372:3: grpc.WithBlock is deprecated: this DialOption is not supported by NewClient. Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay.go:393:15: grpc.Dial is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay.go:394:28: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay_test.go:259:15: grpc.Dial is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay_test.go:260:28: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay_test.go:461:17: grpc.DialContext is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay_test.go:461:74: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay_test.go:551:17: grpc.DialContext is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay_test.go:551:74: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay_test.go:572:16: grpc.DialContext is deprecated: use NewClient instead.  Will be supported throughout 1.x.  (SA1019)
rpcreplay/rpcreplay_test.go:572:73: grpc.WithInsecure is deprecated: use WithTransportCredentials and insecure.NewCredentials() instead. Will be supported throughout 1.x.  (SA1019)
third_party/pkgsite/synopsis.go:64:5: the surrounding loop is unconditionally terminated (SA4004)
@julieqiu julieqiu added the triage me I really want to be triaged. label Aug 28, 2024
@codyoss
Copy link
Member

codyoss commented Aug 28, 2024

@julieqiu We do actually run staticcheck, but we ignore some warnings from it today. See:

staticcheck -go 1.15 ./... 2>&1 | (
grep -v SA1019 |
grep -v go-cloud-debug-agent |
grep -v internal/btree/btree.go |
grep -v httpreplay/internal/proxy/debug.go |
grep -v third_party/go/doc |
grep -v third_party/pkgsite/synopsis.go
) |
tee /dev/stderr | (! read)

@quartzmo
Copy link
Member

@codyoss Do you think any of the current ignores could be addressed and removed at this time?

@quartzmo quartzmo added type: process A process-related concern. May include testing, release, or the like. and removed triage me I really want to be triaged. labels Aug 28, 2024
@quartzmo
Copy link
Member

@julieqiu: #10780 removes some of these SA1019 failures. Did you run into trouble trying to remove the remaining SA1019 failures?

@codyoss
Copy link
Member

codyoss commented Aug 28, 2024

I honestly don't know without looking. Some things could likely be cleaned up but I think some of these that we skip would take a breaking change to actually fix the issue -- at least I vaguely recall this being the case from a couple of years back when I had looked at this.

@egonelbre
Copy link
Contributor

egonelbre commented Aug 29, 2024

There's an existing related issue #9784

Most linting tooling works in a module boundary. So if you run go vet ./... or staticcheck ./... it only verifies the current module, and in this repo most folders are separate modules.

Or in other words neither go vet or staticcheck are running for the submodules.

I created a proof of concept script in https://github.com/egonelbre/google-cloud-go/pull/1/files#diff-25b8fc8d894e1d2f71768a6e38f68cecaad5d7d861ba6b16f1ad40f9f94516fb -- but it probably would need to be adjusted to fit Google's needs better. More of a starting point.

And I've been fixing linting issues for spanner https://github.com/googleapis/google-cloud-go/pulls/egonelbre

@quartzmo
Copy link
Member

@egonelbre Thank you (again!) for your help with these issues. Appreciate your comments and POC!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: process A process-related concern. May include testing, release, or the like.
Projects
None yet
Development

No branches or pull requests

5 participants
@egonelbre @quartzmo @julieqiu @codyoss and others