HYPERFLEET-295: Fix the error from presubmit jobs: lint/unit/integration #6
Conversation
WalkthroughThis PR adds defensive error handling and safer type assertions across many packages, updates method signatures and signatures usage (notably pkg/client/ocm.Client.Close and treeWalkForSqlizer), refines HTTP response writing to handle marshal/write errors, tightens JSON marshal/unmarshal fallbacks in several API types, updates test code and mocks, switches testcontainer readiness to port checks, replaces active authz init with a mock, and updates tooling (Dockerfile APT sandbox flag, openapitools.json, golangci-lint/.bingo/.mod, and .golangci.yml). Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/config_test.go (1)
11-24: Moveos.Removedefers after error checks to avoid possible nil derefUsing
//nolint:errcheckon theos.Removecalls is reasonable in tests, but placing thedeferbefore checkingerrmeansstringFile.Name()/intFile.Name()/boolFile.Name()/stringFile.Name()are evaluated even whencreateConfigFilereturns(nil, err), which would panic on a rare filesystem failure.Reordering to check
errfirst removes that edge‑case panic while preserving current behavior:func TestConfigReadStringFile(t *testing.T) { RegisterTestingT(t) - stringFile, err := createConfigFile("string", "example\n") - defer os.Remove(stringFile.Name()) //nolint:errcheck - if err != nil { - log.Fatal(err) - } + stringFile, err := createConfigFile("string", "example\n") + if err != nil { + log.Fatal(err) + } + defer os.Remove(stringFile.Name()) //nolint:errcheck @@ func TestConfigReadIntFile(t *testing.T) { @@ - intFile, err := createConfigFile("int", "123") - defer os.Remove(intFile.Name()) //nolint:errcheck - if err != nil { - log.Fatal(err) - } + intFile, err := createConfigFile("int", "123") + if err != nil { + log.Fatal(err) + } + defer os.Remove(intFile.Name()) //nolint:errcheck @@ func TestConfigReadBoolFile(t *testing.T) { @@ - boolFile, err := createConfigFile("bool", "true") - defer os.Remove(boolFile.Name()) //nolint:errcheck - if err != nil { - log.Fatal(err) - } + boolFile, err := createConfigFile("bool", "true") + if err != nil { + log.Fatal(err) + } + defer os.Remove(boolFile.Name()) //nolint:errcheck @@ func TestConfigReadQuotedFile(t *testing.T) { @@ - stringFile, err := createConfigFile("string", "example") - defer os.Remove(stringFile.Name()) //nolint:errcheck - if err != nil { - log.Fatal(err) - } + stringFile, err := createConfigFile("string", "example") + if err != nil { + log.Fatal(err) + } + defer os.Remove(stringFile.Name()) //nolint:errcheckAlso applies to: 26-39, 41-54, 56-69
🧹 Nitpick comments (15)
pkg/services/types.go (1)
31-45: Guarded parsing forpage/sizelooks correct and preserves defaultsThe new
err == nilguards aroundstrconv.Atoi/strconv.ParseIntmean invalid numeric query values no longer overwrite the defaultPage=1andSize=100, while still respecting valid values for bothpageSizeand legacysize. The MaxListSize clamp below continues to provide a hard upper bound, so behavior remains sane even for large inputs.If you ever want to tighten semantics further, you could optionally normalize
page <= 0back to1, but that’s not required for this PR.Based on learnings, this keeps ListArguments-based pagination/search behavior aligned with the repo’s conventions.
pkg/api/presenters/slice_filter.go (1)
147-153: Escapenameto avoid regex metacharacter issues.If
namecontains regex metacharacters (e.g.,.,*,+), the pattern may not match as intended. Useregexp.QuoteMeta()to escape the name.func removeStar(in map[string]bool, name string) map[string]bool { - pattern := `(` + name + `\..*)` + pattern := `(` + regexp.QuoteMeta(name) + `\..*)` pat, err := regexp.Compile(pattern) if err != nil { // Invalid pattern, return unchanged return in }pkg/services/cluster.go (1)
132-133: Comment is misleading.The comment says "Build the list of ConditionAvailable" but the code builds a list of
ResourceConditionobjects (one per adapter, derived from each adapter's "Available" condition). Consider updating to something like "Build the list of ResourceConditions from adapter Available conditions".pkg/api/cluster_types.go (1)
138-157: Consider logging marshal/unmarshal failures for observability.While the fallback behavior is correct, silent failures could mask data corruption or programming errors. Consider adding debug-level logging when these fallbacks trigger, to aid troubleshooting in production.
Note: The marshal of an empty slice at lines 154-157 cannot realistically fail, so that particular error check is purely defensive.
pkg/api/adapter_status_types.go (1)
71-74: Inconsistent error handling for Metadata unmarshal.Other unmarshal calls (Conditions at line 55, Data at line 64) now have explicit error handling with fallback defaults, but Metadata still uses
_ =to ignore errors. Consider handling this consistently for uniformity.// Unmarshal Metadata var metadata *openapi.AdapterStatusBaseMetadata if len(as.Metadata) > 0 { - _ = json.Unmarshal(as.Metadata, &metadata) + if err := json.Unmarshal(as.Metadata, &metadata); err != nil { + metadata = nil + } }pkg/auth/context.go (1)
82-96: JWT claim extraction is now safe; consider an optional helper to reduce repetitionThe checked string assertions for
username,first_name,last_name,clientIdcorrectly avoid panics on unexpected types and keep behavior clear.If you want to DRY this up, you could optionally introduce a small helper and use it here:
- if username, ok := claims["username"].(string); ok { - payload.Username = username - } - if firstName, ok := claims["first_name"].(string); ok { - payload.FirstName = firstName - } - if lastName, ok := claims["last_name"].(string); ok { - payload.LastName = lastName - } - if email, ok := claims["email"].(string); ok { - payload.Email = email - } - if clientID, ok := claims["clientId"].(string); ok { - payload.ClientID = clientID - } + payload.Username = getStringClaim(claims, "username") + payload.FirstName = getStringClaim(claims, "first_name") + payload.LastName = getStringClaim(claims, "last_name") + payload.Email = getStringClaim(claims, "email") + payload.ClientID = getStringClaim(claims, "clientId")And define (outside this range) something like:
func getStringClaim(claims jwt.MapClaims, key string) string { if v, ok := claims[key].(string); ok { return v } return "" }Purely optional; current code is correct and clear.
pkg/db/db_session/testcontainer.go (3)
47-55: Port-based wait strategy is correct; consider delegating topostgres.BasicWaitStrategies.Using
wait.ForListeningPort("5432/tcp").WithStartupTimeout(60*time.Second)viatestcontainers.WithWaitStrategyis idiomatic for testcontainers-go v0.33.0 and matches the docs for the HostPort wait strategy (string"5432/tcp"is the expected form, and 60s is the default timeout anyway). (golang.testcontainers.org)For future-proofing and cross‑platform stability (especially for non‑Linux developers), you might consider letting the Postgres module apply its own recommended combination of log+port wait strategies instead of hand-rolling:
- testcontainers.WithWaitStrategy( - wait.ForListeningPort("5432/tcp"). - WithStartupTimeout(60*time.Second)), + postgres.BasicWaitStrategies(),
postgres.BasicWaitStrategies()composes the documented pattern of waiting for the"database system is ready to accept connections"log twice plus a host‑side port readiness check, which the module authors call out as the reliable default. (golang.testcontainers.org)
76-78: Optionally tuneMaxIdleConnsalongsideMaxOpenConnsfor the test DB pool.You’re setting
SetMaxOpenConns, but leavingMaxIdleConnsat the default. For integration workloads that open and close many short‑lived connections, explicitly setting a sensibleMaxIdleConns(often equal to or slightly belowMaxOpenConns) can reduce churn and make behavior more predictable:f.sqlDB.SetMaxOpenConns(config.MaxOpenConnections) +if config.MaxIdleConnections > 0 { + f.sqlDB.SetMaxIdleConns(config.MaxIdleConnections) +}(Adjust the field name if your
DatabaseConfiguses a different one.)Based on learnings, this aligns with the repo’s guidance for configuring DB connection pooling under
pkg/db.
9-20: Logging in this helper still usesglog; consider migrating to the structured OCM logger.This file imports and uses
github.com/golang/glogfor all its messages. Repo guidelines for Go code preferlogger.NewOCMLogger(ctx)with contextual fields (opid, accountID, tx_id) for structured logging; even though this is a test helper and the logging style predates this PR, it’s a good candidate for a future cleanup to keep logging consistent across the codebase.You could, for example, initialize an OCM logger in
Initand thread it through the methods instead of using globalglog.As per coding guidelines, structured logging should be used in
**/*.go.pkg/services/generic_test.go (1)
23-23: Consider asserting on dbFactory.Close() instead of suppressing errcheckIgnoring the
Close()error is safe enough for a mock in tests, but you could strengthen the test and drop the nolint by checking it explicitly, e.g.:- defer dbFactory.Close() //nolint:errcheck + defer func() { + err := dbFactory.Close() + Expect(err).ToNot(HaveOccurred()) + }()This keeps the linter happy and will flag unexpected teardown failures in the future.
pkg/services/node_pool.go (1)
132-140: Clearer local error naming in unmarshal loopRenaming the inner error to
unmarshalErrremoves any ambiguity with the outererrused later in the function; behavior is unchanged and the intent is clearer.Makefile (1)
111-118: Directgolangci-lintinvocation simplifies the lint workflowSwitching to
golangci-lint run -D unusedremoves the indirection through a custom binary path and should resolve the presubmit path issues, assuminggolangci-lintis on$PATHin CI/Prow images.Separately from this change, you may want to align the
binarytarget with theinstall/cmdstargets by explicitly settingCGO_ENABLED=$(CGO_ENABLED)andGOEXPERIMENT=boringcryptothere as well for consistent FIPS‑compliant builds. As per coding guidelines, this keeps all build paths using the same crypto settings.Dockerfile.openapi (1)
3-5: Sandbox flag onapt-getis a pragmatic Prow/rootless workaroundUsing
-o APT::Sandbox::User=rootfor bothapt-get updateandapt-get installmatches the documented workaround for rootless Podman setgroups issues in CI and keeps the package set identical to before.pkg/db/sql_helpers.go (1)
83-87: Consider using a named constant for the magic number 11.The length check
len(leftStr) < 11corresponds tolen("properties."), which is correct. However, this magic number could be clearer.+const propertiesPrefix = "properties." + func propertiesNodeConverter(n tsl.Node) tsl.Node { // Get the left side operator. l, ok := n.Left.(tsl.Node) if !ok { return n } // Get the property name. leftStr, ok := l.Left.(string) - if !ok || len(leftStr) < 11 { + if !ok || len(leftStr) < len(propertiesPrefix) { return n } - propetyName := leftStr[11:] + propetyName := leftStr[len(propertiesPrefix):]Also note: there's a typo in
propetyName(missing 'r').pkg/db/transaction_middleware.go (1)
44-52: Consider logging errors for observability.The error handling is correct—headers are already sent, so status code changes aren't possible. However, these silent failures could hinder debugging in production. Based on learnings, this project uses structured logging via
logger.NewOCMLogger(ctx).Since this function lacks context access, you could either:
- Pass context as a parameter for logging capability
- Accept the silent failure for this low-level helper (reasonable given the caller already logged the original error)
The current approach is acceptable if the calling code logs the original error before calling
writeJSONResponse. Verify that callers handle logging appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/openapi.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/types.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/pkg/handlers/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Files:
pkg/handlers/node_pool.gopkg/handlers/helpers.gopkg/handlers/cluster.gopkg/handlers/openapi.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Files:
pkg/handlers/node_pool.gopkg/services/node_pool.gopkg/services/cluster.gopkg/api/node_pool_types.gopkg/db/db_session/test.gopkg/db/db_session/testcontainer.gopkg/db/sql_helpers.gopkg/handlers/helpers.gopkg/handlers/cluster.gopkg/db/db_session/default.gopkg/api/adapter_status_types.gopkg/auth/helpers.gopkg/handlers/openapi.gocmd/hyperfleet-api/main.gopkg/client/ocm/client.gopkg/services/generic_test.gocmd/hyperfleet-api/server/api_server.gopkg/api/cluster_types.gopkg/config/config_test.gopkg/services/types.gopkg/services/generic.gocmd/hyperfleet-api/environments/framework.gopkg/db/transaction_middleware.gocmd/hyperfleet-api/environments/framework_test.gopkg/api/presenters/slice_filter.gopkg/auth/context.go
**/pkg/services/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Files:
pkg/services/node_pool.gopkg/services/cluster.gopkg/services/generic_test.gopkg/services/types.gopkg/services/generic.go
**/pkg/{services,dao}/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Files:
pkg/services/node_pool.gopkg/services/cluster.gopkg/services/generic_test.gopkg/services/types.gopkg/services/generic.go
**/pkg/db/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/pkg/db/*.go: All database connections must respect db-sslmode parameter (disable | require | verify-ca | verify-full) with production defaulting to verify-full
Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Files:
pkg/db/sql_helpers.gopkg/db/transaction_middleware.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
All unit tests must run with OCM_ENV=unit_testing environment variable for proper test isolation
Files:
pkg/services/generic_test.gopkg/config/config_test.gocmd/hyperfleet-api/environments/framework_test.go
**/pkg/db/transaction_middleware.go
📄 CodeRabbit inference engine (CLAUDE.md)
All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Files:
pkg/db/transaction_middleware.go
{Makefile,*.mk,**/Makefile,**/*.mk}
📄 CodeRabbit inference engine (CLAUDE.md)
Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Files:
Makefile
{Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Podman for containerized development and build consistency (database, code generation)
Files:
Makefile
🧠 Learnings (23)
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : NodePool resources must enforce owner_references.id (parent cluster ID) via foreign key constraint in database schema
Applied to files:
pkg/services/node_pool.gopkg/api/node_pool_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
Applied to files:
pkg/services/node_pool.gopkg/services/cluster.gopkg/api/node_pool_types.gopkg/api/adapter_status_types.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/services/*.go : Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Applied to files:
pkg/services/node_pool.gopkg/services/cluster.gopkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Applied to files:
pkg/services/cluster.gopkg/db/db_session/default.gopkg/api/adapter_status_types.gopkg/services/generic_test.gopkg/services/types.gopkg/services/generic.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to openapi/openapi.yaml : Maintain openapi/openapi.yaml as the source of truth (32KB with $ref) and regenerate pkg/api/openapi/api/openapi.yaml (44KB fully resolved) via 'make generate' using Podman
Applied to files:
pkg/api/node_pool_types.gopkg/handlers/openapi.gopkg/api/cluster_types.goDockerfile.openapi
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use TypeSpec to define API specifications, which compile to OpenAPI 3.0.3, then generate Go models using openapi-generator-cli v7.16.0
Applied to files:
pkg/api/node_pool_types.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*service*,*handler*}.go : Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Applied to files:
pkg/db/db_session/test.gopkg/db/db_session/default.gocmd/hyperfleet-api/server/api_server.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/*.go : Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Applied to files:
pkg/db/db_session/test.gopkg/db/db_session/testcontainer.gopkg/db/db_session/default.gocmd/hyperfleet-api/server/api_server.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/test/integration/**/*.go : Integration tests must use Testcontainers to create isolated PostgreSQL instances for each test suite to ensure state isolation
Applied to files:
pkg/db/db_session/testcontainer.gopkg/services/generic_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/test/integration/**/*_test.go : All integration tests must run with OCM_ENV=integration_testing environment variable and fresh PostgreSQL database per test
Applied to files:
pkg/db/db_session/testcontainer.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
pkg/handlers/helpers.gopkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/handlers/*.go : HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Applied to files:
pkg/handlers/helpers.gopkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*db*}.go : All database operations must accept context.Context parameter to enable proper transaction propagation from middleware
Applied to files:
pkg/db/db_session/default.gocmd/hyperfleet-api/server/api_server.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/transaction_middleware.go : All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Applied to files:
pkg/db/db_session/default.gocmd/hyperfleet-api/server/api_server.gopkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Use polymorphic adapter_statuses table with owner_type and owner_id columns to avoid separate status tables for each resource type
Applied to files:
pkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/cmd/hyperfleet/environments/*.go : Use environment-based configuration via OCM_ENV variable (development, unit_testing, integration_testing, production) for environment-specific behavior
Applied to files:
.golangci.ymlcmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/environments/framework_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/api/*embed*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for runtime availability and self-contained binary
Applied to files:
.golangci.ymlpkg/handlers/openapi.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to pkg/api/openapi/api/openapi.yaml : Only edit openapi/openapi.yaml as the source; the generated pkg/api/openapi/api/openapi.yaml will be overwritten by 'make generate'
Applied to files:
pkg/handlers/openapi.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Store resource specifications in JSONB columns (spec field) to enable flexible cloud provider configuration
Applied to files:
pkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*handler*}.go : List API endpoints must support pagination and search via ListArgs with limit, offset, and filter parameters
Applied to files:
pkg/services/types.gopkg/services/generic.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to {Makefile,*.mk,**/Makefile,**/*.mk} : Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Applied to files:
Makefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use Go 1.24.9 or later for all builds and ensure Go version compatibility
Applied to files:
MakefileDockerfile.openapi
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to {Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile} : Use Podman for containerized development and build consistency (database, code generation)
Applied to files:
Dockerfile.openapi
🧬 Code graph analysis (8)
pkg/handlers/node_pool.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
pkg/db/sql_helpers.go (1)
pkg/errors/errors.go (1)
BadRequest(196-198)
pkg/handlers/cluster.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
pkg/api/adapter_status_types.go (1)
pkg/api/metadata_types.go (1)
Metadata(28-34)
cmd/hyperfleet-api/server/api_server.go (2)
cmd/hyperfleet-api/environments/types.go (1)
Database(36-38)pkg/db/session.go (1)
SessionFactory(12-20)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
cmd/hyperfleet-api/environments/framework.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Clients(67-69)
cmd/hyperfleet-api/environments/framework_test.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Services(44-47)
🔇 Additional comments (27)
pkg/api/presenters/slice_filter.go (2)
114-116: LGTM! Proper error propagation from recursive validation.This fix correctly surfaces validation errors from nested struct fields, preventing silent failures.
194-196: LGTM! Safe handling of nil time pointers.The added nil check and guarded type assertion correctly prevent panics when encountering nil
*time.Timefields.pkg/services/cluster.go (1)
139-141: LGTM!Renaming
errtounmarshalErravoids shadowing the outererrvariable and improves clarity. The silentcontinueon unmarshal failure is appropriate here since corrupted adapter data shouldn't block aggregation of other adapters.pkg/api/cluster_types.go (1)
72-75: Defensive error handling looks good.Falling back to empty structures when unmarshalling fails prevents panics and ensures the API returns valid responses even with corrupted database data.
pkg/api/node_pool_types.go (2)
82-100: LGTM!Defensive error handling for Spec, Labels, and StatusConditions unmarshalling is consistent with the pattern in
cluster_types.go. Fallback to empty structures ensures robust API responses.
159-178: LGTM!Marshal error handling with fallbacks is consistent with
cluster_types.go. The sequentialerrreuse is safe here since each error is checked immediately after its assignment.pkg/auth/context.go (3)
45-48: Safer username extraction from contextGuarding the type assertion with
okand falling back to""removes the risk of panics if the value is missing or not a string, while preserving expected behavior forSetUsernameContext.
100-115: Alternative claim fallbacks are robust and nil-safeThe fallback reads from
preferred_username,given_name, andfamily_namenow also use checked string assertions, which safely handle missing or non-string claims without panicking while keeping the previous behavior of gradually filling in missing fields.
119-126:name-based fallback is safely guarded and avoids panicsThe additional
okcheck onclaims["name"]combined with the length check onnamesensures this fallback path can’t panic on non-string or empty values, and preserves the original intent of deriving first/last name from a singlenameclaim.pkg/services/generic.go (3)
43-56: listContext expansion is coherent with downstream usageThe added fields (pagingMeta, ulog, resourceList, disallowedFields, resourceType, joins, groupBy, set) are all correctly initialized or safely zero-valued and are used consistently in
List,buildPreload,addJoins, andloadList. No issues from this refactor as written.
151-176: buildSearchValues correctly adapts to the simplified treeWalkForSqlizer APISwapping to
sqlizer, serviceErr := s.treeWalkForSqlizer(listCtx, tslTree)and dropping the intermediate TSL node return fits the actual usage pattern here (the transformed tree isn’t needed outside SQL generation). Error propagation remains clear and consistent.
332-346: treeWalkForSqlizer refactor looks correct and keeps semanticsThe new signature returning only
squirrel.Sqlizersimplifies callers without changing behavior:
db.FieldNameWalkis still applied first to validate/normalize fields usingdisallowedFields.- Failures from either
FieldNameWalkorsqlFilter.Walkare converted into*errors.ServiceErrorwithErrorBadRequest, and on success you now return just thesqlizer.This is a clean internal API tightening with no obvious edge‑case regressions.
pkg/services/generic_test.go (1)
62-76: Tests correctly updated for new treeWalkForSqlizer return signatureThe call to
treeWalkForSqlizernow binds onlysqlizerandserviceErr, and the subsequentToSql()assertions still validate both SQL and bound values. This matches the service’s refactored API and preserves the original test intent.cmd/hyperfleet-api/server/api_server.go (1)
146-151: Good addition to surface DB close failuresChecking and logging the error from
SessionFactory.Close()avoids silently ignoring shutdown problems while still keeping shutdown flow simple. This aligns well with the documented “call exactly once at exit” contract on the session factory.openapitools.json (1)
1-6: OpenAPI Generator version bump looks consistentSetting the CLI
"version": "7.16.0"is consistent with theopenapitools/openapi-generator-cli:v7.16.0base image inDockerfile.openapi, so the tooling versions line up. You may want to double‑check any external scripts or local workflows that still assume 5.4.0.pkg/db/db_session/default.go (1)
110-113: Now logging listener Ping failures is a nice observability winCapturing and logging
l.Ping()errors (even at V(10)) gives you visibility into notification channel issues without changing the behavior of the retry loop.pkg/handlers/cluster.go (1)
72-77: Defensive marshal error handling inPatchis appropriateAdding the
json.Marshalerror check and returning aGeneralErrorensures bad or unexpectedspecpayloads are surfaced as structured API errors instead of causing unhandled failures, while leaving the happy path unchanged.pkg/db/sql_helpers.go (2)
28-31: LGTM! Defensive type assertion prevents potential panic.The safe type assertion with
okcheck before usingleftStris the correct approach. This prevents a panic ifl.Leftis not a string.
145-150: LGTM! Proper error handling for invalid node structure.The explicit type-assertion check with a descriptive
BadRequesterror aligns with the pattern used elsewhere in this file and follows the project's error handling conventions frompkg/errors.pkg/handlers/helpers.go (1)
17-27: LGTM! Consistent defensive error handling.This pattern matches the error handling in
pkg/db/transaction_middleware.goandpkg/auth/helpers.go, creating a consistent approach across the codebase. Since headers are written before the payload (line 15), early returns on error are the correct behavior.pkg/auth/helpers.go (1)
29-39: LGTM! Consistent with project-wide error handling pattern.The defensive error handling aligns with the changes in
pkg/handlers/helpers.goandpkg/db/transaction_middleware.go. SincehandleError(the caller at line 22) already logs the original error before callingwriteJSONResponse, the silent early return here is acceptable.pkg/handlers/openapi.go (2)
58-65: LGTM! Defensive write error handling for OpenAPI endpoint.Properly handles the case where response writing fails after headers are committed. The early return is appropriate since no recovery is possible at this point.
67-74: LGTM! Consistent with GetOpenAPI error handling.Same defensive pattern applied to the OpenAPI UI endpoint.
cmd/hyperfleet-api/main.go (1)
28-30: LGTM! Explicit error handling for flag parsing.Converting silent flag parsing to explicit error handling with
glog.Fatalfis the right approach—a flag parsing failure at startup should halt the program. This aligns with the PR's broader goal of adding defensive error handling.pkg/handlers/node_pool.go (1)
73-77: LGTM! Proper error handling for JSON marshalling.The defensive check prevents silent failures when marshalling patch.Spec fails. This follows the handler pattern of handling errors with appropriate error responses.
pkg/client/ocm/client.go (1)
78-83: LGTM! Error propagation from Close() improves observability.The updated signature allows callers to handle and log close failures appropriately, as demonstrated in the corresponding
Teardown()changes inframework.go.cmd/hyperfleet-api/environments/framework_test.go (1)
25-56: Reflection fix is correct. Verify test runs withOCM_ENV=unit_testing.The change to
reflect.ValueOf(&env.Services).Elem()is the correct fix to obtain an addressablereflect.Value, which is necessary for proper field iteration.As per coding guidelines, unit tests should run with
OCM_ENV=unit_testingfor proper test isolation. Verify this test is executed with the correct environment variable set.
yingzhanredhat
force-pushed
the
ying-test
branch
from
b7202fe to
e45d034
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
Dockerfile.openapi (1)
8-8: Upgrade Go version to 1.24.9 or later.The Dockerfile installs go1.24.0, but per coding guidelines, all builds should use Go 1.24.9 or later for full compatibility. The PR build log also reports "non-expected Go version" warnings. Consider upgrading to a compatible patch version.
Apply this change to upgrade the Go version:
-RUN wget https://go.dev/dl/go1.24.0.linux-amd64.tar.gz && \ - tar -C /usr/local -xzf go1.24.0.linux-amd64.tar.gz && \ - rm go1.24.0.linux-amd64.tar.gz +RUN wget https://go.dev/dl/go1.24.9.linux-amd64.tar.gz && \ + tar -C /usr/local -xzf go1.24.9.linux-amd64.tar.gz && \ + rm go1.24.9.linux-amd64.tar.gzpkg/db/sql_helpers.go (2)
84-84: Extract magic number to a named constant.The magic number
11represents the length of the"properties."prefix. Consider extracting it to a named constant for clarity and maintainability.Apply this diff to improve code clarity:
+const propertiesPrefix = "properties." +const propertiesPrefixLen = len(propertiesPrefix) + func propertiesNodeConverter(n tsl.Node) tsl.Node { // Get the left side operator. l, ok := n.Left.(tsl.Node) if !ok { return n } // Get the property name. leftStr, ok := l.Left.(string) - if !ok || len(leftStr) < 11 { + if !ok || len(leftStr) < propertiesPrefixLen { return n } - propetyName := leftStr[11:] + propetyName := leftStr[propertiesPrefixLen:]
87-87: Fix typo in variable name.The variable
propetyNameshould bepropertyNamefor correct spelling.Apply this diff:
- propetyName := leftStr[11:] + propertyName := leftStr[11:] // Build a new node that converts: // ( properties.<name> = <value> ) to // ( properties ->> <name> = <value> ) propertyNode := tsl.Node{ Func: n.Func, Left: tsl.Node{ Func: tsl.IdentOp, - Left: fmt.Sprintf("properties ->> '%s'", propetyName), + Left: fmt.Sprintf("properties ->> '%s'", propertyName), }, Right: n.Right, }pkg/api/adapter_status_types.go (2)
54-68: Good defensive error handling for JSON unmarshalling.The explicit error handling with fallbacks to empty slice/map for
ConditionsandDatais a solid improvement over silently ignoring errors.Note: Line 73 still uses
_ = json.Unmarshal(...)forMetadata, which is inconsistent with the new pattern. Consider applying the same error handling there for consistency.
51-96: Consider adding logging for marshal/unmarshal fallbacks.Per the coding guidelines, structured logging via
logger.NewOCMLogger(ctx)should be used. The silent fallbacks here could make debugging difficult when JSON operations fail in production. Consider accepting acontext.Contextparameter and logging warnings when fallbacks are triggered.This is a low-priority enhancement that could be addressed in a follow-up.
Also applies to: 98-163
pkg/db/transaction_middleware.go (1)
44-52: Marshal/write errors are silently swallowed and the comment is misleadingRight now both
json.Marshalandw.Writeerrors cause an earlyreturnwith no logging, while the comment on Line 46 says “Log error”. That’s safe but makes these failures hard to diagnose and the comment inaccurate.Consider either:
- Plumb a logger/context into
writeJSONResponse(or accept a logger) and actually log the marshal/write errors, or- If you intentionally don’t want logging here, update the comment to reflect that the error is intentionally ignored after headers are sent.
This is non-blocking but will make behavior clearer and debugging easier.
pkg/services/node_pool.go (1)
132-140: Consider logging adapter condition unmarshal failures for observabilityRenaming the inner error to
unmarshalErris clearer, but these failures are still silently skipped. To make bad adapter payloads debuggable and align with the structured logging guidance, consider logging at warn/debug level before continuing:func (s *sqlNodePoolService) UpdateNodePoolStatusFromAdapters(ctx context.Context, nodePoolID string) (*api.NodePool, *errors.ServiceError) { - // Build the list of ConditionAvailable - adapters := []openapi.ResourceCondition{} + logger := logger.NewOCMLogger(ctx) + // Build the list of ConditionAvailable + adapters := []openapi.ResourceCondition{} @@ - if unmarshalErr := json.Unmarshal(adapterStatus.Conditions, &conditions); unmarshalErr != nil { - continue // Skip if can't unmarshal - } + if unmarshalErr := json.Unmarshal(adapterStatus.Conditions, &conditions); unmarshalErr != nil { + logger.Warnf("Failed to unmarshal adapter conditions for adapter %s on NodePool %s: %v", + adapterStatus.Adapter, nodePoolID, unmarshalErr) + continue // Skip if can't unmarshal + }As per coding guidelines, this also helps keep logging context-aware via
logger.NewOCMLogger(ctx).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/openapi.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/types.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (16)
- cmd/hyperfleet-api/environments/framework_test.go
- cmd/hyperfleet-api/main.go
- pkg/services/cluster.go
- pkg/api/cluster_types.go
- pkg/db/db_session/default.go
- pkg/auth/helpers.go
- Makefile
- pkg/handlers/openapi.go
- cmd/hyperfleet-api/server/api_server.go
- pkg/client/ocm/client.go
- pkg/auth/context.go
- .golangci.yml
- pkg/db/db_session/testcontainer.go
- pkg/handlers/helpers.go
- openapitools.json
- pkg/services/types.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Files:
pkg/api/presenters/slice_filter.gopkg/api/adapter_status_types.gopkg/services/generic_test.gopkg/services/generic.gopkg/db/db_session/test.gopkg/db/sql_helpers.gopkg/handlers/cluster.gopkg/services/node_pool.gopkg/db/transaction_middleware.gopkg/api/node_pool_types.gopkg/handlers/node_pool.gocmd/hyperfleet-api/environments/framework.go
**/pkg/services/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Files:
pkg/services/generic_test.gopkg/services/generic.gopkg/services/node_pool.go
**/pkg/{services,dao}/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Files:
pkg/services/generic_test.gopkg/services/generic.gopkg/services/node_pool.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
All unit tests must run with OCM_ENV=unit_testing environment variable for proper test isolation
Files:
pkg/services/generic_test.go
**/pkg/db/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/pkg/db/*.go: All database connections must respect db-sslmode parameter (disable | require | verify-ca | verify-full) with production defaulting to verify-full
Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Files:
pkg/db/sql_helpers.gopkg/db/transaction_middleware.go
**/pkg/handlers/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Files:
pkg/handlers/cluster.gopkg/handlers/node_pool.go
**/pkg/db/transaction_middleware.go
📄 CodeRabbit inference engine (CLAUDE.md)
All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Files:
pkg/db/transaction_middleware.go
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
Applied to files:
pkg/api/adapter_status_types.gopkg/services/node_pool.gopkg/api/node_pool_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/services/*.go : Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Applied to files:
pkg/api/adapter_status_types.gopkg/services/node_pool.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Use polymorphic adapter_statuses table with owner_type and owner_id columns to avoid separate status tables for each resource type
Applied to files:
pkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Applied to files:
pkg/api/adapter_status_types.gopkg/services/generic_test.gopkg/services/generic.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
pkg/api/adapter_status_types.gopkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/api/*embed*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for runtime availability and self-contained binary
Applied to files:
pkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to pkg/api/openapi/api/openapi.yaml : Only edit openapi/openapi.yaml as the source; the generated pkg/api/openapi/api/openapi.yaml will be overwritten by 'make generate'
Applied to files:
pkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/test/factories/**/*.go : Use test factories in test/factories/ for consistent test data generation with builder pattern
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/test/integration/**/*.go : Integration tests must use Testcontainers to create isolated PostgreSQL instances for each test suite to ensure state isolation
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*handler*}.go : List API endpoints must support pagination and search via ListArgs with limit, offset, and filter parameters
Applied to files:
pkg/services/generic.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to openapi/openapi.yaml : Maintain openapi/openapi.yaml as the source of truth (32KB with $ref) and regenerate pkg/api/openapi/api/openapi.yaml (44KB fully resolved) via 'make generate' using Podman
Applied to files:
Dockerfile.openapipkg/api/node_pool_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to {Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile} : Use Podman for containerized development and build consistency (database, code generation)
Applied to files:
Dockerfile.openapi
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use Go 1.24.9 or later for all builds and ensure Go version compatibility
Applied to files:
Dockerfile.openapi
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/transaction_middleware.go : All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/handlers/*.go : HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : NodePool resources must enforce owner_references.id (parent cluster ID) via foreign key constraint in database schema
Applied to files:
pkg/api/node_pool_types.gopkg/handlers/node_pool.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Store resource specifications in JSONB columns (spec field) to enable flexible cloud provider configuration
Applied to files:
pkg/api/node_pool_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/cmd/hyperfleet/environments/*.go : Use environment-based configuration via OCM_ENV variable (development, unit_testing, integration_testing, production) for environment-specific behavior
Applied to files:
cmd/hyperfleet-api/environments/framework.go
🧬 Code graph analysis (6)
pkg/api/adapter_status_types.go (1)
pkg/api/metadata_types.go (1)
Metadata(28-34)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
pkg/db/sql_helpers.go (1)
pkg/errors/errors.go (1)
BadRequest(196-198)
pkg/handlers/cluster.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
pkg/handlers/node_pool.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
cmd/hyperfleet-api/environments/framework.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Clients(67-69)
🔇 Additional comments (17)
Dockerfile.openapi (1)
3-5: Sandbox workaround for Prow rootless podman environment looks good.The addition of
-o APT::Sandbox::User=rootwith the explanatory comment correctly addresses the rootless podman setgroups error in the Prow environment, aligning with the PR objectives.cmd/hyperfleet-api/environments/framework.go (1)
158-162: LGTM! Properly addresses the previous review concern.The nil check prevents potential panic if
LoadClients()fails or is skipped, and the error handling ensures close failures are logged. This implementation matches the defensive pattern used forDatabase.SessionFactorycleanup above.pkg/db/sql_helpers.go (3)
28-29: LGTM! Defensive type assertion prevents panic.The checked type assertion properly guards against potential panics when
l.Leftis not a string, gracefully returningfalseinstead.
83-87: LGTM! Defensive checks prevent panics.The type assertion and length validation properly guard against:
- Panic from unchecked type assertion
- Out-of-bounds panic when slicing strings shorter than 11 characters
145-150: LGTM! Proper error handling for invalid node structure.The checked type assertion properly guards against panics and returns a clear
BadRequesterror when the node structure is invalid. This is consistent with the error handling pattern used elsewhere in the function (lines 127-128, 188).pkg/api/presenters/slice_filter.go (3)
114-116: LGTM! Essential fix to propagate nested validation errors.Previously, validation errors from nested struct fields were silently ignored. This change ensures they bubble up correctly, preventing silent failures.
149-153: Good defensive error handling.The pattern is constructed from struct field JSON tags and should almost never fail to compile. Returning the input unchanged on error is a reasonable fallback that prevents panics while preserving existing entries.
194-196: LGTM! Prevents nil pointer dereference.The nil check ensures that time.Time pointers are validated before calling
Format(), preventing runtime panics on optional/nil time fields.pkg/db/db_session/test.go (1)
216-222: Inconsistent error handling: choose either panic or error return.The function signature declares
erroras a return type but will never actually return an error—it either panics (line 218) or returnsnil(line 221). This creates a misleading API for callers who expect to handle returned errors.Consider one of the following consistent approaches:
- Remove the error return and keep panic (appropriate for test helpers):
-func (f *Test) ResetDB() error { +func (f *Test) ResetDB() { if err := resetDB(f.config); err != nil { panic(fmt.Sprintf("failed to reset test database: %v", err)) } f.wasDisconnected = true - return nil }
- Return the error instead of panicking (allows callers to handle failures):
func (f *Test) ResetDB() error { if err := resetDB(f.config); err != nil { - panic(fmt.Sprintf("failed to reset test database: %v", err)) + return fmt.Errorf("failed to reset test database: %w", err) } f.wasDisconnected = true return nil }Option 1 is simpler for test helpers where failures should abort the test. Option 2 provides more flexibility if some callers need graceful error handling. Verify all callers of
ResetDB()handle the returned error appropriately if option 2 is chosen.pkg/api/adapter_status_types.go (3)
122-127: LGTM: Defensive error handling for conditions marshalling.The fallback to an empty JSON array prevents failures from propagating while maintaining valid JSON structure.
130-140: Fix correctly addresses the scoping issue from past review.Using a dedicated
dataErrvariable inside theif req.Data != nilblock ensures the error check only applies whenjson.Marshalwas actually called, preventing false fallbacks whenreq.Datais nil.
144-150: LGTM: Proper error handling for optional metadata.The error check is correctly scoped immediately after the assignment, and falling back to
nilis appropriate for optional metadata.pkg/services/generic.go (1)
173-176: treeWalkForSqlizer signature & call-site update look correctThe refactor on Line 332 to return only
(squirrel.Sqlizer, *errors.ServiceError)is wired correctly intobuildSearchValues(Line 173):
- Errors from
db.FieldNameWalkandsqlFilter.Walknow consistently returnnilfor theSqlizerplus a non‑nil*ServiceError.- All visible call sites (
buildSearchValueshere and the tests inpkg/services/generic_test.go) checkserviceErrbefore callingToSql(), so thenilreturn on error is safe and won’t lead to panics.- Behavior and error semantics (BadRequest vs general error) are preserved while simplifying the API.
No changes needed from my side.
Also applies to: 332-346
pkg/services/generic_test.go (1)
23-24: Test updates for treeWalkForSqlizer and dbFactory cleanup look good; verify OCM_ENV setup
- Line 23: Deferring
dbFactory.Close()with//nolint:errcheckis acceptable for cleanup in tests. IfClose()can fail meaningfully, consider asserting on the error instead of suppressing it.- Lines 70–73: The test correctly uses the two-value
(sqlizer, serviceErr)return fromtreeWalkForSqlizerand guards onserviceErrbefore callingToSql(), which aligns with the new signature.Per coding guidelines, all unit tests must run with
OCM_ENV=unit_testingfor proper test isolation. Verify that this environment variable is configured in your test runner, Makefile, or shared test setup so this test inherits the correct isolation environment.pkg/handlers/cluster.go (1)
72-75: Marshal error handling onpatch.Speclooks correctChecking
json.Marshal(*patch.Spec)and returning aerrors.GeneralErroron failure prevents silently persisting an invalid spec and integrates cleanly with the existinghandlerConfig/handleErrorflow. No issues from a handler-pattern or error-propagation standpoint.pkg/handlers/node_pool.go (1)
73-76: Consistent, safer handling ofpatch.SpecmarshalingAdding
json.Marshal(*patch.Spec)with an explicit error check and returningerrors.GeneralErroron failure avoids persisting malformed specs and keeps Cluster and NodePool patch behavior consistent. This fits the expected handler pattern.pkg/api/node_pool_types.go (1)
80-101: Defensive JSON defaults for Spec/Labels/StatusConditions are reasonableThe new patterns that default Spec/Labels/StatusConditions to
{}/[]on marshal/unmarshal failure make the NodePool model more robust against unexpected or corrupt JSON while keeping the DB JSONB fields non-null and the OpenAPI view predictable. The nil→empty transitions are a minor semantic shift but should be safe and improve API stability.Also applies to: 159-178
yingzhanredhat
force-pushed
the
ying-test
branch
from
e45d034 to
e76006d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.golangci.yml (1)
56-62: Redundant and incorrectexclude-filesentry needs cleanup.The
exclude-filesentry on line 61 uses a directory path (pkg/api/openapi/) instead of a glob pattern, and it duplicates the exclusion already provided byexclude-dirson line 57. Remove this redundant entry or replace it with a file-specific glob pattern if targeting generated files by pattern (e.g.,.*_generated\.go$).Apply this diff to remove the redundant entry:
exclude-dirs: - pkg/api/openapi # Exclude files that require generated code exclude-files: - - pkg/api/openapi/ # Generated codeAlternatively, if you need file-specific exclusion:
exclude-files: + - ".*_generated\\.go$" # Generated files by naming convention
🧹 Nitpick comments (6)
cmd/hyperfleet-api/server/routes.go (1)
53-57: Missingvar err errordeclaration in commented-out block.The TODO block is reasonable for temporarily disabling authz. However, note that if this code is uncommented later, it will fail to compile because
erris not declared. The JWT middleware pattern (lines 45-47) correctly usesvar err errorbefore the assignment.For future re-enablement, the block should be:
// TODO: if env().Config.Server.EnableAuthz { // var err error // authzMiddleware, err = auth.NewAuthzMiddleware() // check(err, "Unable to create authz middleware") // }cmd/hyperfleet-api/server/logging/writer.go (1)
21-29: Consider capturing full body and default status for more accurate response logsUnrelated to this PR’s specific change, but while you’re touching this wrapper:
Writeonly keeps the last chunk (writer.responseBody = body), so multi-chunk responses are logged incompletely.- If a handler never calls
WriteHeaderexplicitly,responseStatusstays at its zero value (0) even though the underlyingResponseWriterwill send 200, so logs can show an incorrect status.You could optionally improve logging fidelity like this:
func (writer *loggingWriter) Write(body []byte) (int, error) { - writer.responseBody = body - return writer.ResponseWriter.Write(body) + if writer.responseStatus == 0 { + // Match net/http default status code when Write is called without WriteHeader. + writer.responseStatus = http.StatusOK + } + n, err := writer.ResponseWriter.Write(body) + // Append to preserve the full body across multiple writes. + writer.responseBody = append(writer.responseBody, body[:n]...) + return n, err }This is optional and only affects log accuracy, not handler behavior.
pkg/handlers/helpers.go (1)
18-26: Safer JSON response handling; consider future restructuring if you need 5xx on marshal errorsChecking both
json.Marshalandw.Writeerrors and returning early avoids silent failures and is an improvement over ignoring them. If you ever need to change the status code on marshal failure, you might later refactor to marshal before callingWriteHeader, but that’s not required for this PR.pkg/auth/helpers.go (1)
30-38: Consistent JSON response error handling in auth helpersThe added checks on
json.Marshalandw.Writemake auth responses safer and consistent with the handler/db helpers. If duplication between thesewriteJSONResponsevariants becomes a maintenance burden, you could later centralize this into a shared helper, but it’s fine as-is for this PR.pkg/db/transaction_middleware.go (1)
44-52: Clarify comment vs behavior in db writeJSONResponseHandling
json.Marshalandw.Writeerrors with early returns is good. The comment “Log error but don't expose to client” is a bit misleading since no log is actually emitted in this helper (and there’s no context/logger available here). Consider either adjusting the comment to reflect the current behavior or, if you later thread a logger/context into this function, actually logging the marshal error there.pkg/api/cluster_types.go (1)
138-141: Defensive marshal fallbacks are fine; consider logging unexpected errorsHandling marshal errors for
Spec,Labels, and the initial emptyStatusConditionsby falling back to"{}"/"[]"keeps inserts robust even if something unexpected happens during JSON encoding, and it avoids violating the JSONBNOT NULLintent. Since marshal failures here should be highly unusual (given input came from JSON already), you might optionally log these errors before substituting the default payloads to aid debugging if they ever occur, but the current behavior is functionally safe.Also applies to: 148-151, 154-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
.golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(1 hunks)pkg/services/types.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- cmd/hyperfleet-api/server/logging/request_logging_middleware.go
- pkg/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- cmd/hyperfleet-api/main.go
- pkg/services/types.go
- pkg/db/db_session/default.go
- pkg/db/db_session/testcontainer.go
- pkg/api/presenters/slice_filter.go
- pkg/api/node_pool_types.go
- pkg/api/adapter_status_types.go
- pkg/auth/context.go
- pkg/handlers/node_pool.go
- pkg/db/sql_helpers.go
- cmd/hyperfleet-api/server/api_server.go
- pkg/db/db_session/test.go
- pkg/services/node_pool.go
- Makefile
- cmd/hyperfleet-api/environments/framework_test.go
- openapitools.json
- pkg/services/generic_test.go
🧰 Additional context used
📓 Path-based instructions (6)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Files:
pkg/auth/helpers.gopkg/handlers/openapi.gopkg/db/transaction_middleware.gopkg/services/status_aggregation.gopkg/handlers/framework.gopkg/services/cluster.gopkg/handlers/cluster.gocmd/hyperfleet-api/environments/framework.gopkg/client/ocm/client.gopkg/handlers/nodepool_status.gocmd/hyperfleet-api/server/logging/writer.gopkg/handlers/helpers.gocmd/hyperfleet-api/server/routes.gopkg/handlers/cluster_status.gopkg/api/cluster_types.gopkg/services/generic.go
**/pkg/handlers/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Files:
pkg/handlers/openapi.gopkg/handlers/framework.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/helpers.gopkg/handlers/cluster_status.go
**/pkg/db/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/pkg/db/*.go: All database connections must respect db-sslmode parameter (disable | require | verify-ca | verify-full) with production defaulting to verify-full
Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Files:
pkg/db/transaction_middleware.go
**/pkg/db/transaction_middleware.go
📄 CodeRabbit inference engine (CLAUDE.md)
All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Files:
pkg/db/transaction_middleware.go
**/pkg/services/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Files:
pkg/services/status_aggregation.gopkg/services/cluster.gopkg/services/generic.go
**/pkg/{services,dao}/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Files:
pkg/services/status_aggregation.gopkg/services/cluster.gopkg/services/generic.go
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
pkg/auth/helpers.gopkg/handlers/openapi.gopkg/db/transaction_middleware.gopkg/handlers/framework.gopkg/handlers/helpers.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/handlers/*.go : HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Applied to files:
pkg/auth/helpers.gopkg/handlers/openapi.gopkg/handlers/framework.gopkg/handlers/helpers.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/api/*embed*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for runtime availability and self-contained binary
Applied to files:
pkg/handlers/openapi.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.go.golangci.ymlpkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to openapi/openapi.yaml : Maintain openapi/openapi.yaml as the source of truth (32KB with $ref) and regenerate pkg/api/openapi/api/openapi.yaml (44KB fully resolved) via 'make generate' using Podman
Applied to files:
pkg/handlers/openapi.go.golangci.ymlpkg/api/cluster_types.goDockerfile.openapi
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to pkg/api/openapi/api/openapi.yaml : Only edit openapi/openapi.yaml as the source; the generated pkg/api/openapi/api/openapi.yaml will be overwritten by 'make generate'
Applied to files:
pkg/handlers/openapi.go.golangci.ymlpkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/transaction_middleware.go : All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
Applied to files:
pkg/services/status_aggregation.gopkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/services/*.go : Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Applied to files:
pkg/services/status_aggregation.gopkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Use polymorphic adapter_statuses table with owner_type and owner_id columns to avoid separate status tables for each resource type
Applied to files:
pkg/services/status_aggregation.gopkg/handlers/cluster_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Store resource specifications in JSONB columns (spec field) to enable flexible cloud provider configuration
Applied to files:
pkg/services/cluster.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/cmd/hyperfleet/environments/*.go : Use environment-based configuration via OCM_ENV variable (development, unit_testing, integration_testing, production) for environment-specific behavior
Applied to files:
cmd/hyperfleet-api/environments/framework.go.golangci.yml
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/*.go : Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Applied to files:
pkg/handlers/nodepool_status.gopkg/handlers/cluster_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/cmd/hyperfleet/server/*.go : Use JWT authentication from Red Hat SSO (via --jwk-cert-url flag) with account ID and username extraction from claims
Applied to files:
cmd/hyperfleet-api/server/routes.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use TypeSpec to define API specifications, which compile to OpenAPI 3.0.3, then generate Go models using openapi-generator-cli v7.16.0
Applied to files:
pkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to {Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile} : Use Podman for containerized development and build consistency (database, code generation)
Applied to files:
Dockerfile.openapi
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use Go 1.24.9 or later for all builds and ensure Go version compatibility
Applied to files:
Dockerfile.openapi
🧬 Code graph analysis (5)
pkg/handlers/cluster.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
cmd/hyperfleet-api/environments/framework.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Clients(67-69)
pkg/handlers/nodepool_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
pkg/handlers/cluster_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
🔇 Additional comments (16)
Dockerfile.openapi (1)
3-5: Correct and well-documented APT sandbox workaround for rootless Podman in Prow.The changes appropriately apply the
-o APT::Sandbox::User=rootflag to bothapt-get updateandapt-get installcommands, resolving the setgroups error encountered in rootless Podman environments. The explanatory comment on line 3 makes the intent clear. This aligns with the PR objective and is a minimal, targeted fix..golangci.yml (2)
49-52: ✅ Staticcheck linter re-enabled with proper justification.The previous concern about staticcheck being disabled has been resolved—it is now properly enabled on line 46. The current disable list (unused, gosec, unparam) is well-justified with clear explanations for each choice (false positives, known CVE issues, strictness concerns).
1-76: Configuration looks solid and well-documented.The configuration is well-structured with clear rationale for linter selections and customizations. The 5-minute timeout, enabled test linting, and targeted linter settings align well with the PR objectives (zero lint issues post-
make lint). The note at the top (lines 4–5) appropriately reminds developers to runmake generatebefore linting, which prevents generated code from causing lint failures.cmd/hyperfleet-api/server/logging/writer.go (1)
45-51: Change towriter.Header()is correct and slightly more idiomaticUsing the promoted
Header()method (writer.Header()) instead ofwriter.ResponseWriter.Header()is behaviorally equivalent here and marginally improves readability/future-proofing (e.g., ifloggingWriterever overridesHeader). No issues from this change.pkg/services/cluster.go (1)
139-141: LGTM! Clear error handling.The rename to
unmarshalErrwith an explicit nil check improves code clarity and aligns with the PR's pattern of defensive error handling for JSON operations.pkg/services/generic.go (2)
173-173: LGTM!The call site correctly updated to match the new
treeWalkForSqlizersignature, dropping the unusedtslTreereturn value.
332-345: LGTM!The refactored function signature correctly removes the unused
tsl.Nodereturn value. The modified tree fromFieldNameWalkis only needed internally to generate thesqlizer, so returning it was unnecessary. All return statements properly match the new signature.cmd/hyperfleet-api/environments/framework.go (1)
158-162: LGTM! Nil guard addresses previous review concern.The nil check before calling
Close()prevents the potential panic that was flagged in the previous review. Error handling is consistent with the database teardown pattern above.pkg/client/ocm/client.go (1)
78-83: Verify all callers of Close() handle the new error return and confirm structured logging compliance.The signature change from
Close()toClose() errorenables proper error propagation and the nil check is defensive. However, all call sites must be updated to handle the returned error, and the implementation should include structured logging vialogger.NewOCMLogger(ctx)per coding guidelines for Go files.pkg/services/status_aggregation.go (1)
155-163: Switch on adapter availability is correct and clearerUsing a switch on
adapterInfo.availablepreserves the existingTrue/Falsebehavior and now explicitly logs unexpected values while still treating them asNotReady, which matches the documented Ready/Failed/NotReady semantics. Based on learnings, this aligns with the Kubernetes-style condition aggregation guidance.pkg/handlers/openapi.go (1)
61-64: Write error checks in OpenAPI handlers look goodAdding error checks around
w.Writein bothGetOpenAPIandGetOpenAPIUIand returning once a write fails is correct given the response is already committed and avoids silent IO errors.Also applies to: 70-73
pkg/handlers/framework.go (1)
109-114: Idiomatic switch on serviceErr in handleGetSwitching directly on
serviceErrwith anilcase keeps the same behavior as the previous boolean switch and reads more idiomatically without changing status or error handling.pkg/handlers/nodepool_status.go (1)
11-13: Good structured logging for non-fatal nodepool aggregation errorsImporting the logger and emitting a
Warningwithnodepool_idanderrorwhenUpdateNodePoolStatusFromAdaptersfails makes these soft failures observable without breaking the request, and useslogger.NewOCMLogger(ctx)in line with the structured logging guidance. As per coding guidelines, this aligns with the recommended logging practice.Also applies to: 92-96
pkg/handlers/cluster_status.go (1)
11-13: Consistent clustered status aggregation loggingThe added
loggerimport and warning log onUpdateClusterStatusFromAdaptersfailure provide visibility into aggregation issues while keeping the Create operation non-fatal, and uselogger.NewOCMLogger(ctx)with structured fields (cluster_id,error) consistently with the nodepool handler. As per coding guidelines, this is aligned with the structured logging requirement.Also applies to: 92-96
pkg/handlers/cluster.go (1)
72-75: Solid JSON marshal error handling in PATCH pathAdding explicit
json.Marshalerror handling and returningerrors.GeneralErrorkeeps invalid specs from being persisted and aligns with the structured error pattern used elsewhere in handlers. No further changes needed here.pkg/api/cluster_types.go (1)
72-75: Improved resilience when unmarshalling JSON fieldsThe new error-handling around
Spec,Labels, andStatusConditionsJSON unmarshalling (falling back to empty maps/slices on error) is a good defensive improvement and prevents partially decoded or corrupt JSON from leaking into the OpenAPI representation. The explicit note thatStatusConditionsare stored asResourceConditionin the DB also clarifies the model mapping and is consistent with the conditions guidance for status fields.Also applies to: 80-83, 85-90
yingzhanredhat
force-pushed
the
ying-test
branch
from
e76006d to
5ce95ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Makefile (2)
120-125: Apply FIPS-compliant flags to thebinarytarget for consistency.The
installtarget (line 129) correctly usesCGO_ENABLED=$(CGO_ENABLED) GOEXPERIMENT=boringcrypto, but thebinarytarget (line 124) does not set these flags. This creates an inconsistency: binaries built withmake binarywill not be FIPS-compliant, while those built withmake installwill be.Based on learnings, FIPS-compliant crypto is required for enterprise/government deployments.
Apply this diff to add FIPS flags to the
binarytarget:binary: check-gopath echo "Building version: ${build_version}" - ${GO} build -ldflags="$(ldflags)" -o hyperfleet-api ./cmd/hyperfleet-api + CGO_ENABLED=$(CGO_ENABLED) GOEXPERIMENT=boringcrypto ${GO} build -ldflags="$(ldflags)" -o hyperfleet-api ./cmd/hyperfleet-api
254-261: Add FIPS-compliant flagGOEXPERIMENT=boringcryptoto thecmdstarget.The
cmdstarget setsCGO_ENABLEDbut omitsGOEXPERIMENT=boringcrypto. For consistency with theinstalltarget and to ensure FIPS compliance across all built binaries, this flag should be added.Apply this diff to add the missing FIPS flag:
.PHONY: cmds cmds: for cmd in $$(ls cmd); do \ - CGO_ENABLED=$(CGO_ENABLED) ${GO} build \ + CGO_ENABLED=$(CGO_ENABLED) GOEXPERIMENT=boringcrypto ${GO} build \ -ldflags="$(ldflags)" \ -o "$${cmd}" \ "./cmd/$${cmd}" \
♻️ Duplicate comments (1)
pkg/services/cluster.go (1)
132-132: Comment improvement addresses past review feedback.The updated comment now accurately describes building a list of
ResourceConditionfrom adapter Available conditions, addressing the previous review concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
.golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(1 hunks)pkg/services/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- pkg/handlers/node_pool.go
- pkg/api/node_pool_types.go
- pkg/handlers/cluster_status.go
- pkg/db/db_session/test.go
- cmd/hyperfleet-api/environments/framework.go
- pkg/api/presenters/slice_filter.go
- cmd/hyperfleet-api/server/logging/writer.go
- pkg/db/sql_helpers.go
- pkg/handlers/cluster.go
- pkg/handlers/framework.go
- pkg/services/status_aggregation.go
- .golangci.yml
- pkg/services/node_pool.go
- pkg/services/types.go
- pkg/db/db_session/testcontainer.go
- cmd/hyperfleet-api/main.go
- pkg/config/config_test.go
- pkg/services/generic_test.go
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Files:
pkg/auth/helpers.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/logging/request_logging_middleware.gopkg/handlers/openapi.gocmd/hyperfleet-api/environments/framework_test.gopkg/api/adapter_status_types.gopkg/handlers/helpers.gopkg/api/cluster_types.gopkg/db/transaction_middleware.gopkg/services/cluster.gopkg/services/generic.gopkg/db/db_session/default.gopkg/client/ocm/client.gopkg/handlers/nodepool_status.gopkg/auth/context.gocmd/hyperfleet-api/server/routes.go
**/pkg/handlers/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Files:
pkg/handlers/openapi.gopkg/handlers/helpers.gopkg/handlers/nodepool_status.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
All unit tests must run with OCM_ENV=unit_testing environment variable for proper test isolation
Files:
cmd/hyperfleet-api/environments/framework_test.go
**/pkg/db/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/pkg/db/*.go: All database connections must respect db-sslmode parameter (disable | require | verify-ca | verify-full) with production defaulting to verify-full
Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Files:
pkg/db/transaction_middleware.go
**/pkg/db/transaction_middleware.go
📄 CodeRabbit inference engine (CLAUDE.md)
All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Files:
pkg/db/transaction_middleware.go
**/pkg/services/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Files:
pkg/services/cluster.gopkg/services/generic.go
**/pkg/{services,dao}/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Files:
pkg/services/cluster.gopkg/services/generic.go
{Makefile,*.mk,**/Makefile,**/*.mk}
📄 CodeRabbit inference engine (CLAUDE.md)
Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Files:
Makefile
{Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Podman for containerized development and build consistency (database, code generation)
Files:
Makefile
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to openapi/openapi.yaml : Maintain openapi/openapi.yaml as the source of truth (32KB with $ref) and regenerate pkg/api/openapi/api/openapi.yaml (44KB fully resolved) via 'make generate' using Podman
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use Go 1.24.9 or later for all builds and ensure Go version compatibility
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/api/*embed*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for runtime availability and self-contained binary
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/*.go : Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
pkg/auth/helpers.gopkg/handlers/openapi.gopkg/handlers/helpers.gopkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/handlers/*.go : HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Applied to files:
pkg/auth/helpers.gopkg/handlers/openapi.gopkg/handlers/helpers.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*service*,*handler*}.go : Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Applied to files:
cmd/hyperfleet-api/server/api_server.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*db*,*model*,migrations}.go : Use GORM for database access with automatic migrations via gorm.AutoMigrate at startup
Applied to files:
cmd/hyperfleet-api/server/api_server.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/*.go : Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Applied to files:
cmd/hyperfleet-api/server/api_server.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to openapi/openapi.yaml : Maintain openapi/openapi.yaml as the source of truth (32KB with $ref) and regenerate pkg/api/openapi/api/openapi.yaml (44KB fully resolved) via 'make generate' using Podman
Applied to files:
Dockerfile.openapipkg/handlers/openapi.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to {Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile} : Use Podman for containerized development and build consistency (database, code generation)
Applied to files:
Dockerfile.openapi
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use Go 1.24.9 or later for all builds and ensure Go version compatibility
Applied to files:
Dockerfile.openapiMakefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/api/*embed*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for runtime availability and self-contained binary
Applied to files:
pkg/handlers/openapi.gopkg/api/cluster_types.gopkg/handlers/nodepool_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to pkg/api/openapi/api/openapi.yaml : Only edit openapi/openapi.yaml as the source; the generated pkg/api/openapi/api/openapi.yaml will be overwritten by 'make generate'
Applied to files:
pkg/handlers/openapi.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/cmd/hyperfleet/environments/*.go : Use environment-based configuration via OCM_ENV variable (development, unit_testing, integration_testing, production) for environment-specific behavior
Applied to files:
cmd/hyperfleet-api/environments/framework_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
Applied to files:
pkg/api/adapter_status_types.gopkg/api/cluster_types.gopkg/services/cluster.gopkg/handlers/nodepool_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Use polymorphic adapter_statuses table with owner_type and owner_id columns to avoid separate status tables for each resource type
Applied to files:
pkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/services/*.go : Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Applied to files:
pkg/api/adapter_status_types.gopkg/services/cluster.gopkg/handlers/nodepool_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Applied to files:
pkg/api/adapter_status_types.gopkg/services/cluster.gopkg/services/generic.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Store resource specifications in JSONB columns (spec field) to enable flexible cloud provider configuration
Applied to files:
pkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use TypeSpec to define API specifications, which compile to OpenAPI 3.0.3, then generate Go models using openapi-generator-cli v7.16.0
Applied to files:
pkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/transaction_middleware.go : All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/*.go : Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Applied to files:
pkg/handlers/nodepool_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/cmd/hyperfleet/server/*.go : Use JWT authentication from Red Hat SSO (via --jwk-cert-url flag) with account ID and username extraction from claims
Applied to files:
cmd/hyperfleet-api/server/routes.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to {Makefile,*.mk,**/Makefile,**/*.mk} : Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Applied to files:
Makefile
🧬 Code graph analysis (4)
cmd/hyperfleet-api/server/api_server.go (2)
cmd/hyperfleet-api/environments/types.go (1)
Database(36-38)pkg/db/session.go (1)
SessionFactory(12-20)
cmd/hyperfleet-api/environments/framework_test.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Services(44-47)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
pkg/handlers/nodepool_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
🔇 Additional comments (27)
cmd/hyperfleet-api/server/logging/request_logging_middleware.go (1)
14-14: Boolean simplification preserves behaviorThe simplified
doLogcondition correctly keeps logging disabled only for/api/hyperfleet(with or without trailing slash after normalization) and enabled for all other paths; behavior and intent are preserved while improving readability.Dockerfile.openapi (1)
3-5: Podman rootless sandbox workaround is well-documented and correctly applied.The changes appropriately address the Prow/rootless podman compatibility issue described in the PR objectives. The explanatory comment is clear and helpful for future maintainers.
Makefile (2)
114-118: Approved: lint target simplification with direct golangci-lint invocation.Removing the
GOLANGCI_LINT_BINvariable and invokinggolangci-lintdirectly is cleaner and aligns with the new.golangci.ymlconfiguration. The change assumesgolangci-lintis inPATH, which is reasonable for a tool that developers typically install globally or viago install.
55-56: Verify Go version constraint enforces 1.24.9 minimum.The
GO_VERSIONpatterngo1.24.will match any Go 1.24.x version (including 1.24.0), but the learnings state "Use Go 1.24.9 or later for all builds." The current pattern does not enforce the minimum patch version (1.24.9). Consider whether this is intentional leniency or if stricter matching (e.g., checking for patch ≥ 9) is needed.pkg/services/generic.go (2)
43-43: LGTM: Typo fixed.The comment now correctly spells "function".
173-173: LGTM: Clean refactoring to remove unused return value.The signature change simplifies the API by eliminating the unused
tsl.Nodereturn value. The call site at line 173 and all three return statements (lines 336, 342, 345) have been updated consistently to match the new signature.Also applies to: 332-345
openapitools.json (1)
5-5: Confirm that generated code was properly regenerated with v7.16.0 and validate compatibility.This is a major version jump (5.4.0 → 7.16.0) with documented breaking changes between v6.0.0 and v7.16.0, including:
- Templating refactors affecting variable names and template structure
- Inline schema/$ref resolution behavior changes that can alter generated model structures
- Enum and name sanitization rule changes that can modify identifier names in generated code
- CodegenOperation/CodegenProperty API changes affecting operation and parameter handling
Ensure that:
- All generated files in
pkg/api/openapi/api/were properly regenerated viamake generate- Generated model names, field names, and operation signatures remain compatible with handler implementations
- Any enum sanitization changes did not break code that references these enums
Review the v6.0.0 and v7.x release notes (particularly v7.10–v7.16) to identify generator-specific changes for your use case.
cmd/hyperfleet-api/environments/framework_test.go (1)
39-39: LGTM: Safe reflection pattern.The change to use
reflect.ValueOf(&env.Services).Elem()creates an addressable reflection value, which is a common Go idiom. While addressability isn't strictly required for the read-only operations performed here (field iteration and nil checks), the pattern is safe, correct, and may have addressed a linter concern.cmd/hyperfleet-api/server/api_server.go (1)
149-151: LGTM: Proper error handling for database cleanup.The explicit error check and logging for
SessionFactory.Close()follows Go best practices and aligns with the broader error-handling improvements across this PR.pkg/db/db_session/default.go (1)
111-113: LGTM: Proper error handling for async ping.Handling and logging the error from
l.Ping()in the goroutine improves observability without affecting control flow.pkg/db/transaction_middleware.go (1)
44-52: LGTM: Explicit error handling for response marshaling and writing.The early returns on marshal/write errors follow best practices and align with similar improvements across the codebase (pkg/auth/helpers.go, pkg/handlers/helpers.go).
pkg/auth/helpers.go (1)
30-38: LGTM: Consistent error handling pattern.The explicit error checks for JSON marshaling and response writing match the pattern applied in pkg/db/transaction_middleware.go and pkg/handlers/helpers.go, ensuring consistent error handling across authentication flows.
pkg/services/cluster.go (1)
139-139: LGTM: Clearer variable naming.Renaming the error variable to
unmarshalErrimproves readability by distinguishing it from other error variables in outer scopes.pkg/handlers/helpers.go (1)
18-26: LGTM: Essential error handling for widely-used helper.Since
writeJSONResponseis called by multiple handlers throughout the codebase, the explicit error handling for marshaling and writing improves reliability across all API endpoints.pkg/handlers/nodepool_status.go (1)
92-95: LGTM: Improved error visibility with structured logging.The inline error handling and structured logging with context (
nodepool_id,error) improves observability while correctly treating aggregation failures as non-fatal. This follows the coding guideline to uselogger.NewOCMLogger(ctx)with context-aware fields.Based on coding guidelines, this follows the structured logging pattern.
pkg/client/ocm/client.go (1)
78-82: Verify structured logging requirement and error handling by all Close() callers.The signature change to return
erroraligns with Go conventions, and the nil connection handling is correct. However, per coding guidelines, all Go files must use structured logging vialogger.NewOCMLogger(ctx)with context-aware fields (opid, accountID, tx_id). Confirm whether Close() should include structured logging for error/success reporting, or if this method is exempt. Additionally, verify that all callers ofClient.Close()have been updated to handle the returned error appropriately.pkg/api/adapter_status_types.go (5)
55-58: LGTM!The error handling ensures that unmarshal failures result in a safe empty slice rather than a nil value or panic.
64-67: LGTM!Consistent error handling pattern that ensures Data is always a valid map, preventing downstream nil dereferences.
122-127: LGTM!The fallback to empty array JSON is appropriate. While marshal failures are rare for well-formed Go structs, this ensures the database receives valid JSON.
130-140: LGTM!The error scope issue from the previous review has been correctly resolved by using a separate
dataErrvariable. The logic now properly handles both nil data and marshal failures.
145-149: LGTM!The nil fallback for optional metadata is appropriate and maintains consistency with the field's optional nature.
pkg/auth/context.go (3)
45-48: LGTM!The safe type assertion with ok-check prevents potential panics when the context value is not a string.
82-96: LGTM!The safe type assertions for all primary claims prevent panics when JWT tokens contain unexpected claim types or are malformed.
100-127: LGTM!All fallback claim extractions use safe type assertions, and the name-splitting logic is properly guarded with an ok-check. This robustly handles different JWT token formats from various issuers (RHSSO vs RHD).
pkg/handlers/openapi.go (1)
61-64: LGTM!Checking the
Writeerror is good defensive coding practice, even though the response has already been committed and no error can be sent to the client. This makes error handling explicit and aligns with the broader project pattern of explicit error checking observed across handlers.Also applies to: 70-73
pkg/api/cluster_types.go (2)
72-90: LGTM!The error handling for all unmarshal operations ensures safe defaults (empty maps/slices) when JSONB data is corrupt or invalid. The comment on Line 85 helpfully clarifies the storage type for StatusConditions.
138-157: LGTM!The error handling for all marshal operations ensures the database always receives valid JSON, with appropriate fallbacks. This pattern is consistent with the defensive approach in
adapter_status_types.go.
yingzhanredhat
force-pushed
the
ying-test
branch
from
5ce95ab to
08bcd18
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Makefile (2)
56-56: Enforce Go 1.24.9 minimum rather than permissive 1.24.x pattern.The
GO_VERSION:=go1.24.pattern matches any Go 1.24.x release (1.24.0, 1.24.1, 1.24.9, etc.). Per learnings, Go 1.24.9 or later is required for all builds. The current pattern allows older patch versions that may have bugs or security issues. Specify the minimum version explicitly.Apply this diff to enforce the minimum required version:
-GO_VERSION:=go1.24. +GO_VERSION:=go1.24.9Note: Update the verify and install targets' grep checks to use a regex or exact match as appropriate for your version pinning strategy.
122-125: Binary target missing FIPS-compliant crypto flags.The
binarytarget does not applyCGO_ENABLED=$(CGO_ENABLED) GOEXPERIMENT=boringcrypto, while theinstalltarget (line 129) andcmdstarget (line 256) both correctly use these flags. Per coding guidelines, Go binaries must be built with FIPS-compliant crypto for enterprise/government deployments.Apply this diff to enforce FIPS compliance in the binary target:
binary: check-gopath echo "Building version: ${build_version}" - ${GO} build -ldflags="$(ldflags)" -o hyperfleet-api ./cmd/hyperfleet-api + CGO_ENABLED=$(CGO_ENABLED) GOEXPERIMENT=boringcrypto ${GO} build -ldflags="$(ldflags)" -o hyperfleet-api ./cmd/hyperfleet-api
🧹 Nitpick comments (5)
pkg/client/ocm/client.go (1)
78-83: Consider makingClose()tolerant of a nil receiverThe new
Close() errorimplementation and nil-check onconnectionlook good and integrate cleanly with callers that log the returned error.You might optionally make
Close()safe to call on a nil*Clientto avoid potential panics if future code ever doesvar c *Client; c.Close():func (c *Client) Close() error { if c == nil || c.connection == nil { return nil } return c.connection.Close() }Not required for this PR, but it can make the API more defensive.
pkg/db/db_session/testcontainer.go (2)
76-79: ConfigureSetMaxIdleConnsas well to fully follow pooling guidanceYou only set
SetMaxOpenConns; consider also configuringSetMaxIdleConns(e.g., fromconfig.MaxIdleConnectionsif available) so this session factory fully matches the db pooling guidance and behaves like the non-test session setup.Based on learnings, configuring both max open and max idle connections is recommended for
pkg/dbcode.
44-105: Optional: migrateglogusage to structuredlogger.NewOCMLogger(ctx)This helper still uses
glogwithcontext.Background(); over time it would be better to switch to the structured logger with context fields (opid, accountID, tx_id) so logs from testcontainers are consistent with the rest of the codebase.As per coding guidelines, new or touched Go code should prefer
logger.NewOCMLogger(ctx)over bareglog.pkg/handlers/helpers.go (1)
17-26: Handler JSON responses now guard marshal/write failures.Adding explicit checks around JSON marshaling and
w.Writealigns handlers with the new defensive pattern and avoids hidden failures while keeping existing HTTP semantics.pkg/services/node_pool.go (1)
132-141: Local unmarshal error variable avoids shadowing outer errors.Renaming the JSON unmarshal error to
unmarshalErrkeeps it clearly scoped and prevents accidental reuse of the function-levelerr, improving readability and safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
.golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(1 hunks)pkg/services/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- pkg/handlers/openapi.go
- pkg/handlers/cluster_status.go
- cmd/hyperfleet-api/server/api_server.go
- pkg/api/cluster_types.go
- pkg/db/sql_helpers.go
- cmd/hyperfleet-api/server/logging/writer.go
- pkg/config/config_test.go
- pkg/api/node_pool_types.go
- pkg/services/cluster.go
- cmd/hyperfleet-api/server/logging/request_logging_middleware.go
- cmd/hyperfleet-api/server/routes.go
- pkg/handlers/framework.go
- pkg/services/status_aggregation.go
- Dockerfile.openapi
- pkg/api/presenters/slice_filter.go
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/db/transaction_middleware.go
- pkg/services/types.go
- .golangci.yml
- openapitools.json
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Files:
cmd/hyperfleet-api/main.gopkg/handlers/nodepool_status.gopkg/auth/context.gopkg/handlers/node_pool.gopkg/auth/helpers.gopkg/services/node_pool.gopkg/handlers/cluster.gopkg/services/generic.gopkg/db/db_session/test.gopkg/api/adapter_status_types.gopkg/db/db_session/testcontainer.gopkg/db/db_session/default.gopkg/services/generic_test.gopkg/handlers/helpers.gocmd/hyperfleet-api/environments/framework.gopkg/client/ocm/client.go
**/pkg/handlers/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Files:
pkg/handlers/nodepool_status.gopkg/handlers/node_pool.gopkg/handlers/cluster.gopkg/handlers/helpers.go
**/pkg/services/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Files:
pkg/services/node_pool.gopkg/services/generic.gopkg/services/generic_test.go
**/pkg/{services,dao}/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Files:
pkg/services/node_pool.gopkg/services/generic.gopkg/services/generic_test.go
{Makefile,*.mk,**/Makefile,**/*.mk}
📄 CodeRabbit inference engine (CLAUDE.md)
Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Files:
Makefile
{Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Podman for containerized development and build consistency (database, code generation)
Files:
Makefile
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
All unit tests must run with OCM_ENV=unit_testing environment variable for proper test isolation
Files:
pkg/services/generic_test.go
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use Go 1.24.9 or later for all builds and ensure Go version compatibility
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to openapi/openapi.yaml : Maintain openapi/openapi.yaml as the source of truth (32KB with $ref) and regenerate pkg/api/openapi/api/openapi.yaml (44KB fully resolved) via 'make generate' using Podman
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
Applied to files:
pkg/handlers/nodepool_status.gopkg/services/node_pool.gopkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/api/*embed*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for runtime availability and self-contained binary
Applied to files:
pkg/handlers/nodepool_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/*.go : Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Applied to files:
pkg/handlers/nodepool_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/services/*.go : Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Applied to files:
pkg/handlers/nodepool_status.gopkg/services/node_pool.gopkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
pkg/auth/helpers.gopkg/handlers/helpers.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/handlers/*.go : HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Applied to files:
pkg/auth/helpers.gopkg/handlers/helpers.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to {Makefile,*.mk,**/Makefile,**/*.mk} : Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Applied to files:
Makefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use Go 1.24.9 or later for all builds and ensure Go version compatibility
Applied to files:
Makefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*service*,*handler*}.go : Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Applied to files:
pkg/db/db_session/test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/test/integration/**/*.go : Integration tests must use Testcontainers to create isolated PostgreSQL instances for each test suite to ensure state isolation
Applied to files:
pkg/db/db_session/test.gopkg/db/db_session/testcontainer.gopkg/services/generic_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/*.go : Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Applied to files:
pkg/db/db_session/test.gopkg/db/db_session/testcontainer.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Use polymorphic adapter_statuses table with owner_type and owner_id columns to avoid separate status tables for each resource type
Applied to files:
pkg/api/adapter_status_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Applied to files:
pkg/api/adapter_status_types.gopkg/services/generic_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/cmd/hyperfleet/environments/*.go : Use environment-based configuration via OCM_ENV variable (development, unit_testing, integration_testing, production) for environment-specific behavior
Applied to files:
cmd/hyperfleet-api/environments/framework.go
🧬 Code graph analysis (6)
pkg/handlers/nodepool_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
pkg/handlers/node_pool.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
pkg/handlers/cluster.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
pkg/api/adapter_status_types.go (1)
pkg/api/metadata_types.go (1)
Metadata(28-34)
cmd/hyperfleet-api/environments/framework.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Clients(67-69)
🔇 Additional comments (15)
Makefile (1)
115-117: Lint target refactor aligns with centralized .golangci.yml.Invoking
golangci-lint rundirectly with the new.golangci.ymlconfiguration is a cleaner approach than the previous$(GOLANGCI_LINT_BIN)variable. This allows lint rules and exclusions to be centrally managed.pkg/handlers/nodepool_status.go (1)
11-11: Structured logging on aggregation failure looks goodUsing
logger.NewOCMLogger(ctx)and loggingnodepool_idplus the aggregation error at warning level improves observability while keeping the aggregation failure non-fatal for the request. This aligns with the handler flow and logging guidelines; no further changes needed.Also applies to: 92-95
pkg/db/db_session/test.go (1)
216-221: Fail-fast ResetDB improves test reliability and state tracking
ResetDBnow panics onresetDBfailure and only setswasDisconnectedafter a successful reset. This removes the prior silent failure mode and ensures tests never continue with a dirty or partially reset database, while keeping the reconnection logic viawasDisconnectedintact. Looks good.cmd/hyperfleet-api/main.go (1)
28-30: Flag parsing error handling is correct and consistentWrapping
flag.CommandLine.Parse([]string{})with an error check andglog.Fatalfprovides clearer startup failure behavior and is consistent with the laterrootCmd.Execute()error handling. No further changes needed here.cmd/hyperfleet-api/environments/framework.go (1)
158-162: OCM teardown is now nil-safe and error-awareThe additional guard around
e.Clients.OCMand logging ofClose()errors looks correct and prevents panics when the client isn’t initialized, while preserving teardown error visibility. No further changes needed here.pkg/db/db_session/default.go (1)
111-113: LGTM! Proper error handling added.The error handling for
l.Ping()correctly addresses linting issues by checking and logging the error. The verbose logging level is appropriate for this background health-check operation.pkg/db/db_session/testcontainer.go (1)
47-55: Port-based readiness check looks correct and should reduce flakinessSwitching from log-matching to a port-based readiness check with the same 60s timeout is a cleaner, less brittle way to wait for Postgres to be usable. This aligns well with the integration testcontainer usage in this package.
pkg/handlers/cluster.go (1)
71-77: Spec patch now safely fails on marshal errors.Marshalling
patch.Specand surfacing aGeneralErrorbefore assigningfound.Specis correct and keeps cluster specs from being corrupted by invalid JSON; behavior is consistent with the node pool handler.pkg/auth/helpers.go (1)
29-38: Defensive response writing is correctly added.The added marshal and write error checks make
writeJSONResponsesafer without changing its external behavior, which is appropriate given headers are already committed.pkg/handlers/node_pool.go (1)
72-78: Node pool Spec patch correctly validates JSON before persistence.Marshalling
patch.Specwith error handling and only assigningfound.Specon success provides a safe, consistent patch flow and surfaces failures as aGeneralError.pkg/api/adapter_status_types.go (1)
55-68: AdapterStatus marshalling/unmarshalling is now robust to bad JSON.Defaulting Conditions/Data/Metadata to safe empty values on marshal/unmarshal failure and scoping Data’s error to
dataErrremoves the earlier error-reuse pitfall and ensures AdapterStatus objects are always serializable without panicking.Also applies to: 122-127, 130-140, 145-149
pkg/auth/context.go (1)
40-49: JWT claim extraction is now type-safe.Switching to
ok-checked string assertions for context username and all JWT claims (including fallbacks from alternate claim keys andname) hardens these helpers against malformed tokens without changing their intended semantics.Also applies to: 80-127
pkg/services/generic_test.go (1)
23-24: Test updates correctly track API changes and lint expectations.Deferring
dbFactory.Close()with//nolint:errcheckis appropriate for test cleanup, and adapting totreeWalkForSqlizer’s new(Sqlizer, error)signature preserves the existing SQL translation assertions.Also applies to: 66-71
pkg/services/generic.go (2)
173-176: LGTM! Call site correctly updated to match narrowed signature.The change from capturing three return values (ignoring the first) to capturing two return values aligns with the updated
treeWalkForSqlizersignature. Error handling remains correct.
332-345: LGTM! Signature narrowing improves clarity.The refactoring correctly removes the unused
tsl.Nodereturn value. The function now clearly expresses its purpose: converting a search tree to a SQL-izer, not transforming the tree. Error returns properly usenilfor the primary value, and the success path returns the expected(sqlizer, nil).
yingzhanredhat
force-pushed
the
ying-test
branch
from
08bcd18 to
bff8479
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/client/ocm/client.go (1)
78-83: Close implementation is correct; optionally make it idempotent by nil‑ing the connectionThe new signature and error propagation look good and fix the earlier issue of ignoring
Closeerrors. IfClosemight be called multiple times on the same client, consider settingc.connection = nilafter a successful close so subsequent calls are guaranteed to be no‑ops instead of re‑closing the same connection.func (c *Client) Close() error { - if c.connection != nil { - return c.connection.Close() - } - return nil + if c.connection == nil { + return nil + } + + if err := c.connection.Close(); err != nil { + return err + } + + c.connection = nil + return nil }pkg/handlers/cluster_status.go (1)
92-95: Good observable handling of aggregation failures; consider minor logging refinementsLogging
UpdateClusterStatusFromAdaptersfailures instead of silently ignoring them is a solid improvement, and keeping the request successful makes sense since the adapter status upsert already succeeded. As an optional tweak, you might consider:
- Logging at an error level if aggregation failures are expected to be rare and actionable.
- Including any available adapter identifier(s) in the log context to aid debugging.
Overall, the pattern
logger.NewOCMLogger(ctx).Extra(...).Warning(...)aligns with the structured logging guideline for handlers. As per coding guidelines, this is the right logger to use with the request context.pkg/handlers/helpers.go (1)
17-26: Marshal/write error handling is correct; consider pre‑marshalling or logging as a future refinementThe added checks for
json.Marshalandw.Writefix the previous silent failure/ignored‑error issue and are fine to ship. If you touch this again, you might consider:
- Pre‑marshalling before
WriteHeaderso a marshal failure can still translate into a 5xx, and/or- Adding a way to log marshal/write failures (e.g., by passing a
context.Contextor logger into this helper) so these bugs don’t get swallowed.These are optional and not required for this PR. As per coding guidelines, we prefer structured logging for unexpected failures.
pkg/db/transaction_middleware.go (1)
44-52: Behavior is fine; align comment with implementation or add loggingThe new marshal/write error checks look good and resolve ignored‑error issues. The inline comment “Log error but don't expose to client…” is now misleading because no logging happens here and there’s no context/logger in scope.
Either:
- Wire a logger/context into this helper and actually log marshal/write failures using structured logging, or
- Simplify the comment to just state that we return since headers are already sent and there’s nothing else we can do.
This is a minor readability issue, not a blocker. As per coding guidelines, structured logging is preferred for unexpected errors.
pkg/auth/helpers.go (1)
29-38: Consistent error handling for JSON responses; optional room for richer failure reportingThe marshal/write checks here are correct and consistent with the other helpers, and they cleanly address the previous ignored‑error behavior. Since
handleErroralready logs the service error, this is acceptable as‑is.If you revisit this, you might optionally:
- Pre‑marshal before
WriteHeaderto allow turning marshal failures into 5xx, and/or- Add a way to log marshal/write failures separately from the business error if that becomes important.
No changes are required for this PR. Based on learnings, the use of the structured error type and JSON encoding of
AsOpenapiErrorremains aligned with the API error conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(1 hunks)pkg/services/types.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- cmd/hyperfleet-api/server/logging/writer.go
- Dockerfile.openapi
- pkg/handlers/nodepool_status.go
- cmd/hyperfleet-api/server/logging/request_logging_middleware.go
- pkg/handlers/openapi.go
- pkg/db/sql_helpers.go
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/handlers/framework.go
- pkg/services/node_pool.go
- pkg/db/db_session/test.go
- pkg/db/db_session/testcontainer.go
- openapitools.json
- pkg/handlers/node_pool.go
- cmd/hyperfleet-api/server/routes.go
- pkg/auth/context.go
- pkg/services/types.go
- pkg/handlers/cluster.go
- cmd/hyperfleet-api/environments/framework.go
- pkg/services/generic.go
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Files:
pkg/api/node_pool_types.gopkg/db/transaction_middleware.gopkg/handlers/helpers.gopkg/api/cluster_types.gopkg/handlers/cluster_status.gopkg/db/db_session/default.gopkg/api/presenters/slice_filter.gocmd/hyperfleet-api/main.gopkg/api/adapter_status_types.gopkg/services/generic_test.gopkg/auth/helpers.gopkg/services/status_aggregation.gocmd/hyperfleet-api/server/api_server.gopkg/client/ocm/client.gopkg/services/cluster.gopkg/config/config_test.go
**/pkg/db/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/pkg/db/*.go: All database connections must respect db-sslmode parameter (disable | require | verify-ca | verify-full) with production defaulting to verify-full
Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Files:
pkg/db/transaction_middleware.go
**/pkg/db/transaction_middleware.go
📄 CodeRabbit inference engine (CLAUDE.md)
All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Files:
pkg/db/transaction_middleware.go
**/pkg/handlers/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Files:
pkg/handlers/helpers.gopkg/handlers/cluster_status.go
**/pkg/services/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Files:
pkg/services/generic_test.gopkg/services/status_aggregation.gopkg/services/cluster.go
**/pkg/{services,dao}/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Files:
pkg/services/generic_test.gopkg/services/status_aggregation.gopkg/services/cluster.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
All unit tests must run with OCM_ENV=unit_testing environment variable for proper test isolation
Files:
pkg/services/generic_test.gopkg/config/config_test.go
{Makefile,*.mk,**/Makefile,**/*.mk}
📄 CodeRabbit inference engine (CLAUDE.md)
Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Files:
Makefile
{Makefile,*.mk,**/Makefile,**/*.mk,docker-compose*,**/Dockerfile}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Podman for containerized development and build consistency (database, code generation)
Files:
Makefile
🧠 Learnings (21)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : NodePool resources must enforce owner_references.id (parent cluster ID) via foreign key constraint in database schema
Applied to files:
pkg/api/node_pool_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Store resource specifications in JSONB columns (spec field) to enable flexible cloud provider configuration
Applied to files:
pkg/api/node_pool_types.gopkg/api/cluster_types.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*model*,*dao*,*service*}.go : Use Kubernetes-style conditions in adapter status to support multiple independent adapters without coordination
Applied to files:
pkg/api/node_pool_types.gopkg/api/cluster_types.gopkg/handlers/cluster_status.gopkg/api/adapter_status_types.gopkg/services/status_aggregation.gopkg/services/cluster.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to openapi/openapi.yaml : Maintain openapi/openapi.yaml as the source of truth (32KB with $ref) and regenerate pkg/api/openapi/api/openapi.yaml (44KB fully resolved) via 'make generate' using Podman
Applied to files:
pkg/api/node_pool_types.gopkg/api/cluster_types.goMakefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to pkg/api/openapi/api/openapi.yaml : Only edit openapi/openapi.yaml as the source; the generated pkg/api/openapi/api/openapi.yaml will be overwritten by 'make generate'
Applied to files:
pkg/api/node_pool_types.gopkg/api/cluster_types.goMakefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/transaction_middleware.go : All HTTP requests automatically receive database session via TransactionMiddleware with auto-commit on success and rollback on error
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
pkg/db/transaction_middleware.gopkg/handlers/helpers.gopkg/auth/helpers.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*db*}.go : All database operations must accept context.Context parameter to enable proper transaction propagation from middleware
Applied to files:
pkg/db/transaction_middleware.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/handlers/*.go : HTTP handlers must follow the pattern: parse request → call service/DAO → handle errors with errors.SendError() → send response with appropriate status code
Applied to files:
pkg/handlers/helpers.gopkg/auth/helpers.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use TypeSpec to define API specifications, which compile to OpenAPI 3.0.3, then generate Go models using openapi-generator-cli v7.16.0
Applied to files:
pkg/api/cluster_types.goMakefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/api/*embed*.go : Embed OpenAPI specification at compile time using Go 1.16+ //go:embed directive for runtime availability and self-contained binary
Applied to files:
pkg/api/cluster_types.gopkg/handlers/cluster_status.goMakefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/services/*.go : Calculate aggregate status.phase from adapter conditions (Ready if all adapters=True, Failed if any=False, NotReady otherwise) without manual phase setting
Applied to files:
pkg/handlers/cluster_status.gopkg/api/adapter_status_types.gopkg/services/status_aggregation.gopkg/services/cluster.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/migrations/*.go : Use polymorphic adapter_statuses table with owner_type and owner_id columns to avoid separate status tables for each resource type
Applied to files:
pkg/handlers/cluster_status.gopkg/api/adapter_status_types.gopkg/services/status_aggregation.gopkg/services/cluster.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/*.go : Use structured logging via logger.NewOCMLogger(ctx) with context-aware fields including opid, accountID, and tx_id
Applied to files:
pkg/handlers/cluster_status.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/{services,dao}/*.go : Increment generation field on each resource spec update to enable optimistic concurrency and adapter reconciliation tracking
Applied to files:
pkg/api/adapter_status_types.gopkg/services/generic_test.gopkg/services/cluster.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/test/integration/**/*.go : Integration tests must use Testcontainers to create isolated PostgreSQL instances for each test suite to ensure state isolation
Applied to files:
pkg/services/generic_test.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to {Makefile,*.mk,**/Makefile,**/*.mk} : Build Go binary using FIPS-compliant crypto with CGO_ENABLED=1 and GOEXPERIMENT=boringcrypto for enterprise/government deployments
Applied to files:
Makefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Use Go 1.24.9 or later for all builds and ensure Go version compatibility
Applied to files:
Makefile
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/{*dao*,*service*,*handler*}.go : Retrieve database sessions from context via db.NewContext(ctx) rather than creating new GORM connections
Applied to files:
cmd/hyperfleet-api/server/api_server.go
📚 Learning: 2025-11-28T06:28:37.091Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T06:28:37.091Z
Learning: Applies to **/pkg/db/*.go : Configure database connection pooling via SetMaxOpenConns() and SetMaxIdleConns() for performance optimization
Applied to files:
cmd/hyperfleet-api/server/api_server.go
🧬 Code graph analysis (3)
pkg/handlers/cluster_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
pkg/api/adapter_status_types.go (1)
pkg/api/metadata_types.go (1)
Metadata(28-34)
cmd/hyperfleet-api/server/api_server.go (2)
cmd/hyperfleet-api/environments/types.go (1)
Database(36-38)pkg/db/session.go (1)
SessionFactory(12-20)
🔇 Additional comments (25)
pkg/db/db_session/default.go (1)
111-113: LGTM! Good error handling improvement.Adding error handling for the
Ping()call addresses the silent failure and aligns with best practices. The verbose logging level V(10) is appropriate for periodic health checks.Makefile (4)
128-128: FIPS-compliant build configuration in install target looks good.The install target correctly uses
CGO_ENABLED=$(CGO_ENABLED)andGOEXPERIMENT=boringcryptoto enable FIPS-compliant cryptography, aligning with the enterprise/government deployment requirements.
13-13: Container tooling properly configured for Podman-first development.The Makefile correctly defaults to Podman (
container_tool ?= podman) and uses this consistently across containerized workflows (generate, generate-vendor, and database targets), supporting reproducible builds and development environments as per coding guidelines.Also applies to: 210-227
210-217: Verify APT sandbox workaround implementation for Prow/podman compatibility.The PR objectives state: "Updates Makefile to add -o APT::Sandbox::User=root to accommodate Prow using podman when running 'make generate'". The provided Makefile snippet (lines 210-217) does not show this modification. The workaround may be in
Dockerfile.openapi(referenced in the generate target) or requires manual verification to confirm it has been applied elsewhere in the codebase or if it still needs to be added.
115-116: Verify the narrowed lint scope doesn't create coverage gaps.The lint target has been narrowed to only
./pkg/api/openapi.... If this is intentional, clarify the rationale for excluding./cmd/...and most of./pkg/...from lint coverage, especially given the PR objectives state "make lint: ran golangci-lint over ./cmd/... and ./pkg/... with 0 issues reported". If unintentional, restore the broader scope:- golangci-lint run -D unused \ - ./pkg/api/openapi... + golangci-lint run -D unused \ + ./cmd/... \ + ./pkg/...pkg/services/status_aggregation.go (1)
155-163: LGTM! Improved observability for unexpected adapter statuses.The switch statement is clearer than the previous if-else chain and explicitly handles unexpected statuses in the default case. Logging unexpected values like "Unknown" improves debugging without changing the behavior for "True" and "False" cases.
cmd/hyperfleet-api/server/api_server.go (1)
149-151: LGTM: Proper error handling added for database cleanup.The error from
SessionFactory.Close()is now appropriately checked and logged during shutdown. Logging without fatal termination is suitable for cleanup code, ensuring visibility of connection closure issues without disrupting graceful shutdown.cmd/hyperfleet-api/main.go (1)
28-30: LGTM: Defensive error handling added for flag parsing.The error from
flag.CommandLine.Parse()is now explicitly checked. While unlikely to fail with an empty slice, this defensive check ensures initialization errors are surfaced rather than silently discarded. Usingglog.Fatalfis appropriate for startup failures.pkg/handlers/cluster_status.go (1)
11-11: Logger import usage is correct and aligns with guidelinesThe added
loggerimport is used below for structured, context-aware logging and is consistent with the handler logging conventions.pkg/api/presenters/slice_filter.go (3)
114-116: LGTM: Essential error propagation for nested struct validation.This correctly propagates validation errors from nested structs. Without this, validation failures in nested fields would be silently ignored, leading to incorrect validation results.
149-153: LGTM: Defensive error handling for regex compilation.Good defensive programming. While pattern compilation is unlikely to fail (since
namecomes from JSON struct field tags), handling the error prevents potential panics. The fallback of returning the unchanged map is reasonable given the context.
194-196: LGTM: Critical nil pointer safety check.This properly guards against nil pointer dereference when formatting time.Time values. The type assertion with the nil check prevents panics that would occur if
Format()were called on a nil pointer.pkg/config/config_test.go (1)
15-15: LGTM: nolint directives appropriate for test cleanup.The
//nolint:errcheckdirectives on deferredos.Remove()calls are appropriate. In test code, ignoring cleanup errors for temporary files is standard practice since failures don't affect test correctness.Also applies to: 30-30, 45-45, 60-60
pkg/services/generic_test.go (3)
23-23: LGTM: nolint directive appropriate for test cleanup.The
//nolint:errcheckdirective on the deferreddbFactory.Close()call is appropriate for test cleanup code where cleanup failures don't affect test correctness.
70-70: LGTM: Signature update correctly reflects refactored method.The test correctly adapts to the simplified
treeWalkForSqlizersignature. The removed first return value (tsl.Node) was unused, and the test still properly validates SQL generation viasqlizer.ToSql().
20-77: Ensure OCM_ENV=unit_testing is set when running this test.All unit tests must run with
OCM_ENV=unit_testingfor proper test isolation per coding guidelines. Check that this environment variable is configured in your test execution setup, Makefile test targets, or CI/CD pipeline.pkg/api/adapter_status_types.go (3)
55-67: LGTM: Robust error handling for JSON unmarshalling.The explicit error checks with sensible fallbacks (empty slice for conditions, empty map for data) ensure the system remains resilient to corrupted JSONB data in the database.
130-140: LGTM: Scope bug fixed correctly.The refactoring properly addresses the previous scope issue by introducing
dataErrwithin the conditional block. The error handling now correctly applies only whenjson.Marshal(req.Data)is actually invoked.
123-127: LGTM: Consistent error handling for JSON marshalling.The explicit error checks with fallbacks ("[]" for conditions, nil for metadata) prevent potential panics from programming errors and ensure deterministic behavior.
Also applies to: 145-149
pkg/api/cluster_types.go (2)
72-90: LGTM: Robust unmarshalling with appropriate fallbacks.The error handling for Spec, Labels, and StatusConditions follows a consistent defensive pattern. The comment at line 85 helpfully clarifies that StatusConditions are stored as
ResourceConditionin the database.
138-157: LGTM: Consistent marshalling error handling.The explicit error checks with fallbacks ("{}" for Spec and Labels, "[]" for StatusConditions) ensure deterministic behavior and prevent potential panics.
pkg/api/node_pool_types.go (2)
82-100: LGTM: Consistent error handling across resource types.The unmarshalling error handling mirrors the pattern in
cluster_types.go, ensuring uniform resilience across the codebase. The clarifying comment at line 95 is helpful.
159-178: LGTM: Defensive marshalling with sensible fallbacks.The error handling follows the established pattern, using appropriate fallbacks for each data type.
pkg/services/cluster.go (2)
132-132: LGTM: Comment accurately reflects the code.The updated comment correctly describes that
ResourceConditionobjects are built from adapter Available conditions, addressing the previous review feedback.
139-141: LGTM: Better variable naming prevents shadowing.Using
unmarshalErrinstead oferrmakes the error handling more explicit and avoids potential confusion with theerrvariable used elsewhere in the function (e.g., lines 122, 208, 228).
|
@yingzhanredhat can you create a task for this in jira and add it to the sprint so we get a better estimate of team velocity please 🙏 |
yingzhanredhat
force-pushed
the
ying-test
branch
from
bff8479 to
84d8b35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/integration/node_pools_test.go (1)
75-81: Check the HTTP request error to prevent test panics.Discarding the error from
resty.R().Post()is unsafe. If the HTTP request fails (network error, connection refused, etc.),restyRespcould be nil, causing a panic when callingrestyResp.StatusCode()on line 81.Apply this diff to properly handle the error:
- restyResp, _ := resty.R(). + restyResp, err := resty.R(). SetHeader("Content-Type", "application/json"). SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). SetBody(`{ this is invalid }`). Post(h.RestURL(fmt.Sprintf("/clusters/%s/nodepools", cluster.ID))) - + Expect(err).NotTo(HaveOccurred(), "HTTP request should complete successfully") Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest))test/integration/clusters_test.go (2)
68-74: Check the HTTP request error to prevent test panics.Discarding the error from
resty.R().Post()is unsafe. If the HTTP request fails (network error, connection refused, etc.),restyRespcould be nil, causing a panic when callingrestyResp.StatusCode()on line 74.Apply this diff to properly handle the error:
- restyResp, _ := resty.R(). + restyResp, err := resty.R(). SetHeader("Content-Type", "application/json"). SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). SetBody(`{ this is invalid }`). Post(h.RestURL("/clusters")) - + Expect(err).NotTo(HaveOccurred(), "HTTP request should complete successfully") Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest))
238-242: Check the error to ensure the test validates the correct behavior.While the nil check for
respprevents a panic, discarding the error means the test cannot distinguish between a request failure and a successful response. This could hide real issues and make the test less informative.Apply this diff to properly handle the error:
- _, resp, _ = client.DefaultAPI.PostCluster(ctx).ClusterCreateRequest(emptyNameInput).Execute() - // Should either accept empty name or return 400 - if resp != nil { - t.Logf("Empty name test returned status: %d", resp.StatusCode) - } + _, resp, err = client.DefaultAPI.PostCluster(ctx).ClusterCreateRequest(emptyNameInput).Execute() + // Should either accept empty name or return 400 + if err != nil { + Expect(resp).NotTo(BeNil(), "Response should be available even on error") + Expect(resp.StatusCode).To(BeElementOf([]int{http.StatusBadRequest, http.StatusInternalServerError}), + "Empty name should return 400 or 500") + } else { + t.Logf("Empty name was accepted, returned status: %d", resp.StatusCode) + Expect(resp.StatusCode).To(Equal(http.StatusCreated)) + }
♻️ Duplicate comments (1)
cmd/hyperfleet-api/server/routes.go (1)
53-59: Authorization middleware remains commented out, contradicting stated preference.The authorization middleware initialization is still commented out and using only the mock. This contradicts yasun1's explicit comment: "I'd like to leave the entry here but not comment it out."
Security impact: Real authorization checks are bypassed, degrading the security posture.
Note: The compilation bug in the commented code has been correctly fixed by adding
var err erroron line 56.Recommendation: Uncomment the authorization middleware initialization block as requested, or clarify the current plan if authorization needs to remain disabled for this PR.
🧹 Nitpick comments (5)
pkg/handlers/validation.go (1)
41-55: Prefer deleting unused code (or add clearer rationale) instead of commenting it outThe
validateInclusionInhelper is now fully commented out to satisfy unused-lint, but leaving large commented blocks tends to accumulate noise over time. If this validator is not expected to come back soon, consider removing it entirely; if it is expected to return, a brief comment referencing the reason or a tracking issue (e.g.// TODO(HF-123): reintroduce when X is implemented) would be more informative than “Comment it as it is not used currently.” This also keeps the file cleaner for future readers.cmd/hyperfleet-api/server/logging/writer.go (1)
47-47: Consider consistency with lines 23 and 28.The change to
writer.Header()is functionally correct and more idiomatic. However, lines 23 and 28 explicitly usewriter.ResponseWriter.Write()andwriter.ResponseWriter.WriteHeader(). For consistency, consider either using the shorter form everywhere or the explicit form everywhere.Apply this diff to make all three consistent (shorter form):
func (writer *loggingWriter) Write(body []byte) (int, error) { writer.responseBody = body - return writer.ResponseWriter.Write(body) + return writer.Write(body) } func (writer *loggingWriter) WriteHeader(status int) { writer.responseStatus = status - writer.ResponseWriter.WriteHeader(status) + writer.WriteHeader(status) }Note: The above diff would cause infinite recursion. To use the shorter form, these methods would need to be removed entirely (letting the embedded type handle them) or the logic would need restructuring. The explicit form is safer here.
Better approach - keep the explicit form everywhere:
func (writer *loggingWriter) prepareResponseLog(elapsed string) (string, error) { info := &ResponseInfo{ - Header: writer.Header(), + Header: writer.ResponseWriter.Header(), Body: writer.responseBody, Status: writer.responseStatus, Elapsed: elapsed, }pkg/db/sql_helpers.go (1)
83-88: Tighten prefix check and remove magic index inpropertiesNodeConverterThe new string/assertion and length guard are good defensive additions. You can make this a bit more robust and self-documenting by:
- Reusing
startsWithPropertiesto ensure the expected prefix, not just length.- Deriving the slice offset from the prefix length instead of hardcoding
11.- Fixing the
propetyNametypo.For example:
- leftStr, ok := l.Left.(string) - if !ok || len(leftStr) < 11 { - return n - } - propetyName := leftStr[11:] + leftStr, ok := l.Left.(string) + if !ok || !startsWithProperties(leftStr) { + return n + } + propertyName := leftStr[len("properties."):] @@ - Func: tsl.IdentOp, - Left: fmt.Sprintf("properties ->> '%s'", propetyName), + Func: tsl.IdentOp, + Left: fmt.Sprintf("properties ->> '%s'", propertyName),This keeps the function safe if it’s ever called outside the current
hasPropertygate and avoids the magic number.pkg/auth/context.go (1)
118-127: Consider preserving already-populated fields and handling multi-word names.Two edge cases in the name parsing logic:
Overwrites existing firstName: If
firstNamewas already set fromgiven_namebutlastNameis empty, this code will overwritefirstNamewith data from thenameclaim.Multi-word names: For names like "Mary Jane Watson", only the first two words are used, resulting in
lastName = "Jane"instead of"Watson"or"Jane Watson".If you want to address these:
if payload.FirstName == "" || payload.LastName == "" { if name, ok := claims["name"].(string); ok { names := strings.Split(name, " ") if len(names) > 1 { - payload.FirstName = names[0] - payload.LastName = names[1] + if payload.FirstName == "" { + payload.FirstName = names[0] + } + if payload.LastName == "" { + payload.LastName = strings.Join(names[1:], " ") + } } else { - payload.FirstName = names[0] + if payload.FirstName == "" { + payload.FirstName = names[0] + } } } }pkg/api/presenters/slice_filter.go (1)
149-153: Good defensive programming: regexp compilation error is now handled gracefully.The change from
MustCompile(which panics) toCompilewith error handling prevents potential runtime panics if the pattern contains unexpected characters. Returning the unchanged map is a safe fallback that avoids removing the wrong fields.Optionally, consider logging the error when pattern compilation fails to aid debugging, since this scenario indicates either a programming error in pattern construction or an unusual field name:
pat, err := regexp.Compile(pattern) if err != nil { - // Invalid pattern, return unchanged + // Invalid pattern, return unchanged + // Consider: log.Warnf("Failed to compile pattern %q: %v", pattern, err) return in }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/framework_test.go(0 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/handlers/validation.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(1 hunks)pkg/services/types.go(1 hunks)test/helper.go(4 hunks)test/integration/clusters_test.go(2 hunks)test/integration/node_pools_test.go(1 hunks)test/mocks/jwk_cert_server.go(1 hunks)test/mocks/ocm.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/handlers/framework_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/services/node_pool.go
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (16)
- cmd/hyperfleet-api/environments/framework.go
- cmd/hyperfleet-api/server/api_server.go
- pkg/api/cluster_types.go
- pkg/handlers/cluster_status.go
- cmd/hyperfleet-api/server/logging/request_logging_middleware.go
- openapitools.json
- pkg/db/db_session/testcontainer.go
- pkg/api/node_pool_types.go
- pkg/db/db_session/default.go
- pkg/handlers/openapi.go
- Dockerfile.openapi
- pkg/services/types.go
- pkg/services/cluster.go
- pkg/config/config_test.go
- Makefile
- pkg/handlers/cluster.go
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
test/integration/clusters_test.gotest/mocks/jwk_cert_server.gopkg/handlers/node_pool.gocmd/hyperfleet-api/server/logging/writer.gopkg/handlers/framework.gopkg/handlers/validation.gocmd/hyperfleet-api/main.gopkg/db/db_session/test.gotest/integration/node_pools_test.gotest/mocks/ocm.gopkg/db/sql_helpers.gocmd/hyperfleet-api/environments/framework_test.gopkg/api/adapter_status_types.gopkg/handlers/nodepool_status.gopkg/handlers/helpers.gopkg/auth/context.gopkg/services/generic.gopkg/services/generic_test.gocmd/hyperfleet-api/server/routes.gotest/helper.gopkg/api/presenters/slice_filter.gopkg/db/transaction_middleware.gopkg/services/status_aggregation.gopkg/client/ocm/client.gopkg/auth/helpers.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/clusters_test.gotest/integration/node_pools_test.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/handlers/node_pool.gopkg/handlers/framework.gopkg/handlers/validation.gopkg/handlers/nodepool_status.gopkg/handlers/helpers.gopkg/services/generic.gopkg/services/generic_test.gopkg/services/status_aggregation.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Files:
pkg/handlers/node_pool.gopkg/handlers/framework.gopkg/handlers/validation.gopkg/handlers/nodepool_status.gopkg/handlers/helpers.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/handlers/node_pool.gopkg/handlers/framework.gopkg/handlers/validation.gopkg/handlers/nodepool_status.gopkg/handlers/helpers.gopkg/services/generic.gopkg/services/generic_test.gopkg/services/status_aggregation.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/handlers/node_pool.gopkg/handlers/framework.gopkg/handlers/validation.gopkg/db/db_session/test.gopkg/db/sql_helpers.gopkg/api/adapter_status_types.gopkg/handlers/nodepool_status.gopkg/handlers/helpers.gopkg/auth/context.gopkg/services/generic.gopkg/services/generic_test.gopkg/api/presenters/slice_filter.gopkg/db/transaction_middleware.gopkg/services/status_aggregation.gopkg/client/ocm/client.gopkg/auth/helpers.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/db_session/test.gopkg/db/sql_helpers.gopkg/db/transaction_middleware.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/adapter_status_types.gopkg/api/presenters/slice_filter.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/generic.gopkg/services/generic_test.gopkg/services/status_aggregation.go
🧠 Learnings (13)
📚 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/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:
pkg/handlers/node_pool.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/handlers/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/framework.gopkg/handlers/nodepool_status.gopkg/handlers/helpers.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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/handlers/framework.gopkg/handlers/nodepool_status.gopkg/handlers/helpers.gopkg/db/transaction_middleware.gopkg/auth/helpers.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/{handlers,services}/**/*.go : Always retrieve database session from context via db.NewContext() instead of creating new connections
Applied to files:
pkg/db/db_session/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:
cmd/hyperfleet-api/environments/framework_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 : Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Applied to files:
pkg/api/adapter_status_types.gopkg/services/status_aggregation.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/services/**/*.go : Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Applied to files:
pkg/api/adapter_status_types.gopkg/handlers/nodepool_status.gopkg/services/status_aggregation.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:
pkg/api/adapter_status_types.gopkg/handlers/nodepool_status.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/{handlers,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
pkg/handlers/nodepool_status.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:
pkg/auth/context.gocmd/hyperfleet-api/server/routes.gopkg/db/transaction_middleware.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/dao/**/*.go : Use context.Context as the first parameter in all DAO methods for transaction propagation
Applied to files:
pkg/auth/context.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 : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/services/generic_test.go
🧬 Code graph analysis (5)
pkg/handlers/node_pool.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
cmd/hyperfleet-api/environments/framework_test.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Services(44-47)
pkg/api/adapter_status_types.go (1)
pkg/api/metadata_types.go (1)
Metadata(28-34)
pkg/handlers/nodepool_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
🔇 Additional comments (31)
test/mocks/ocm.go (1)
62-65: LGTM! Critical correctness fix.Changing from a value receiver to a pointer receiver is correct and necessary. With a value receiver, the method would operate on a copy of the struct, discarding the field resets when the method returned. The pointer receiver ensures the changes persist, making the mock reusable as intended in the usage example at line 30.
pkg/client/ocm/client.go (1)
78-83: Close now correctly surfaces connection close errorsThe new
Close() errorsignature and implementation look good: it cleanly handles thenilconnection case and propagatesc.connection.Close()errors to callers. Just ensure all existing call sites have been updated to check/log the returned error appropriately (the compile-time signature change should help catch any missed sites).pkg/db/db_session/test.go (1)
216-221: ResetDB now fails fast and keeps state consistent — looks good.
ResetDBno longer swallowsresetDBerrors and only setswasDisconnectedwhen the reset succeeds, which fixes the earlier reliability concern and is appropriate behavior for a test helper (panic on unrecoverable setup failure).pkg/db/sql_helpers.go (2)
20-34: Safer left-hand identifier handling inhasPropertylooks goodThe added string assertion and guard on
l.Leftbefore callingstartsWithPropertiesavoids potential panics from invalid node structures while preserving existing behavior.
143-151: Good defensive type check before recursing onn.LeftAdding the
tsl.Nodetype assertion onn.Leftand returning aBadRequestfor invalid structures makesFieldNameWalkfail fast on malformed trees instead of panicking, which aligns well with the rest of the validation logic.pkg/services/generic_test.go (2)
23-23: LGTM: Suppressing errcheck on deferred Close() is acceptable in test code.The
//nolint:errcheckdirective is appropriate here since errors fromClose()in test teardown are typically non-actionable and the mock factory likely returns nil anyway.
70-75: Test correctly updated for newtreeWalkForSqlizersignature.The test now captures only
(sqlizer, serviceErr)which aligns with the simplified return type ingeneric.go. The subsequent assertions onserviceErrandsqlizer.ToSql()remain valid.pkg/services/generic.go (3)
43-43: LGTM: Typo fix.
173-176: LGTM: Call site correctly updated for the simplified return type.The unused
tsl.Nodereturn value was previously discarded with_. This cleanup properly captures only the two values that are actually used.
332-345: Good API simplification: Removed unusedtsl.Nodereturn value.Removing the unused first return value follows Go best practices—returning only values that callers need. The error handling is consistent:
nil, erron failure andsqlizer, nilon success.pkg/services/status_aggregation.go (1)
155-163: LGTM! Clean refactor to switch statement.The switch statement improves readability and explicitly handles unexpected adapter statuses via the default case with appropriate logging. The logic correctly implements the adapter status aggregation guideline: Ready when all required adapters report "True", Failed when any report "False", and NotReady otherwise.
Based on coding guidelines, adapter status aggregation is correctly implemented.
cmd/hyperfleet-api/main.go (1)
28-30: LGTM: Explicit error handling for flag parsing.The change from silent error suppression to explicit error checking and fatal logging aligns well with the PR's broader error-handling improvements. While parsing an empty slice should virtually never fail in practice, this defensive approach ensures any unexpected failure is caught early.
test/helper.go (4)
83-84: LGTM: Acceptable error suppression for test log level configuration.Intentionally ignoring the error from setting the log level flag is appropriate for test code, where custom log levels are optional and failure should not break test setup.
181-183: LGTM: Reasonable error handling for server restart.Logging a warning instead of propagating the error on
stopAPIServerfailure is appropriate for the restart flow, ensuring the test helper remains resilient and continues to start the new server instance.
204-206: LGTM: Proper error handling for flag setup in Reset.Treating
AddFlagserrors as fatal duringResetis appropriate, as flag setup is critical for proper test environment configuration.
441-444: LGTM: Well-documented suppression of deprecation warning.The nolint directive for
ParseRSAPrivateKeyFromPEMWithPasswordis properly justified—the comment explains that no suitable alternative exists for the test fixture keys, making continued use of the deprecated function acceptable in this context.pkg/auth/context.go (3)
45-48: LGTM! Safe type assertion prevents potential panic.Good defensive improvement. The guarded type assertion properly handles cases where the context value is not a string.
82-96: LGTM! Safe claim extraction with proper nil handling.All type assertions are now guarded, preventing panics when claims are missing or have unexpected types (e.g., numbers, nested objects). This aligns with JWT middleware best practices. Based on coding guidelines, this properly extracts username from claims.
99-115: LGTM! Fallback claims consistently guarded.Good consistency with the primary claim extraction pattern. The RHD fallback claims (
preferred_username,given_name,family_name) are now safely extracted.test/mocks/jwk_cert_server.go (1)
33-35: LGTM: Defensive error handling for test mock.The error check for
fmt.Fprintfensures that write failures in the mock JWKS server are detected and reported during tests.pkg/auth/helpers.go (1)
30-38: LGTM: Explicit error handling for JSON response writing.The function now validates both
json.Marshalandw.Writeresults, returning early on errors. This prevents silent failures when serializing or writing responses. The approach correctly acknowledges that after headers are sent, changing the status code is not possible.pkg/db/transaction_middleware.go (1)
44-52: LGTM: Consistent error handling pattern.The error checks for
json.Marshalandw.Writematch the pattern used inpkg/auth/helpers.go, ensuring consistent response-writing behavior across the codebase.pkg/handlers/framework.go (1)
109-110: LGTM: Idiomatic switch refactor.Switching directly on
serviceErrinstead of boolean conditions is more idiomatic Go. The behavior remains unchanged.pkg/handlers/nodepool_status.go (1)
92-96: LGTM: Improved observability for aggregation failures.The inline error check with structured logging provides better visibility when status aggregation fails. Using
Warninglevel is appropriate since the request succeeds but the aggregation is deferred. The logging pattern with context propagation aligns with the coding guidelines.As per coding guidelines, pkg/{handlers,services,dao}/**/*.go should use structured logging via pkg/logger/logger.go with context propagation.
pkg/api/adapter_status_types.go (2)
55-58: LGTM: Defensive unmarshalling with safe fallbacks.The error checks for
json.Unmarshalwith empty slice/map fallbacks prevent panics when database JSONB data is malformed. This pattern ensuresToOpenAPIalways returns valid structures.Also applies to: 64-67
122-144: LGTM: Resolved previous scope issue with guarded marshalling.The fallback pattern (default value + conditional override on successful marshal) correctly addresses the previous review concern about error variable scope. Each field now has:
- A safe default fallback (
[]for conditions,{}for data/metadata)- Marshal error is checked within the conditional scope
- Only successful marshals override the default
This approach prevents fallback logic from using stale error values and ensures deterministic JSON output.
pkg/handlers/node_pool.go (1)
73-77: LGTM: Explicit marshalling error handling in Patch.The error check for
json.Marshalensures that spec marshalling failures are caught and returned as aGeneralErrorrather than silently ignored. This aligns with the coding guideline to return errors using the structured ServiceError type.As per coding guidelines, pkg/{handlers,services,dao}/**/*.go should return errors using the structured ServiceError type from pkg/errors/.
pkg/handlers/helpers.go (1)
17-25: LGTM: Consistent error handling pattern across response writers.The explicit error checks for
json.Marshalandw.Writematch the pattern implemented inpkg/auth/helpers.go,pkg/db/transaction_middleware.go, and other response-writing paths, ensuring uniform behavior throughout the codebase.pkg/api/presenters/slice_filter.go (2)
114-116: Good fix: nested validation errors are now properly propagated.Previously, validation errors from nested struct fields were silently discarded. This change ensures that validation failures in nested structures are correctly returned to the caller, preventing invalid nested fields from passing validation.
194-196: Good nil-safety check prevents potential panic.The added nil check ensures that
Format()is only called on non-nil*time.Timepointers, preventing a runtime panic. The compound check correctly verifies both the type assertion and nil condition.cmd/hyperfleet-api/environments/framework_test.go (1)
39-55: Reflection now uses an addressable value; behavior remains correctSwitching to
reflect.ValueOf(&env.Services).Elem()means you’re reflecting on the actualServicesfield rather than a copy, yielding an addressable structValue. For the current read-only iteration andIsNilchecks on function-typed exported fields, this is functionally equivalent to the old code and safe, while also being a better base if you ever need to set fields via reflection in the future.
yingzhanredhat
force-pushed
the
ying-test
branch
from
84d8b35 to
3129fc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Dockerfile.openapi (1)
23-23: Enable FIPS-compliant crypto configuration.Line 23 sets
CGO_ENABLED=0, but project learnings requireCGO_ENABLED=1withGOEXPERIMENT=boringcryptofor FIPS compliance. Update both the CGO_ENABLED flag and add the GOEXPERIMENT environment variable.Apply this diff:
-ENV CGO_ENABLED=0 +ENV CGO_ENABLED=1 +ENV GOEXPERIMENT=boringcryptotest/integration/node_pools_test.go (1)
73-82: Reorder assertions to avoid potential nil dereference onrestyRespRight now
restyResp.StatusCode()is called (Line 81) before assertingerris nil (Line 82). If the POST ever fails at the transport layer,errwill be non‑nil andrestyRespwill be nil, causing a panic when callingStatusCode().Safer to assert on
errfirst, then inspect the response:- Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) - Expect(err).NotTo(HaveOccurred(), "Error posting object: %v", err) + Expect(err).NotTo(HaveOccurred(), "Error posting object: %v", err) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest))test/integration/clusters_test.go (1)
231-244: Align empty-name boundary test assertion with the documented expected behaviorThe comment (Lines 239–242) says: “Should either accept empty name or return 400”, implying both behaviors are acceptable. The new assertion
Expect(err).ToNot(HaveOccurred())(Line 243) only allows the “accept empty name” path—if the API ever legitimately returns400here (as the comment allows),errwill be non‑nil and this test will fail.Consider either:
- Making the behavior strict by deciding on one expected outcome (e.g., must succeed) and updating the comment and assertions accordingly (e.g., assert 201/200), or
- Allowing both behaviors explicitly, e.g. by asserting on
resp.StatusCodeto permit either 2xx success or a controlled 400, and adjusting theerrexpectation based on the status.
♻️ Duplicate comments (2)
Dockerfile.openapi (1)
8-10: Go version must be updated to 1.24.9.Line 8 specifies
go1.24.0.linux-amd64.tar.gz, but project requirements mandate Go 1.24.9 or later (per learnings and the prior review flagged in the PR history). This issue was marked as addressed in commit 08bcd18, but the code still reflects the older version. Update all references on lines 8–10 to usego1.24.9.linux-amd64.tar.gz.Apply this diff:
-RUN wget https://go.dev/dl/go1.24.0.linux-amd64.tar.gz && \ - tar -C /usr/local -xzf go1.24.0.linux-amd64.tar.gz && \ - rm go1.24.0.linux-amd64.tar.gz +RUN wget https://go.dev/dl/go1.24.9.linux-amd64.tar.gz && \ + tar -C /usr/local -xzf go1.24.9.linux-amd64.tar.gz && \ + rm go1.24.9.linux-amd64.tar.gzcmd/hyperfleet-api/server/routes.go (1)
53-59: Authorization middleware remains permanently mocked—wire real authz behind config
authzMiddlewareis always a mock, and the realauth.NewAuthzMiddlewareinitialization is fully commented out. That means route-level authorization is effectively disabled even ifenv().Config.Server.EnableAuthzis set, which weakens the security posture. This mirrors a concern raised in a previous review on this area.At minimum, please either:
- Gate the real middleware behind
EnableAuthz(keeping the mock as the default when false), or- Clearly document and track in an issue that authorization is intentionally disabled and must be enabled before any non-test deployment.
For example:
- authzMiddleware := auth.NewAuthzMiddlewareMock() - // TODO: Create issue to track enabling authorization middleware - // if env().Config.Server.EnableAuthz { - // var err error - // authzMiddleware, err = auth.NewAuthzMiddleware() - // check(err, "Unable to create authz middleware") - // } + authzMiddleware := auth.NewAuthzMiddlewareMock() + // TODO: link tracking issue for enabling authorization middleware in non-test environments. + if env().Config.Server.EnableAuthz { + var err error + authzMiddleware, err = auth.NewAuthzMiddleware() + check(err, "Unable to create authz middleware") + }
🧹 Nitpick comments (4)
pkg/api/node_pool_types.go (2)
80-101: Defensive JSON unmarshal defaults inToOpenAPIlook goodThe added error checks for
Spec,Labels, andStatusConditionsgive you safe empty defaults instead of propagating bad JSON to the API level, and they align with the patterns noted for clusters/adapter status types. Only optional consideration: if you ever wantSpec/Labels/Conditionsto always serialize as{}/[](rather thannull) even when the DB field is empty, you could initialize them to empty values up front instead of only on unmarshal errors. Based on learnings, this keeps behavior consistent acrosspkg/apitypes.
159-178: Marshal error handling inNodePoolFromOpenAPICreateis robust and consistentCatching
json.Marshalerrors forSpec,Labels, andStatusConditionsand falling back to"{}"/"[]"ensures the JSONB columns remain valid even in unexpected cases, and matches the defensive style used elsewhere in the API models. If you later introduce structured logging, it might be worth logging these rare marshal errors before defaulting so they’re observable, but that’s optional and not required for this PR’s goal of stabilizing lint/tests.pkg/handlers/helpers.go (1)
17-25: Error handling added, but errors are silently discarded.The error checks prevent panics and satisfy linter requirements. However, these errors are silently swallowed without any logging, making debugging difficult in production.
Consider adding a context parameter to enable structured logging for these failure cases:
-func writeJSONResponse(w http.ResponseWriter, code int, payload interface{}) { +func writeJSONResponse(ctx context.Context, w http.ResponseWriter, code int, payload interface{}) { + log := logger.NewOCMLogger(ctx) w.Header().Set("Content-Type", "application/json") // ... if payload != nil { response, err := json.Marshal(payload) if err != nil { - // Headers already sent, can't change status code + log.Errorf("Failed to marshal JSON response: %v", err) return } if _, err := w.Write(response); err != nil { - // Writing failed, nothing we can do at this point + log.Errorf("Failed to write response: %v", err) return } } }Based on learnings, handlers should use structured logging via
pkg/logger/logger.gowith context propagation.pkg/auth/helpers.go (1)
30-38: Same silent error handling pattern aspkg/handlers/helpers.go.This file already imports
loggerand the callerhandleErrorhas context. Consider passing context towriteJSONResponseto enable logging of marshal/write failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/framework_test.go(0 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/handlers/validation.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(1 hunks)pkg/services/types.go(1 hunks)test/helper.go(4 hunks)test/integration/clusters_test.go(2 hunks)test/integration/node_pools_test.go(1 hunks)test/mocks/jwk_cert_server.go(1 hunks)test/mocks/ocm.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/handlers/framework_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- pkg/config/config_test.go
- pkg/handlers/cluster_status.go
- pkg/services/cluster.go
- pkg/handlers/validation.go
- pkg/db/db_session/test.go
- cmd/hyperfleet-api/server/api_server.go
- pkg/db/sql_helpers.go
- pkg/handlers/framework.go
- pkg/services/node_pool.go
- cmd/hyperfleet-api/environments/framework.go
- pkg/services/types.go
- pkg/db/db_session/default.go
- pkg/services/status_aggregation.go
- pkg/handlers/openapi.go
- pkg/auth/context.go
- pkg/api/cluster_types.go
- .golangci.yml
- pkg/db/transaction_middleware.go
- Makefile
- openapitools.json
- cmd/hyperfleet-api/server/logging/writer.go
- pkg/api/adapter_status_types.go
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
test/mocks/ocm.gopkg/handlers/node_pool.gocmd/hyperfleet-api/server/routes.gotest/mocks/jwk_cert_server.gotest/integration/node_pools_test.gopkg/handlers/helpers.gopkg/api/node_pool_types.gocmd/hyperfleet-api/main.gotest/helper.gopkg/auth/helpers.gopkg/services/generic_test.gopkg/services/generic.gopkg/client/ocm/client.gocmd/hyperfleet-api/environments/framework_test.gopkg/handlers/cluster.gotest/integration/clusters_test.gopkg/api/presenters/slice_filter.gopkg/db/db_session/testcontainer.gopkg/handlers/nodepool_status.gocmd/hyperfleet-api/server/logging/request_logging_middleware.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/handlers/node_pool.gopkg/handlers/helpers.gopkg/services/generic_test.gopkg/services/generic.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Files:
pkg/handlers/node_pool.gopkg/handlers/helpers.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/handlers/node_pool.gopkg/handlers/helpers.gopkg/services/generic_test.gopkg/services/generic.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.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/handlers/node_pool.gopkg/handlers/helpers.gopkg/api/node_pool_types.gopkg/auth/helpers.gopkg/services/generic_test.gopkg/services/generic.gopkg/client/ocm/client.gopkg/handlers/cluster.gopkg/api/presenters/slice_filter.gopkg/db/db_session/testcontainer.gopkg/handlers/nodepool_status.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/node_pools_test.gotest/integration/clusters_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/node_pool_types.gopkg/api/presenters/slice_filter.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/generic_test.gopkg/services/generic.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/db_session/testcontainer.go
🧠 Learnings (14)
📚 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:
pkg/handlers/node_pool.gotest/integration/node_pools_test.gopkg/api/node_pool_types.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/**/*.go : Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Applied to files:
cmd/hyperfleet-api/server/routes.gotest/mocks/jwk_cert_server.gopkg/auth/helpers.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/handlers/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/helpers.gopkg/handlers/nodepool_status.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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/handlers/helpers.gopkg/handlers/nodepool_status.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:
pkg/api/node_pool_types.gopkg/handlers/nodepool_status.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 : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/services/generic_test.gopkg/db/db_session/testcontainer.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:
cmd/hyperfleet-api/environments/framework_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/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/{handlers,services}/**/*.go : Always retrieve database session from context via db.NewContext() instead of creating new connections
Applied to files:
pkg/db/db_session/testcontainer.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/**/*.go : Use PostgreSQL 14.2 with GORM ORM for all database operations
Applied to files:
pkg/db/db_session/testcontainer.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/{handlers,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
pkg/handlers/nodepool_status.gocmd/hyperfleet-api/server/logging/request_logging_middleware.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/services/**/*.go : Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Applied to files:
pkg/handlers/nodepool_status.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/openapi/**/*.go : Use make generate to regenerate Go models from openapi/openapi.yaml via openapi-generator-cli v7.16.0 in Podman
Applied to files:
Dockerfile.openapi
📚 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 **/*.go : Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Applied to files:
Dockerfile.openapi
🧬 Code graph analysis (5)
pkg/handlers/node_pool.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
cmd/hyperfleet-api/environments/framework_test.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Services(44-47)
pkg/handlers/cluster.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
pkg/handlers/nodepool_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
🔇 Additional comments (24)
cmd/hyperfleet-api/server/logging/request_logging_middleware.go (1)
14-28: SimplifieddoLoginitialization looks correctThe boolean now directly encodes the intended behavior: log everything except
/api/hyperfleet(with trailing slash already stripped). This is clearer than a multi-step assignment and preserves existing semantics.Dockerfile.openapi (1)
3-5: Approve the APT sandbox workaround for rootless podman.The
-o APT::Sandbox::User=rootflag correctly addresses the Prow/podman setgroups error without affecting subsequent build logic.test/integration/clusters_test.go (1)
66-75: Good addition: explicitly asserting no transport error for invalid JSON testAdding
Expect(err).ToNot(HaveOccurred(), ...)before checkingrestyResp.StatusCode()ensures the test fails cleanly on network/transport issues while still asserting the 400 application‑level response. The ordering (error check first, then status) is correct.test/helper.go (4)
83-84: LGTM: Appropriate error handling for optional test configuration.Explicitly ignoring the error from setting the log level flag is reasonable in test code, and the inline comment clearly documents the intent.
181-183: LGTM: Improved error visibility in test server restart.The change adds error checking and warning logging while pragmatically allowing the restart to proceed. This improves observability without breaking test robustness.
204-206: LGTM: Proper defensive error handling added.The fatal error on
AddFlagsfailure is appropriate for theResetfunction, which requires a clean environment state to proceed correctly.
441-445: LGTM: Well-documented use of deprecated function in test fixtures.The comment clearly explains why the deprecated function is still necessary (no suitable alternative for password-protected test fixture keys), and the
nolint:staticcheckdirective is appropriately placed to suppress the linter warning.pkg/client/ocm/client.go (1)
78-83: Verify all callers ofClient.Close()handle error returns and confirm coding guideline compliance.The Close() method now correctly returns an error, following Go best practices for resource cleanup. This is a breaking API change.
Ensure:
- All callers of
Client.Close()have been updated to handle the error return value- JWT authentication middleware is implemented to validate tokens and extract account ID and username from claims (coding guideline for
pkg/**/*.go)- Code complies with Go 1.24.9 using FIPS-compliant crypto (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
pkg/db/db_session/testcontainer.go (1)
52-54: Good fix—port-based wait is simpler and resolves the test failure.Switching from log-based to port-based readiness checking eliminates the log-parsing flakiness that caused the presubmit failure. Since the immediate connection attempt at lines 63–74 provides implicit verification of database readiness, this approach is acceptable for integration tests.
For even more robust readiness checking, consider using a SQL-based wait strategy if available in your testcontainers-go version (e.g.,
wait.ForSQL()), which explicitly verifies that PostgreSQL accepts queries before proceeding.Please monitor integration test runs over the next few days to confirm this change doesn't introduce intermittent flakiness.
pkg/handlers/cluster.go (1)
72-75: Spec marshal error handling looks correct and consistentCapturing the json.Marshal error and converting it to a ServiceError via
errors.GeneralErroris appropriate and keeps handler error handling consistent across the API surface.test/mocks/jwk_cert_server.go (1)
33-35: Good addition of write-error reporting in mock JWK serverChecking the
fmt.Fprintferror and failing the test viat.Errorfmakes the mock more robust without changing its semantics.pkg/handlers/node_pool.go (1)
73-76: NodePool spec marshal error is handled appropriatelyMarshaling
patch.Specand returning aerrors.GeneralErroron failure cleanly propagates bad-spec issues through the existing ServiceError flow, matching the cluster handler behavior.cmd/hyperfleet-api/environments/framework_test.go (1)
39-39: Reflection change is safe and slightly more robustUsing
reflect.ValueOf(&env.Services).Elem()still inspects the same struct type while producing an addressable value, which keeps the test behavior intact and future-proofs against potential reflective writes.cmd/hyperfleet-api/main.go (1)
28-30: Flag parse error handling is appropriate for the entrypointChecking the error from
flag.CommandLine.Parse([]string{})and exiting viaglog.Fatalfis correct and avoids silent startup failures.test/mocks/ocm.go (1)
62-65: Pointer receiver on Reset fixes state reset semanticsSwitching
Resetto a pointer receiver aligns with the other methods and ensures the recordedAction/ResourceTypeare actually cleared on the shared mock instance.pkg/handlers/nodepool_status.go (1)
11-11: Structured logging for aggregation failures is a good patternLogging
UpdateNodePoolStatusFromAdaptersfailures withlogger.NewOCMLogger(ctx)while keeping the write-path successful is a sensible choice for best-effort aggregation, and it follows the structured logging guidance for handlers.Also applies to: 92-95
pkg/api/presenters/slice_filter.go (3)
114-116: LGTM! Correct error propagation from nested validation.Previously, errors from nested
validatecalls were silently discarded. Now they are properly returned, ensuring validation failures in nested structs surface to the caller.
149-153: Defensive error handling is good, though pattern is always valid.The regex pattern
(name\..*)constructed fromnamewill always compile successfully sincenamecomes from JSON struct tags (alphanumeric/underscores). However, the defensive check is a reasonable safeguard against future changes.
194-196: LGTM! Proper nil guard for time pointer.The nil check prevents a panic when formatting a nil
*time.Timevalue.pkg/services/generic_test.go (2)
23-23: LGTM! Acceptable use of nolint directive for test cleanup.Ignoring the error from a deferred
Close()in tests is a common and acceptable pattern.
70-70: LGTM! Correctly updated to match newtreeWalkForSqlizersignature.The call site is properly updated to expect
(Sqlizer, error)instead of the previous three-value return.pkg/services/generic.go (3)
43-43: LGTM! Typo fix.
173-173: LGTM! Call site correctly updated for simplified return signature.
332-345: LGTM! Good simplification of the return signature.Removing the unused
tsl.Nodereturn value simplifies the API. The function now returns only(squirrel.Sqlizer, *errors.ServiceError), which is all that callers need. Error handling paths correctly return(nil, error).
yingzhanredhat
force-pushed
the
ying-test
branch
from
3129fc3 to
0128ec8
Compare
yingzhanredhat
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Dockerfile.openapi (2)
8-10: ❌ Go version must be updated to 1.24.9 (regression from prior review).The code still specifies Go 1.24.0, but the prior review flagged this must be Go 1.24.9 (marked as "Addressed in commit 08bcd18"). Additionally, per project learnings, Go 1.24.9 is required for FIPS-compliant crypto builds. This appears to be a regression or incomplete fix.
Update lines 8–10 to use
go1.24.9.linux-amd64.tar.gz:-RUN wget https://go.dev/dl/go1.24.0.linux-amd64.tar.gz && \ - tar -C /usr/local -xzf go1.24.0.linux-amd64.tar.gz && \ - rm go1.24.0.linux-amd64.tar.gz +RUN wget https://go.dev/dl/go1.24.9.linux-amd64.tar.gz && \ + tar -C /usr/local -xzf go1.24.9.linux-amd64.tar.gz && \ + rm go1.24.9.linux-amd64.tar.gz
23-23: ❌ CGO_ENABLED must be 1 and GOEXPERIMENT must be set for FIPS-compliant crypto.Per project learnings, builds must use
CGO_ENABLED=1withGOEXPERIMENT=boringcryptofor FIPS-compliant cryptography. Currently, CGO is disabled (line 23), which prevents boring crypto integration.Update lines 23 to enable CGO and add the GOEXPERIMENT setting:
-ENV CGO_ENABLED=0 +ENV CGO_ENABLED=1 +ENV GOEXPERIMENT=boringcryptotest/integration/node_pools_test.go (1)
73-83: Avoid potential nil deref by checkingerrbefore usingrestyRespThe new
Expect(err).NotTo(HaveOccurred())is good, butrestyResp.StatusCode()is still called first. If the POST fails before a response is created,restyRespcan be nil and this will panic instead of cleanly failing the test.Consider reordering:
- Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) - Expect(err).NotTo(HaveOccurred(), "Error posting object: %v", err) + Expect(err).NotTo(HaveOccurred(), "Error posting object: %v", err) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest))
♻️ Duplicate comments (2)
cmd/hyperfleet-api/server/routes.go (1)
53-59: Authorization middleware still permanently mocked; consider enabling behind config + link tracking issueEven with the new TODO,
authzMiddlewareis alwaysauth.NewAuthzMiddlewareMock()at runtime, so no real authz is ever performed. That may be acceptable only for tests/dev; it’s risky if this binary is used in any production-like environment.Consider uncommenting the block and gating the real middleware on
env().Config.Server.EnableAuthz, keeping the mock only when the flag is false, and update the TODO to reference the concrete tracking issue ID so it’s clear when/where authz must be enabled:- authzMiddleware := auth.NewAuthzMiddlewareMock() - // TODO: Create issue to track enabling authorization middleware - // if env().Config.Server.EnableAuthz { - // var err error - // authzMiddleware, err = auth.NewAuthzMiddleware() - // check(err, "Unable to create authz middleware") - // } + authzMiddleware := auth.NewAuthzMiddlewareMock() + if env().Config.Server.EnableAuthz { + var err error + authzMiddleware, err = auth.NewAuthzMiddleware() + check(err, "Unable to create authz middleware") + } + // TODO(HF-XXXX): ensure production runs with EnableAuthz=true; mock is for dev/test onlypkg/api/cluster_types.go (1)
70-73: LGTM - addresses past review feedback.The change to use
make(map[string]interface{})addresses the suggestion from the previous review comment by yasun1.
🧹 Nitpick comments (11)
pkg/db/db_session/test.go (1)
216-221: Previous concern addressed: error is no longer silently ignored.The panic-on-failure approach is valid for test helpers where tests cannot reliably continue with an unclean database. The error is now properly checked, and
wasDisconnectedis only set after a successful reset.Optionally, returning the error (
func (f *Test) ResetDB() error) instead of panicking would give individual test callers more control—for example, to log additional context or perform cleanup before failing. However, since panicking was one of the three approaches suggested in the previous review and is reasonable for test infrastructure, the current implementation is acceptable.pkg/client/ocm/client.go (1)
78-83: Close implementation is correct; consider clearing the connection after closeReturning the underlying
Close()error and treating a nil connection as a no-op is appropriate here. You might optionally setc.connection = nilafter a successful close to avoid accidental reuse or double-close, but that's not strictly required.pkg/services/generic_test.go (1)
23-24: Consider checking dbFactory.Close error instead of suppressingUsing
//nolint:errcheckis acceptable in tests, but you might get slightly better safety by asserting the error is nil instead of ignoring it altogether (e.g.,require.NoError(t, dbFactory.Close())in at.Cleanup). Not required for this PR, just a minor improvement to catch regressions in the mock.pkg/handlers/openapi.go (2)
61-64: Consider logging write errors for observability.While the early return is correct since headers are already committed, silently discarding the error makes debugging difficult. Logging the error would help identify network issues or client disconnects.
+ "github.com/golang/glog"if _, err := w.Write(h.openAPIDefinitions); err != nil { - // Response already committed, can't report error + glog.Warningf("Failed to write OpenAPI response: %v", err) return }
70-73: Same logging improvement applies here.For consistency, log the write error before returning.
pkg/auth/helpers.go (1)
30-38: Consider logging marshal/write failures.The error handling logic is correct, but silent failures make debugging difficult. When
json.Marshalfails, the client receives the HTTP status code with an empty body, which could be confusing. Logging these errors would help diagnose issues.if payload != nil { response, err := json.Marshal(payload) if err != nil { - // Headers already sent, can't change status code + // Headers already sent, can't change status code - log for debugging + logger.NewOCMLogger(context.Background()).Errorf("Failed to marshal response: %v", err) return } if _, err := w.Write(response); err != nil { - // Writing failed, nothing we can do at this point + logger.NewOCMLogger(context.Background()).Warningf("Failed to write response: %v", err) return } }Note: You'd need to add
"context"to imports or accept a context parameter.pkg/api/presenters/slice_filter.go (1)
149-153: Silent return on regex compile failure may mask issues.While the pattern is constructed from JSON struct tags (developer-controlled), silently returning the unchanged map on compile failure could lead to unexpected behavior where fields meant to be filtered remain in the output. Consider logging a warning to aid debugging.
pat, err := regexp.Compile(pattern) if err != nil { - // Invalid pattern, return unchanged + // Invalid pattern - this shouldn't happen with valid JSON tags + // but log for debugging if it does return in }Alternatively, since the pattern is constructed programmatically and should always be valid, you could use
regexp.MustCompilewith a comment explaining the invariant.pkg/api/cluster_types.go (2)
78-88: Consider logging unmarshal errors as they may indicate data corruption.While defaulting to empty values is reasonable for presentation,
json.Unmarshalfailures on database-sourced JSONB columns could indicate data corruption or schema issues. Logging these errors would help identify data integrity problems.if len(c.Labels) > 0 { if err := json.Unmarshal(c.Labels, &labels); err != nil { + // Log: could indicate data corruption labels = make(map[string]string) } } // Unmarshal StatusConditions (stored as ResourceCondition in DB) var statusConditions []openapi.ResourceCondition if len(c.StatusConditions) > 0 { if err := json.Unmarshal(c.StatusConditions, &statusConditions); err != nil { + // Log: could indicate data corruption statusConditions = []openapi.ResourceCondition{} } }
136-155: Defensive defaults are appropriate, but consider logging.Marshaling simple Go types rarely fails, so these defaults are mostly defensive. However, if marshaling does fail, it could indicate a deeper issue (e.g., unexpected data types). Consider logging when these fallback paths are taken.
pkg/api/node_pool_types.go (1)
82-84: Consider logging unmarshal errors instead of silently using fallbacks.While the fallbacks prevent panics and provide safe defaults, silently ignoring unmarshal errors in
ToOpenAPIcould mask data corruption or schema issues in the database. If JSONB data becomes malformed, you won't know until you investigate why a nodepool has no spec or labels.Consider adding structured logging for these unmarshal failures:
var spec map[string]interface{} if len(np.Spec) > 0 { if err := json.Unmarshal(np.Spec, &spec); err != nil { + // Log the error with context + // logger.Errorf("Failed to unmarshal NodePool spec for ID %s: %v", np.ID, err) spec = make(map[string]interface{}) } }Apply similar logging for Labels and StatusConditions.
As per coding guidelines, use structured logging via pkg/logger/logger.go with context propagation.
Also applies to: 90-92, 98-100
pkg/api/adapter_status_types.go (1)
55-58: Consider logging unmarshal errors instead of silently using fallbacks.Similar to the pattern in
node_pool_types.go, silently falling back to empty defaults when unmarshaling from the database could mask data corruption or schema migration issues. If adapter status data becomes malformed in the DB, these silent fallbacks will hide the problem.Add structured logging for unmarshal failures:
if len(as.Conditions) > 0 { if err := json.Unmarshal(as.Conditions, &conditions); err != nil { - // If unmarshal fails, use empty slice + // Log unmarshal failure with context + // logger.Errorf("Failed to unmarshal AdapterStatus conditions for ID %s: %v", as.ID, err) conditions = []openapi.AdapterCondition{} } }As per coding guidelines, use structured logging via pkg/logger/logger.go with context propagation.
Also applies to: 64-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
.golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/framework_test.go(0 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/handlers/validation.go(1 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(1 hunks)pkg/services/types.go(1 hunks)test/helper.go(4 hunks)test/integration/clusters_test.go(2 hunks)test/integration/node_pools_test.go(1 hunks)test/mocks/jwk_cert_server.go(1 hunks)test/mocks/ocm.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/handlers/framework_test.go
🚧 Files skipped from review as they are similar to previous changes (19)
- pkg/config/config_test.go
- .golangci.yml
- pkg/handlers/framework.go
- pkg/handlers/nodepool_status.go
- pkg/db/sql_helpers.go
- pkg/services/status_aggregation.go
- pkg/handlers/cluster.go
- test/mocks/jwk_cert_server.go
- cmd/hyperfleet-api/server/logging/request_logging_middleware.go
- pkg/handlers/cluster_status.go
- pkg/services/types.go
- cmd/hyperfleet-api/main.go
- pkg/services/cluster.go
- pkg/db/db_session/default.go
- test/integration/clusters_test.go
- test/mocks/ocm.go
- pkg/db/db_session/testcontainer.go
- openapitools.json
- pkg/handlers/helpers.go
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
pkg/api/adapter_status_types.gocmd/hyperfleet-api/server/routes.gopkg/handlers/openapi.gopkg/db/transaction_middleware.gotest/integration/node_pools_test.gopkg/auth/context.gopkg/db/db_session/test.gopkg/api/node_pool_types.gopkg/services/node_pool.gocmd/hyperfleet-api/environments/framework_test.gopkg/handlers/validation.gocmd/hyperfleet-api/environments/framework.gopkg/api/presenters/slice_filter.gopkg/api/cluster_types.gopkg/auth/helpers.gopkg/services/generic.gopkg/services/generic_test.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/logging/writer.gopkg/client/ocm/client.gotest/helper.gopkg/handlers/node_pool.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/adapter_status_types.gopkg/api/node_pool_types.gopkg/api/presenters/slice_filter.gopkg/api/cluster_types.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/api/adapter_status_types.gopkg/handlers/openapi.gopkg/db/transaction_middleware.gopkg/auth/context.gopkg/db/db_session/test.gopkg/api/node_pool_types.gopkg/services/node_pool.gopkg/handlers/validation.gopkg/api/presenters/slice_filter.gopkg/api/cluster_types.gopkg/auth/helpers.gopkg/services/generic.gopkg/services/generic_test.gopkg/client/ocm/client.gopkg/handlers/node_pool.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/handlers/openapi.gopkg/services/node_pool.gopkg/handlers/validation.gopkg/services/generic.gopkg/services/generic_test.gopkg/handlers/node_pool.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Files:
pkg/handlers/openapi.gopkg/handlers/validation.gopkg/handlers/node_pool.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/handlers/openapi.gopkg/services/node_pool.gopkg/handlers/validation.gopkg/services/generic.gopkg/services/generic_test.gopkg/handlers/node_pool.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/transaction_middleware.gopkg/db/db_session/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/node_pools_test.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/node_pool.gopkg/services/generic.gopkg/services/generic_test.go
🧠 Learnings (15)
📚 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 : Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Applied to files:
pkg/api/adapter_status_types.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/services/**/*.go : Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Applied to files:
pkg/api/adapter_status_types.gopkg/services/node_pool.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:
pkg/api/adapter_status_types.gopkg/handlers/openapi.gopkg/api/node_pool_types.goMakefilepkg/api/cluster_types.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:
cmd/hyperfleet-api/server/routes.gopkg/auth/context.gopkg/auth/helpers.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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/db/transaction_middleware.gopkg/handlers/node_pool.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/node_pools_test.gopkg/api/node_pool_types.gopkg/api/cluster_types.gopkg/handlers/node_pool.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/dao/**/*.go : Use context.Context as the first parameter in all DAO methods for transaction propagation
Applied to files:
pkg/auth/context.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/{handlers,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
pkg/auth/context.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/{handlers,services}/**/*.go : Always retrieve database session from context via db.NewContext() instead of creating new connections
Applied to files:
pkg/db/db_session/test.gocmd/hyperfleet-api/environments/framework.gocmd/hyperfleet-api/server/api_server.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/openapi/**/*.go : Use make generate to regenerate Go models from openapi/openapi.yaml via openapi-generator-cli v7.16.0 in Podman
Applied to files:
Makefilepkg/api/cluster_types.goDockerfile.openapi
📚 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 **/*.go : Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Applied to files:
MakefileDockerfile.openapi
📚 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:
Makefilecmd/hyperfleet-api/environments/framework_test.gocmd/hyperfleet-api/environments/framework.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 openapi/openapi.yaml : TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
Applied to files:
pkg/api/cluster_types.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 : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/services/generic_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/**/*.go : Use PostgreSQL 14.2 with GORM ORM for all database operations
Applied to files:
cmd/hyperfleet-api/server/api_server.go
🧬 Code graph analysis (5)
pkg/api/adapter_status_types.go (1)
pkg/api/metadata_types.go (1)
Metadata(28-34)
cmd/hyperfleet-api/environments/framework_test.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Services(44-47)
cmd/hyperfleet-api/environments/framework.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Clients(67-69)
cmd/hyperfleet-api/server/api_server.go (2)
cmd/hyperfleet-api/environments/types.go (1)
Database(36-38)pkg/db/session.go (1)
SessionFactory(12-20)
pkg/handlers/node_pool.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
🔇 Additional comments (20)
Dockerfile.openapi (1)
3-5: ✓ APT sandbox workaround correctly applied.The
-o APT::Sandbox::User=rootflag is properly applied to both apt-get invocations with a clear explanatory comment. This addresses the Prow rootless podman integration issue mentioned in the PR objectives.cmd/hyperfleet-api/server/logging/writer.go (1)
47-47: LGTM! More idiomatic use of embedded method.Using the promoted
Header()method directly is cleaner and follows Go conventions for embedded types.Makefile (1)
116-117: Verify.golangci.ymlconfiguration exists and properly excludes generated code.The lint target now relies on a
.golangci.ymlconfiguration file instead of explicit flags and paths. Ensure this config file exists, excludespkg/api/openapi(generated code), and doesn't unintentionally enable the unused check by default.cmd/hyperfleet-api/server/api_server.go (1)
149-151: LGTM! Proper error handling for database cleanup.The addition of error checking and logging for the database connection close operation is a good defensive programming practice. Logging the error (rather than treating it as fatal) is appropriate for cleanup code that runs after the server has already exited.
cmd/hyperfleet-api/environments/framework.go (1)
158-162: OCM teardown now safely guarded and error-awareThe nil check around
e.Clients.OCMand logging of anyClose()error bring this in line with the database teardown pattern and avoid potential panics when the client isn’t initialized.cmd/hyperfleet-api/environments/framework_test.go (1)
39-39: Reflecting on&env.Servicesis safe and future‑proofSwitching to
reflect.ValueOf(&env.Services).Elem()correctly targets the live Services struct rather than a copy and keeps the field-inspection logic intact; this is a fine and slightly more robust approach.pkg/services/generic.go (3)
43-56: Comment typo fix is correct and keeps intent clearThe updated comment (“LIST function”) is accurate and keeps the purpose of
listContextclear. No further changes needed here.
151-186: buildSearchValues correctly adapts to new treeWalkForSqlizer signatureThe call
sqlizer, serviceErr := s.treeWalkForSqlizer(listCtx, tslTree)matches the simplified return type, and the error handling (if serviceErr != nil { return "", nil, serviceErr }) preserves the previous control flow and behavior. This refactor is safe and keeps the search pipeline behavior unchanged.
332-346: treeWalkForSqlizer refactor improves clarity and error semanticsThe new
(squirrel.Sqlizer, *errors.ServiceError)signature and returningnilon error (both forFieldNameWalkandsqlFilter.Walk) align with how callers actually use the function and avoid leaking partially processed state. Usingerrors.BadRequestfor walker failures is consistent with the other TSL walkers in this file. No issues from a correctness or API-usage standpoint.pkg/services/generic_test.go (1)
70-76: Test correctly updated for new treeWalkForSqlizer return typeThe test now binds only
sqlizer, serviceErrand asserts onserviceErrandsqlizer.ToSql()output, which matches the updated production signature and behavior. This keeps the SQL translation coverage intact.pkg/handlers/node_pool.go (1)
73-77: LGTM!Good addition of error handling for JSON marshaling. The use of
errors.GeneralErroraligns with the structured ServiceError pattern, and the error message provides sufficient context for debugging.pkg/api/presenters/slice_filter.go (2)
114-116: LGTM!Good fix - propagating the validation error from nested structs prevents silent failures and ensures callers are aware of invalid field requests.
194-196: LGTM!Good defensive nil-pointer check for
*time.Timefields before formatting, preventing potential panics.pkg/services/node_pool.go (1)
139-140: Good fix for variable shadowing.Renaming the unmarshal error variable to
unmarshalErrprevents shadowing the outererrvariable and makes the error handling clearer.pkg/api/adapter_status_types.go (1)
129-135: LGTM: Scope bug fixed with safe fallback pattern.The current implementation correctly addresses the variable scope issue flagged in previous reviews. The pattern of initializing
dataJSONandmetadataJSONwith fallback defaults, then conditionally overriding on successful marshal, is clean and avoids the scope bug.Also applies to: 138-144
pkg/auth/context.go (2)
45-48: Good defensive fix for type assertion.Replacing the direct type assertion with a safe checked assertion prevents potential panics if the context value is not a string.
82-127: Excellent defensive programming for JWT claims extraction.Converting all JWT claim extractions from direct type assertions to checked assertions (
value, ok := claim.(string)) prevents panics when:
- Claims are missing
- Claims have unexpected types
- Malformed tokens are processed
This aligns with the JWT authentication middleware coding guideline and ensures robust token validation.
test/helper.go (3)
180-185: Graceful restart logging improvement looks goodLogging a warning when
stopAPIServerfails before restarting improves debuggability without changing the external behavior of tests. The restart path remains straightforward.
200-215: Stricter error handling onenv.AddFlagsduring reset is appropriateTreating failures from
env.AddFlags(flagset)as fatal duringReset()makes test environment initialization fail fast and avoids running tests under a partially configured env. This aligns with how initialization is handled inNewHelper.
441-445: Clear justification for//nolint:staticcheckaround deprecated key parsingThe added comment explains why
ParseRSAPrivateKeyFromPEMWithPasswordis still used for test fixtures and why the staticcheck warning is suppressed locally. This keeps the linter noise scoped and documented.
yingzhanredhat
force-pushed
the
ying-test
branch
from
0128ec8 to
95f28bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pkg/db/transaction_middleware.go (1)
44-52: Comment states "Log error" but logging is not implemented.The comment at line 46 says "Log error but don't expose to client since headers already sent" yet no logging is performed. Since the
loggerpackage is already imported, consider adding actual logging to match the stated intent.Based on coding guidelines, use structured logging via pkg/logger/logger.go with context propagation. The function would need a context parameter to obtain the logger.
cmd/hyperfleet-api/server/routes.go (1)
53-59: Authorization still uses mock only; TODO should be tied to a real issue and/or config‑gated implementationYou’ve fixed the compile bug in the commented block (the
errdeclaration), which is good. However, as written:
authzMiddlewareis alwaysauth.NewAuthzMiddlewareMock().- The
EnableAuthzconfig flag is effectively unused because the real initialization is entirely commented out.- The TODO is generic and doesn’t reference an actual Jira/issue, so it’s easy to lose track of when real authz must be enabled.
Given this was already flagged earlier, I’d suggest at minimum:
- Update the TODO to reference the concrete tracking item (e.g., a HYPERFLEET‑xxx Jira), and clearly state that real authorization must be enabled before any production deployment.
- Optionally, consider restoring the conditional initialization as real code (gated by
env().Config.Server.EnableAuthz) and keeping the mock as the default when the flag is false, instead of relying on commented code.For example, when you’re ready to re‑enable it, something along these lines would remove the need for commented blocks:
- authzMiddleware := auth.NewAuthzMiddlewareMock() - // TODO(<JIRA-ID>): Create issue to track enabling authorization middleware - // if env().Config.Server.EnableAuthz { - // var err error - // authzMiddleware, err = auth.NewAuthzMiddleware() - // check(err, "Unable to create authz middleware") - // } + authzMiddleware := auth.NewAuthzMiddlewareMock() + if env().Config.Server.EnableAuthz { + var err error + authzMiddleware, err = auth.NewAuthzMiddleware() + check(err, "Unable to create authz middleware") + } + // TODO(<JIRA-ID>): Verify EnableAuthz is correctly set in prod configs before rollout.test/helper.go (1)
81-85: Fix flag name passed topflag.CommandLine.SetsoLOGLEVELworks
FlagSet.Setexpects the flag name without a leading dash; using"-v"will always return an error, which you now explicitly ignore, soLOGLEVELnever actually changes verbosity.You can keep ignoring the error in tests while fixing the flag name:
- if logLevel := os.Getenv("LOGLEVEL"); logLevel != "" { - glog.Infof("Using custom loglevel: %s", logLevel) - // Intentionally ignore error from Set — acceptable for tests - _ = pflag.CommandLine.Set("-v", logLevel) - } + if logLevel := os.Getenv("LOGLEVEL"); logLevel != "" { + glog.Infof("Using custom loglevel: %s", logLevel) + // Intentionally ignore error from Set — acceptable for tests + _ = pflag.CommandLine.Set("v", logLevel) + }
🧹 Nitpick comments (7)
pkg/handlers/nodepool_status.go (1)
11-12: Non-fatal aggregation logging is consistent and well-structuredUsing
logger.NewOCMLogger(ctx)withExtra("nodepool_id", ...)andExtra("error", aggregateErr)gives good observability while keeping the Create path successful when status aggregation fails, which fits the handler pattern (primary write succeeds, derived status is best-effort). This is aligned with the structured logging guidance for handlers.Also applies to: 92-96
pkg/handlers/cluster_status.go (1)
11-12: Improved visibility into cluster status aggregation failuresThe inline aggregation call with a structured warning log (including
cluster_idand the error) cleanly surfaces failures without changing the Create response semantics. This matches the nodepool handler and aligns with the structured logging expectations.Also applies to: 92-96
test/integration/clusters_test.go (1)
231-244: Clarify expected behavior for empty cluster name; assertion now enforces only the “accept” pathOn Line 243 you assert
Expect(err).ToNot(HaveOccurred())after the empty‑name POST, but the comment on Lines 239–241 says “Should either accept empty name or return 400”. With this assertion, the test will now fail if the API evolves to start returning a 4xx (which the comment currently treats as acceptable).Consider either:
- Making the test explicit about the required behavior (e.g., require 201 and non‑empty ID, and update the comment accordingly), or
- Keeping the test tolerant by branching on
resp.StatusCodeand asserting appropriately for both success and 400 cases, while still checking thaterrmatches the status‑code branch.For example:
_, resp, err = client.DefaultAPI.PostCluster(ctx).ClusterCreateRequest(emptyNameInput).Execute() if resp != nil && resp.StatusCode == http.StatusBadRequest { Expect(err).To(HaveOccurred(), "Should return an error for empty name with 400 status") } else { Expect(err).NotTo(HaveOccurred(), "Should accept empty name when API allows it") Expect(resp.StatusCode).To(Equal(http.StatusCreated)) }This keeps the linter happy and aligns the assertions with the intent described in the comment.
pkg/db/db_session/default.go (1)
111-113: Nice addition of defensive error handling aroundl.Ping()Checking the
Pingerror and logging it at low verbosity improves observability without changing control flow or introducing new failure modes. This is a clean, low‑risk improvement to the listener health‑check path.If you ever revisit this area, you might consider including the channel name or other context in the ping‑failure log to ease correlation with specific listeners, but this is strictly a nice‑to‑have and not required for this PR.
pkg/api/adapter_status_types.go (2)
54-68: Consider logging unmarshal failures to detect data integrity issues.The explicit error handling with fallbacks is good defensive programming. However, if
json.Unmarshalfails on data stored in the database, it indicates a data integrity problem (corrupted JSON in theconditionsordataJSONB columns). Silent fallbacks mask these issues and could hide database corruption or encoding problems.Consider adding structured logging when unmarshaling fails:
if err := json.Unmarshal(as.Conditions, &conditions); err != nil { // Log data integrity issue // logger.Warnf("Failed to unmarshal adapter conditions for ID %s: %v", as.ID, err) conditions = []openapi.AdapterCondition{} }Apply similar logging for the
Dataunmarshal at lines 64-67.
122-129: Implement or remove the commented-out logging statement.The commented-out
logger.Errorfat lines 126-127 suggests logging was considered but not implemented. Sincejson.Marshalfailures on a well-formed struct indicate programming errors (e.g., non-serializable types), these failures should be surfaced rather than silently ignored.Either implement the logging as suggested:
conditionsJSON, err := json.Marshal(adapterConditions) if err != nil { - // Fallback to empty array JSON - // Log marshal failure - this indicates a programming error - // logger.Errorf("Failed to marshal adapter conditions: %v", err) + // Log marshal failure - this indicates a programming error + logger.Errorf("Failed to marshal adapter conditions: %v", err) conditionsJSON = []byte("[]") }Or remove the commented-out code if logging is not desired at this time.
Dockerfile.openapi (1)
3-5: APT sandbox workaround is fine; consider minor Dockerfile tightening (optional).The
-o APT::Sandbox::User=rootflag with the added comment is a reasonable workaround for rootless Podman setgroups issues in Prow. As an optional cleanup, you might later combineupdateandinstallinto a singleRUNand clearaptlists to keep the image a bit leaner, but that isn't required for correctness here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.bingo/golangci-lint.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
.bingo/Variables.mk(1 hunks).bingo/golangci-lint.mod(1 hunks).golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/framework_test.go(0 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/handlers/validation.go(0 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(1 hunks)pkg/services/types.go(1 hunks)test/helper.go(4 hunks)test/integration/clusters_test.go(2 hunks)test/integration/node_pools_test.go(1 hunks)test/mocks/jwk_cert_server.go(1 hunks)test/mocks/ocm.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/handlers/framework_test.go
- pkg/handlers/validation.go
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/config/config_test.go
- pkg/services/generic_test.go
- pkg/db/sql_helpers.go
- pkg/handlers/openapi.go
- test/integration/node_pools_test.go
- pkg/services/types.go
- pkg/api/cluster_types.go
- openapitools.json
- pkg/db/db_session/testcontainer.go
- cmd/hyperfleet-api/server/api_server.go
- pkg/client/ocm/client.go
- cmd/hyperfleet-api/server/logging/writer.go
- pkg/services/node_pool.go
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/api/presenters/slice_filter.go
- Makefile
- pkg/api/node_pool_types.go
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
cmd/hyperfleet-api/server/routes.gopkg/db/db_session/default.gopkg/db/transaction_middleware.gopkg/auth/context.gopkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/auth/helpers.gopkg/db/db_session/test.gopkg/handlers/cluster_status.gocmd/hyperfleet-api/main.gotest/mocks/jwk_cert_server.gopkg/handlers/node_pool.gotest/mocks/ocm.gocmd/hyperfleet-api/server/logging/request_logging_middleware.gopkg/handlers/helpers.gopkg/handlers/framework.gotest/integration/clusters_test.gotest/helper.gopkg/handlers/cluster.gopkg/services/status_aggregation.gocmd/hyperfleet-api/environments/framework.gopkg/api/adapter_status_types.gopkg/services/generic.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/db_session/default.gopkg/db/transaction_middleware.gopkg/db/db_session/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/db/db_session/default.gopkg/db/transaction_middleware.gopkg/auth/context.gopkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/auth/helpers.gopkg/db/db_session/test.gopkg/handlers/cluster_status.gopkg/handlers/node_pool.gopkg/handlers/helpers.gopkg/handlers/framework.gopkg/handlers/cluster.gopkg/services/status_aggregation.gopkg/api/adapter_status_types.gopkg/services/generic.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/handlers/node_pool.gopkg/handlers/helpers.gopkg/handlers/framework.gopkg/handlers/cluster.gopkg/services/status_aggregation.gopkg/services/generic.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/handlers/node_pool.gopkg/handlers/helpers.gopkg/handlers/framework.gopkg/handlers/cluster.gopkg/services/status_aggregation.gopkg/services/generic.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/cluster.gopkg/services/status_aggregation.gopkg/services/generic.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Files:
pkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/handlers/node_pool.gopkg/handlers/helpers.gopkg/handlers/framework.gopkg/handlers/cluster.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/clusters_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/adapter_status_types.go
🧠 Learnings (14)
📚 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:
cmd/hyperfleet-api/server/routes.gopkg/db/transaction_middleware.gopkg/auth/context.gotest/mocks/jwk_cert_server.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/{handlers,services}/**/*.go : Always retrieve database session from context via db.NewContext() instead of creating new connections
Applied to files:
pkg/db/db_session/default.gopkg/db/db_session/test.gocmd/hyperfleet-api/environments/framework.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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/db/transaction_middleware.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/handlers/helpers.gopkg/handlers/framework.gopkg/handlers/cluster.gopkg/api/adapter_status_types.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/{handlers,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
pkg/db/transaction_middleware.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gocmd/hyperfleet-api/server/logging/request_logging_middleware.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/dao/**/*.go : Use context.Context as the first parameter in all DAO methods for transaction propagation
Applied to files:
pkg/auth/context.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/services/**/*.go : Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Applied to files:
pkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/status_aggregation.gopkg/api/adapter_status_types.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 : Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Applied to files:
pkg/services/cluster.gopkg/services/status_aggregation.gopkg/api/adapter_status_types.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:
pkg/handlers/nodepool_status.go.golangci.ymlpkg/handlers/cluster_status.gopkg/api/adapter_status_types.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/handlers/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/handlers/helpers.gopkg/handlers/framework.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 **/*.go : Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Applied to files:
.golangci.yml.bingo/Variables.mk.bingo/golangci-lint.modDockerfile.openapi
📚 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/openapi/**/*.go : Use make generate to regenerate Go models from openapi/openapi.yaml via openapi-generator-cli v7.16.0 in Podman
Applied to files:
.golangci.ymlDockerfile.openapi
📚 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:
pkg/handlers/node_pool.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 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/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 cmd/hyperfleet/environments/**/*.go : Use OCM_ENV environment variable to select configuration environment (development, unit_testing, integration_testing, production)
Applied to files:
cmd/hyperfleet-api/environments/framework.go
🧬 Code graph analysis (7)
pkg/handlers/nodepool_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
pkg/handlers/cluster_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
pkg/handlers/node_pool.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
pkg/handlers/cluster.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
cmd/hyperfleet-api/environments/framework.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Clients(67-69)
pkg/api/adapter_status_types.go (1)
pkg/api/metadata_types.go (1)
Metadata(28-34)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
🔇 Additional comments (29)
test/mocks/jwk_cert_server.go (1)
33-35: Good addition of write error handling in the mockChecking the result of
fmt.Fprintfand surfacing failures viat.Errorfmakes this mock more robust and easier to debug if theResponseWritermisbehaves. No functional behavior change for successful writes, and no further changes are needed here.test/mocks/ocm.go (1)
62-65: Pointer receiver onResetcorrectly updates shared mock stateSwitching
Resetto use a pointer receiver matches the other methods and ensures calls likeauthzMock.Reset()and any use throughocm.Authorizationmutate the original mock instance instead of a copy. The body is simple and correct; no further changes needed here.cmd/hyperfleet-api/environments/framework.go (1)
158-162: OCM client teardown nil-guard correctly prevents panicsThe added
nilcheck arounde.Clients.OCM.Close()cleanly avoids potential panics in cases where the OCM client was never initialized, while still logging any shutdown errors. This matches the updatedClose()signature and is appropriate for teardown paths.pkg/services/cluster.go (1)
132-141: Comment clarification and local error var rename look goodThe updated comment now accurately describes the
adaptersslice, and renaming the innerjson.Unmarshalerror tounmarshalErravoids confusion with the outererrwhile preserving behavior. No issues from a correctness or aggregation standpoint.pkg/db/db_session/test.go (1)
216-221: ResetDB now fails fast on DB reset errors (good change).Panicking on
resetDBfailure and only settingwasDisconnectedafter a successful reset prevents tests from running against a dirty database while preserving the existing reconnection behavior inNew(). This resolves the earlier issue of silently ignored reset failures.pkg/auth/context.go (4)
45-48: LGTM! Defensive type assertion prevents panics.The guarded type assertion correctly handles cases where the context value might not be a string, returning an empty string instead of panicking.
82-96: LGTM! Guarded claim extractions prevent type assertion panics.All primary claim extractions now use the ok pattern to safely handle unexpected types in JWT claims, leaving fields at their zero values when claims are missing or not strings.
100-114: LGTM! Consistent defensive pattern for fallback claims.The fallback claim extractions (preferred_username, given_name, family_name) consistently apply the same guarded type assertion pattern as the primary claims.
119-127: LGTM! Safe name parsing with guarded extraction.The name claim extraction and parsing logic correctly uses a guarded type assertion and safely handles both single and multi-part names. The split logic is safe even for edge cases like empty strings.
pkg/services/generic.go (3)
43-56: LGTM!The added documentation comment clarifies the purpose of the
listContextstruct, improving code readability.
172-176: LGTM!The call site correctly adapts to the simplified
treeWalkForSqlizersignature. The previously returnedtslTreewas unused after this point, so removing it from the return is appropriate.
332-345: Clean signature simplification.The refactor correctly removes the unused
tsl.Nodereturn value. ThetslTreeis still properly modified byFieldNameWalk(line 334) and consumed bysqlFilter.Walk(line 340) internally. Since only thesqlizeris needed by callers, this simplifies the API without changing behavior.cmd/hyperfleet-api/server/logging/request_logging_middleware.go (1)
14-14: LGTM—cleaner boolean assignment.The simplified assignment preserves the original logic while improving readability.
pkg/handlers/helpers.go (1)
17-25: LGTM—proper error handling for response marshaling and writing.The early returns with explanatory comments appropriately handle marshal and write errors. Since headers are already sent (line 14), returning silently is the correct approach.
cmd/hyperfleet-api/main.go (1)
28-30: LGTM—proper error handling for flag parsing.Checking the parse error and failing fast with a clear message prevents silent initialization failures.
pkg/auth/helpers.go (1)
30-38: LGTM—consistent error handling pattern.The error checks for marshal and write operations follow the same pattern as other response utilities in this PR, with appropriate early returns after headers are sent.
pkg/services/status_aggregation.go (1)
155-163: LGTM—more idiomatic with explicit handling of unexpected statuses.The switch statement is clearer than the if-else chain, and the default case with logging improves robustness by explicitly handling unexpected adapter status values.
pkg/handlers/cluster.go (1)
72-76: LGTM—proper error handling for spec marshaling.The marshal error check prevents silent failures and uses the structured
ServiceErrortype to propagate a descriptive error to the client.pkg/handlers/framework.go (1)
109-114: LGTM—more idiomatic switch usage.Switching directly on
serviceErris clearer and more idiomatic than using boolean conditions in the cases.pkg/handlers/node_pool.go (1)
73-77: Good defensive error handling added.The error handling for
json.Marshalis a solid defensive improvement. While marshaling well-formed structs rarely fails, this change properly captures and surfaces any potential issues (e.g., cyclic references or unsupported types) using the structuredGeneralErrorpattern.test/integration/clusters_test.go (1)
66-75: Good: now asserting client-level success before checking for 400 Bad RequestAdding
Expect(err).ToNot(HaveOccurred())on Line 73 ensures transport/configuration issues from therestycall don’t get mistaken for an expected 400 response and will fail the test loudly instead. This is a solid tightening of the assertion around the bad‑JSON path.pkg/api/adapter_status_types.go (2)
131-137: Excellent fix for the scope bug from past review.The pattern used here—default fallback with conditional override—cleanly resolves the previous issue where
errwas checked outside its assignment scope. The implementation matches the suggestion from past reviews and correctly handles the optionalDatafield.
140-146: Good implementation of the suggested fallback pattern.The safe fallback approach for
Metadatamarshaling is consistent with theDatahandling and implements the pattern suggested in past reviews. This ensures valid JSON is always stored, even when marshaling fails..bingo/golangci-lint.mod (1)
3-5: golangci-lint v2 module path and Go directive look correct.The
go 1.24.0directive and thegithub.com/golangci/golangci-lint/v2 v2.7.0requirement are consistent with the v2 module layout and the updated Variables.mk wiring; this auto-generated Bingo module is coherent with the new tooling setup.Please just confirm locally that
make lintis invoking this v2.7.0 binary as expected in your environment..golangci.yml (1)
1-4: Minimal GolangCI-Lint v2 config looks appropriate.Setting
version: 2and excludingpkg/api/openapias generated code is a sensible baseline for the upgraded linter and matches the intent to avoid linting generated OpenAPI models.Please run
make lintonce more to confirm golangci-lint v2 picks up this config and correctly skipspkg/api/openapi..bingo/Variables.mk (1)
26-30: GolangCI-Lint v2.7.0 Bingo wiring is consistent and correct.The
GOLANGCI_LINTbinary name, echo message, andgo buildtarget all consistently referencegolangci-lint-v2.7.0and thegithub.com/golangci/golangci-lint/v2/cmd/golangci-lintmodule path, so the upgraded toolchain should install and run cleanly.After running
make vendor-bingo/make lintin CI or locally, verify that the reported golangci-lint version isv2.7.0.test/helper.go (3)
180-185: RestartServer’s softer stop error handling looks reasonableLogging a warning instead of fatalling on
stopAPIServererrors is fine in test helpers, and subsequentstartAPIServer()will still fail fast if the listener cannot be bound, preserving clear failures on real issues.
441-445: Nolint/staticcheck and comments around JWT key parsing are appropriate for testsGiven this is test-only code and you’ve documented why the deprecated helper is acceptable here, the
//nolint:staticcheckand explanatory comments are a reasonable way to keep static analysis clean without reworking the fixtures.
200-208: NewFlagSetin Reset is never parsed; CLI flags on reset are effectively ignoredThe added fatal on
env.AddFlags(flagset)is good, but note that you add flags to a freshFlagSetand then callpflag.Parse()on the globalpflag.CommandLine. The newflagsetis never parsed, so any flags added there won't influence configuration duringReset.If the intent is to re-read CLI flags on reset without redefining glog flags, consider either:
- Adding flags to the global set again, or
- Parsing the new
flagsetexplicitly and using it consistently.For example, if you're okay reusing the global set:
- // Also on reset, we don't care to be re-defining 'v' and other glog flags - flagset := pflag.NewFlagSet(helper.NewID(), pflag.ContinueOnError) - if err := env.AddFlags(flagset); err != nil { + // Also on reset, we don't care to be re-defining 'v' and other glog flags + flagset := pflag.CommandLine + if err := env.AddFlags(flagset); err != nil { glog.Fatalf("Unable to add environment flags on Reset: %s", err.Error()) } pflag.Parse()
yingzhanredhat
force-pushed
the
ying-test
branch
from
95f28bb to
ef90317
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/api/node_pool_types.go (1)
159-163: Treat marshal failures as hard errors rather than silently defaultingRight now, if
json.Marshalfails forSpec,Labels, orStatusConditions, you silently store"{}"or"[]". That can hide bad user input and persist data that doesn’t reflect what was requested.Consider changing
NodePoolFromOpenAPICreateto return an error and fail fast on marshal issues:-func NodePoolFromOpenAPICreate(req *openapi.NodePoolCreateRequest, ownerID, createdBy string) *NodePool { +func NodePoolFromOpenAPICreate(req *openapi.NodePoolCreateRequest, ownerID, createdBy string) (*NodePool, error) { // Marshal Spec specJSON, err := json.Marshal(req.Spec) - if err != nil { - //logger.Errorf("Failed to marshal NodePool spec: %v", err) - specJSON = []byte("{}") - } + if err != nil { + return nil, fmt.Errorf("failed to marshal NodePool spec: %w", err) + } @@ - labelsJSON, err := json.Marshal(labels) - if err != nil { - labelsJSON = []byte("{}") - } + labelsJSON, err := json.Marshal(labels) + if err != nil { + return nil, fmt.Errorf("failed to marshal NodePool labels: %w", err) + } @@ - statusConditionsJSON, err := json.Marshal([]openapi.ResourceCondition{}) - if err != nil { - statusConditionsJSON = []byte("[]") - } + statusConditionsJSON, err := json.Marshal([]openapi.ResourceCondition{}) + if err != nil { + return nil, fmt.Errorf("failed to marshal NodePool status conditions: %w", err) + } @@ - return &NodePool{ + return &NodePool{ @@ - } + }, nilCallers can then translate these errors into appropriate HTTP 4xx responses. If changing the signature is too invasive right now, at least log the marshal errors instead of silently masking them.
Also applies to: 170-173, 176-179
🧹 Nitpick comments (4)
pkg/db/db_session/default.go (1)
100-116: Error handling onPinglooks good; consider context and log level as optional improvementsThe new
if err := l.Ping(); err != nil { ... }guard is correct and avoids silent failures fromPing, 👍.Two optional refinements you might consider:
Context‑aware shutdown:
waitForNotificationcurrently loops only onl.Notifyandtime.After, whilenewListenertakes actxbut never checksctx.Done(). If you ever need to stop the listener cleanly (e.g., on server shutdown), it may be worth threadingctxintowaitForNotificationand adding acase <-ctx.Done(): returnbranch.Log severity for ping failures: A failed
Pingusually indicates a connectivity or backend problem. If this signal is important operationally, you may want to log it at a higher level thanV(10)(e.g., a warning/error logger call) so it’s not lost in very‑verbose logs.test/integration/node_pools_test.go (1)
73-82: Good addition to assert no client/network error; consider reordering expectationsThe new
Expect(err).NotTo(HaveOccurred(), ...)is useful to ensure only the server’s 400 response is under test, not a client/network failure. For extra safety and clarity, you might want to assert onerrbefore callingrestyResp.StatusCode()so the test can’t accidentally dereference a nil response if Resty’s behavior changes or a transport error occurs in the future.pkg/services/node_pool.go (1)
141-142: Good variable naming to avoid shadowing.Renaming to
unmarshalErrprevents shadowing the outererrvariable and makes the error handling clearer.However, consider logging unmarshal failures at Warning level to surface potential data corruption or schema mismatches in adapter status conditions, rather than silently skipping adapters.
Apply this diff to add logging:
if unmarshalErr := json.Unmarshal(adapterStatus.Conditions, &conditions); unmarshalErr != nil { + logger.NewOCMLogger(ctx).Extra("adapter_status_id", adapterStatus.ID).Extra("error", unmarshalErr).Warning("Failed to unmarshal adapter conditions, skipping") continue // Skip if can't unmarshal }As per coding guidelines, use structured logging via pkg/logger/logger.go with context propagation.
pkg/auth/context.go (1)
80-127: JWT claim extraction is now safely typed; minor name parsing nitThe guarded
stringassertions for all claims plus the fallback logic from alternative keys andnamemakeGetAuthPayloadFromContextmuch more robust against malformed tokens.If you ever touch this again, consider
strings.Fields(name)instead ofstrings.Split(name, " ")so multiple spaces or leading/trailing whitespace don’t produce empty elements, but current behavior is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.bingo/golangci-lint.sumis excluded by!**/*.sum
📒 Files selected for processing (46)
.bingo/Variables.mk(1 hunks).bingo/golangci-lint.mod(1 hunks).golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/framework_test.go(0 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/handlers/validation.go(0 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(2 hunks)pkg/services/types.go(1 hunks)test/helper.go(4 hunks)test/integration/clusters_test.go(2 hunks)test/integration/node_pools_test.go(1 hunks)test/mocks/jwk_cert_server.go(1 hunks)test/mocks/ocm.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/handlers/validation.go
- pkg/handlers/framework_test.go
🚧 Files skipped from review as they are similar to previous changes (28)
- test/integration/clusters_test.go
- cmd/hyperfleet-api/server/logging/request_logging_middleware.go
- pkg/services/status_aggregation.go
- pkg/api/presenters/slice_filter.go
- cmd/hyperfleet-api/server/api_server.go
- pkg/handlers/helpers.go
- pkg/handlers/openapi.go
- pkg/handlers/framework.go
- pkg/api/cluster_types.go
- pkg/services/cluster.go
- pkg/db/transaction_middleware.go
- cmd/hyperfleet-api/main.go
- Makefile
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/handlers/cluster_status.go
- pkg/handlers/cluster.go
- pkg/services/types.go
- pkg/handlers/node_pool.go
- cmd/hyperfleet-api/server/routes.go
- pkg/api/adapter_status_types.go
- test/mocks/jwk_cert_server.go
- pkg/auth/helpers.go
- test/helper.go
- pkg/db/db_session/test.go
- .bingo/Variables.mk
- cmd/hyperfleet-api/server/logging/writer.go
- .bingo/golangci-lint.mod
- Dockerfile.openapi
🧰 Additional context used
📓 Path-based instructions (9)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
pkg/config/config_test.gopkg/db/sql_helpers.gopkg/services/node_pool.gopkg/db/db_session/default.gotest/integration/node_pools_test.gopkg/client/ocm/client.gopkg/auth/context.gopkg/api/node_pool_types.gopkg/handlers/nodepool_status.gopkg/services/generic_test.gopkg/services/generic.gocmd/hyperfleet-api/environments/framework.gotest/mocks/ocm.gopkg/db/db_session/testcontainer.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/config/config_test.gopkg/db/sql_helpers.gopkg/services/node_pool.gopkg/db/db_session/default.gopkg/client/ocm/client.gopkg/auth/context.gopkg/api/node_pool_types.gopkg/handlers/nodepool_status.gopkg/services/generic_test.gopkg/services/generic.gopkg/db/db_session/testcontainer.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/sql_helpers.gopkg/db/db_session/default.gopkg/db/db_session/testcontainer.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/services/node_pool.gopkg/handlers/nodepool_status.gopkg/services/generic_test.gopkg/services/generic.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/services/node_pool.gopkg/handlers/nodepool_status.gopkg/services/generic_test.gopkg/services/generic.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/node_pool.gopkg/services/generic_test.gopkg/services/generic.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/node_pools_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/node_pool_types.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Files:
pkg/handlers/nodepool_status.go
🧠 Learnings (15)
📚 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/services/**/*.go : Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Applied to files:
pkg/services/node_pool.gopkg/handlers/nodepool_status.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/{handlers,services}/**/*.go : Always retrieve database session from context via db.NewContext() instead of creating new connections
Applied to files:
pkg/db/db_session/default.gocmd/hyperfleet-api/environments/framework.gopkg/db/db_session/testcontainer.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/node_pools_test.gopkg/api/node_pool_types.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/node_pools_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:
pkg/auth/context.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/dao/**/*.go : Use context.Context as the first parameter in all DAO methods for transaction propagation
Applied to files:
pkg/auth/context.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:
pkg/api/node_pool_types.gopkg/handlers/nodepool_status.go.golangci.yml
📚 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/openapi/**/*.go : Use make generate to regenerate Go models from openapi/openapi.yaml via openapi-generator-cli v7.16.0 in Podman
Applied to files:
pkg/api/node_pool_types.go.golangci.ymlopenapitools.json
📚 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,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
pkg/handlers/nodepool_status.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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/handlers/nodepool_status.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/handlers/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/nodepool_status.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 **/*.go : Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Applied to files:
.golangci.yml
📚 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 : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/services/generic_test.gopkg/db/db_session/testcontainer.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:
cmd/hyperfleet-api/environments/framework.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/**/*.go : Use PostgreSQL 14.2 with GORM ORM for all database operations
Applied to files:
pkg/db/db_session/testcontainer.go
🧬 Code graph analysis (4)
pkg/db/sql_helpers.go (1)
pkg/errors/errors.go (1)
BadRequest(196-198)
pkg/handlers/nodepool_status.go (1)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)
pkg/services/generic.go (2)
pkg/errors/errors.go (2)
ServiceError(94-101)BadRequest(196-198)pkg/db/sql_helpers.go (1)
FieldNameWalk(108-199)
cmd/hyperfleet-api/environments/framework.go (1)
cmd/hyperfleet-api/environments/types.go (1)
Clients(67-69)
🔇 Additional comments (12)
test/mocks/ocm.go (1)
62-65: Pointer receiver onResetcorrectly clears mock stateSwitching
Resetto a pointer receiver aligns it with the other methods and ensuresAction/ResourceTypeare actually cleared on the shared mock instance instead of a copy. This is the correct behavior for a reusable test mock.pkg/handlers/nodepool_status.go (1)
92-95: Excellent error handling improvement.The inline error handling with structured logging properly captures aggregation failures while maintaining request success for eventual consistency. The logging includes appropriate context (nodepool_id, error) and uses the correct Warning level.
As per coding guidelines, this follows the structured logging pattern via pkg/logger/logger.go with context propagation.
pkg/client/ocm/client.go (1)
78-82: Close now safely propagates connection errorsThe nil guard plus returning
c.connection.Close()is correct and lets callers handle teardown failures explicitly. No further changes needed here.cmd/hyperfleet-api/environments/framework.go (1)
158-162: Teardown OCM close is now nil‑safe and loggedGuarding
e.Clients.OCMbefore callingClose()and logging any error mirrors the DB teardown pattern and prevents possible panics during shutdown.pkg/auth/context.go (1)
40-49: Defensive username lookup avoids bad type panicsAdding the nil check and guarded
stringassertion ensuresGetUsernameFromContextreturns""instead of panicking when the value is missing or of an unexpected type.pkg/api/node_pool_types.go (1)
82-84: Resilient JSON unmarshal in ToOpenAPIFalling back to
{}forSpec/Labelsand[]forStatusConditionswhenjson.Unmarshalfails prevents panics and still produces a sane OpenAPI representation even if the stored JSON is malformed.Also applies to: 90-92, 98-100
.golangci.yml (1)
1-4: Verify minimal golangci-lint v2 configuration is complete and functional.The configuration includes
version: 2andexclude-dirsfor the generated code directory. According to golangci-lint v2 documentation, this minimal configuration will use the default "standard" linters set. However, verification of the following requires direct repository access and cannot be completed:
- Confirm the file is complete (no additional configuration beyond line 4)
- Verify
golangci-lint runexecutes successfully with this config- Validate that the default "standard" linters are appropriate for the codebase
- Confirm
pkg/api/openapiis properly excluded from lintingNote: The prior concerns about staticcheck being disabled and redundant exclude-files entries are no longer present, aligning with the PR objective.
openapitools.json (1)
5-5: Confirm OpenAPI generator output compatibility after v7.16.0 upgrade.The version upgrade from 5.4.0 to 7.16.0 introduces significant breaking changes in code generation including model naming, enum handling, allOf/inline model resolution, and OpenAPI 3.1 / JSON Schema semantics. Before merging, verify that:
- Generated code in
pkg/api/openapiwas regenerated with v7.16.0- Generated Go code integrates correctly with downstream usage
- No unexpected type or API definition changes broke existing code paths
pkg/db/sql_helpers.go (2)
20-34: LGTM! Defensive type assertion prevents panic.The added type assertion on
l.Left(line 28) safely guards against non-string values and prevents potential runtime panics.
144-154: LGTM! Proper type validation prevents panic.The type assertion check on
n.Left(lines 145-149) prevents potential panics from invalid node structures and returns an appropriate user-facing error. This is consistent with the defensive patterns used elsewhere in this function.pkg/services/generic_test.go (1)
22-24: Updated Close() nolint and treeWalkForSqlizer usage look correctUsing
//nolint:errcheckon the deferreddbFactory.Close()is reasonable for test cleanup, and the updatedtreeWalkForSqlizercall correctly targets the new(Sqlizer, *ServiceError)signature while preserving the existing assertions on the error and the generated SQL.Also applies to: 68-72
pkg/services/generic.go (1)
45-45: treeWalkForSqlizer signature change and call-site update are consistentThe refactor to have
treeWalkForSqlizerreturn only(squirrel.Sqlizer, *errors.ServiceError)aligns with how it’s used inbuildSearchValues(the transformed tree isn’t needed), and the updated call-site correctly handles the new two-value return. Field-name validation viadb.FieldNameWalkand the SQL filter construction now both surface*errors.ServiceError, keeping error propagation consistent with the service layer’s conventions.Also applies to: 153-188, 334-348
| stringFile, err := createConfigFile("string", "example\n") | ||
| defer os.Remove(stringFile.Name()) | ||
| defer os.Remove(stringFile.Name()) //nolint:errcheck | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Defer cleanup after checking for file creation errors
createConfigFile can return (nil, err) (e.g., if os.CreateTemp fails), but Name() is called in the defer before checking err, which would panic in that case. The new //nolint:errcheck is fine for ignoring best-effort cleanup, but it’d be safer to move the defer after the error check in all four tests.
For example:
- stringFile, err := createConfigFile("string", "example\n")
- defer os.Remove(stringFile.Name()) //nolint:errcheck
- if err != nil {
- log.Fatal(err)
- }
+ stringFile, err := createConfigFile("string", "example\n")
+ if err != nil {
+ log.Fatal(err)
+ }
+ defer os.Remove(stringFile.Name()) //nolint:errcheck(and similarly for the int/bool/quoted tests).
Also applies to: 29-33, 44-48, 59-63
🤖 Prompt for AI Agents
In pkg/config/config_test.go around lines 14-18 (and similarly at 29-33, 44-48,
59-63), the test calls createConfigFile and immediately defers
os.Remove(stringFile.Name()) before verifying err, which can panic if
createConfigFile returned nil and an error; move each defer
os.Remove(file.Name()) to after the subsequent if err != nil check so you only
defer cleanup when file creation succeeded (keep the //nolint:errcheck if
desired).
| testcontainers.WithWaitStrategy( | ||
| wait.ForLog("database system is ready to accept connections"). | ||
| WithOccurrence(2). | ||
| wait.ForListeningPort("5432/tcp"). | ||
| WithStartupTimeout(60*time.Second)), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
testcontainers-go PostgreSQL ForListeningPort wait strategy best practices
💡 Result:
Short best-practices for using wait.ForListeningPort with PostgreSQL in testcontainers-go
-
Prefer a combined wait strategy (logs + listening port) instead of only ForListeningPort. Example recommended by the postgres module: wait for the Postgres "ready" log (often with WithOccurrence(2) for Postgres self‑restart) then wait.ForListeningPort("5432/tcp"). This reduces flakiness across platforms. [1]
-
If you need to ensure DB is actually usable (not just a bound port), add wait.ForSQL (run a simple SELECT 1) after the listening-port check. That verifies the server accepts queries. [2][3]
-
Increase startup timeout and poll interval for slow CI/containers. Defaults are 60s timeout / 100ms poll; set WithStartupTimeout(...) when needed. Combined strategies can set per-strategy timeouts or an overall deadline (wait.ForAll / ForAll.WithDeadline). [4][2]
-
On macOS/Windows (or Docker Desktop/Rancher Desktop) explicitly wait for the mapped host port — ForListeningPort checks the container side but you should also ensure the host-side mapping is ready (the postgres module example calls out this proxy issue). [1][5]
-
Avoid time.Sleep-based waits. Prefer deterministic checks (log, listening port, SQL). [4]
-
For compose-based runs, ensure you reference the correct service/container name when waiting for a service; otherwise ForListeningPort may target the wrong container. (Give unique container_name if necessary.) [6]
Concise example (pattern):
- wait.ForLog("database system is ready to accept connections").WithOccurrence(2)
- wait.ForListeningPort("5432/tcp").WithStartupTimeout(120*time.Second)
- wait.ForSQL(nat.Port("5432/tcp"), "postgres", dbURL).WithQuery("SELECT 1").WithStartupTimeout(30*time.Second)
Sources:
[1] testcontainers-go Postgres module docs (wait strategy examples).
[2] testcontainers-go wait/host_port docs (ForListeningPort behavior/options).
[3] testcontainers-go wait/sql docs (ForSQL usage).
[4] testcontainers-go wait/introduction & multi (timeouts, combining strategies).
[5] testcontainers-go networking / postgres module note about host proxy on non‑Linux OSes.
[6] testcontainers-go GitHub / compose guidance (unique container_name for waits).
🏁 Script executed:
cat -n pkg/db/db_session/testcontainer.go | head -120Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 214
Incomplete wait strategy—combine log, port, and SQL verification for reliability.
Using only ForListeningPort does not follow testcontainers-go best practices for PostgreSQL. The recommended pattern combines three checks:
- Log-based detection (
wait.ForLog("database system is ready to accept connections").WithOccurrence(2)) — catches PostgreSQL initialization - Port listening (
wait.ForListeningPort("5432/tcp")) — confirms the port is bound - SQL verification (
wait.ForSQL(...)with aSELECT 1query) — ensures the database actually accepts connections
Port listening alone is insufficient because a bound port does not guarantee the database is accepting queries. Add the log-based check and SQL verification to prevent flaky tests across different platforms and CI environments. Also consider increasing the timeout beyond 60s for slower CI environments.
yingzhanredhat
force-pushed
the
ying-test
branch
from
ef90317 to
b550730
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
cmd/hyperfleet-api/server/routes.go (1)
54-59: Commented authz init is now syntactically correct; tie TODO to a concrete tracking issue.The previously noted
errscoping bug in the commented block is fixed, and the snippet will compile when uncommented. Since this block governs real authorization, consider tightening the TODO to reference the actual Jira (e.g.,// TODO(HYPERFLEET-XXX): enable authorization middleware) so it’s easy to track and enforce before any production rollout.pkg/db/transaction_middleware.go (2)
44-48: Past review comment not addressed: implement the logging mentioned in the comment.The comment on line 46 states "Log error but don't expose to client" but no logging is actually performed. A previous review already flagged this issue and suggested using the
loggerpackage (which is imported) to log the marshaling error. This prevents debugging issues when JSON marshaling fails silently.As suggested in the previous review, add context propagation and logging:
-func writeJSONResponse(w http.ResponseWriter, code int, payload interface{}) { +func writeJSONResponse(w http.ResponseWriter, code int, payload interface{}, ctx context.Context) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(code) if payload != nil { response, err := json.Marshal(payload) if err != nil { - // Log error but don't expose to client since headers already sent + logger.NewOCMLogger(ctx).Errorf("Failed to marshal JSON response: %v", err) return }Then update the call site at line 23 to pass
ctx.
49-52: Add logging for response write failures.While nothing can be sent to the client after headers are written, the write error should still be logged for debugging and observability purposes. As per coding guidelines, use structured logging with context propagation.
Apply this diff (assuming
ctxis added to the function signature as suggested above):if _, err := w.Write(response); err != nil { - // Response writing failed, nothing we can do at this point + logger.NewOCMLogger(ctx).Warningf("Failed to write response: %v", err) return }Based on coding guidelines, structured logging should be used throughout pkg/**/*.go.
test/helper.go (1)
83-84: Fix the flag name: use"v"instead of"-v".The
pflag.FlagSet.Setmethod expects the flag name without a leading dash. Using"-v"causes the call to fail, so theLOGLEVELenvironment variable never adjusts verbosity.Apply this diff:
- // Intentionally ignore error from Set — acceptable for tests - _ = pflag.CommandLine.Set("-v", logLevel) + if err := pflag.CommandLine.Set("v", logLevel); err != nil { + glog.Warningf("Unable to set loglevel from LOGLEVEL=%s: %v", logLevel, err) + }
🧹 Nitpick comments (2)
pkg/handlers/nodepool_status.go (1)
11-11: Aggregation error logging and handler flow look correctUsing
logger.NewOCMLogger(ctx)and loggingnodepool_idplus the aggregation error while keeping the request successful preserves the intended “best‑effort aggregation” behavior and aligns with the structured logging guidance for handlers. The overall flow (parse request → service calls → handle errors → send response) remains intact, and the non‑fatal aggregation failure is now observable without impacting clients.If you want to enrich observability further, you could consider adding the adapter name (from
req.Adapter) into the log context so issues can be correlated to specific adapters when debugging.Based on learnings, this aligns with the structured logging requirements for handlers.
Also applies to: 92-95
pkg/auth/context.go (1)
98-115: Consider making name fallbacks more robust and less overwritingThe guarded fallbacks (
preferred_username,given_name,family_name) are good hardening. For thename-based fallback, there are a couple of small refinements you might consider (not required for this PR):
- The comment says “If given and family names are not present,” but the condition is
if payload.FirstName == "" || payload.LastName == "", which can overwrite an already-populated first or last name.strings.Split(name, " ")is brittle for multiple or irregular spaces and multi-word names.If you want to tighten this up later, something along these lines would avoid overwriting existing values and handle spacing better:
- if payload.FirstName == "" || payload.LastName == "" { - if name, ok := claims["name"].(string); ok { - names := strings.Split(name, " ") - if len(names) > 1 { - payload.FirstName = names[0] - payload.LastName = names[1] - } else { - payload.FirstName = names[0] - } - } - } + if (payload.FirstName == "" || payload.LastName == "") { + if name, ok := claims["name"].(string); ok { + parts := strings.Fields(name) + if len(parts) > 0 && payload.FirstName == "" { + payload.FirstName = parts[0] + } + if len(parts) > 1 && payload.LastName == "" { + // Use the last part as surname to better support multi-word names + payload.LastName = parts[len(parts)-1] + } + } + }Also applies to: 117-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.bingo/golangci-lint.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (47)
.bingo/Variables.mk(1 hunks).bingo/golangci-lint.mod(1 hunks).golangci.yml(1 hunks)Dockerfile.openapi(1 hunks)Makefile(1 hunks)cmd/hyperfleet-api/environments/framework.go(1 hunks)cmd/hyperfleet-api/environments/framework_test.go(1 hunks)cmd/hyperfleet-api/main.go(1 hunks)cmd/hyperfleet-api/server/api_server.go(1 hunks)cmd/hyperfleet-api/server/logging/request_logging_middleware.go(1 hunks)cmd/hyperfleet-api/server/logging/writer.go(1 hunks)cmd/hyperfleet-api/server/routes.go(1 hunks)go.mod(2 hunks)openapitools.json(1 hunks)pkg/api/adapter_status_types.go(2 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/node_pool_types.go(2 hunks)pkg/api/presenters/slice_filter.go(3 hunks)pkg/auth/context.go(2 hunks)pkg/auth/helpers.go(1 hunks)pkg/client/ocm/client.go(1 hunks)pkg/config/config_test.go(4 hunks)pkg/db/db_session/default.go(1 hunks)pkg/db/db_session/test.go(1 hunks)pkg/db/db_session/testcontainer.go(1 hunks)pkg/db/sql_helpers.go(4 hunks)pkg/db/transaction_middleware.go(1 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_status.go(2 hunks)pkg/handlers/framework.go(1 hunks)pkg/handlers/framework_test.go(0 hunks)pkg/handlers/helpers.go(1 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(2 hunks)pkg/handlers/openapi.go(1 hunks)pkg/handlers/validation.go(0 hunks)pkg/services/cluster.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(2 hunks)pkg/services/node_pool.go(1 hunks)pkg/services/status_aggregation.go(2 hunks)pkg/services/types.go(1 hunks)test/helper.go(4 hunks)test/integration/clusters_test.go(2 hunks)test/integration/node_pools_test.go(1 hunks)test/mocks/jwk_cert_server.go(1 hunks)test/mocks/ocm.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/handlers/framework_test.go
- pkg/handlers/validation.go
✅ Files skipped from review due to trivial changes (1)
- pkg/services/status_aggregation.go
🚧 Files skipped from review as they are similar to previous changes (26)
- pkg/config/config_test.go
- cmd/hyperfleet-api/environments/framework.go
- test/integration/node_pools_test.go
- pkg/services/cluster.go
- pkg/handlers/helpers.go
- .golangci.yml
- pkg/handlers/openapi.go
- test/integration/clusters_test.go
- pkg/api/node_pool_types.go
- pkg/api/cluster_types.go
- pkg/auth/helpers.go
- Dockerfile.openapi
- pkg/db/db_session/testcontainer.go
- .bingo/golangci-lint.mod
- openapitools.json
- pkg/services/node_pool.go
- pkg/api/adapter_status_types.go
- Makefile
- .bingo/Variables.mk
- pkg/services/types.go
- pkg/api/presenters/slice_filter.go
- pkg/client/ocm/client.go
- test/mocks/jwk_cert_server.go
- cmd/hyperfleet-api/environments/framework_test.go
- pkg/handlers/cluster_status.go
- cmd/hyperfleet-api/server/logging/writer.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
pkg/db/db_session/default.gopkg/db/transaction_middleware.gopkg/handlers/framework.gotest/helper.gopkg/handlers/node_pool.gopkg/services/generic_test.gopkg/handlers/cluster.gocmd/hyperfleet-api/main.gopkg/auth/context.gopkg/db/db_session/test.gopkg/handlers/nodepool_status.gotest/mocks/ocm.gocmd/hyperfleet-api/server/logging/request_logging_middleware.gopkg/services/generic.gocmd/hyperfleet-api/server/api_server.gocmd/hyperfleet-api/server/routes.gopkg/db/sql_helpers.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/db_session/default.gopkg/db/transaction_middleware.gopkg/db/db_session/test.gopkg/db/sql_helpers.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/db/db_session/default.gopkg/db/transaction_middleware.gopkg/handlers/framework.gopkg/handlers/node_pool.gopkg/services/generic_test.gopkg/handlers/cluster.gopkg/auth/context.gopkg/db/db_session/test.gopkg/handlers/nodepool_status.gopkg/services/generic.gopkg/db/sql_helpers.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/handlers/framework.gopkg/handlers/node_pool.gopkg/services/generic_test.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.gopkg/services/generic.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Files:
pkg/handlers/framework.gopkg/handlers/node_pool.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/handlers/framework.gopkg/handlers/node_pool.gopkg/services/generic_test.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.gopkg/services/generic.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/generic_test.gopkg/services/generic.go
🧠 Learnings (13)
📓 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 pkg/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
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,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
📚 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,services}/**/*.go : Always retrieve database session from context via db.NewContext() instead of creating new connections
Applied to files:
pkg/db/db_session/default.gopkg/db/db_session/test.gocmd/hyperfleet-api/server/api_server.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:
pkg/db/transaction_middleware.gopkg/auth/context.gocmd/hyperfleet-api/server/routes.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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/db/transaction_middleware.gopkg/handlers/framework.gopkg/handlers/nodepool_status.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/{handlers,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
pkg/db/transaction_middleware.gopkg/handlers/nodepool_status.gocmd/hyperfleet-api/server/logging/request_logging_middleware.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/handlers/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/framework.gopkg/handlers/nodepool_status.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:
pkg/handlers/node_pool.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 : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/services/generic_test.gogo.mod
📚 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/dao/**/*.go : Increment generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/handlers/cluster.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/dao/**/*.go : Use context.Context as the first parameter in all DAO methods for transaction propagation
Applied to files:
pkg/auth/context.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/services/**/*.go : Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Applied to files:
pkg/handlers/nodepool_status.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:
pkg/handlers/nodepool_status.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/**/*.go : Use PostgreSQL 14.2 with GORM ORM for all database operations
Applied to files:
go.mod
🧬 Code graph analysis (2)
pkg/handlers/node_pool.go (1)
pkg/errors/errors.go (1)
GeneralError(164-166)
cmd/hyperfleet-api/server/api_server.go (2)
cmd/hyperfleet-api/environments/types.go (1)
Database(36-38)pkg/db/session.go (1)
SessionFactory(12-20)
🔇 Additional comments (25)
cmd/hyperfleet-api/server/logging/request_logging_middleware.go (1)
14-14: Simplified logging condition keeps behavior and improves clarityUsing
doLog := path != "/api/hyperfleet"preserves the existing behavior (only that endpoint is skipped) while making the intent clearer and removing redundant branching. No issues spotted.go.mod (3)
3-5: Verify Go 1.24.0 compatibility with all dependencies.The Go version is being updated to 1.24.0 (with toolchain 1.24.9), which is quite recent. The PR notes mention "unexpected Go version (go1.24)" warnings during the build. Before merging, ensure that all direct and indirect dependencies are compatible with these Go versions and that the version bump is intentional.
12-12: LGTM!Adding
github.com/docker/go-connections v0.6.0as a direct dependency is appropriate given the integration test updates that use Testcontainers for PostgreSQL (as indicated in the PR objectives and learnings).
33-33: LGTM!Adding
go.uber.org/mock v0.6.0as a direct dependency aligns with the test mock updates mentioned in the AI summary and supports the shift from active authz initialization to mocking in tests.test/mocks/ocm.go (1)
62-65: LGTM! Correct receiver type for mutating method.Changing Reset to use a pointer receiver ensures the method actually modifies the mock's state. With a value receiver, Reset would have operated on a copy, leaving the original
ActionandResourceTypefields unchanged. This change aligns with the pointer receivers used bySelfAccessReviewandAccessReview, maintaining consistency across the mock.pkg/handlers/cluster.go (1)
72-76: LGTM! Defensive error handling for spec marshaling.The addition of explicit error handling for
json.Marshalis good defensive programming. Usingerrors.GeneralError()follows the project's structured error pattern.pkg/handlers/node_pool.go (1)
73-77: LGTM! Consistent defensive error handling.The addition of explicit error handling for
json.Marshalmatches the pattern incluster.goand provides appropriate defensive programming. The structured error response aligns with project conventions.pkg/auth/context.go (2)
40-49: Safe username extraction from context looks goodGuarding the type assertion with
if str, ok := username.(string); okremoves the panic risk from unexpected context values while preserving the empty-string fallback. No further changes needed here.
80-96: Defensive JWT claim extraction is correctUsing guarded string assertions for all primary claims (
username,first_name,last_name,clientId) is a solid hardening step: it avoids panics on malformed tokens and cleanly integrates with the existing “empty means try fallbacks” logic.pkg/services/generic.go (3)
45-58: listContext remains a cohesive LIST pipeline containerThe expanded
listContextstill cleanly centralizes all state needed across LIST builders (context, paging, logging, joins, etc.); the updated comment accurately reflects that role and doesn’t introduce any behavioral change.
153-188: buildSearchValues correctly adapts to the new treeWalkForSqlizer signatureThe updated call to
treeWalkForSqlizerand subsequent error handling preserve the previous control flow: on error you return early with theServiceError, and on successsqlizer.ToSql()is invoked exactly as before. There’s no new path where a nilsqlizeris used without an accompanying error.
334-348: treeWalkForSqlizer’s simplified return type is correct and keeps error semantics intactThe refactored
treeWalkForSqlizer:
- Validates field names via
db.FieldNameWalkusing*listCtx.disallowedFieldsand returns theServiceErrorunchanged.- Converts the resulting tree with
sqlFilter.Walkand wraps failures inerrors.BadRequest.This matches the previous behavior while simplifying the API to return only the
squirrel.Sqlizerplus*errors.ServiceError, and continues to use the structuredServiceErrortype as required for services. As per coding guidelines, this keeps error propagation consistent inpkg/services.pkg/services/generic_test.go (2)
20-26: Deferred dbFactory.Close improves test resource cleanupAdding
defer dbFactory.Close() //nolint:errcheckensures the mock session factory is closed after the test run without complicating assertions, which is appropriate in this test context.
62-72: Tests correctly updated for the new treeWalkForSqlizer return valuesThe test now consumes
(sqlizer, serviceErr)directly fromtreeWalkForSqlizer, aligning with the new signature while preserving the existing expectations on generated SQL and parameters. This keeps coverage for the happy-path translation intact.cmd/hyperfleet-api/server/api_server.go (1)
149-151: LGTM!Explicit error handling during shutdown is good practice. Logging the Close error ensures visibility into potential connection pool cleanup issues.
pkg/db/db_session/test.go (1)
217-219: LGTM!The panic-on-failure approach ensures tests cannot continue with an inconsistent database state. This properly addresses the previous review concern about silently ignoring reset errors.
pkg/db/db_session/default.go (1)
111-113: LGTM!Adding error handling for the Ping call prevents silently ignoring connection issues in the background goroutine. The V(10) log level is appropriate for verbose diagnostics.
pkg/handlers/framework.go (1)
109-110: LGTM!Switching on the
serviceErrvalue instead of using boolean cases within a valueless switch is cleaner and more idiomatic.cmd/hyperfleet-api/main.go (1)
28-30: LGTM!Explicit error handling for flag parsing prevents silent failures during initialization. The fatal log ensures visibility if parsing fails.
pkg/db/sql_helpers.go (3)
28-29: LGTM!The type assertion with error checking prevents a potential panic if
l.Leftis not a string. The short-circuit evaluation ensuresleftStris only used when the type assertion succeeds.
83-87: LGTM!The corrected variable name (
propertyName) and tightened length validation (<= 11) prevent generating SQL with an empty property accessor likeproperties ->> ''.
145-150: LGTM!Type assertion validation before the recursive call prevents a runtime panic if
n.Leftis not atsl.Node, and returns a clear error to the caller instead.test/helper.go (3)
181-183: LGTM!Logging a warning on stop failure allows the restart to proceed while still surfacing the issue. This is appropriate for test harness robustness.
204-206: LGTM!Fatal logging on
AddFlagsfailure ensures test initialization errors are visible and block execution, preventing tests from running with incomplete configuration.
441-444: LGTM!Documenting why the deprecated function is still used and suppressing the staticcheck warning with
//nolint:staticcheckis appropriate for test fixtures with legacy key formats.
|
/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 c962755
into
openshift-hyperfleet:main
4 participants
pkg/db/db_session/testcontainer.go: fix the test-integration error
Makefile: Prow only supports podman tool, it will meet error when run "make generate".So add -o APT::Sandbox::User=root to fix it
Summary by CodeRabbit
Bug Fixes
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.