NO-ISSUE: bump golangci-lint to v2.11.3#602
NO-ISSUE: bump golangci-lint to v2.11.3#602openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@simonpasquier: This pull request explicitly references no jira issue. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
approved
WalkthroughThis pull request modernizes the codebase with four major initiatives: upgrading golangci-lint from v1 to v2, standardizing error handling across packages to use Go's standard ChangesBuild Configuration & Linting
Code Quality—Error Handling & Resource Management
Observability & Instrumentation
Authorization Enhancements & Protocol Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/bradfitz/gomemcache@v0.0.0-20230905024940-24af94b03874: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-oidc/v3@v3.9.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/efficientgo/core@v1.0.0-rc.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-chi/chi@v4.0.4+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-jose/go-jose/v3@v3.0.5: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-kit/log@v0.2.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/gogo/protobuf@v ... [truncated 5767 characters] ... .org/x/text@v0.28.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgoogle.golang.org/genproto/googleapis/api@v0.0.0-20231127180814-3a041ad873d4: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgoogle.golang.org/genproto/googleapis/rpc@v0.0.0-20231120223509-83a465c0220f: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgoogle.golang.org/grpc@v1.59.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgoogle.golang.org/protobuf@v1.36.8: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/telemeter-benchmark/main.go (1)
117-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
--to-uploadflag is always overwritten when--tois provided.When both
--toand--to-uploadare specified, the explicitly provided--to-uploadvalue is unconditionally overwritten on lines 122-124. This differs from the handling of--to-authwhich correctly checksif toAuthorize == nilbefore defaulting.Proposed fix
if toAuthorize == nil { u := *to u.Path = path.Join(to.Path, "authorize") toAuthorize = &u } - u := *to - u.Path = path.Join(to.Path, "upload") - toUpload = &u + if toUpload == nil { + u := *to + u.Path = path.Join(to.Path, "upload") + toUpload = &u + } }🤖 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/telemeter-benchmark/main.go` around lines 117 - 125, The current logic always overwrites an explicit --to-upload because the block that sets toUpload from to runs unconditionally; change the code in main.go so that the upload target is only defaulted when toUpload == nil (same pattern used for toAuthorize), i.e. guard the path.Join(..., "upload") assignment with if toUpload == nil and only then copy *to into u and set u.Path to path.Join(to.Path, "upload") to assign toUpload; reference variables/functions: toUpload, toAuthorize, to, path.Join.pkg/receive/handler.go (1)
151-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResponse body is not explicitly closed in the error path.
The
//nolint:bodycloseannotation suppresses the linter warning, but the response body fromh.client.Do(req)is never explicitly closed in this function. While the body may be read in the error path (line 161),resp.Body.Close()should be called to release resources.🔧 Proposed fix to close response body
- resp, err := h.client.Do(req) //nolint:bodyclose + resp, err := h.client.Do(req) if err != nil { h.forwardRequestsTotal.WithLabelValues("error").Inc() level.Error(logger).Log("msg", "failed to forward request", "err", err) http.Error(w, err.Error(), http.StatusBadGateway) return } + defer func() { _ = resp.Body.Close() }() if resp.StatusCode/100 != 2 {🤖 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 `@pkg/receive/handler.go` around lines 151 - 172, In the request-forwarding block inside the handler that calls h.client.Do(req), ensure the response body is always closed: after successfully getting resp (err == nil) immediately arrange to close resp.Body (e.g., defer resp.Body.Close()) so that both the error path where you read the body (io.ReadAll(resp.Body)) and the success path (writing resp.StatusCode) release resources; update the logic around resp handling in the function that performs h.client.Do to call resp.Body.Close() in all paths instead of relying on //nolint:bodyclose.pkg/authorize/jwt/client_authorizer.go (1)
12-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment mentions non-existent parameter.
The comment references "If lookup is true" but the function has no
lookupparameter. This appears to be copy-pasted documentation that doesn't apply to this function.📝 Suggested fix
// NewClientAuthorizer authenticates tokens as JWT tokens produced by JWTTokenGenerator -// Token signatures are verified using each of the given public keys until one works (allowing key rotation) -// If lookup is true, the service account and secret referenced as claims inside the token are retrieved and verified with the provided ServiceAccountTokenGetter +// Token signatures are verified using each of the given public keys until one works (allowing key rotation). +// The provided Validator is used for domain-specific token validation after signature verification. func NewClientAuthorizer(issuer string, keys []crypto.PublicKey, v Validator) *clientAuthorizer {🤖 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 `@pkg/authorize/jwt/client_authorizer.go` around lines 12 - 14, The function comment for NewClientAuthorizer incorrectly references a non-existent "lookup" parameter; update the docstring for NewClientAuthorizer in client_authorizer.go to remove the "If lookup is true" clause and instead describe the actual behavior and parameters of NewClientAuthorizer (how token signatures are verified using the provided public keys and how service account/secret handling actually works) so the comment matches the function signature and behavior.
🧹 Nitpick comments (3)
cmd/telemeter-server/main.go (1)
633-633: 💤 Low valueMultiple
%wverbs in a singlefmt.Errorfmay obscure error chain.Using multiple
%wverbs in a single format string (e.g.,"'%w', '%w' and '%w'") is valid since Go 1.20, buterrors.Is()anderrors.As()will only match against the first wrapped error in the chain. Consider usingerrors.Join()to combine all three errors if you want all of them to be unwrappable.♻️ Alternative using errors.Join
- return nil, fmt.Errorf("unable to parse private key data: '%w', '%w' and '%w'", err0, err1, err2) + return nil, fmt.Errorf("unable to parse private key data: %w", errors.Join(err0, err1, err2))🤖 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/telemeter-server/main.go` at line 633, Replace the current fmt.Errorf that uses three %w verbs with a single wrapped joined error so callers can unwrap all errors: construct a combinedErr := errors.Join(err0, err1, err2) and return fmt.Errorf("unable to parse private key data: %w", combinedErr); ensure the errors package is imported and reference the existing err0, err1, err2 used in the failing return expression..golangci.yml (1)
55-73: ⚡ Quick winTest files are excluded from all linters, effectively disabling linting on tests.
The exclusion rule at lines 56-73 lists every enabled linter under the
_test\.gopath pattern. According to the comment on line 58-59, this is intentional and "any linter that needs to be run on test files should be dropped from here." However, since all enabled linters are listed, no linting runs on test files at all.Consider removing at least
govet,staticcheck, andunusedfrom this exclusion list, as these catch real bugs in test code.♻️ Suggested change to enable key linters on test files
rules: - path: _test\.go linters: - # The list of linters below is exhaustive with respect to the linters defined above, and as such, and linter - # that needs to be run on test files should be dropped from here. + # Linters disabled for test files - removed govet, staticcheck, unused to catch bugs in tests - depguard - errcheck - errorlint - bodyclose - ineffassign - misspell - nolintlint - nosprintfhostport - - staticcheck - usetesting - unconvert - - unused - wastedassign - whitespace🤖 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 @.golangci.yml around lines 55 - 73, The test-file rule currently excludes all linters by listing every enabled linter under the path pattern `_test\.go`, which disables linting for tests; edit the `_test\.go` rule in .golangci.yml (the rules -> - path: _test\.go block) and remove at least `govet`, `staticcheck`, and `unused` from the linters array so those linters run on test files (you may also remove any other linters you want to apply to tests), then validate the YAML and run golangci-lint to ensure the intended linters now execute against test files.cmd/telemeter-client/main.go (1)
378-379: 💤 Low valueConsider caching the format to avoid duplicate computation.
expfmt.NewFormat(expfmt.TypeTextPlain)is called twice. Store it in a variable for clarity and minor efficiency.♻️ Suggested refactor
families := worker.LastMetrics() - w.Header().Set("Content-Type", string(expfmt.NewFormat(expfmt.TypeTextPlain))) - encoder := expfmt.NewEncoder(w, expfmt.NewFormat(expfmt.TypeTextPlain)) + format := expfmt.NewFormat(expfmt.TypeTextPlain) + w.Header().Set("Content-Type", string(format)) + encoder := expfmt.NewEncoder(w, format) for _, family := range families {🤖 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/telemeter-client/main.go` around lines 378 - 379, Duplicate calls to expfmt.NewFormat(expfmt.TypeTextPlain) appear in the response writer setup; cache the result in a local variable (e.g., fmt := expfmt.NewFormat(expfmt.TypeTextPlain)) and reuse it in w.Header().Set("Content-Type", string(fmt)) and in expfmt.NewEncoder(w, fmt) to avoid repeated computation and improve clarity in the code around the w.Header().Set and encoder := expfmt.NewEncoder(...) lines.
🤖 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.
Outside diff comments:
In `@cmd/telemeter-benchmark/main.go`:
- Around line 117-125: The current logic always overwrites an explicit
--to-upload because the block that sets toUpload from to runs unconditionally;
change the code in main.go so that the upload target is only defaulted when
toUpload == nil (same pattern used for toAuthorize), i.e. guard the
path.Join(..., "upload") assignment with if toUpload == nil and only then copy
*to into u and set u.Path to path.Join(to.Path, "upload") to assign toUpload;
reference variables/functions: toUpload, toAuthorize, to, path.Join.
In `@pkg/authorize/jwt/client_authorizer.go`:
- Around line 12-14: The function comment for NewClientAuthorizer incorrectly
references a non-existent "lookup" parameter; update the docstring for
NewClientAuthorizer in client_authorizer.go to remove the "If lookup is true"
clause and instead describe the actual behavior and parameters of
NewClientAuthorizer (how token signatures are verified using the provided public
keys and how service account/secret handling actually works) so the comment
matches the function signature and behavior.
In `@pkg/receive/handler.go`:
- Around line 151-172: In the request-forwarding block inside the handler that
calls h.client.Do(req), ensure the response body is always closed: after
successfully getting resp (err == nil) immediately arrange to close resp.Body
(e.g., defer resp.Body.Close()) so that both the error path where you read the
body (io.ReadAll(resp.Body)) and the success path (writing resp.StatusCode)
release resources; update the logic around resp handling in the function that
performs h.client.Do to call resp.Body.Close() in all paths instead of relying
on //nolint:bodyclose.
---
Nitpick comments:
In @.golangci.yml:
- Around line 55-73: The test-file rule currently excludes all linters by
listing every enabled linter under the path pattern `_test\.go`, which disables
linting for tests; edit the `_test\.go` rule in .golangci.yml (the rules -> -
path: _test\.go block) and remove at least `govet`, `staticcheck`, and `unused`
from the linters array so those linters run on test files (you may also remove
any other linters you want to apply to tests), then validate the YAML and run
golangci-lint to ensure the intended linters now execute against test files.
In `@cmd/telemeter-client/main.go`:
- Around line 378-379: Duplicate calls to expfmt.NewFormat(expfmt.TypeTextPlain)
appear in the response writer setup; cache the result in a local variable (e.g.,
fmt := expfmt.NewFormat(expfmt.TypeTextPlain)) and reuse it in
w.Header().Set("Content-Type", string(fmt)) and in expfmt.NewEncoder(w, fmt) to
avoid repeated computation and improve clarity in the code around the
w.Header().Set and encoder := expfmt.NewEncoder(...) lines.
In `@cmd/telemeter-server/main.go`:
- Line 633: Replace the current fmt.Errorf that uses three %w verbs with a
single wrapped joined error so callers can unwrap all errors: construct a
combinedErr := errors.Join(err0, err1, err2) and return fmt.Errorf("unable to
parse private key data: %w", combinedErr); ensure the errors package is imported
and reference the existing err0, err1, err2 used in the failing return
expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54ceaf78-7f7f-4969-9917-261b5ca59117
📒 Files selected for processing (32)
.golangci.ymlMakefilecmd/rhelemeter-server/main.gocmd/telemeter-benchmark/main.gocmd/telemeter-client/main.gocmd/telemeter-server/main.gocmd/telemeter-server/main_e2e_test.gopkg/authorize/handler.gopkg/authorize/jwt/client_authorizer.gopkg/authorize/jwt/client_authorizer_test.gopkg/authorize/jwt/handler.gopkg/authorize/jwt/handler_test.gopkg/authorize/jwt/validator.gopkg/authorize/roundtripper.gopkg/authorize/stub/stub.gopkg/authorize/token_store.gopkg/authorize/tollbooth/mock.gopkg/authorize/tollbooth/tollbooth.gopkg/benchmark/benchmark.gopkg/cache/cache.gopkg/cache/memcached/memcached.gopkg/fnv/hash.gopkg/forwarder/forwarder.gopkg/http/client.gopkg/metricfamily/overwrite.gopkg/metricsclient/metricsclient.gopkg/receive/handler.gopkg/runutil/runutil.gopkg/server/forward.gopkg/server/validator.gopkg/tracing/tracing.gotest/e2e/receive_test.go
💤 Files with no reviewable changes (1)
- pkg/metricfamily/overwrite.go
|
/verified by tests |
openshift-ci-robot
added
the
verified
|
@simonpasquier: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @jan--f |
jan--f
left a comment
jan--f
left a comment
There was a problem hiding this comment.
Lovely refactor, thanks!
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jan--f, simonpasquier The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@simonpasquier: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary by CodeRabbit
New Features
Improvements
Chores