fix(lint): properly handle all errcheck violations from golangci-lint v2#1265
Open
cristim wants to merge 2 commits into
Open
fix(lint): properly handle all errcheck violations from golangci-lint v2#1265cristim wants to merge 2 commits into
cristim wants to merge 2 commits into
Conversation
golangci-lint v2 activated errcheck.check-blank:true which exposed 107 pre-existing blank-identifier error discards. This commit handles each violation properly: - Exclude generated mock/test-helper paths from errcheck in .golangci.yml (internal/mocks/, internal/auth/test_helpers.go, internal/database/postgres/testhelpers/) - Handle errors in production code with log/return as appropriate - Use //nolint:errcheck with specific justification only for documented fire-and-forget cases (singleflight, best-effort STS, w.Write) - Extract readLine helper (configure_azure.go) and getGCPProjectID, logMigrateVersion, createAzureServicePrincipal, payloadTypeMatches helpers to keep cyclomatic complexity within the gocyclo <=10 gate - Save purchase execution errors on money path are now logged as Errorf - errgroup.Wait + ctx.Err() checked in scheduler fan-out loops
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reached
More reviews will be available in 10 minutes and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR is a broad errcheck remediation pass: previously-ignored errors across API handlers, CLI commands, database transactions, services, and providers are now either logged as warnings, propagated to callers, or explicitly annotated with ChangesErrcheck Cleanup and Azure All-Failed Guard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/configure_gcp.go (1)
388-389:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid re-creating stdin readers inside command execution.
Line [415] creates a fresh
bufio.Reader(os.Stdin)rather than reusing the caller’sreader. In piped/scripted flows this can lose buffered bytes and stall follow-up prompts.Suggested fix
func promptAndRunGCPCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { @@ switch choice { case "r", "run", "": - return executeGCPCommand(displayCmd, program, args...) + return executeGCPCommand(reader, displayCmd, program, args...) @@ -func executeGCPCommand(displayCmd string, program string, args ...string) error { +func executeGCPCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { @@ if err != nil { fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") - reader := bufio.NewReader(os.Stdin) response, rfErr := readLine(reader)Also applies to: 415-418
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/configure_gcp.go` around lines 388 - 389, The code is creating a fresh bufio.Reader from os.Stdin instead of reusing the reader parameter passed to the function, which causes loss of buffered bytes in piped/scripted flows. Replace the bufio.Reader(os.Stdin) instantiation (around line 415) with the existing reader parameter that was passed to the current function, ensuring the same reader instance is reused throughout the command execution flow to preserve any buffered input.cmd/configure_azure.go (1)
386-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse the same buffered reader for follow-up prompts.
Line [413] creates a new
bufio.Reader(os.Stdin)insideexecuteExplicitCommand. If input is piped and the caller’s reader already buffered bytes, this can drop buffered input and block/hang on retries.Suggested fix
func promptAndRunExplicitCommand(reader *bufio.Reader, name, displayCmd string, program string, args ...string) error { @@ switch choice { case "r", "run", "": - return executeExplicitCommand(displayCmd, program, args...) + return executeExplicitCommand(reader, displayCmd, program, args...) @@ -func executeExplicitCommand(displayCmd string, program string, args ...string) error { +func executeExplicitCommand(reader *bufio.Reader, displayCmd string, program string, args ...string) error { @@ if err != nil { fmt.Printf("Command failed: %v\n", err) fmt.Print("Continue anyway? [y/N]: ") - reader := bufio.NewReader(os.Stdin) response, readErr := readLine(reader)Also applies to: 413-417
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/configure_azure.go` around lines 386 - 387, The executeExplicitCommand function creates a new bufio.Reader(os.Stdin) at lines 413-417, which drops any buffered bytes from the caller's existing reader and causes input loss when piped. Modify executeExplicitCommand to accept a *bufio.Reader parameter instead of creating a new one internally. Then update the function call at lines 386-387 (and any other calls to executeExplicitCommand) to pass the caller's existing buffered reader as an argument, ensuring the same reader instance is reused for all prompts and input operations.providers/azure/recommendations.go (1)
167-167:⚠️ Potential issue | 🟠 MajorHandle
g.Wait()error instead of blank-discarding it.With
check-blank: trueenabled in.golangci.yml, blank-discarding the error fromg.Wait()at line 167 violates lint rules and masks potential failures. Add proper error handling:Proposed fix
- _ = g.Wait() + if err := g.Wait(); err != nil { + return nil, err + } if err := ctx.Err(); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/recommendations.go` at line 167, The g.Wait() error is being blank-discarded with the underscore assignment which violates the golangci-yml check-blank rule and hides potential failures. Replace the `_ = g.Wait()` statement with proper error handling by capturing the returned error, checking if it is not nil, and then either returning the error to the caller, logging it appropriately, or handling it based on the context of the recommendations.go function logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/configure_azure.go`:
- Around line 261-266: In the readLine function, the issue is that when io.EOF
is encountered (indicating closed or non-interactive stdin), the error is being
silently converted to nil even when no data was read. This allows empty input
from closed stdin to be treated as a valid empty choice. Fix this by checking
whether the trimmed line is actually empty when io.EOF is encountered. If the
line is empty and we received an EOF error, return the EOF error instead of nil
to properly indicate that no valid input was provided. Only convert EOF to nil
success when actual data was successfully read before the EOF.
In `@internal/credentials/resolver.go`:
- Around line 411-416: The current code treats all LoadRaw errors uniformly by
setting raw to nil, but according to the CredentialStore interface, only
not-found cases should fall back to the federated path while other errors
represent real store failures that need to be surfaced. Modify the error
handling for the store.LoadRaw call to use errors.Is(loadErr, pgx.ErrNoRows) to
specifically check if the error is a not-found error, and only set raw = nil in
that case. For other non-nil errors, propagate or return the error instead of
silently collapsing it to nil.
In `@internal/scheduler/scheduler.go`:
- Around line 352-355: In the collectAllProviders function, the error returned
from g.Wait() is being logged but not propagated, which allows the function to
continue and return success even when goroutines have failed. Instead of only
logging the waitErr, check if waitErr is not nil and return it immediately to
ensure that any goroutine errors are properly surfaced to the caller and prevent
partial/incorrect results from being returned.
---
Outside diff comments:
In `@cmd/configure_azure.go`:
- Around line 386-387: The executeExplicitCommand function creates a new
bufio.Reader(os.Stdin) at lines 413-417, which drops any buffered bytes from the
caller's existing reader and causes input loss when piped. Modify
executeExplicitCommand to accept a *bufio.Reader parameter instead of creating a
new one internally. Then update the function call at lines 386-387 (and any
other calls to executeExplicitCommand) to pass the caller's existing buffered
reader as an argument, ensuring the same reader instance is reused for all
prompts and input operations.
In `@cmd/configure_gcp.go`:
- Around line 388-389: The code is creating a fresh bufio.Reader from os.Stdin
instead of reusing the reader parameter passed to the function, which causes
loss of buffered bytes in piped/scripted flows. Replace the
bufio.Reader(os.Stdin) instantiation (around line 415) with the existing reader
parameter that was passed to the current function, ensuring the same reader
instance is reused throughout the command execution flow to preserve any
buffered input.
In `@providers/azure/recommendations.go`:
- Line 167: The g.Wait() error is being blank-discarded with the underscore
assignment which violates the golangci-yml check-blank rule and hides potential
failures. Replace the `_ = g.Wait()` statement with proper error handling by
capturing the returned error, checking if it is not nil, and then either
returning the error to the caller, logging it appropriately, or handling it
based on the context of the recommendations.go function logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a5dd19b-1687-434c-a0a1-ceb2589cf9eb
📒 Files selected for processing (24)
.golangci.ymlcmd/cleanup-lambda/main.gocmd/configure_azure.gocmd/configure_gcp.gocmd/rekey/main.gointernal/api/handler.gointernal/api/handler_accounts.gointernal/api/handler_dashboard.gointernal/api/handler_recommendations_refresh.gointernal/api/handler_registrations.gointernal/api/ri_utilization_cache.gointernal/api/validation.gointernal/auth/service_apikeys.gointernal/auth/service_mfa.gointernal/commitmentopts/store_postgres.gointernal/credentials/cipher.gointernal/credentials/resolver.gointernal/database/connection.gointernal/database/postgres/migrations/migrate.gointernal/purchase/execution.gointernal/scheduler/scheduler.gointernal/server/http.goproviders/azure/recommendations.goproviders/azure/recommendations_test.go
Three Major CR findings on the errcheck changes, each fixed at root cause rather than silencing the linter: - cmd/configure_azure.go: readLine treated a zero-byte io.EOF as a successful empty line, so a closed/non-interactive stdin looked like an empty choice and fell through to the default Run in prompt handlers. Now EOF is success only when bytes were actually read (final line without a trailing newline); an EOF with no data surfaces the error. Uses the raw read buffer (not the trimmed text) so a bare newline still counts as a valid empty line rather than a closed stream. - internal/credentials/resolver.go: resolveGCPWIFCredential swallowed every LoadRaw error by setting raw=nil and falling back to the federated path. LoadRaw already maps not-found to (nil, nil), so any non-nil error is a real store failure (DB connectivity, decrypt) that must be surfaced. Matches the established pattern in resolveAccessKeyProvider. - internal/scheduler/scheduler.go: collectAllProviders logged g.Wait()'s error but still merged outcomes, risking a partial aggregate reported as success. It now returns the error. fanOutPerAccount has no error return, so an unexpected g.Wait() error is reflected into the accountOutcome (FailedCount/LastErr) instead of being silently swallowed. Regression tests added: TestReadLine_EOFHandling (table-driven, covers the closed-stream-surfaces-EOF and bare-newline cases) and TestResolveGCPWIF_LoadRawError_Surfaces (wires the full federated path so the pre-fix code would mask the store error). Both fail on the pre-fix code and pass after.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
golangci-lint v2 activated
errcheck.check-blank: truewhich exposed 107 pre-existing blank-identifier error discards (_ = someCall()). This causedmainCI to fail on every lint run.Approach
The owner decided to properly handle each error, not flip
check-blankto false.Config changes (
.golangci.yml)Three path exclusions for generated/test-helper code where the pattern is unavoidable:
internal/mocks/- testify mockargs.Get(0).(*Type)type assertionsinternal/auth/test_helpers.gointernal/database/postgres/testhelpers/Production code changes
Each of the 16 remaining sites is handled appropriately:
internal/purchase/execution.go):SavePurchaseExecutionerrors are now logged asErrorf(AUDIT LOSS level); early-exit save extracted tosaveExecWithLoghelperinternal/scheduler/scheduler.go):g.Wait()andGetRecommendationserrors are now checked and loggedinternal/server/http.go):w.Writeerror loggedcmd/cleanup-lambda/main.go,cmd/rekey/main.go,internal/commitmentopts/store_postgres.go): proper rollback error handling witherrors.Is(pgx.ErrTxClosed)guardinternal/api/handler.go, various):buildResponseerrors propagated or logged;resolveAWSCallerIdentityannotated with//nolint:errcheck(best-effort, STS failure = empty AccountID = security gate)internal/auth/service_mfa.go,internal/auth/service_apikeys.go): update errors logged; singleflight.Doannotated with justificationinternal/credentials/cipher.go):DevKeypanics on invalid compile-time constant (programming error)cmd/configure_azure.go,cmd/configure_gcp.go):bufio.Reader.ReadStringerrors handled viareadLinehelper; subcommands extracted to keep gocyclo within gateComplexity helpers extracted (to keep gocyclo gate green)
readLine(reader)- bufio.ReadString + io.EOF handling (configure_azure.go)getGCPProjectID(reader)- read + validate GCP project ID (configure_gcp.go)createAzureServicePrincipal(reader, subscriptionID)- SP creation block (configure_azure.go)payloadTypeMatches(payload, want)- type-safe payload type check (internal/api/validation.go)logMigrateVersion(m, steps)- pre-rollback version logging (migrate.go)saveExecWithLog(ctx, exec, accountID)- best-effort save in early-exit path (execution.go)Test plan
go build ./...passesgolangci-lint run --enable errcheckreports 0 errcheck violationsgocyclo -over 10 $(git ls-files "*.go" | grep -v _test.go | grep -v vendor/)reports 0 violationsgo test ./...passesSummary by CodeRabbit
Release Notes
Bug Fixes
Tests