Fix the lint failures #12
Conversation
WalkthroughThis PR adds and suppresses error handling across test and middleware files. The changes include: adding explicit error checks for json.Unmarshal operations in presenter tests, suppressing errors from JSON encoding and request body closing in middleware error handling, ignoring error returns from HTTP requests and environment variable operations in integration tests, and wrapping resource cleanup in deferred functions. No functional behavior changes are introduced; the modifications refine error handling practices and test robustness. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/integration/clusters_test.go (1)
323-328: Potential nil pointer panic: check error before using resp2.Suppressing the error from
resty.R().Post()is dangerous. If the request fails (network error, timeout, etc.),resp2could be nil, and accessingresp2.StatusCode()on line 329 will panic.Check the error before using resp2:
- resp2, _ := resty.R(). + resp2, err := resty.R(). SetHeader("Content-Type", "application/json"). SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). SetBody(invalidTypeJSON). Post(h.RestURL("/clusters")) + + if err != nil { + t.Fatalf("HTTP request failed: %v", err) + } if resp2.StatusCode() == http.StatusBadRequest {
🧹 Nitpick comments (1)
pkg/middleware/schema_validation.go (1)
26-26: Error suppression acceptable here, but consider logging.Suppressing the JSON encoding error in an error handler is reasonable since there's limited recovery options. However, if encoding fails, the client receives an incomplete or malformed response.
Consider logging the encoding error for observability:
- _ = json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) + if encodeErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encodeErr != nil { + log.Errorf("Failed to encode validation error response: %v", encodeErr) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/api/presenters/cluster_test.go(3 hunks)pkg/api/presenters/node_pool_test.go(2 hunks)pkg/middleware/schema_validation.go(2 hunks)test/integration/clusters_test.go(4 hunks)test/integration/integration_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
pkg/middleware/schema_validation.gopkg/api/presenters/cluster_test.gopkg/api/presenters/node_pool_test.gotest/integration/integration_test.gotest/integration/clusters_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Files:
pkg/middleware/schema_validation.gopkg/api/presenters/cluster_test.gopkg/api/presenters/node_pool_test.go
pkg/api/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Files:
pkg/api/presenters/cluster_test.gopkg/api/presenters/node_pool_test.go
test/integration/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
test/integration/**/*.go: Create integration tests using Testcontainers for isolated PostgreSQL instances per test
All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Files:
test/integration/integration_test.gotest/integration/clusters_test.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/integration/**/*.go : All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/handlers/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/api/**/*.go : Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Applied to files:
test/integration/integration_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to cmd/hyperfleet/environments/**/*.go : Use OCM_ENV environment variable to select configuration environment (development, unit_testing, integration_testing, production)
Applied to files:
test/integration/integration_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/integration/**/*.go : All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Applied to files:
test/integration/integration_test.gotest/integration/clusters_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Applied to files:
test/integration/clusters_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/**/*.go : Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Applied to files:
test/integration/clusters_test.go
🔇 Additional comments (9)
pkg/middleware/schema_validation.go (1)
47-47: Body.Close() error suppression is acceptable.The request body has already been successfully read and will be replaced with a NopCloser. Suppressing the Close() error here is a standard pattern.
pkg/api/presenters/cluster_test.go (3)
89-90: Good improvement: explicit error handling added.The test now properly checks that unmarshalling succeeds before asserting on the result values. This prevents misleading test failures if the JSON format is incorrect.
110-111: Good improvement: explicit error handling added.The test now properly checks that unmarshalling succeeds before asserting on the result values.
141-142: Good improvement: explicit error handling added.The test now properly checks that unmarshalling succeeds before asserting on the result values.
pkg/api/presenters/node_pool_test.go (2)
127-128: Good improvement: explicit error handling added.The test now properly checks that unmarshalling succeeds before asserting on the result values. This aligns with the improvements made in cluster_test.go.
148-149: Good improvement: explicit error handling added.The test now properly checks that unmarshalling succeeds before asserting on the result values.
test/integration/clusters_test.go (3)
333-333: Error suppression acceptable in test context.Suppressing the unmarshal error here is acceptable since this is within a conditional block that already validated the status code. If unmarshalling fails, the test will fail on the subsequent assertions.
392-392: Standard defer cleanup pattern.Wrapping
Body.Close()in a deferred function with error suppression is a standard cleanup pattern for HTTP responses in tests.
549-549: Ignoring return values is acceptable here.The test doesn't need the returned cluster objects, but properly checks for errors. This is appropriate for this test scenario.
| glog.Warningf("Schema file not found at %s: %v, skipping OPENAPI_SCHEMA_PATH setup", schemaPath, err) | ||
| } else { | ||
| os.Setenv("OPENAPI_SCHEMA_PATH", schemaPath) | ||
| _ = os.Setenv("OPENAPI_SCHEMA_PATH", schemaPath) |
There was a problem hiding this comment.
Suppressing os.Setenv error could hide test setup issues.
If os.Setenv fails (e.g., due to OS limits on environment variable size or permissions), the tests will proceed with incorrect configuration, potentially causing confusing failures.
Consider logging the error or using a fatal check:
- _ = os.Setenv("OPENAPI_SCHEMA_PATH", schemaPath)
+ if err := os.Setenv("OPENAPI_SCHEMA_PATH", schemaPath); err != nil {
+ glog.Warningf("Failed to set OPENAPI_SCHEMA_PATH: %v", err)
+ } else {
+ glog.Infof("Set OPENAPI_SCHEMA_PATH=%s for integration tests", schemaPath)
+ }
- glog.Infof("Set OPENAPI_SCHEMA_PATH=%s for integration tests", schemaPath)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ = os.Setenv("OPENAPI_SCHEMA_PATH", schemaPath) | |
| if err := os.Setenv("OPENAPI_SCHEMA_PATH", schemaPath); err != nil { | |
| glog.Warningf("Failed to set OPENAPI_SCHEMA_PATH: %v", err) | |
| } else { | |
| glog.Infof("Set OPENAPI_SCHEMA_PATH=%s for integration tests", schemaPath) | |
| } |
🤖 Prompt for AI Agents
In test/integration/integration_test.go around line 41, the call to os.Setenv
ignores the returned error which can hide test setup failures; update the code
to check the error and fail the test on error (e.g., if inside a test function
call t.Fatalf with a clear message including the error, or if not available use
log.Fatalf/panic) so any environment variable setup failure stops the test
immediately and surfaces the underlying cause.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yasun1, yingzhanredhat 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 b8897da
into
openshift-hyperfleet:main
2 participants
HYPERFLEET-295:
Fix the lint error that happens in presubmit job
https://qe-private-deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gs/qe-private-deck/pr-logs/pull/openshift_release/72205/rehearse-72205-pull-ci-openshift-hyperfleet-hyperfleet-api-main-lint/1997915431523848192
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.