Hyperfleet-455 - fix: use standard .golangci.yml and fix lint issues #42
Conversation
… gocritic, gosec, unparam and lll lint issues
WalkthroughThis pull request applies comprehensive code quality and formatting improvements across 50+ files. The main change is a significantly expanded Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/auth/authz_middleware.go`:
- Around line 49-50: The middleware (authz_middleware.go) currently returns
early on missing auth/context errors without writing a response; update the
error branches in the AuthzMiddleware handler to create and send proper HTTP
error responses (use api.E401 or api.E403 for auth failures and api.E500 for
backend errors) via api.SendError(w, r, &body) instead of returning silently,
and remove the no-op fmt.Errorf calls; apply the same change to the other
early-return branches referenced (the ones around lines 59-60 and 69-70) so
every failure path writes an appropriate error body and status.
🧹 Nitpick comments (7)
pkg/db/transactions.go (1)
14-21: Don’t discardctx; use it when starting the transaction.
Line 14 now explicitly ignores context. Consider usingBeginTx(ctx, ...)(or a context-derived session) to honor cancellation/timeouts and align with repo guidance.Suggested adjustment
-func newTransaction(_ context.Context, connection SessionFactory) (*transaction.Transaction, error) { +func newTransaction(ctx context.Context, connection SessionFactory) (*transaction.Transaction, error) { @@ - tx, err := dbx.Begin() + tx, err := dbx.BeginTx(ctx, nil)As per coding guidelines: Always retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections directly.
pkg/services/cluster_test.go (3)
322-331: Inconsistent use of"Ready"literal vsconditionTypeReadyconstant.Line 324 uses the string literal
"Ready"whileconditionTypeAvailableconstant is used on Line 316. For consistency and maintainability, consider usingconditionTypeReadyhere as well.Suggested fix
{ - Type: "Ready", + Type: conditionTypeReady, Status: api.ConditionFalse,
392-408: Same inconsistency:"Ready"literal at Line 394.This test case uses
"Ready"string literal while other tests in the file useconditionTypeReady. Consider using the constant for consistency.
676-684: Inconsistent use of"Ready"literal at Line 678.Similar to earlier instances, Line 678 uses string literal
"Ready"while Line 670 usesconditionTypeAvailable. Consider usingconditionTypeReadyfor consistency.pkg/services/cluster.go (2)
173-179: Inconsistent use of"Available"string literal.Line 175 uses the string literal
"Available"to find the Available condition. However,pkg/services/node_pool.gousesconditionTypeAvailableconstant at the equivalent location (Line 176). Consider using the constant here for consistency.Suggested fix
// Find the "Available" condition var availableCondition *api.AdapterCondition for i := range conditions { - if conditions[i].Type == "Available" { + if conditions[i].Type == conditionTypeAvailable { availableCondition = &conditions[i] break }
266-279: Inconsistent use of"Available"string literal at Line 269.Similar to the issue at Line 175, this uses the string literal
"Available"whilenode_pool.gousesconditionTypeAvailable. Using the constant ensures both files stay synchronized.Suggested fix
// Find the "Available" condition hasAvailableCondition := false for _, cond := range conditions { - if cond.Type != "Available" { + if cond.Type != conditionTypeAvailable { continue }pkg/services/status_aggregation.go (1)
70-103: Consider extracting the common adapter map building logic.Both
ComputeAvailableConditionandComputeReadyConditioncontain nearly identical code for building theadapterMap(unmarshaling conditions JSON and extracting the "Available" condition). This could be refactored into a shared helper function to reduce duplication.♻️ Example helper extraction
// buildAdapterAvailabilityMap extracts availability info from adapter statuses func buildAdapterAvailabilityMap(adapterStatuses api.AdapterStatusList) map[string]struct { available string observedGeneration int32 } { adapterMap := make(map[string]struct { available string observedGeneration int32 }) for _, adapterStatus := range adapterStatuses { var conditions []struct { Type string `json:"type"` Status string `json:"status"` } if len(adapterStatus.Conditions) > 0 { if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err == nil { for _, cond := range conditions { if cond.Type == conditionTypeAvailable { adapterMap[adapterStatus.Adapter] = struct { available string observedGeneration int32 }{ available: cond.Status, observedGeneration: adapterStatus.ObservedGeneration, } break } } } } } return adapterMap }Also applies to: 139-174
| // body := api.E500.Format(r, "Authentication details not present in context") | ||
| // api.SendError(w, r, &body) |
There was a problem hiding this comment.
Return explicit HTTP errors instead of silent early exits.
These branches return without writing a response, which yields misleading 200/empty bodies for auth failures or backend errors. Send 401/403/500 (or your standard error body) and remove the no-op fmt.Errorf calls.
🛠️ Suggested fix
-import (
- "fmt"
- "net/http"
-
- "github.com/openshift-hyperfleet/hyperfleet-api/pkg/client/ocm"
-)
+import (
+ "net/http"
+
+ "github.com/openshift-hyperfleet/hyperfleet-api/pkg/client/ocm"
+)
@@
username := GetUsernameFromContext(ctx)
if username == "" {
- _ = fmt.Errorf("authenticated username not present in request context")
- // TODO
- // body := api.E500.Format(r, "Authentication details not present in context")
- // api.SendError(w, r, &body)
+ http.Error(w, "authentication details not present in context", http.StatusUnauthorized)
return
}
@@
if err != nil {
- _ = fmt.Errorf("unable to make authorization request: %s", err)
- // TODO
- // body := api.E500.Format(r, "Unable to make authorization request")
- // api.SendError(w, r, &body)
+ http.Error(w, "unable to make authorization request", http.StatusInternalServerError)
return
}
if allowed {
next.ServeHTTP(w, r)
+ return
}
- // TODO
- // body := api.E403.Format(r, "")
- // api.SendError(w, r, &body)
+ http.Error(w, "forbidden", http.StatusForbidden)
})
}Also applies to: 59-60, 69-70
🤖 Prompt for AI Agents
In `@pkg/auth/authz_middleware.go` around lines 49 - 50, The middleware
(authz_middleware.go) currently returns early on missing auth/context errors
without writing a response; update the error branches in the AuthzMiddleware
handler to create and send proper HTTP error responses (use api.E401 or api.E403
for auth failures and api.E500 for backend errors) via api.SendError(w, r,
&body) instead of returning silently, and remove the no-op fmt.Errorf calls;
apply the same change to the other early-return branches referenced (the ones
around lines 59-60 and 69-70) so every failure path writes an appropriate error
body and status.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ciaranRoche 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 |
openshift-merge-bot
bot
merged commit 1c08cb2
into
openshift-hyperfleet:main
2 participants
Summary
revivein .golangci.yml, because it reports 405 lint errors (e.g., missing comments on exported functions/types), which would require more effortTesting
Summary by CodeRabbit
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.