ci(lint): run golangci-lint across the full repo on master push - #4904
ci(lint): run golangci-lint across the full repo on master push#4904Hakai-Shin wants to merge 3 commits into
Conversation
golangci-lint's errcheck currently only runs against PR diffs, so
these have accumulated undetected across the repo. Fixes span three
patterns:
- Real handling added where a dropped error could mask a bug
(temp/cache file cleanup, MRD pool shutdown, GCS reader closes).
- Best-effort defer/cleanup calls (Close, Remove, RemoveAll, Unmount,
Setenv/Unsetenv, os.Stdout writes) explicitly discarded via `_ =`,
since their errors are not actionable at those call sites.
- hash.Hash.Write calls discarded, since crypto/*Hash implementations
are documented to never return an error.
Previously the lint job only ran (and only checked new issues) on pull_request events, so violations in already-merged code were never caught. Now the job runs on both events, checking only the diff for PRs (only-new-issues: true) and the whole codebase after merge to master (only-new-issues: false).
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request systematically addresses unhandled errors across the codebase by explicitly ignoring them using the blank identifier (_), particularly for file closures, removals, and environment variable operations. The reviewer's feedback is highly actionable and should be kept. It suggests explicitly checking write/flush errors on file closures in production code, utilizing the idiomatic t.Setenv helper in tests to simplify environment cleanup, and asserting on Close() errors in tests (especially integration tests where closing triggers GCS flushes) rather than silently discarding them.
- sparse_downloads_job.go: cacheFile is opened for writing, so a Close error can mean a flush/disk-full failure; log-and-swallow could return success on a corrupted cache file. Explicitly close and check the error on the success path, keeping the deferred close only as a cleanup guard (mirrors the pattern in SharedChunkCacheReader.downloadChunk). - cfg/decode_hook_test.go, internal/util/util_test.go: replace manual os.Setenv + deferred os.Unsetenv with t.Setenv, which is idiomatic (Go 1.17+) and self-cleaning. - shared_chunk_cache_reader_test.go, config_test.go, rapid_operations/appends_test.go: assert Close() succeeds instead of discarding the error, since a close failure there (flush/sync to GCS, or writing test fixtures) should fail the test rather than be silently ignored.
1 participant
Summary
lintCI job only checked issues on the PR diff (only-new-issues: true), so violations already merged intomasterwere never caught. This PR changes the job so PRs still only check the diff, but pushes tomaster(post-merge) scan the whole codebase (only-new-issues: false).errcheckviolations that surfaced once the full-repo scan was run, so the new master job doesn't start out red. Handling falls into three patterns:Close,Remove,RemoveAll,Unmount,Setenv/Unsetenv,os.Stdoutwrites) explicitly discarded via_ =, since their errors aren't actionable at those call sites.hash.Hash.Writecalls discarded, sincecrypto/*Hashimplementations are documented to never return an error.Known follow-up (needs maintainer input)
Turning on the full-repo scan also surfaces a pre-existing
govet/staticcheck/unused/ineffassignbacklog that was never caught before (golangci-lint caps reported output at 50 issues/linter by default, so the true count is larger than what any single run shows — roughly 319 govet, mostly from 51 files still importing the deprecatedgolang.org/x/net/contextinstead of stdlibcontext; ~178 staticcheck, mostly style/deprecation nits; a few unused/ineffassign). I deliberately left these out of this PR to keep it scoped to the errcheck issue this ticket describes, but as-is, merging this PR will leavemaster's Lint job red until that backlog is cleaned up.Options, happy to go either way:
x/net/contextswap is mechanical; the rest is mostly small style fixes) before/shortly after this merges.masterstays red until a separate cleanup lands — team's call on priority.Testing details
masterruns the full-repo scan and correctly reports the pre-existing errcheck backlog is clear (only the known govet/staticcheck/unused items above remain).go testacross all packages excepttools/integration_tests) passes;go build ./...andgo vet ./...are clean.Any backward incompatible change? If so, please explain.
No. This only changes CI lint scope/config and error-handling around already-executed cleanup paths; no functional behavior changes.