Implement Schema Validation for Cluster and NodePool Specifications #8
Conversation
WalkthroughAdds request-level OpenAPI schema validation initialized from OPENAPI_SCHEMA_PATH (default openapi/openapi.yaml). Implements a SchemaValidator (pkg/validators) that loads ClusterSpec and NodePoolSpec and validates spec objects. Introduces SchemaValidationMiddleware (pkg/middleware) that buffers/restores request bodies, extracts top-level "spec" for POST/PATCH to cluster/nodepool routes, invokes validation, and returns OpenAPI-style errors with field-level details on failure. Also adds domain status types and numerous presenter, handler, service, and test changes to adopt internal models and marshaling between OpenAPI and domain representations. Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Router as Server/Router
participant Middleware as SchemaValidationMiddleware
participant Validator as SchemaValidator
participant Handler as Request Handler
participant Service as Service/DB
participant Presenter as Presenter
participant ErrorResp as Error Response
Client->>Router: POST/PATCH /clusters or /nodepools (JSON body)
Router->>Middleware: incoming request
Middleware->>Middleware: shouldValidateRequest(path, method)
alt should validate
Middleware->>Middleware: read & buffer body
Middleware->>Middleware: parse JSON, extract "spec"
Middleware->>Validator: Validate(resourceType, specMap)
alt validation OK
Middleware->>Handler: forward request (restored body)
Handler->>Presenter: Convert request -> domain model
Presenter->>Service: Create/Update model
Service-->>Handler: success
Handler->>Presenter: Present model -> OpenAPI
Handler-->>Client: 200/201 response
else validation failed
Validator-->>Middleware: ValidationWithDetails
Middleware->>ErrorResp: format OpenAPI error (with details)
ErrorResp-->>Client: 400 Bad Request (JSON)
end
else skip validation
Middleware->>Handler: forward request
Handler-->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas needing extra attention:
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: 1
🧹 Nitpick comments (12)
pkg/api/cluster_types.go (1)
118-125: Consider validating StatusPhase before conversion to ResourcePhase.The direct cast
openapi.ResourcePhase(c.StatusPhase)assumesStatusPhasecontains a valid enum value. If the database contains an invalid value (e.g., from a bug or manual edit), the API response will include an invalid enum, potentially causing client parsing failures or schema validation issues.Consider adding validation or a fallback:
// Set phase, use NOT_READY as default if not set phase := openapi.NOT_READY if c.StatusPhase != "" { - phase = openapi.ResourcePhase(c.StatusPhase) + switch c.StatusPhase { + case string(openapi.NOT_READY), string(openapi.READY), string(openapi.FAILED): + phase = openapi.ResourcePhase(c.StatusPhase) + default: + // Invalid phase in database, default to NOT_READY + phase = openapi.NOT_READY + } }test/integration/integration_test.go (1)
20-30: Usefilepathoperations for more robust path manipulation.The string slicing approach on line 26 is fragile and could break if:
- The file is moved or renamed
- The path contains unexpected characters or symlinks
- The filename is somehow shorter than the suffix (would panic)
Consider using
filepath.Dirfor safer path traversal:+ "path/filepath" ... if os.Getenv("OPENAPI_SCHEMA_PATH") == "" { - // Get the repo root directory (2 levels up from test/integration) - // Use runtime.Caller to find this file's path _, filename, _, _ := runtime.Caller(0) - // filename is like: /path/to/repo/test/integration/integration_test.go - // We need: /path/to/repo/openapi/openapi.yaml - repoRoot := filename[:len(filename)-len("/test/integration/integration_test.go")] - schemaPath := repoRoot + "/openapi/openapi.yaml" + // Navigate 2 levels up from test/integration to repo root + repoRoot := filepath.Dir(filepath.Dir(filepath.Dir(filename))) + schemaPath := filepath.Join(repoRoot, "openapi", "openapi.yaml") os.Setenv("OPENAPI_SCHEMA_PATH", schemaPath) glog.Infof("Set OPENAPI_SCHEMA_PATH=%s for integration tests", schemaPath) }pkg/api/node_pool_types.go (1)
139-146: Same concern ascluster_types.go: consider validatingStatusPhasebefore casting.The direct cast to
openapi.ResourcePhasewithout validation could propagate invalid enum values to API responses. Consider applying the same validation pattern suggested forCluster.ToOpenAPI()to maintain consistency.cmd/hyperfleet-api/server/routes.go (1)
112-121: Use structured logging instead offmt.Printf/fmt.Println.Per coding guidelines, use structured logging via
logger.NewOCMLogger(ctx)with context-aware fields. The currentfmt.Printf/Printlnstatements bypass observability infrastructure.Since this is initialization code without a request context, consider using
glogwhich is already imported in other parts of the codebase:+ "github.com/golang/glog" ... if err != nil { - // Log warning but don't fail - schema validation is optional - fmt.Printf("Warning: Failed to load schema validator from %s: %v\n", schemaPath, err) - fmt.Println("Schema validation is disabled. Spec fields will not be validated.") - fmt.Println("To enable schema validation:") - fmt.Println(" - Local: Run from repo root, or set OPENAPI_SCHEMA_PATH=openapi/openapi.yaml") - fmt.Println(" - Production: Helm sets OPENAPI_SCHEMA_PATH=/etc/hyperfleet/schemas/openapi.yaml") + glog.Warningf("Failed to load schema validator from %s: %v", schemaPath, err) + glog.Warning("Schema validation is disabled. Spec fields will not be validated.") + glog.Info("To enable: set OPENAPI_SCHEMA_PATH to schema location") } else { - // Apply schema validation middleware - fmt.Printf("Schema validation enabled using: %s\n", schemaPath) + glog.Infof("Schema validation enabled using: %s", schemaPath) router.Use(middleware.SchemaValidationMiddleware(schemaValidator)) }test/integration/clusters_test.go (1)
330-333: Handlejson.Unmarshalerror to avoid masking test failures.Ignoring the unmarshal error could cause the subsequent assertions to pass/fail for the wrong reasons if the response body is malformed.
- json.Unmarshal(resp2.Body(), &errorResponse) - Expect(errorResponse.Code).ToNot(BeNil()) + err := json.Unmarshal(resp2.Body(), &errorResponse) + Expect(err).NotTo(HaveOccurred(), "Failed to unmarshal error response") + Expect(errorResponse.Code).ToNot(BeNil())Apply the same pattern to lines 397 and 443.
pkg/validators/schema_validator_test.go (2)
340-352: Remove stale comment in helper function.The comment on lines 345-346 is misleading since the
errorspackage is already imported at the top of the file.func getServiceError(err error) *errors.ServiceError { if err == nil { return nil } - // Import errors package at the top - // This is a type assertion to check if err is a *errors.ServiceError if serviceErr, ok := err.(*errors.ServiceError); ok { return serviceErr } return nil }
324-338: Consider extracting the test schema as a shared test fixture.The
testSchemaconstant is duplicated between this file andpkg/middleware/schema_validation_test.go(with slight variations). Consider extracting a shared test schema to a common test utilities package to reduce duplication and ensure consistency.pkg/middleware/schema_validation.go (2)
23-27: Handle potential JSON encoding error.The error from
json.NewEncoder(w).Encode()is silently ignored. While unlikely to fail after a successful write header, logging the error would help with debugging.// Write JSON error response w.Header().Set("Content-Type", "application/json") w.WriteHeader(err.HttpCode) - json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) + if encodeErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encodeErr != nil { + log.Errorf("Failed to encode error response: %v", encodeErr) + } }
115-132: Path matching could be more precise.The current
strings.Contains(path, "/nodepools")could match unintended paths like/api/v1/nodepools-archiveor/api/v1/my-nodepools. Consider using a regex or more explicit path segment matching for stricter validation.func shouldValidateRequest(path string) (bool, string) { - // Check nodepools first (more specific path) - // POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools - // PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} - if strings.Contains(path, "/nodepools") { + // Check nodepools first (more specific path) + // POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools + // PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} + if strings.Contains(path, "/nodepools/") || strings.HasSuffix(path, "/nodepools") { return true, "nodepool" }pkg/errors/errors.go (1)
218-223: Simplify format string usage.The format string
"%s"with a single argument is unnecessary overhead.// ValidationWithDetails creates a validation error with field-level details func ValidationWithDetails(reason string, details []ValidationDetail) *ServiceError { - err := New(ErrorValidation, "%s", reason) + err := New(ErrorValidation, reason) err.Details = details return err }pkg/validators/schema_validator.go (1)
63-88: Remove unnecessary JSON round-trip.The input
specis already amap[string]interface{}, which is directly usable byVisitJSON. The JSON marshal/unmarshal round-trip on lines 66-75 adds overhead without benefit.// validateSpec performs the actual validation and converts errors to our error format func (v *SchemaValidator) validateSpec(spec map[string]interface{}, schemaRef *openapi3.SchemaRef, resourceType string) error { - // Convert spec to JSON for validation - specJSON, err := json.Marshal(spec) - if err != nil { - return fmt.Errorf("failed to marshal spec: %w", err) - } - - // Unmarshal back to interface{} for validation - var specData interface{} - if err := json.Unmarshal(specJSON, &specData); err != nil { - return fmt.Errorf("failed to unmarshal spec: %w", err) - } - // Validate against schema - if err := schemaRef.Value.VisitJSON(specData); err != nil { + if err := schemaRef.Value.VisitJSON(spec); err != nil { // Convert validation error to our error format with details validationDetails := convertValidationError(err, "spec") return errors.ValidationWithDetails( fmt.Sprintf("Invalid %s", resourceType), validationDetails, ) } return nil }openapi/openapi.yaml (1)
756-757: Prefer direct$refoverallOfwith single reference.Using
allOfwith a single$refis valid but unconventional. Since no additional properties or constraints are being layered, a direct$refwould be cleaner and more standard:- phase: - allOf: - - $ref: '#/components/schemas/ResourcePhase' + phase: + $ref: '#/components/schemas/ResourcePhase'This applies to
ClusterStatus.phase(line 756),ConditionBase.status(line 806), andNodePoolStatus.phase(line 1109). If future constraints are planned (e.g.,minLength,maxLength),allOfwould become appropriate.Also applies to: 806-807, 1109-1111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(7 hunks)pkg/api/cluster_types.go(1 hunks)pkg/api/node_pool_types.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(1 hunks)
🧰 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:
cmd/hyperfleet-api/server/routes.gotest/integration/clusters_test.gopkg/middleware/schema_validation.gopkg/api/node_pool_types.gopkg/validators/schema_validator_test.gopkg/validators/schema_validator.gopkg/middleware/schema_validation_test.gotest/integration/adapter_status_test.gotest/integration/integration_test.gopkg/api/cluster_types.gopkg/errors/errors.go
**/test/integration/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Integration tests must use Testcontainers to create isolated PostgreSQL instances for each test suite to ensure state isolation
Files:
test/integration/clusters_test.gotest/integration/adapter_status_test.gotest/integration/integration_test.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:
test/integration/clusters_test.gopkg/validators/schema_validator_test.gopkg/middleware/schema_validation_test.gotest/integration/adapter_status_test.gotest/integration/integration_test.go
**/test/integration/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
All integration tests must run with OCM_ENV=integration_testing environment variable and fresh PostgreSQL database per test
Files:
test/integration/clusters_test.gotest/integration/adapter_status_test.gotest/integration/integration_test.go
**/{*handler*,*error*}.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Files:
pkg/errors/errors.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
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
Files:
openapi/openapi.yaml
🧠 Learnings (10)
📚 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:
cmd/hyperfleet-api/server/routes.gopkg/validators/schema_validator.gogo.mod
📚 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:
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 **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
cmd/hyperfleet-api/server/routes.gopkg/errors/errors.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/node_pool_types.gotest/integration/adapter_status_test.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 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/validators/schema_validator.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:
test/integration/adapter_status_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/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:
test/integration/adapter_status_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:
test/integration/integration_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:
test/integration/integration_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.go : All unit tests must run with OCM_ENV=unit_testing environment variable for proper test isolation
Applied to files:
test/integration/integration_test.go
🧬 Code graph analysis (5)
test/integration/clusters_test.go (2)
test/registration.go (1)
RegisterIntegration(13-24)pkg/errors/errors.go (1)
Href(182-184)
pkg/middleware/schema_validation.go (4)
pkg/errors/errors.go (3)
ServiceError(100-109)MalformedRequest(225-227)Validation(214-216)pkg/logger/logger.go (1)
NewOCMLogger(36-44)pkg/logger/operationid_middleware.go (1)
GetOperationID(38-43)pkg/validators/schema_validator.go (1)
SchemaValidator(14-18)
pkg/validators/schema_validator_test.go (2)
pkg/validators/schema_validator.go (2)
NewSchemaValidator(21-51)SchemaValidator(14-18)pkg/errors/errors.go (1)
ServiceError(100-109)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (2)
ValidationWithDetails(219-223)ValidationDetail(68-71)
pkg/middleware/schema_validation_test.go (2)
pkg/middleware/schema_validation.go (1)
SchemaValidationMiddleware(30-113)pkg/validators/schema_validator.go (2)
SchemaValidator(14-18)NewSchemaValidator(21-51)
🪛 OSV Scanner (2.3.0)
go.mod
[HIGH] 16-16: github.com/golang-jwt/jwt/v4 4.5.0: Improper error handling in ParseWithClaims and bad documentation may cause dangerous situations in github.com/golang-jwt/jwt
(GO-2024-3250)
[HIGH] 16-16: github.com/golang-jwt/jwt/v4 4.5.0: Excessive memory allocation during header parsing in github.com/golang-jwt/jwt
(GO-2025-3553)
[HIGH] 16-16: github.com/golang-jwt/jwt/v4 4.5.0: Bad documentation of error handling in ParseWithClaims can lead to potentially dangerous situations
[HIGH] 16-16: github.com/golang-jwt/jwt/v4 4.5.0: jwt-go allows excessive memory allocation during header parsing
🔇 Additional comments (20)
test/integration/adapter_status_test.go (1)
223-223: LGTM! Type-safe enum constants improve reliability.Using
openapi.FALSEandopenapi.TRUEinstead of string literals ensures compile-time detection of enum changes and aligns assertions with the OpenAPI-generatedConditionStatustype.Also applies to: 245-245, 263-263
go.mod (1)
13-13: LGTM!kin-openapiis appropriate for OpenAPI schema validation.The
kin-openapi v0.133.0dependency provides robust OpenAPI schema loading and validation capabilities needed by the new schema validator.cmd/hyperfleet-api/server/routes.go (1)
100-107: LGTM! The schema loading approach with environment variable override is appropriate.The pattern of using an environment variable with a sensible default allows flexibility between local development and production deployments. The non-blocking behavior on failure is appropriate for optional validation.
test/integration/clusters_test.go (2)
354-363: Good skip condition for provider-specific schema tests.The condition correctly handles both relative and absolute paths by checking for the
openapi/openapi.yamlsuffix, ensuring this test only runs when a provider-specific schema is configured.
414-459: Comprehensive error structure validation test.This test effectively verifies that schema validation errors include all expected fields (
Kind,Code,Reason,Href,OperationId), ensuring the error response conforms to the OpenAPI error specification.pkg/validators/schema_validator_test.go (1)
56-72: Well-structured test for schema validator initialization.The test properly validates the SchemaValidator initialization including internal field assertions. The use of
t.TempDir()for cleanup is appropriate.pkg/middleware/schema_validation.go (1)
29-113: Middleware implementation follows good patterns.The middleware correctly:
- Uses structured logging via
logger.NewOCMLogger(ctx)as per coding guidelines- Buffers and restores the request body for downstream handlers
- Returns structured errors with
ServiceErrortype- Validates only applicable HTTP methods and paths
pkg/errors/errors.go (2)
67-71: Well-designed validation detail structure.The
ValidationDetailstruct provides a clean, JSON-serializable format for field-level validation errors that integrates well with the OpenAPI error response structure.
153-176: Good integration with OpenAPI error response.The
AsOpenapiErrormethod correctly mapsValidationDetailentries to the OpenAPIErrorDetailsInnerstructure, maintaining consistency with the API specification.pkg/validators/schema_validator.go (2)
20-51: Schema loading and validation is well-implemented.The
NewSchemaValidatorfunction properly:
- Loads the OpenAPI spec from the configured path
- Validates the document structure
- Extracts required schemas with clear error messages
This aligns with the learning that
openapi/openapi.yamlis the source of truth.
90-143: Robust error conversion with appropriate fallbacks.The
convertValidationErrorfunction handles multiple error types from the kin-openapi library gracefully, includingMultiErrorfor aggregated errors andSchemaErrorfor detailed field paths. The string parsing fallback is reasonable for edge cases.pkg/middleware/schema_validation_test.go (4)
45-75: Comprehensive POST request validation test.The test properly verifies that valid requests pass through the middleware and reach the next handler with the expected status code.
291-317: Verify intended behavior for missing spec field.The test confirms that requests without a
specfield pass through to the next handler. Ensure this is the intended behavior, as it could allow malformed requests to bypass validation entirely.If
specis required for cluster/nodepool creation, consider returning a validation error instead of passing through.
378-393: Test helper follows good patterns.The
setupTestValidatorhelper properly usest.TempDir()for automatic cleanup and fails fast witht.Fatalfon setup errors.
115-144: PATCH request validation test potentially incomplete for partial updates.The PATCH test validates a complete spec replacement. However, in typical REST semantics, PATCH requests often contain partial updates. Verify if the middleware's schema validation:
- Properly handles partial payloads with missing required fields for PATCH operations
- Distinguishes between PATCH (partial updates) and PUT (full replacements) when validating against the OpenAPI schema
- Should either skip required field validation for PATCH or the schema should not mark fields as required for patch operations
Additionally, ensure this test file has proper environment setup for
OCM_ENV=unit_testingper coding guidelines.openapi/openapi.yaml (5)
839-845: VerifyConditionStatusenum value quoting aligns with generated code.The enum uses mixed quoting:
'True'and'False'are quoted strings, butUnknownis unquoted. This is valid YAML but creates type inconsistency. Confirm this matches the intended Go enum generation and that existing tests/code usingopenapi.TRUE,openapi.FALSE, etc. will correctly map to these values.
863-874: Field-level validation error structure looks good.The
Error.detailsfield for capturing field-level validation errors is well-structured and supports the PR's schema validation improvements.
1190-1196:ResourcePhaseenum is correctly defined.The new
ResourcePhaseenum (NotReady, Ready, Failed) is well-formed and supports type-safe phase handling across Cluster and NodePool status objects.
1-1: Verify source-of-truth regeneration.Per coding guidelines,
openapi/openapi.yaml(source, ~32KB with $ref) should be regenerated intopkg/api/openapi/api/openapi.yaml(fully resolved, ~44KB) viamake generate. Confirm this step was completed after schema changes to ensure generated Go enums and schemas are in sync with these source changes.Also applies to: 1-1200
612-625:BearerAuthschema definition appears redundant and non-standard.In OpenAPI 3.0, HTTP Bearer authentication is defined only via the
securitySchemesobject (lines 1198-1200) withtype: httpandscheme: bearer. A separate schema component incomponents.schemas(lines 612-625) is not required by the OpenAPI specification and is not standard practice.Verify whether this schema component is referenced elsewhere in the spec (in request bodies, responses, or parameters). If it is only applied as a security requirement and not used as a data schema, consider removing it to maintain clean OpenAPI structure.
yasun1
force-pushed
the
schema-validator
branch
from
d147076 to
c8e514e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
pkg/validators/schema_validator.go (3)
65-75: Consider removing redundant JSON round-trip.The spec is already a
map[string]interface{}which can be passed directly toVisitJSON. The marshal/unmarshal round-trip adds unnecessary overhead and could potentially alter numeric types (e.g., integers becoming floats).Apply this diff to simplify:
func (v *SchemaValidator) validateSpec(spec map[string]interface{}, schemaRef *openapi3.SchemaRef, resourceType string) error { - // Convert spec to JSON for validation - specJSON, err := json.Marshal(spec) - if err != nil { - return fmt.Errorf("failed to marshal spec: %w", err) - } - - // Unmarshal back to interface{} for validation - var specData interface{} - if err := json.Unmarshal(specJSON, &specData); err != nil { - return fmt.Errorf("failed to unmarshal spec: %w", err) - } - // Validate against schema - if err := schemaRef.Value.VisitJSON(specData); err != nil { + if err := schemaRef.Value.VisitJSON(spec); err != nil { // Convert validation error to our error format with details validationDetails := convertValidationError(err, "spec")
94-100: Consider usingerrors.Asfor type assertion.Using
errors.Asis the idiomatic Go 1.13+ approach for unwrapping and type-asserting errors, which handles wrapped errors correctly.+import "errors" // standard library errors + func convertValidationError(err error, prefix string) []errors.ValidationDetail { var details []errors.ValidationDetail - switch e := err.(type) { - case openapi3.MultiError: + var multiErr openapi3.MultiError + var schemaErr *openapi3.SchemaError + + if errors.As(err, &multiErr) { // Recursively process each sub-error - for _, subErr := range e { + for _, subErr := range multiErr { subDetails := convertValidationError(subErr, prefix) details = append(details, subDetails...) } - case *openapi3.SchemaError: + } else if errors.As(err, &schemaErr) { // Extract field path from SchemaError
120-140: Fallback regex-like parsing is fragile.The string parsing logic for extracting property names from error messages is brittle and may break if the error message format changes in future library versions. Consider logging a warning when falling back to this path.
default: // Fallback: try to extract field path from error message field := prefix errMsg := err.Error() + // Log that we're using fallback parsing (for debugging) + // Consider: log.Debugf("Using fallback error parsing for: %s", errMsg) // Try to parse field path from common error patternstest/integration/clusters_test.go (1)
359-363: Improve skip condition to handle path variations.The skip condition checks for exact suffix
openapi/openapi.yaml, but the path could have different separators or be normalized differently. Consider usingfilepath.Basefor more robust matching.+ "path/filepath" // Check if we're using a provider schema or base schema // If base schema, skip detailed validation tests schemaPath := os.Getenv("OPENAPI_SCHEMA_PATH") - if schemaPath == "" || strings.HasSuffix(schemaPath, "openapi/openapi.yaml") { + if schemaPath == "" || filepath.Base(schemaPath) == "openapi.yaml" { t.Skip("Skipping provider schema validation test - using base schema") return }pkg/middleware/schema_validation.go (1)
15-27: Check json.Encode error.The error from
json.NewEncoder(w).Encode()is ignored. While unlikely to fail after setting headers, it's a best practice to handle it.func handleValidationError(w http.ResponseWriter, r *http.Request, err *errors.ServiceError) { log := logger.NewOCMLogger(r.Context()) operationID := logger.GetOperationID(r.Context()) // Log validation errors as info (user error, not server error) log.Infof(err.Error()) // Write JSON error response w.Header().Set("Content-Type", "application/json") w.WriteHeader(err.HttpCode) - json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) + if encErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encErr != nil { + log.Errorf("Failed to encode error response: %v", encErr) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(7 hunks)pkg/api/cluster_types.go(1 hunks)pkg/api/node_pool_types.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/integration/adapter_status_test.go
- pkg/validators/schema_validator_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/api/cluster_types.gopkg/middleware/schema_validation.gopkg/validators/schema_validator.gopkg/api/node_pool_types.gopkg/middleware/schema_validation_test.gotest/integration/clusters_test.gocmd/hyperfleet-api/server/routes.gotest/integration/integration_test.gopkg/errors/errors.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/middleware/schema_validation_test.gotest/integration/clusters_test.gotest/integration/integration_test.go
**/test/integration/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Integration tests must use Testcontainers to create isolated PostgreSQL instances for each test suite to ensure state isolation
Files:
test/integration/clusters_test.gotest/integration/integration_test.go
**/test/integration/**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
All integration tests must run with OCM_ENV=integration_testing environment variable and fresh PostgreSQL database per test
Files:
test/integration/clusters_test.gotest/integration/integration_test.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
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
Files:
openapi/openapi.yaml
**/{*handler*,*error*}.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Files:
pkg/errors/errors.go
🧠 Learnings (11)
📚 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/cluster_types.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/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/middleware/schema_validation.gopkg/validators/schema_validator.gogo.modcmd/hyperfleet-api/server/routes.gotest/integration/integration_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/migrations/*.go : Store resource specifications in JSONB columns (spec field) to enable flexible cloud provider configuration
Applied to files:
pkg/middleware/schema_validation.gopkg/validators/schema_validator.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/validators/schema_validator.gogo.modcmd/hyperfleet-api/server/routes.gotest/integration/integration_test.goopenapi/openapi.yaml
📚 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/validators/schema_validator.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:
go.mod
📚 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:
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 **/{*handler*,*error*}.go : Use structured error type from pkg/errors/ with HttpCode, Code, and Reason fields for consistent API error responses
Applied to files:
cmd/hyperfleet-api/server/routes.gopkg/errors/errors.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:
test/integration/integration_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:
test/integration/integration_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.go : All unit tests must run with OCM_ENV=unit_testing environment variable for proper test isolation
Applied to files:
test/integration/integration_test.go
🧬 Code graph analysis (5)
pkg/middleware/schema_validation.go (4)
pkg/errors/errors.go (3)
ServiceError(100-109)MalformedRequest(225-227)Validation(214-216)pkg/logger/logger.go (1)
NewOCMLogger(36-44)pkg/logger/operationid_middleware.go (1)
GetOperationID(38-43)pkg/validators/schema_validator.go (1)
SchemaValidator(14-18)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (2)
ValidationWithDetails(219-223)ValidationDetail(68-71)
pkg/middleware/schema_validation_test.go (2)
pkg/middleware/schema_validation.go (1)
SchemaValidationMiddleware(30-113)pkg/validators/schema_validator.go (2)
SchemaValidator(14-18)NewSchemaValidator(21-51)
test/integration/clusters_test.go (2)
test/registration.go (1)
RegisterIntegration(13-24)pkg/errors/errors.go (1)
Href(182-184)
cmd/hyperfleet-api/server/routes.go (2)
pkg/validators/schema_validator.go (1)
NewSchemaValidator(21-51)pkg/middleware/schema_validation.go (1)
SchemaValidationMiddleware(30-113)
🔇 Additional comments (16)
pkg/api/cluster_types.go (1)
118-125: LGTM! Safe default handling for phase conversion.The guarded phase computation correctly defaults to
NOT_READYand only overrides whenStatusPhaseis non-empty. This aligns with the OpenAPI schema'sResourcePhaseenum and ensures consistent API responses.Note: The direct cast
openapi.ResourcePhase(c.StatusPhase)assumesStatusPhasecontains valid enum values. This is safe given theBeforeCreatehook (line 57-59) and database default ensure valid values, but consider adding validation if external writes toStatusPhasebecome possible.pkg/api/node_pool_types.go (1)
139-146: LGTM! Consistent phase handling with Cluster implementation.The phase computation mirrors the
Cluster.ToOpenAPI()pattern, maintaining consistency across resource types. The defaultNOT_READYphase and guarded override provide safe API responses.go.mod (1)
13-20: LGTM! Appropriate dependency updates for schema validation feature.
kin-openapi v0.133.0: Required for OpenAPI schema validation middleware.golang-jwt/jwt/v4 v4.5.2: Addresses the security vulnerabilities (GO-2024-3250, GO-2025-3553) flagged in the previous review.gorilla/mux v1.8.0: Minor version upgrade.openapi/openapi.yaml (3)
839-845: LGTM! Well-defined enums for type-safe status handling.The new
ConditionStatusandResourcePhaseenums provide:
- Type-safe condition status values (
'True','False',Unknown)- Consistent phase values (
NotReady,Ready,Failed) across Cluster and NodePoolThese align with the Go code changes in
cluster_types.goandnode_pool_types.gothat useopenapi.ResourcePhaseandopenapi.ConditionStatusconstants.Also applies to: 1190-1196
863-874: LGTM! Field-level validation error details support.The
detailsarray in the Error schema enables structured field-level validation error reporting, which complements theValidationDetailtype andValidationWithDetailsfunction added inpkg/errors/errors.go.
612-625: Remove potentially unused BearerAuth schema definition.The
BearerAuthschema object at lines 612-625 defines a bearer auth configuration structure, but OpenAPI security schemes are already defined atcomponents.securitySchemes.BearerAuth(lines 1197-1200). Confirm this schema is not referenced elsewhere before removal; if unused, it should be deleted as dead code.pkg/errors/errors.go (4)
67-71: LGTM! Clean validation detail structure.The
ValidationDetailstruct provides a clear, JSON-serializable representation of field-level validation errors, matching the OpenAPI schema'sError.detailsstructure.
82-97: LGTM! Improved struct literal style.Using named fields (
{Code: ..., Reason: ..., HttpCode: ...}) instead of positional literals improves readability and reduces risk of field ordering errors.
153-176: LGTM! Correct OpenAPI error detail mapping.The
AsOpenapiErrormethod correctly mapsValidationDetailentries toopenapi.ErrorDetailsInner, ensuring field-level validation errors are properly serialized in API responses. As per coding guidelines, this maintains consistent API error responses with the structured error type.
218-223: LGTM! Convenient helper for validation errors with details.
ValidationWithDetailsfollows the established pattern of other error constructors while adding support for field-level details. This integrates well with the schema validation middleware.pkg/middleware/schema_validation_test.go (4)
45-75: LGTM! Comprehensive POST validation test.The test properly validates that the middleware passes valid requests through to the next handler and returns the expected status code.
77-113: LGTM! Good coverage of invalid spec handling.The test correctly verifies that validation failures prevent the next handler from being called and return proper error responses.
222-258: LGTM! NodePool validation test with helpful debugging.The test includes useful debug logging when validation unexpectedly fails, which aids troubleshooting.
378-393: LGTM! Clean test helper setup.The
setupTestValidatorhelper properly usest.TempDir()for automatic cleanup and fails fast with clear error messages.pkg/middleware/schema_validation.go (2)
29-113: LGTM! Well-structured middleware implementation.The middleware correctly:
- Filters by HTTP method (POST/PATCH only)
- Buffers and restores request body for downstream handlers
- Handles missing/invalid spec gracefully
- Preserves ServiceError details for structured error responses
- Uses structured logging per coding guidelines
115-132: Path matching usingstrings.Contains()is vulnerable to false positives; use path segment boundaries instead.The current logic with
strings.Contains(path, "/clusters")could match unintended paths like/clusters-backupor/api/v1/myclustersmith. Go best practices recommend explicit prefix patterns or router-level matching rather than ad-hoc substring checks. Consider:
strings.HasPrefix(path, "/api/hyperfleet/v1/clusters/") || strings.HasSuffix(path, "/api/hyperfleet/v1/clusters")- Or leverage your router's path matching rules (e.g., gorilla/mux, chi) with named segments
pkg/validators/schema_validator.go
Outdated
| } | ||
|
|
||
| // validateSpec performs the actual validation and converts errors to our error format | ||
| func (v *SchemaValidator) validateSpec(spec map[string]interface{}, schemaRef *openapi3.SchemaRef, resourceType string) error { |
There was a problem hiding this comment.
The last parameter name is resourceType but we use also that term for values like cluster and nodepool
Could this be resourceSpecType or should we talk in the other cases of "resourceKinds" instead of resourceTypes ?
There was a problem hiding this comment.
We will use specTypeName to make the semantics clearer.
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Maybe something we should discuss on the sync tomorrow. But having read this PR, I am wondering if we could leverage kin-openapi a lot more. There is a validateRequest function that would remove the need for VisitJSON(). The lib will also give us route matching without the need for manual string matching on paths.
I guess my main question is why are we focused on the spec field and not validation of the full request path params, query params, full body? Because if we want to validate all we can leverage kin-openapi spec a lot more and reduce a lot of this code
yasun1
force-pushed
the
schema-validator
branch
from
c8e514e to
cb756f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (14)
pkg/api/node_pool_types_test.go (1)
133-147: Consider asserting StatusPhase usingPhaseNotReadyconstant instead of string literalThe
TestNodePool_BeforeCreate_StatusPhaseDefaultandTestNodePool_BeforeCreate_Completetests currently assert"NotReady"directly. SinceBeforeCreatenow setsStatusPhase = string(PhaseNotReady), you could strengthen type-safety by asserting against the enum:-Expect(nodepool.StatusPhase).To(Equal("NotReady")) +Expect(nodepool.StatusPhase).To(Equal(string(PhaseNotReady)))and similarly in the “Complete” test. This keeps the tests aligned if the enum value ever changes.
pkg/validators/schema_validator_test.go (1)
56-343: Validator tests provide good coverage of constructor and spec error pathsThe suite thoroughly exercises
NewSchemaValidator(valid schema, bad path, missing components) and both cluster/nodepool spec validation, including required fields, enums, nested fields, and type/range violations withServiceError.Detailschecks. This gives strong confidence in the validator behavior. If you find more of these YAML-based schemas cropping up in other tests (e.g., middleware), consider a shared test helper to avoid duplication, but it’s not required.pkg/api/presenters/node_pool.go (1)
12-44: Align StatusPhase with enum constant and consider handling JSON (un)marshal errorsTwo small points here:
StatusPhase initialization in
ConvertNodePoolYou’re already using
api.ResourcePhase/PhaseNotReadyelsewhere (e.g., inBeforeCreateand status types). To avoid another hard-coded phase string, consider:
StatusPhase: "NotReady",
StatusPhase: string(api.PhaseNotReady),This keeps all phase values flowing from the same enum source.
Silent JSON (un)marshal failures
In both
ConvertNodePoolandPresentNodePool,json.Marshal/json.Unmarshalerrors are ignored (_ = ...). While failures are unlikely given these structs come from JSON HTTP payloads/DB, any corruption or unexpected type would be silently dropped, which can be hard to trace.If you want to tighten this up, an optional improvement would be to at least log or
panicon these errors (since they indicate programmer/configuration error, not user input), for example:
- specJSON, _ := json.Marshal(req.Spec)
- specJSON, err := json.Marshal(req.Spec)
- if err != nil {
panic(fmt.Sprintf("failed to marshal NodePool spec: %v", err))- }
and similarly for labels/status conditions and their unmarshalling in `PresentNodePool`. Also applies to: 49-136 </blockquote></details> <details> <summary>openapi/openapi.yaml (1)</summary><blockquote> `612-625`: **Potentially unnecessary BearerAuth schema definition.** This schema defines BearerAuth as a component schema, but OpenAPI security authentication is typically defined only in `securitySchemes` (lines 1198-1200), not as a data schema. Unless this schema is referenced by request/response bodies elsewhere in the spec, it appears redundant. Consider verifying if this schema is used anywhere: ```shell #!/bin/bash # Search for references to BearerAuth schema outside securitySchemes rg -n "BearerAuth" openapi/openapi.yaml | grep -v "securitySchemes" | grep -v "^612:" | grep -v "^613:" | grep -v "^614:"pkg/api/cluster_types_test.go (1)
56-203: Well-structured BeforeCreate tests with good coverage.The tests thoroughly validate both default value assignment and preservation of pre-set values for all key fields (ID, Kind, Generation, StatusPhase, Href). The final
TestCluster_BeforeCreate_Completetest provides good integration coverage.Consider using the
PhaseNotReadyconstant fromstatus_types.goinstead of the string literal"NotReady"in assertions (lines 139, 201) for consistency with the implementation change incluster_types.go.func TestCluster_BeforeCreate_StatusPhaseDefault(t *testing.T) { RegisterTestingT(t) // Test default StatusPhase cluster := &Cluster{ Name: "test-cluster", } err := cluster.BeforeCreate(nil) Expect(err).To(BeNil()) - Expect(cluster.StatusPhase).To(Equal("NotReady")) + Expect(cluster.StatusPhase).To(Equal(string(PhaseNotReady))) }pkg/handlers/cluster_status.go (1)
91-95: Missing log statement for aggregation error.The comment indicates the error should be logged, but no actual logging occurs. Consider adding structured logging here.
// Trigger status aggregation _, aggregateErr := h.clusterService.UpdateClusterStatusFromAdapters(ctx, clusterID) if aggregateErr != nil { - // Log error but don't fail the request - // The status will be computed on next update + // Log error but don't fail the request - status will be computed on next update + log := logger.NewOCMLogger(ctx) + log.Warningf("Failed to aggregate cluster status: %v", aggregateErr) }pkg/api/presenters/cluster.go (2)
14-14: Silent JSON marshal errors could hide data issues.While
json.Marshalrarely fails for simple types, silently ignoring errors could mask unexpected issues during development. Consider logging or returning errors for defensive coding.// Marshal Spec - specJSON, _ := json.Marshal(req.Spec) + specJSON, err := json.Marshal(req.Spec) + if err != nil { + // This shouldn't happen with valid input, but log for debugging + specJSON = []byte("{}") + }Also applies to: 21-21, 24-24
31-32: Use constant instead of string literal for StatusPhase.Line 92 correctly uses
api.PhaseNotReady, but here a string literal is used. Prefer the constant for consistency and compile-time safety.Labels: labelsJSON, Generation: 1, - StatusPhase: "NotReady", + StatusPhase: string(api.PhaseNotReady), StatusObservedGeneration: 0,pkg/middleware/schema_validation.go (2)
23-27: Unhandled JSON encoding error.If
json.NewEncoder(w).Encode()fails, the response may be partially written with no indication to the caller.// Write JSON error response w.Header().Set("Content-Type", "application/json") w.WriteHeader(err.HttpCode) - json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) + if encErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encErr != nil { + log.Errorf("Failed to encode error response: %v", encErr) + } }
99-121: Path matching may have edge cases with non-API paths.The current logic could match unintended paths (e.g.,
/other/clustersor/nodepools-legacy). Consider anchoring to the expected API prefix for stricter matching.+const apiPrefix = "/api/hyperfleet/v1" + func shouldValidateRequest(method, path string) (bool, string) { // Only validate POST and PATCH requests if method != http.MethodPost && method != http.MethodPatch { return false, "" } + // Ensure path is under the API prefix + if !strings.HasPrefix(path, apiPrefix) { + return false, "" + } + // Check nodepools first (more specific path)pkg/api/presenters/adapter_status.go (1)
36-50: Silent JSON marshal errors - consistent with cluster.go but worth noting.Same pattern as
cluster.go. While failures are unlikely with well-formed input, consider at minimum logging marshaling failures for debugging purposes.pkg/api/presenters/adapter_status_test.go (1)
89-131: Optionally tighten timestamp assertions to catch more regressionsYou already verify ObservedTime is propagated and that CreatedTime falls within a reasonable window when ObservedTime is zero. To make these tests even more robust, consider also:
- Asserting LastReportTime matches CreatedTime/ObservedTime in the zero‑ObservedTime path.
- In the complete/round‑trip tests, asserting LastTransitionTime on conditions aligns with the chosen timestamp (ObservedTime or now).
This would better lock in the intended temporal semantics if ConvertAdapterStatus or PresentAdapterStatus change later.
Also applies to: 220-237, 395-416
pkg/validators/schema_validator.go (2)
93-118: Consider avoiding the extra marshal/unmarshal step in validateSpecRight now you JSON‑marshal the spec and immediately unmarshal into an interface{} before calling VisitJSON. Given the input is already
map[string]interface{}, you can likely pass it (or a shallow copy if you’re worried about mutation) directly toVisitJSONto skip the double conversion and reduce allocations on hot paths.
120-170: Improve field path derivation for schema errorsIn
convertValidationError, you currently preferSchemaFieldoverJSONPointerwhen building theFieldstring. In practice,SchemaFieldoften reflects the schema keyword (type,pattern, etc.), whileJSONPointer()tends to capture the path to the offending data. Flipping that precedence (use JSONPointer first, then fall back to SchemaField and finally message parsing) would likely yield more helpfulFieldvalues likespec.gcp.regioninstead ofspec.pattern, improving API error responses without changing error structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
README.md(1 hunks)cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(7 hunks)pkg/api/adapter_status_types.go(0 hunks)pkg/api/adapter_status_types_test.go(1 hunks)pkg/api/cluster_types.go(2 hunks)pkg/api/cluster_types_test.go(1 hunks)pkg/api/node_pool_types.go(1 hunks)pkg/api/node_pool_types_test.go(1 hunks)pkg/api/presenters/adapter_status.go(1 hunks)pkg/api/presenters/adapter_status_test.go(1 hunks)pkg/api/presenters/cluster.go(1 hunks)pkg/api/presenters/cluster_test.go(1 hunks)pkg/api/presenters/node_pool.go(1 hunks)pkg/api/presenters/node_pool_test.go(1 hunks)pkg/api/status_types.go(1 hunks)pkg/api/status_types_test.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_nodepools.go(1 hunks)pkg/handlers/cluster_status.go(4 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(4 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/services/cluster.go(2 hunks)pkg/services/node_pool.go(2 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/api/adapter_status_types.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- test/integration/clusters_test.go
- test/integration/adapter_status_test.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/handlers/cluster.gopkg/handlers/cluster_nodepools.gopkg/api/presenters/cluster_test.gopkg/middleware/schema_validation.gopkg/api/node_pool_types_test.gotest/integration/integration_test.gopkg/api/node_pool_types.gopkg/handlers/node_pool.gopkg/services/node_pool.gopkg/api/cluster_types_test.gopkg/api/presenters/node_pool_test.gopkg/middleware/schema_validation_test.gopkg/services/cluster.gocmd/hyperfleet-api/server/routes.gopkg/api/presenters/cluster.gopkg/api/status_types.gopkg/errors/errors.gopkg/api/presenters/adapter_status.gopkg/handlers/nodepool_status.gopkg/validators/schema_validator.gopkg/api/adapter_status_types_test.gopkg/api/status_types_test.gopkg/validators/schema_validator_test.gopkg/api/presenters/adapter_status_test.gopkg/api/cluster_types.gopkg/handlers/cluster_status.gopkg/api/presenters/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/cluster.gopkg/handlers/cluster_nodepools.gopkg/handlers/node_pool.gopkg/services/node_pool.gopkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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/cluster.gopkg/handlers/cluster_nodepools.gopkg/handlers/node_pool.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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/cluster.gopkg/handlers/cluster_nodepools.gopkg/handlers/node_pool.gopkg/services/node_pool.gopkg/services/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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/cluster.gopkg/handlers/cluster_nodepools.gopkg/api/presenters/cluster_test.gopkg/middleware/schema_validation.gopkg/api/node_pool_types_test.gopkg/api/node_pool_types.gopkg/handlers/node_pool.gopkg/services/node_pool.gopkg/api/cluster_types_test.gopkg/api/presenters/node_pool_test.gopkg/middleware/schema_validation_test.gopkg/services/cluster.gopkg/api/presenters/cluster.gopkg/api/status_types.gopkg/errors/errors.gopkg/api/presenters/adapter_status.gopkg/handlers/nodepool_status.gopkg/validators/schema_validator.gopkg/api/adapter_status_types_test.gopkg/api/status_types_test.gopkg/validators/schema_validator_test.gopkg/api/presenters/adapter_status_test.gopkg/api/cluster_types.gopkg/handlers/cluster_status.gopkg/api/presenters/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/presenters/cluster_test.gopkg/api/node_pool_types_test.gopkg/api/node_pool_types.gopkg/api/cluster_types_test.gopkg/api/presenters/node_pool_test.gopkg/api/presenters/cluster.gopkg/api/status_types.gopkg/api/presenters/adapter_status.gopkg/api/adapter_status_types_test.gopkg/api/status_types_test.gopkg/api/presenters/adapter_status_test.gopkg/api/cluster_types.gopkg/api/presenters/node_pool.go
test/integration/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
test/integration/**/*.go: Create integration tests using Testcontainers for isolated PostgreSQL instances per test
All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Files:
test/integration/integration_test.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/cluster.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
Files:
openapi/openapi.yaml
🧠 Learnings (16)
📚 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:
pkg/api/presenters/cluster_test.gopkg/api/node_pool_types_test.gotest/integration/integration_test.gopkg/api/cluster_types_test.gopkg/api/presenters/node_pool_test.gopkg/middleware/schema_validation_test.gopkg/api/adapter_status_types_test.gopkg/api/status_types_test.gopkg/validators/schema_validator_test.gopkg/api/presenters/adapter_status_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/middleware/schema_validation.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/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/middleware/schema_validation.gotest/integration/integration_test.gopkg/api/node_pool_types.gopkg/services/node_pool.gopkg/services/cluster.gocmd/hyperfleet-api/server/routes.gopkg/api/status_types.gopkg/api/presenters/adapter_status.gopkg/handlers/nodepool_status.gopkg/validators/schema_validator.gopkg/api/status_types_test.gopkg/validators/schema_validator_test.gopkg/api/cluster_types.gopkg/handlers/cluster_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/api/node_pool_types_test.gopkg/api/node_pool_types.gopkg/validators/schema_validator.gopkg/api/presenters/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/api/node_pool_types_test.gotest/integration/integration_test.gopkg/api/cluster_types_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/factories/**/*.go : Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Applied to files:
pkg/api/node_pool_types_test.gopkg/api/cluster_types_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:
pkg/api/node_pool_types.gopkg/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/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.gopkg/services/node_pool.gopkg/services/cluster.gopkg/validators/schema_validator.gopkg/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/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/node_pool_types.gopkg/services/node_pool.gopkg/services/cluster.gopkg/api/status_types.gopkg/api/presenters/adapter_status.gopkg/handlers/nodepool_status.gopkg/api/adapter_status_types_test.gopkg/api/status_types_test.gopkg/api/presenters/adapter_status_test.gopkg/handlers/cluster_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 : 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/node_pool.gopkg/services/cluster.gopkg/api/status_types.gopkg/api/presenters/adapter_status.gopkg/handlers/nodepool_status.gopkg/api/adapter_status_types_test.gopkg/api/status_types_test.gopkg/api/presenters/adapter_status_test.gopkg/handlers/cluster_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/services/node_pool.gopkg/services/cluster.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/services/node_pool.gopkg/services/cluster.gocmd/hyperfleet-api/server/routes.gopkg/errors/errors.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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:
cmd/hyperfleet-api/server/routes.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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:
cmd/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: Generate OpenAPI code by running make generate whenever the TypeSpec specification changes
Applied to files:
openapi/openapi.yaml
📚 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
🧬 Code graph analysis (20)
pkg/handlers/cluster.go (1)
pkg/api/presenters/cluster.go (1)
ConvertCluster(12-38)
pkg/handlers/cluster_nodepools.go (1)
pkg/api/presenters/node_pool.go (1)
ConvertNodePool(13-45)
pkg/api/presenters/cluster_test.go (3)
pkg/api/presenters/cluster.go (2)
ConvertCluster(12-38)PresentCluster(41-121)pkg/api/status_types.go (3)
ResourceCondition(28-37)ConditionTrue(20-20)ConditionFalse(21-21)pkg/api/cluster_types.go (1)
Cluster(11-34)
pkg/api/node_pool_types_test.go (2)
pkg/api/node_pool_types.go (2)
NodePoolList(42-42)NodePool(12-40)pkg/errors/errors.go (1)
Href(182-184)
pkg/api/node_pool_types.go (1)
pkg/api/status_types.go (1)
PhaseNotReady(10-10)
pkg/handlers/node_pool.go (1)
pkg/api/presenters/node_pool.go (1)
ConvertNodePool(13-45)
pkg/services/node_pool.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
pkg/api/cluster_types_test.go (2)
pkg/api/cluster_types.go (2)
ClusterList(36-36)Cluster(11-34)pkg/errors/errors.go (1)
Href(182-184)
pkg/api/presenters/node_pool_test.go (4)
pkg/api/presenters/node_pool.go (2)
ConvertNodePool(13-45)PresentNodePool(48-139)pkg/api/status_types.go (2)
ResourceCondition(28-37)ConditionTrue(20-20)pkg/api/node_pool_types.go (1)
NodePool(12-40)pkg/errors/errors.go (1)
Href(182-184)
cmd/hyperfleet-api/server/routes.go (3)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)pkg/validators/schema_validator.go (1)
NewSchemaValidator(26-67)pkg/middleware/schema_validation.go (1)
SchemaValidationMiddleware(30-97)
pkg/api/presenters/cluster.go (3)
pkg/api/cluster_types.go (1)
Cluster(11-34)pkg/api/status_types.go (4)
ResourceCondition(28-37)PhaseNotReady(10-10)ResourcePhase(7-7)ConditionStatus(17-17)pkg/errors/errors.go (1)
Href(182-184)
pkg/api/presenters/adapter_status.go (3)
pkg/api/adapter_status_types.go (1)
AdapterStatus(11-30)pkg/api/status_types.go (2)
AdapterCondition(42-48)ConditionStatus(17-17)pkg/api/metadata_types.go (1)
Metadata(28-34)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (2)
ValidationWithDetails(219-223)ValidationDetail(68-71)
pkg/api/adapter_status_types_test.go (1)
pkg/api/adapter_status_types.go (2)
AdapterStatusList(32-32)AdapterStatus(11-30)
pkg/api/status_types_test.go (1)
pkg/api/status_types.go (10)
PhaseNotReady(10-10)PhaseReady(11-11)PhaseFailed(12-12)ResourcePhase(7-7)ConditionTrue(20-20)ConditionFalse(21-21)ConditionUnknown(22-22)ConditionStatus(17-17)ResourceCondition(28-37)AdapterCondition(42-48)
pkg/validators/schema_validator_test.go (2)
pkg/validators/schema_validator.go (2)
NewSchemaValidator(26-67)SchemaValidator(20-23)pkg/errors/errors.go (1)
ServiceError(100-109)
pkg/api/presenters/adapter_status_test.go (4)
pkg/api/metadata_types.go (1)
Metadata(28-34)pkg/api/presenters/adapter_status.go (2)
ConvertAdapterStatus(13-63)PresentAdapterStatus(66-117)pkg/api/status_types.go (5)
AdapterCondition(42-48)ConditionTrue(20-20)ConditionStatus(17-17)ConditionFalse(21-21)ConditionUnknown(22-22)pkg/api/adapter_status_types.go (1)
AdapterStatus(11-30)
pkg/api/cluster_types.go (1)
pkg/api/status_types.go (1)
PhaseNotReady(10-10)
pkg/handlers/cluster_status.go (1)
pkg/api/presenters/adapter_status.go (2)
PresentAdapterStatus(66-117)ConvertAdapterStatus(13-63)
pkg/api/presenters/node_pool.go (2)
pkg/api/node_pool_types.go (1)
NodePool(12-40)pkg/api/status_types.go (4)
ResourceCondition(28-37)PhaseNotReady(10-10)ResourcePhase(7-7)ConditionStatus(17-17)
🔇 Additional comments (38)
test/integration/integration_test.go (1)
19-45: OPENAPI_SCHEMA_PATH resolution is now robust and fails safelyThe updated logic using
runtime.Caller+filepath.Dirandos.Statavoids brittle string slicing and panics, and degrades gracefully via warnings when the path can’t be resolved or the schema file is missing. This is a solid improvement and should keep integration schema validation aligned with the repo layout.pkg/api/node_pool_types.go (1)
53-63: UsingPhaseNotReadyenum for StatusPhase default is a good alignmentSwitching the default from a hard-coded
"NotReady"tostring(PhaseNotReady)ties the persistence layer directly to the domain enum and OpenAPI representation, reducing the chance of silent drift if enum values ever change.pkg/middleware/schema_validation_test.go (1)
45-393: SchemaValidationMiddleware test coverage looks comprehensiveThese tests exercise all the important branches: method/path filtering, valid and invalid specs for clusters and nodepools, missing
spec, wrong spec type, and malformed JSON, plus verifying that the next handler is or isn’t called and that error responses conform to the OpenAPIErrorshape. This should give strong confidence in the middleware behavior.pkg/api/status_types.go (1)
5-48: Domain status/condition types are well-structured and aligned with OpenAPIDefining
ResourcePhase,ConditionStatus,ResourceCondition, andAdapterConditionas first-class domain types with clear JSON tags is a good move. It centralizes status semantics, keeps values aligned with the OpenAPI enums, and gives presenters/validators a stable type surface to work with.pkg/api/status_types_test.go (1)
11-336: Status/condition tests thoroughly validate enums and JSON behaviorThe suite does a nice job of checking constant values, string conversions, JSON marshaling/unmarshaling, optional fields, and round-trip integrity for both
ResourceConditionandAdapterCondition(including time fields). This gives good confidence that these types will behave correctly in both DB JSONB and API payloads.openapi/openapi.yaml (2)
839-845: Excellent enum extraction for type safety.Extracting
ConditionStatusandResourcePhaseas top-level enum schemas and referencing them viaallOfimproves reusability and enables type-safe constants in generated code. This aligns perfectly with the PR objective of improving type safety for enum handling.Also applies to: 1190-1196
863-874: Good addition of field-level validation error details.The new
detailsarray in theErrorschema enables structured field-level validation feedback, which directly supports the schema validation functionality introduced in this PR. The structure withfieldanderrorproperties is clear and follows common validation error patterns.pkg/handlers/cluster.go (1)
39-40: Clean refactoring to presenter-based conversion.The migration from
api.ClusterFromOpenAPICreatetopresenters.ConvertClusterimproves separation of concerns by moving conversion logic into the presenters layer. This change is consistent with similar refactoring across other handlers and maintains the same functional behavior.pkg/handlers/node_pool.go (1)
41-41: Consistent presenter-based conversion refactoring.Similar to the cluster handler, this change migrates to
presenters.ConvertNodePool, maintaining architectural consistency across the codebase. The conversion properly handles defaults and optional fields.pkg/services/cluster.go (1)
132-177: Improved type separation in status aggregation logic.The migration from
openapi.*types toapi.*types in the status aggregation logic properly separates domain concerns from API boundary concerns. This change maintains the same adapter status aggregation behavior (Ready if all report Ready=True, Failed if any report Ready=False, NotReady otherwise) while improving architectural clarity.Based on learnings about adapter status aggregation implementation.
pkg/api/adapter_status_types_test.go (2)
10-54: Comprehensive test coverage for AdapterStatusList.Index.The test cases thoroughly cover empty lists, single items, multiple items, and duplicate ID handling (where later entries overwrite earlier ones). This provides good confidence in the Index() method behavior.
57-94: Good lifecycle behavior verification.The BeforeCreate tests verify both ID auto-generation and that only the ID field is modified during the hook, preserving all other explicitly-set fields. This ensures the lifecycle hook behaves correctly without unexpected side effects.
pkg/services/node_pool.go (1)
132-176: Consistent type migration for NodePool status aggregation.This mirrors the refactoring in
pkg/services/cluster.go, migrating from OpenAPI types to domain types in the status aggregation logic. The change maintains functional correctness while improving architectural separation between API and domain layers.pkg/handlers/cluster_nodepools.go (1)
142-143: Consistent conversion pattern for cluster-scoped NodePool creation.This change aligns with the presenter-based conversion refactoring across other handlers. The conversion properly passes
cluster.IDas theownerIDparameter to establish the parent relationship.cmd/hyperfleet-api/server/routes.go (1)
101-127: Schema validation integration looks good.The schema validation initialization properly:
- Uses environment variable configuration with sensible defaults
- Implements non-blocking initialization that gracefully degrades on failure
- Applies structured logging with context fields as per coding guidelines
- Conditionally registers middleware only when validator loads successfully
The past review comment about structured logging has been properly addressed in the current implementation using
logger.NewOCMLogger(ctx)withExtra()for field propagation.Note: A past review comment (lines 103) suggests documenting the OPENAPI_SCHEMA_PATH requirement in the README, which remains a valid suggestion for user-facing documentation.
pkg/handlers/nodepool_status.go (2)
9-9: Good refactor to presenter-based conversions.The change from
as.ToOpenAPI()topresenters.PresentAdapterStatus(as)aligns with the architectural decision to keep domain types isolated from presentation layer types. This follows the feedback from previous reviews about ensuring the presentation layer doesn't leak into the business domain.Also applies to: 43-43
82-82: Consistent use of presenter pattern for conversion.Both the conversion from OpenAPI request (
ConvertAdapterStatus) and presentation to OpenAPI response (PresentAdapterStatus) now use the presenters package, maintaining proper separation between domain and presentation layers.Also applies to: 97-98
pkg/api/cluster_types_test.go (1)
1-54: Comprehensive tests for ClusterList.Index().Good coverage of edge cases including empty lists, single elements, multiple elements, and duplicate ID handling. The tests clearly document the expected behavior that later entries overwrite earlier ones with the same ID.
pkg/api/cluster_types.go (2)
55-56: Good use of typed constant for status phase.Using
string(PhaseNotReady)instead of the literal"NotReady"improves consistency and enables compile-time detection if the constant value changes, aligning with the PR's type-safety objectives.
65-70: ClusterPatchRequest extended with Labels field.The addition of the
Labelsfield enables patching cluster labels via the API, which aligns with the OpenAPI schema updates mentioned in the PR summary.pkg/api/presenters/node_pool_test.go (4)
13-27: Well-designed test helper function.The
createTestNodePoolRequesthelper provides consistent test data and reduces duplication across tests. Good practice.
29-69: Thorough conversion test with verification of defaults and marshaling.The test properly validates basic field mapping, default values (StatusPhase, StatusObservedGeneration), and correct JSON marshaling of Spec, Labels, and StatusConditions.
146-229: Comprehensive presentation test covering all output fields.Good coverage of field mapping, JSON unmarshaling, OwnerReferences, Status construction, and timestamp handling. The use of
openapi.READYandopenapi.TRUEconstants aligns with the PR's type-safety objectives.
381-422: Excellent round-trip integrity test.This test validates data preservation through the full conversion cycle (OpenAPI request → domain model → OpenAPI response), which is crucial for ensuring the presenter functions are inverses of each other for the data they handle.
pkg/api/presenters/cluster_test.go (4)
13-26: Consistent test helper pattern.The
createTestClusterRequesthelper follows the same pattern as innode_pool_test.go, maintaining consistency across presenter test files.
110-149: Important test for complex nested spec structures.This test validates that deeply nested objects (config → networking) and arrays (tags) are properly marshaled and preserved through the conversion. This is critical for ensuring spec integrity with real-world cluster configurations.
270-293: Good edge case coverage for nil timestamps.Testing nil timestamp handling ensures the presenter doesn't panic and produces valid zero-time values, which is important for newly created clusters that may not have status timestamps set yet.
359-395: Round-trip integrity test validates presenter symmetry.Similar to the NodePool tests, this validates that data survives the full conversion cycle. The verification of defaults (NOT_READY phase, zero observed generation, empty conditions) also documents expected initial state behavior.
pkg/handlers/cluster_status.go (1)
9-9: LGTM! Clean refactoring to presenters layer.The migration from
api.AdapterStatusFromOpenAPICreateand.ToOpenAPI()methods to the centralizedpresenters.ConvertAdapterStatusandpresenters.PresentAdapterStatusfunctions improves separation of concerns and aligns with the consistent pattern used across handlers.Also applies to: 43-43, 82-82, 97-98
pkg/api/presenters/cluster.go (1)
40-120: LGTM! Comprehensive domain-to-OpenAPI conversion.The
PresentClusterfunction properly handles:
- Nil-safe unmarshaling of JSON fields
- Default value handling for optional time fields
- Correct type conversions for enums using the OpenAPI types
- Fallback Href generation
The condition conversion loop at lines 98-110 correctly maps domain types to OpenAPI types with proper status enum casting.
pkg/middleware/schema_validation.go (1)
29-96: LGTM! Well-structured middleware with proper body preservation.The middleware correctly:
- Buffers the request body and restores it for downstream handlers
- Extracts and validates only the
specfield- Gracefully skips validation when no spec is present
- Returns structured OpenAPI-style errors with field-level details
pkg/api/presenters/adapter_status.go (2)
65-116: LGTM! Clean presentation logic with proper nil handling.The function correctly:
- Handles empty JSON fields with length checks before unmarshaling
- Converts between domain and OpenAPI condition types
- Provides sensible defaults for nil time pointers
24-34: LastTransitionTime always set to current time may lose transition history.For upsert operations,
LastTransitionTimeshould only update when the condition'sStatusactually changes. Setting it unconditionally tonowon line 32 loses the original transition time on subsequent reports with the same status. Verify whether the service/repository layer performs status comparison before updating, or if this transition-time semantics is intended.pkg/errors/errors.go (3)
67-71: LGTM! Well-designed validation detail extension.The
ValidationDetailtype andValidationWithDetailsfunction provide a clean mechanism for field-level validation errors. The integration withAsOpenapiErrorcorrectly maps to the OpenAPI error detail structure only when details are present.Also applies to: 107-108, 218-223
82-97: Good improvement: explicit field names in struct literals.Using named fields (
{Code: ..., Reason: ..., HttpCode: ...}) instead of positional values improves readability and resilience to future field ordering changes.
153-176: LGTM! Clean conditional inclusion of validation details.The details are only added to the OpenAPI response when present, avoiding empty arrays in the JSON output for non-validation errors.
pkg/api/presenters/adapter_status_test.go (1)
13-87: Thorough coverage of conversion and enum status handling looks solidThe tests exercise ConvertAdapterStatus and PresentAdapterStatus end‑to‑end, including TRUE/FALSE/UNKNOWN mappings, nil/empty Data and Metadata, timestamp wiring, and a round‑trip through domain and back to OpenAPI. Assertions align with the implementation in pkg/api/presenters/adapter_status.go and the domain types in pkg/api/status_types.go, so this suite gives good confidence that adapter status conversion is correct.
Also applies to: 186-218, 220-392, 394-423
pkg/validators/schema_validator.go (1)
25-67: Validator initialization and resource schema lookup are well‑structuredLoading the OpenAPI doc once, validating it, and then extracting ClusterSpecCore/NodePoolSpecCore into a keyed map ("cluster"/"nodepool") gives a clean, extensible surface. The top‑level Validate method’s behavior of no‑oping on unknown resource types is explicit in the comment and appropriate for gradual rollout of additional resources.
Also applies to: 69-79
One thought is that if using a provider-specific openapi spec for the request validation, it can contain things that are not relevant for hyperfleet-api
|
yasun1
force-pushed
the
schema-validator
branch
from
cb756f9 to
da3aa19
Compare
yasun1
force-pushed
the
schema-validator
branch
from
da3aa19 to
103f306
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/integration/clusters_test.go (1)
328-337: Check json.Unmarshal error.The error from
json.Unmarshalon Line 332 is being ignored, which could mask parsing failures in the test.Apply this diff to check the unmarshal error:
if resp2.StatusCode() == http.StatusBadRequest { t.Logf("Schema validation correctly rejected invalid spec type") // Verify error response contains details var errorResponse openapi.Error - json.Unmarshal(resp2.Body(), &errorResponse) + err := json.Unmarshal(resp2.Body(), &errorResponse) + Expect(err).To(BeNil(), "Failed to unmarshal error response") Expect(errorResponse.Code).ToNot(BeNil()) Expect(errorResponse.Reason).ToNot(BeNil())
🧹 Nitpick comments (7)
pkg/services/types_test.go (1)
133-206: Search and fields behavior well covered; consider using Gomega for FieldsThe search and fields tests correctly capture trimming behavior and the “always include
id” rule, matching the implementation. For consistency with the rest of the file, you could switch theFieldsassertions to Gomega:Expect(listArgs.Fields).To(Equal(tt.expectedFields), "Fields mismatch")and then drop the
reflectimport.pkg/api/presenters/node_pool.go (1)
49-65: Consider logging JSON unmarshaling errors to detect data corruption.Lines 52, 58, and 64 silently ignore errors from json.Unmarshal. While the code defensively checks for empty JSON before unmarshaling, if the database contains corrupted JSON data, unmarshal failures will go unnoticed and result in empty values being presented to the API client. This could mask data integrity issues.
Add defensive logging for unmarshal failures:
// Unmarshal Spec var spec map[string]interface{} if len(nodePool.Spec) > 0 { - _ = json.Unmarshal(nodePool.Spec, &spec) + if err := json.Unmarshal(nodePool.Spec, &spec); err != nil { + // Log error but continue with empty spec + // In production, this would indicate data corruption + fmt.Printf("Warning: failed to unmarshal nodepool spec for ID %s: %v\n", nodePool.ID, err) + } } // Unmarshal Labels var labels map[string]string if len(nodePool.Labels) > 0 { - _ = json.Unmarshal(nodePool.Labels, &labels) + if err := json.Unmarshal(nodePool.Labels, &labels); err != nil { + fmt.Printf("Warning: failed to unmarshal nodepool labels for ID %s: %v\n", nodePool.ID, err) + } } // Unmarshal StatusConditions var statusConditions []api.ResourceCondition if len(nodePool.StatusConditions) > 0 { - _ = json.Unmarshal(nodePool.StatusConditions, &statusConditions) + if err := json.Unmarshal(nodePool.StatusConditions, &statusConditions); err != nil { + fmt.Printf("Warning: failed to unmarshal nodepool conditions for ID %s: %v\n", nodePool.ID, err) + } }Use structured logging via pkg/logger when implementing this.
pkg/api/presenters/cluster.go (1)
42-58: Consider logging JSON unmarshaling errors to detect data corruption.Similar to the NodePool presenter, lines 45, 51, and 57 silently ignore unmarshal errors. Consider adding defensive logging to detect data corruption issues. Refer to the recommendation in the node_pool.go presenter review.
pkg/middleware/schema_validation.go (1)
99-121: Consider more precise path matching for better specificity.The current path matching using strings.Contains (lines 110, 116) could potentially match unintended paths. For example,
/api/something/nodepools-backupwould match the nodepool check. While this is unlikely to cause issues given the current API structure, consider using more precise path matching.func shouldValidateRequest(method, path string) (bool, string) { // Only validate POST and PATCH requests if method != http.MethodPost && method != http.MethodPatch { return false, "" } // Check nodepools first (more specific path) // POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools // PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} - if strings.Contains(path, "/nodepools") { + if strings.Contains(path, "/nodepools/") || strings.HasSuffix(path, "/nodepools") { return true, "nodepool" } // POST /api/hyperfleet/v1/clusters // PATCH /api/hyperfleet/v1/clusters/{cluster_id} - if strings.HasSuffix(path, "/clusters") || strings.Contains(path, "/clusters/") { + if strings.HasSuffix(path, "/clusters") || (strings.Contains(path, "/clusters/") && !strings.Contains(path, "/nodepools")) { return true, "cluster" } return false, "" }pkg/api/presenters/adapter_status.go (1)
67-95: Consider logging JSON unmarshaling errors to detect data corruption.Lines 70, 88, and 94 silently ignore unmarshal errors. Consider adding defensive logging to detect data corruption issues, similar to the recommendation for Cluster and NodePool presenters.
pkg/validators/schema_validator.go (2)
25-67: Consider broader validation scope per PR feedback.A commenter suggested exploring kin-openapi's validateRequest function, which could:
- Replace custom VisitJSON() logic with built-in route matching
- Potentially validate full request inputs (path params, query params, entire request body) instead of just the spec field
This approach might reduce custom code, though it would need careful consideration of provider-specific OpenAPI specs that may include irrelevant requirements (e.g., provider-specific headers or pagination limits).
95-105: Consider if marshal/unmarshal round-trip is necessary.The code marshals the spec to JSON and then unmarshals it back to validate. This ensures valid JSON but may be unnecessary overhead. Verify if VisitJSON can accept the map[string]interface{} directly.
#!/bin/bash # Description: Check kin-openapi VisitJSON signature and usage patterns # Search for VisitJSON usage in the codebase rg -n 'VisitJSON' --type go # Search for VisitJSON in test files to see usage patterns rg -n -A5 'VisitJSON' --type go -g '*_test.go'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
README.md(1 hunks)cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(7 hunks)pkg/api/adapter_status_types.go(1 hunks)pkg/api/adapter_status_types_test.go(1 hunks)pkg/api/cluster_types.go(3 hunks)pkg/api/cluster_types_test.go(1 hunks)pkg/api/node_pool_types.go(3 hunks)pkg/api/node_pool_types_test.go(1 hunks)pkg/api/presenters/adapter_status.go(1 hunks)pkg/api/presenters/adapter_status_test.go(1 hunks)pkg/api/presenters/cluster.go(1 hunks)pkg/api/presenters/cluster_test.go(1 hunks)pkg/api/presenters/node_pool.go(1 hunks)pkg/api/presenters/node_pool_test.go(1 hunks)pkg/api/status_types.go(1 hunks)pkg/api/status_types_test.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_nodepools.go(1 hunks)pkg/handlers/cluster_status.go(4 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(4 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/services/cluster.go(2 hunks)pkg/services/node_pool.go(2 hunks)pkg/services/types.go(1 hunks)pkg/services/types_test.go(1 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/api/adapter_status_types_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/handlers/node_pool.go
- README.md
- pkg/api/status_types_test.go
- pkg/handlers/cluster_nodepools.go
- pkg/api/presenters/cluster_test.go
- test/integration/adapter_status_test.go
- pkg/api/status_types.go
- pkg/middleware/schema_validation_test.go
- test/integration/integration_test.go
- pkg/handlers/nodepool_status.go
- go.mod
- pkg/api/cluster_types_test.go
- pkg/api/presenters/adapter_status_test.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/handlers/cluster.gopkg/api/presenters/node_pool_test.gocmd/hyperfleet-api/server/routes.gopkg/api/presenters/cluster.gopkg/services/node_pool.gopkg/handlers/cluster_status.gopkg/validators/schema_validator_test.gopkg/validators/schema_validator.gopkg/api/presenters/node_pool.gopkg/services/types.gopkg/middleware/schema_validation.gotest/integration/clusters_test.gopkg/errors/errors.gopkg/services/cluster.gopkg/api/node_pool_types_test.gopkg/api/adapter_status_types.gopkg/services/types_test.gopkg/api/presenters/adapter_status.gopkg/api/cluster_types.gopkg/api/node_pool_types.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/cluster.gopkg/services/node_pool.gopkg/handlers/cluster_status.gopkg/services/types.gopkg/services/cluster.gopkg/services/types_test.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/cluster.gopkg/handlers/cluster_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/cluster.gopkg/services/node_pool.gopkg/handlers/cluster_status.gopkg/services/types.gopkg/services/cluster.gopkg/services/types_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/handlers/cluster.gopkg/api/presenters/node_pool_test.gopkg/api/presenters/cluster.gopkg/services/node_pool.gopkg/handlers/cluster_status.gopkg/validators/schema_validator_test.gopkg/validators/schema_validator.gopkg/api/presenters/node_pool.gopkg/services/types.gopkg/middleware/schema_validation.gopkg/errors/errors.gopkg/services/cluster.gopkg/api/node_pool_types_test.gopkg/api/adapter_status_types.gopkg/services/types_test.gopkg/api/presenters/adapter_status.gopkg/api/cluster_types.gopkg/api/node_pool_types.go
pkg/api/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Files:
pkg/api/presenters/node_pool_test.gopkg/api/presenters/cluster.gopkg/api/presenters/node_pool.gopkg/api/node_pool_types_test.gopkg/api/adapter_status_types.gopkg/api/presenters/adapter_status.gopkg/api/cluster_types.gopkg/api/node_pool_types.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/types.gopkg/services/cluster.gopkg/services/types_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/clusters_test.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
Files:
openapi/openapi.yaml
🧠 Learnings (17)
📓 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 openapi/openapi.yaml : TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
📚 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:
pkg/api/presenters/node_pool_test.gopkg/validators/schema_validator_test.gotest/integration/clusters_test.gopkg/api/node_pool_types_test.gopkg/services/types_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/api/presenters/node_pool_test.gopkg/validators/schema_validator.gopkg/api/presenters/node_pool.gopkg/api/node_pool_types_test.gopkg/api/cluster_types.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 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:
cmd/hyperfleet-api/server/routes.gopkg/services/node_pool.gopkg/handlers/cluster_status.gopkg/validators/schema_validator.gopkg/middleware/schema_validation.gopkg/services/cluster.gopkg/api/adapter_status_types.gopkg/api/presenters/adapter_status.gopkg/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/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/{handlers,services,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
cmd/hyperfleet-api/server/routes.gopkg/services/node_pool.gopkg/services/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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
cmd/hyperfleet-api/server/routes.gopkg/services/node_pool.gopkg/handlers/cluster_status.gopkg/errors/errors.gopkg/services/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/handlers/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
cmd/hyperfleet-api/server/routes.gopkg/handlers/cluster_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:
cmd/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/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/services/node_pool.gopkg/validators/schema_validator.gopkg/services/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/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/cluster_status.gopkg/services/cluster.gopkg/api/adapter_status_types.gopkg/api/presenters/adapter_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 : 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/node_pool.gopkg/handlers/cluster_status.gopkg/services/cluster.gopkg/api/adapter_status_types.gopkg/api/presenters/adapter_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/dao/**/*.go : Increment generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/services/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/api/node_pool_types_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/factories/**/*.go : Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Applied to files:
pkg/api/node_pool_types_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 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:
openapi/openapi.yaml
📚 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: Generate OpenAPI code by running make generate whenever the TypeSpec specification changes
Applied to files:
openapi/openapi.yaml
🧬 Code graph analysis (15)
pkg/handlers/cluster.go (1)
pkg/api/presenters/cluster.go (1)
ConvertCluster(12-38)
pkg/api/presenters/node_pool_test.go (3)
pkg/api/presenters/node_pool.go (2)
ConvertNodePool(13-45)PresentNodePool(48-139)pkg/api/status_types.go (2)
ResourceCondition(28-37)ConditionTrue(20-20)pkg/api/node_pool_types.go (1)
NodePool(12-40)
cmd/hyperfleet-api/server/routes.go (3)
pkg/logger/logger.go (1)
NewOCMLogger(36-44)pkg/validators/schema_validator.go (1)
NewSchemaValidator(26-67)pkg/middleware/schema_validation.go (1)
SchemaValidationMiddleware(30-97)
pkg/handlers/cluster_status.go (1)
pkg/api/presenters/adapter_status.go (2)
PresentAdapterStatus(66-117)ConvertAdapterStatus(13-63)
pkg/validators/schema_validator_test.go (2)
pkg/validators/schema_validator.go (2)
NewSchemaValidator(26-67)SchemaValidator(20-23)pkg/errors/errors.go (1)
ServiceError(100-109)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (2)
ValidationWithDetails(219-223)ValidationDetail(68-71)
pkg/api/presenters/node_pool.go (3)
pkg/api/node_pool_types.go (1)
NodePool(12-40)pkg/api/status_types.go (4)
ResourceCondition(28-37)PhaseNotReady(10-10)ResourcePhase(7-7)ConditionStatus(17-17)pkg/errors/errors.go (1)
Href(182-184)
pkg/middleware/schema_validation.go (4)
pkg/errors/errors.go (3)
ServiceError(100-109)MalformedRequest(225-227)Validation(214-216)pkg/logger/logger.go (1)
NewOCMLogger(36-44)pkg/logger/operationid_middleware.go (1)
GetOperationID(38-43)pkg/validators/schema_validator.go (1)
SchemaValidator(20-23)
test/integration/clusters_test.go (3)
test/registration.go (1)
RegisterIntegration(13-24)pkg/errors/errors.go (1)
Href(182-184)pkg/api/cluster_types.go (1)
Cluster(11-34)
pkg/services/cluster.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
pkg/api/node_pool_types_test.go (1)
pkg/api/node_pool_types.go (2)
NodePoolList(42-42)NodePool(12-40)
pkg/api/adapter_status_types.go (1)
pkg/api/metadata_types.go (1)
Meta(37-42)
pkg/services/types_test.go (1)
pkg/services/types.go (2)
NewListArguments(25-76)MaxListSize(22-22)
pkg/api/cluster_types.go (2)
pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/status_types.go (1)
PhaseNotReady(10-10)
pkg/api/node_pool_types.go (2)
pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/status_types.go (1)
PhaseNotReady(10-10)
🔇 Additional comments (45)
pkg/services/types.go (1)
52-55: Sensible default sort for deterministic listingsDefaulting
OrderByto[]string{"created_time desc"}when noorderByis provided gives stable, predictable pagination and matches the new tests. Please just confirm that all callers and query builders interpret"created_time desc"as expected and don’t rely on anil/emptyOrderByhaving special semantics.pkg/services/types_test.go (2)
11-69: Good coverage of OrderBy parsing and defaultingThis table‑driven test suite hits the important cases for
orderBy(defaulting, multiple fields, whitespace handling) and clearly documents the new"created_time desc"default behavior.
71-131: Thorough pagination and size testsThe default-value and page/pageSize tests line up well with
NewListArguments: they verify precedence betweenpageSizeand legacysize, ensure defaults remain stable, and assert clamping toMaxListSize. This should prevent regressions around pagination behavior.pkg/handlers/cluster.go (1)
39-40: LGTM! Conversion refactored to use presenters.The shift from inline OpenAPI-to-model conversion to
presenters.ConvertClustercentralizes translation logic and improves maintainability.pkg/services/node_pool.go (1)
132-157: LGTM! Type migration to domain layer.The migration from
openapi.*toapi.*types for conditions aligns with the architectural shift to use domain types in the service layer.pkg/services/cluster.go (1)
132-157: LGTM! Type migration to domain layer.Consistent with the broader refactor to use domain status types (
api.ResourceCondition,api.AdapterCondition) instead of OpenAPI types in the service layer.pkg/api/node_pool_types_test.go (1)
1-245: LGTM! Comprehensive test coverage for NodePool types.The test suite thoroughly covers
Index()behavior andBeforeCreate()validations including ID generation, default values, and href auto-generation. The tests follow the project's testing patterns using Gomega.cmd/hyperfleet-api/server/routes.go (1)
110-127: LGTM! Schema validation wiring with structured logging.The non-blocking validator initialization with structured logging (via
logger.NewOCMLogger) properly addresses the previous review feedback. The approach allows the service to start even if schema validation isn't available, with clear operational guidance in logs.Based on coding guidelines requiring structured logging.
test/integration/clusters_test.go (3)
283-350: LGTM! Comprehensive schema validation test coverage.
TestClusterSchemaValidationvalidates the middleware behavior with the base schema, covering valid specs, invalid type rejection, and empty specs. The test correctly handles both validation success and failure scenarios.
355-417: LGTM! Provider-specific schema validation test.
TestClusterSchemaValidationWithProviderSchemaappropriately skips when using the base schema and validates provider-specific requirements when applicable. The error detail verification confirms field-level error reporting works correctly.
475-623: LGTM! Comprehensive sorting test coverage.The sorting tests (
TestClusterList_DefaultSorting,TestClusterList_OrderByName,TestClusterList_OrderByNameDesc) thoroughly verify default descending created_time sorting and custom name-based ordering with appropriate time delays to ensure distinct timestamps.pkg/validators/schema_validator_test.go (1)
1-357: LGTM! Comprehensive schema validator test suite.The test suite thoroughly exercises the SchemaValidator with coverage for:
- Successful initialization and schema loading
- Error cases (invalid paths, missing required schemas)
- ClusterSpec validation (valid specs, missing fields, invalid enums, nested field validation)
- NodePoolSpec validation (valid specs, type mismatches, range violations, length constraints)
The helper functions (
setupTestValidator,getServiceError) are well-designed and the tests follow the project's Gomega-based testing patterns.pkg/api/adapter_status_types.go (1)
12-12: Verify Meta struct field name consistency.The comment references "DeletedTime" but the Meta struct may have a field named
DeletedAtinstead. Please confirm whether the Meta struct usesDeletedTimeorDeletedAtand update the comment accordingly if needed.pkg/api/node_pool_types.go (3)
13-13: LGTM! Meta embedding follows the established pattern.The Meta embedding with CreatedTime/UpdatedTime/DeletedTime aligns with the metadata types definition and provides proper audit trail functionality.
53-76: LGTM! BeforeCreate hook properly initializes all required fields.The lifecycle hook correctly:
- Sets CreatedTime and UpdatedTime to current time
- Uses the PhaseNotReady constant for StatusPhase default
- Generates proper Href and OwnerHref values following REST conventions
- Initializes all default values appropriately
78-81: LGTM! BeforeUpdate hook ensures timestamp accuracy.The BeforeUpdate hook properly refreshes the UpdatedTime field on every update operation, maintaining accurate audit trails.
pkg/api/presenters/node_pool.go (1)
67-139: LGTM! Status construction and Href generation are well-implemented.The code properly:
- Generates fallback Href and OwnerHref values following REST conventions
- Handles nil timestamps with appropriate defaults
- Uses PhaseNotReady as the default phase when not set
- Converts domain ResourceConditions to OpenAPI format with proper type mapping
pkg/api/presenters/cluster.go (1)
60-121: LGTM! Cluster presentation logic is well-implemented.The code properly handles:
- Href fallback generation
- Status construction with appropriate defaults
- Timestamp handling
- Condition conversion to OpenAPI format
pkg/handlers/cluster_status.go (1)
9-98: LGTM! Handler correctly adopts presenter-based conversion pattern.The changes properly:
- Use presenters.PresentAdapterStatus to convert domain models to OpenAPI responses
- Use presenters.ConvertAdapterStatus to convert requests to domain models
- Maintain proper separation of concerns between handlers and presenters
- Follow the established handler pattern of parse → service → present → respond
pkg/api/presenters/node_pool_test.go (2)
1-229: LGTM! Comprehensive test coverage for NodePool presenters.The test suite provides excellent coverage including:
- Complete field conversion with all fields populated
- Kind defaulting behavior (explicit vs. default values)
- Label handling (present vs. nil)
- Href generation with fallbacks
- Status phase defaults
- Status conditions conversion with multiple conditions
- Timestamp handling
The tests appropriately verify round-trip data integrity and edge cases.
381-422: Excellent round-trip test for data integrity.The round-trip test ensures that data is preserved through the ConvertNodePool → PresentNodePool cycle, validating that the conversion functions are inverse operations and maintain data integrity.
pkg/middleware/schema_validation.go (3)
15-27: LGTM! Error handling follows best practices.The error handler correctly:
- Logs validation errors as informational (user errors, not server errors)
- Uses structured logging with context propagation
- Returns OpenAPI-formatted error responses with proper content type and HTTP status
- Includes operation ID for request tracing
68-91: LGTM! Validation logic is well-implemented.The validation logic properly:
- Type-checks the spec field as a map
- Returns appropriate validation errors for type mismatches
- Delegates validation to the injected SchemaValidator
- Handles both structured ServiceError and generic error cases
- Returns field-level validation details when available
40-50: Verify request body size limits to prevent DoS.Line 41 uses
io.ReadAll()without an explicit size limit, which could be exploited for DoS attacks with extremely large request bodies. Confirm that request body size limits are enforced either at the HTTP server level (viaMaxBytesReader) or through an upstream middleware layer before this handler processes the body.pkg/api/cluster_types.go (3)
47-66: LGTM! BeforeCreate hook properly initializes cluster fields.The lifecycle hook correctly:
- Sets CreatedTime and UpdatedTime to current time
- Uses the PhaseNotReady constant for StatusPhase default
- Generates proper Href value following REST conventions
- Initializes defaults for Kind and Generation
The implementation is consistent with the NodePool BeforeCreate pattern.
68-71: LGTM! BeforeUpdate hook maintains timestamp consistency.The BeforeUpdate hook properly refreshes UpdatedTime on each update operation.
73-78: LGTM! Labels field addition enables label patching.The addition of the Labels field to ClusterPatchRequest allows clients to update cluster labels via PATCH operations, providing feature parity with NodePoolPatchRequest.
pkg/api/presenters/adapter_status.go (1)
97-117: LGTM! Timestamp handling and response construction are well-implemented.The code properly:
- Handles nil timestamps with appropriate defaults
- Constructs complete OpenAPI AdapterStatus response
- Preserves all fields from the domain model
openapi/openapi.yaml (5)
612-625: LGTM: BearerAuth security scheme correctly defined.The BearerAuth schema and security scheme are properly defined with lowercase "bearer" as required by HTTP authentication standards (RFC 6750).
Also applies to: 1198-1200
839-845: LGTM: Type-safe enum schemas improve consistency.The new ConditionStatus and ResourcePhase enum schemas provide type-safe constants that align with Kubernetes conventions and improve compile-time safety in generated code.
Also applies to: 1190-1196
756-757: LGTM: Enum references properly refactor status fields.The refactoring to reference ResourcePhase and ConditionStatus enums via allOf and $ref is consistent and maintains schema flexibility while ensuring type safety.
Also applies to: 806-807, 831-831, 1110-1111
863-874: LGTM: Error details field enables field-level validation errors.The details field addition to the Error schema properly supports field-level validation error reporting and aligns with the Go ValidationDetail struct implementation.
1-4: Note: This is a generated file.Based on coding guidelines, remember that TypeSpec definitions are maintained separately in the hyperfleet-api-spec repository. Changes to this file should come from running
make generateafter updating the TypeSpec source.As per coding guidelines, this file is generated from TypeSpec definitions.
pkg/errors/errors.go (5)
67-71: LGTM: ValidationDetail struct is well-defined.The ValidationDetail struct properly supports field-level validation errors with appropriate JSON tags and documentation.
107-108: LGTM: Details field properly extends ServiceError.The Details field addition enables structured field-level validation error reporting while maintaining backward compatibility.
218-223: LGTM: ValidationWithDetails provides convenient API.The function properly creates validation errors with field-level details, maintaining consistency with existing error creation patterns.
153-176: LGTM: AsOpenapiError correctly populates details.The method properly converts ValidationDetail entries to OpenAPI error details while maintaining backward compatibility when no details are present.
84-96: LGTM: Named fields improve code maintainability.The refactoring to use named field syntax in struct literals improves readability and makes the code more resilient to future struct changes.
Also applies to: 118-122
pkg/validators/schema_validator.go (7)
13-23: LGTM: Validator structs are well-designed.The ResourceSchema and SchemaValidator structs provide a clean abstraction for OpenAPI-driven validation with appropriate field types.
69-79: LGTM: Validate method handles unknown types gracefully.The method correctly returns nil for unknown resource types, allowing the validator to be used in contexts where not all resource types need validation.
81-91: LGTM: Deprecated methods provide backward compatibility.The deprecated ValidateClusterSpec and ValidateNodePoolSpec methods maintain backward compatibility while guiding users toward the more general Validate method.
124-130: LGTM: MultiError recursion correctly flattens nested errors.The recursive handling of MultiError properly flattens all validation errors into a single list of ValidationDetail entries.
150-169: LGTM: Default case provides reasonable fallback.The heuristic parsing of error messages is fragile but acceptable as a fallback. The code correctly handles parse failures by using the full error message.
26-32: Verify schemaPath is from a trusted source.The schemaPath parameter is passed to LoadFromFile without validation. Ensure this path comes only from configuration (e.g., OPENAPI_SCHEMA_PATH environment variable) and never from user input to prevent path traversal vulnerabilities.
131-149: Verification of field path construction and coding guideline compliance cannot be completed.Codebase access is required to verify: (1) JSONPointer format and behavior with the kin-openapi library, (2) whether this file properly implements JWT authentication middleware as required by coding guidelines for
pkg/**/*.go, and (3) the correctness of the dot notation field path extraction logic. The original review comment should also be clarified with specific actionable feedback.
yasun1
force-pushed
the
schema-validator
branch
from
103f306 to
fde7ccb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/api/presenters/cluster.go (1)
11-38: JSON marshaling errors are still ignored.This was flagged in a previous review. The
json.Marshalcalls on lines 14, 21, and 24 discard errors which could lead to silent data loss ifreq.Speccontains unsupported types.pkg/api/presenters/node_pool.go (1)
12-45: JSON marshaling errors are still ignored.This was flagged in a previous review. Same issue as the cluster presenter.
test/integration/clusters_test.go (1)
332-335: Check json.Unmarshal error.The error from
json.Unmarshalis being ignored, which could mask issues in the test.var errorResponse openapi.Error - json.Unmarshal(resp2.Body(), &errorResponse) + err := json.Unmarshal(resp2.Body(), &errorResponse) + Expect(err).To(BeNil(), "Failed to unmarshal error response") Expect(errorResponse.Code).ToNot(BeNil())
🧹 Nitpick comments (9)
pkg/services/types_test.go (3)
11-69: OrderBy tests are thorough; consider simplifying Gomega registration in subtestsThe table-driven coverage for
orderBy(defaults, asc/desc, multiple fields, whitespace, empty/whitespace-only) matchesNewListArgumentsbehavior very well. The only minor nit is callingRegisterTestingT(t)both at the top of the test and again inside eacht.Run; you only need it for the*testing.Tthat actually usesExpect, so you can drop the outer call, or switch to the more idiomaticg := NewWithT(t)inside each subtest and useg.Expect(...).
82-131: Good coverage of page/pageSize/size precedence and bounds; consider adding exact-boundary caseThe
PageSizetable covers custom page, legacysize, precedence ofpageSizeoversize, and negative/too-large sizes clamped toMaxListSize, which matches the implementation. As an optional enhancement, you might add a case wherepageSizeequalsMaxListSizeexactly to protect against off‑by‑one regressions in the future.
167-205: Fields behavior is well covered; minor nit on mixing Gomega andreflect.DeepEqualThe
fieldstests nicely validate all key behaviors: defaultnil, auto-appendingidwhen missing, avoiding duplicates, and trimming space/comma noise. For consistency with the rest of the file you could replace the manualreflect.DeepEqual+t.Errorfwith a GomegaExpect(listArgs.Fields).To(Equal(tt.expectedFields)), but the current approach is functionally fine.pkg/validators/schema_validator.go (2)
26-67: LGTM! Consider making resource schema mapping configurable.The validator correctly loads and validates the OpenAPI spec. However, the hard-coded schema names and resource types create tight coupling.
For future extensibility, consider making the resource-to-schema mapping configurable:
type SchemaMapping struct { ResourceType string SchemaName string TypeName string } func NewSchemaValidatorWithMappings(schemaPath string, mappings []SchemaMapping) (*SchemaValidator, error) { // Load spec... schemas := make(map[string]*ResourceSchema) for _, mapping := range mappings { schema := doc.Components.Schemas[mapping.SchemaName] if schema == nil { return nil, fmt.Errorf("%s schema not found", mapping.SchemaName) } schemas[mapping.ResourceType] = &ResourceSchema{ TypeName: mapping.TypeName, Schema: schema, } } // ... }This would allow adding new resource types without modifying the validator code.
71-79: Consider logging unknown resource types.Returning nil for unknown resource types allows graceful handling but could hide configuration errors or typos in resource type names.
Add logging to track when validation is skipped:
func (v *SchemaValidator) Validate(resourceType string, spec map[string]interface{}) error { resourceSchema := v.schemas[resourceType] if resourceSchema == nil { - // Unknown resource type, skip validation + // Unknown resource type, skip validation (may indicate misconfiguration) + // Consider adding: log.Warnf("Unknown resource type %s, skipping validation", resourceType) return nil }This would help detect misconfiguration during development and operations.
pkg/api/presenters/cluster.go (1)
40-58: JSON unmarshal errors are silently ignored in PresentCluster.Similar to the marshaling issue, lines 45, 51, and 57 ignore
json.Unmarshalerrors. While less likely to fail with valid stored data, corrupted or malformed JSON in the database would result in silent partial data loss.Consider logging unmarshal failures at minimum to aid debugging data corruption issues:
// Unmarshal Spec var spec map[string]interface{} if len(cluster.Spec) > 0 { - _ = json.Unmarshal(cluster.Spec, &spec) + if err := json.Unmarshal(cluster.Spec, &spec); err != nil { + // Log but continue - return partial data rather than fail + // Consider adding structured logging here + } }pkg/middleware/schema_validation.go (2)
15-27: Handle JSON encoding error in error response.Line 26 ignores the error from
json.NewEncoder(w).Encode(). While unlikely to fail after setting headers, encoding errors should be logged.// Write JSON error response w.Header().Set("Content-Type", "application/json") w.WriteHeader(err.HttpCode) - json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) + if encErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encErr != nil { + log.Errorf("Failed to encode error response: %v", encErr) + } }
99-121: Path matching could be more precise.The current string-based matching (e.g.,
strings.Contains(path, "/nodepools")) could match unintended paths. While unlikely given the current API structure, consider using the router's path matching or explicit prefix checks for more robustness.func shouldValidateRequest(method, path string) (bool, string) { // Only validate POST and PATCH requests if method != http.MethodPost && method != http.MethodPatch { return false, "" } + const apiPrefix = "/api/hyperfleet/v1" + // Check nodepools first (more specific path) - if strings.Contains(path, "/nodepools") { + if strings.HasPrefix(path, apiPrefix+"/clusters/") && strings.Contains(path, "/nodepools") { return true, "nodepool" } - if strings.HasSuffix(path, "/clusters") || strings.Contains(path, "/clusters/") { + if path == apiPrefix+"/clusters" || strings.HasPrefix(path, apiPrefix+"/clusters/") { return true, "cluster" } return false, "" }As noted in PR comments, kin-openapi's built-in route matching could also eliminate this custom path logic.
test/integration/clusters_test.go (1)
476-525: Consider potential test flakiness with timing-dependent assertions.The test relies on 100ms delays between cluster creations (line 497) to ensure different timestamps. While this works in most cases, it could be flaky under high system load.
Consider using explicit timestamp comparisons rather than relying on creation order:
// Alternative: Compare actual timestamps instead of assuming order for i := 0; i < len(testClusters)-1; i++ { Expect(testClusters[i].CreatedTime.After(testClusters[i+1].CreatedTime) || testClusters[i].CreatedTime.Equal(testClusters[i+1].CreatedTime)).To(BeTrue(), "Clusters should be sorted by created_time desc") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
README.md(1 hunks)cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(7 hunks)pkg/api/adapter_status_types.go(0 hunks)pkg/api/adapter_status_types_test.go(1 hunks)pkg/api/cluster_types.go(3 hunks)pkg/api/cluster_types_test.go(1 hunks)pkg/api/node_pool_types.go(3 hunks)pkg/api/node_pool_types_test.go(1 hunks)pkg/api/presenters/adapter_status.go(1 hunks)pkg/api/presenters/adapter_status_test.go(1 hunks)pkg/api/presenters/cluster.go(1 hunks)pkg/api/presenters/cluster_test.go(1 hunks)pkg/api/presenters/node_pool.go(1 hunks)pkg/api/presenters/node_pool_test.go(1 hunks)pkg/api/status_types.go(1 hunks)pkg/api/status_types_test.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_nodepools.go(1 hunks)pkg/handlers/cluster_status.go(4 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(4 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/services/cluster.go(2 hunks)pkg/services/node_pool.go(2 hunks)pkg/services/types.go(1 hunks)pkg/services/types_test.go(1 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/api/adapter_status_types.go
🚧 Files skipped from review as they are similar to previous changes (15)
- test/integration/integration_test.go
- pkg/handlers/node_pool.go
- pkg/api/adapter_status_types_test.go
- pkg/handlers/cluster.go
- pkg/api/presenters/node_pool_test.go
- pkg/api/presenters/adapter_status.go
- pkg/api/status_types_test.go
- cmd/hyperfleet-api/server/routes.go
- README.md
- pkg/api/node_pool_types_test.go
- pkg/services/types.go
- test/integration/adapter_status_test.go
- pkg/services/node_pool.go
- pkg/api/node_pool_types.go
- pkg/validators/schema_validator_test.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/middleware/schema_validation_test.gopkg/services/cluster.gopkg/handlers/cluster_nodepools.gopkg/middleware/schema_validation.gotest/integration/clusters_test.gopkg/api/presenters/cluster_test.gopkg/errors/errors.gopkg/api/presenters/cluster.gopkg/services/types_test.gopkg/api/presenters/node_pool.gopkg/api/status_types.gopkg/validators/schema_validator.gopkg/handlers/nodepool_status.gopkg/api/cluster_types_test.gopkg/handlers/cluster_status.gopkg/api/cluster_types.gopkg/api/presenters/adapter_status_test.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Files:
pkg/middleware/schema_validation_test.gopkg/services/cluster.gopkg/handlers/cluster_nodepools.gopkg/middleware/schema_validation.gopkg/api/presenters/cluster_test.gopkg/errors/errors.gopkg/api/presenters/cluster.gopkg/services/types_test.gopkg/api/presenters/node_pool.gopkg/api/status_types.gopkg/validators/schema_validator.gopkg/handlers/nodepool_status.gopkg/api/cluster_types_test.gopkg/handlers/cluster_status.gopkg/api/cluster_types.gopkg/api/presenters/adapter_status_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/services/cluster.gopkg/handlers/cluster_nodepools.gopkg/services/types_test.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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/services/cluster.gopkg/handlers/cluster_nodepools.gopkg/services/types_test.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.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/types_test.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/cluster_nodepools.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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/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/presenters/cluster_test.gopkg/api/presenters/cluster.gopkg/api/presenters/node_pool.gopkg/api/status_types.gopkg/api/cluster_types_test.gopkg/api/cluster_types.gopkg/api/presenters/adapter_status_test.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
Files:
openapi/openapi.yaml
🧠 Learnings (14)
📓 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 openapi/openapi.yaml : TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
📚 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:
pkg/middleware/schema_validation_test.gotest/integration/clusters_test.gopkg/api/presenters/cluster_test.gopkg/services/types_test.gopkg/api/cluster_types_test.gopkg/api/presenters/adapter_status_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/middleware/schema_validation_test.gopkg/middleware/schema_validation.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/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/services/cluster.gogo.modpkg/validators/schema_validator.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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:
pkg/services/cluster.gogo.modpkg/validators/schema_validator.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/api/status_types.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/api/presenters/adapter_status_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/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/api/status_types.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/api/presenters/adapter_status_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,dao}/**/*.go : Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Applied to files:
pkg/services/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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/services/cluster.gopkg/middleware/schema_validation.gopkg/errors/errors.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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/middleware/schema_validation.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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:
test/integration/clusters_test.gopkg/api/presenters/cluster_test.gopkg/api/cluster_types_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/api/presenters/node_pool.gopkg/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: Generate OpenAPI code by running make generate whenever the TypeSpec specification changes
Applied to files:
openapi/openapi.yaml
📚 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/factories/**/*.go : Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Applied to files:
pkg/api/cluster_types_test.go
🧬 Code graph analysis (10)
pkg/middleware/schema_validation_test.go (2)
pkg/middleware/schema_validation.go (1)
SchemaValidationMiddleware(30-97)pkg/validators/schema_validator.go (2)
SchemaValidator(20-23)NewSchemaValidator(26-67)
pkg/services/cluster.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
pkg/api/presenters/cluster_test.go (4)
pkg/api/presenters/cluster.go (2)
ConvertCluster(12-38)PresentCluster(41-121)pkg/api/status_types.go (3)
ResourceCondition(28-37)ConditionTrue(20-20)ConditionFalse(21-21)pkg/api/cluster_types.go (1)
Cluster(11-34)pkg/errors/errors.go (1)
Href(182-184)
pkg/api/presenters/cluster.go (3)
pkg/api/cluster_types.go (1)
Cluster(11-34)pkg/api/status_types.go (4)
ResourceCondition(28-37)PhaseNotReady(10-10)ResourcePhase(7-7)ConditionStatus(17-17)pkg/errors/errors.go (1)
Href(182-184)
pkg/services/types_test.go (1)
pkg/services/types.go (2)
NewListArguments(25-82)MaxListSize(22-22)
pkg/api/presenters/node_pool.go (3)
pkg/api/node_pool_types.go (1)
NodePool(12-40)pkg/api/status_types.go (4)
ResourceCondition(28-37)PhaseNotReady(10-10)ResourcePhase(7-7)ConditionStatus(17-17)pkg/errors/errors.go (1)
Href(182-184)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (2)
ValidationWithDetails(219-223)ValidationDetail(68-71)
pkg/handlers/nodepool_status.go (1)
pkg/api/presenters/adapter_status.go (2)
PresentAdapterStatus(66-117)ConvertAdapterStatus(13-63)
pkg/api/cluster_types_test.go (2)
pkg/api/cluster_types.go (2)
ClusterList(36-36)Cluster(11-34)pkg/errors/errors.go (1)
Href(182-184)
pkg/handlers/cluster_status.go (1)
pkg/api/presenters/adapter_status.go (2)
PresentAdapterStatus(66-117)ConvertAdapterStatus(13-63)
🔇 Additional comments (57)
pkg/services/types_test.go (2)
71-80: Default values test aligns correctly with implementationThis test accurately asserts the default
Page,Size,Search, andOrderBy(created_time desc) fromNewListArguments. No functional issues here; this gives a nice guardrail if defaults change later.
133-165: Search tests correctly exercise trimming behaviorThe tests for
search(absent, present, and with surrounding spaces) match thestrings.Trimlogic inNewListArgumentsand look solid. No changes needed.go.mod (2)
17-17: JWT vulnerability fix applied correctly.The upgrade to v4.5.2 addresses both GO-2024-3250 (CVE-2024-51744) and GO-2025-3553 (CVE-2025-30204) that were flagged in the previous review.
14-14: kin-openapi v0.133.0 is secure and suitable for production. This is a stable release published on August 25, 2025, with no known security vulnerabilities. A prior CVE (CVE-2025-30153) affected versions before 0.131.0 but was resolved; v0.133.0 is unaffected.pkg/errors/errors.go (6)
67-71: LGTM!The ValidationDetail struct provides a clean representation for field-level validation errors with appropriate JSON serialization.
82-97: LGTM!The explicit field names in struct literals improve readability and make it clear which fields are being set. The Details field appropriately defaults to nil for these error templates.
107-109: LGTM!The Details field appropriately extends ServiceError to support field-level validation errors. The optional nature is well-documented.
118-123: LGTM!Consistent use of named fields improves code clarity.
154-176: LGTM!The conversion logic correctly transforms ValidationDetail entries into OpenAPI error details format. The conditional population ensures the details field is only included when present.
218-223: LGTM!The ValidationWithDetails function provides a convenient and type-safe way to create validation errors with field-level details.
pkg/api/cluster_types.go (2)
68-71: LGTM!The BeforeUpdate hook ensures UpdatedTime is automatically refreshed on every update, maintaining accurate timestamp tracking without manual intervention.
47-66: Ensure PhaseNotReady constant is defined as a Phase type.The code uses
string(PhaseNotReady)to set the default status phase. Verify this constant is defined in pkg/api/status_types.go and matches the expected Phase type definition used elsewhere in the codebase.pkg/validators/schema_validator.go (1)
121-173: LGTM! String parsing in fallback is acceptably fragile.The error conversion logic handles multiple error types comprehensively. The string parsing fallback (lines 157-164) is brittle but necessary as a last resort when structured error information isn't available.
The approach is pragmatic given the uncertainty of error formats from the validation library.
pkg/services/cluster.go (1)
139-184: LGTM! Correct separation of domain and presentation concerns.The migration from openapi.* types to api.* types in the service layer properly isolates business logic from the presentation layer, addressing the architectural concerns raised in previous reviews.
Based on past review comments about dependency direction.
pkg/handlers/cluster_nodepools.go (1)
142-143: LGTM! Proper use of presenter pattern.Moving the conversion logic to presenters.ConvertNodePool correctly separates presentation concerns from handler logic, aligning with the architectural pattern established in this PR.
pkg/handlers/nodepool_status.go (1)
44-44: LGTM! Consistent use of presenter pattern.All OpenAPI<->domain conversions properly delegate to the presenters package. Returning
&statuson line 99 is safe as Go's escape analysis will heap-allocate the variable.Also applies to: 83-83, 98-99
pkg/middleware/schema_validation_test.go (3)
17-43: LGTM! Well-designed test schema.The test schema provides minimal but sufficient coverage of validation features (required fields, enums, numeric constraints) without unnecessary complexity.
45-376: LGTM! Comprehensive test coverage.The test suite thoroughly covers validation success, validation failure, method/path filtering, and edge cases. The tests properly verify both handler invocation and HTTP responses.
379-393: LGTM! Clean test helper.The setupTestValidator function properly uses t.TempDir() for automatic cleanup and provides clear error messages on failure.
pkg/api/cluster_types_test.go (2)
1-54: Well-structured tests for ClusterList.Index().The tests comprehensively cover empty, single, multiple, and duplicate-ID scenarios. The duplicate-ID test correctly validates the "last wins" behavior.
56-203: Comprehensive BeforeCreate lifecycle hook tests.Good coverage of default value assignment and value preservation for all fields (ID, Kind, Generation, StatusPhase, Href). The final "Complete" test validates all defaults are applied together, which is valuable for regression detection.
pkg/api/presenters/cluster.go (1)
80-118: LGTM - ClusterStatus construction is correct.The status construction properly handles nil time pointers with zero defaults, correctly maps phases, and converts conditions to OpenAPI format.
pkg/api/presenters/node_pool.go (1)
47-139: Consistent implementation with cluster presenter.The presentation logic correctly handles all fields, generates proper fallback Hrefs, and constructs the NodePoolStatus with appropriate defaults.
pkg/handlers/cluster_status.go (2)
28-61: LGTM - List handler correctly uses presenter.The conversion to OpenAPI models now correctly uses
presenters.PresentAdapterStatus, maintaining consistency with the presenter pattern.
63-105: LGTM - Create handler follows proper patterns.The handler correctly:
- Validates the cluster exists before creating status
- Uses
presenters.ConvertAdapterStatusfor request conversion- Handles errors appropriately
- Logs aggregation failures without failing the request (good resilience)
- Returns the presented status
Based on coding guidelines, this follows the expected handler pattern.
pkg/middleware/schema_validation.go (1)
29-97: LGTM - Well-structured validation middleware.The middleware correctly:
- Buffers and restores the request body for downstream handlers
- Skips validation gracefully when no spec field is present
- Handles both
ServiceErrorand generic errors appropriately- Preserves the original request for further processing
pkg/api/presenters/cluster_test.go (9)
1-11: LGTM! Well-organized imports and package declaration.The test file follows Go conventions with appropriate imports for testing, time handling, and the Gomega assertion library.
13-26: Good helper function for test data.The
createTestClusterRequesthelper reduces duplication across tests and provides a realistic test fixture.
28-66: Comprehensive test for complete conversion.This test validates all critical paths: basic fields, defaults (Generation=1, StatusPhase="NotReady"), JSON marshaling for Spec/Labels, and empty StatusConditions initialization.
68-108: Good coverage for labels edge cases.Tests both populated labels and nil labels scenarios, ensuring the conversion handles optional fields correctly.
110-149: Thorough nested spec marshaling test.Tests complex nested structures including maps and arrays, with proper type assertions for verification.
151-229: Excellent presentation test with full field coverage.Validates all aspects of
PresentCluster: basic fields, Spec/Labels unmarshaling, Status with phase/conditions, and timestamp handling. Good use of domain types (api.ConditionTrue) and OpenAPI types (openapi.READY,openapi.TRUE).
231-268: Good edge case coverage for Href generation and empty phase defaulting.These tests verify important runtime defaults that ensure consistent API responses.
270-357: Comprehensive nil timestamp and multi-condition tests.The tests correctly verify zero-time handling for nil timestamps and proper conversion of multiple conditions with different statuses.
359-395: Valuable round-trip test ensures data integrity.This test validates that data survives the ConvertCluster → PresentCluster cycle, which is critical for API correctness.
test/integration/clusters_test.go (4)
283-351: Well-structured schema validation test with proper base schema awareness.The test correctly documents that the base schema accepts any JSON object and tests both valid and invalid spec types. The comment at lines 285-286 clearly explains the test's purpose.
353-418: Good provider-specific schema test with appropriate skip logic.The test correctly skips when using the base schema (lines 361-365) and validates field-level error details when using a provider schema. The error handling at lines 395-403 properly captures and checks errors.
420-474: Thorough error details validation test.This test validates the complete error response structure including Kind, Code, Reason, Href, and OperationId. The acceptance of both
hyperfleet-8andhyperfleet-17error codes (lines 464-465) is well-documented.
527-624: Good coverage for custom sorting by name.Both ascending and descending name sorting tests properly create test data with predictable alphabetical ordering and verify the results. The use of unique prefixes (lines 535, 586) ensures test isolation.
pkg/api/status_types.go (3)
1-13: Well-defined ResourcePhase type with clear documentation.The constants are properly documented with comments indicating their OpenAPI equivalents. The string values match the OpenAPI enum values.
15-23: ConditionStatus type correctly models tri-state condition values.The True/False/Unknown pattern follows Kubernetes condition conventions.
25-48: Well-structured condition types with appropriate JSON serialization.Both
ResourceConditionandAdapterConditionuse snake_case JSON tags for database JSONB compatibility. Optional fields (Reason,Message) correctly use pointer types withomitempty.openapi/openapi.yaml (6)
756-757: Good use of allOf for type composition.Using
allOfto referenceResourcePhaseandConditionStatusenums ensures consistent type usage across ClusterStatus, NodePoolStatus, and ConditionBase schemas.Also applies to: 806-807, 831-831, 1110-1111
839-845: ConditionStatus enum correctly defined.The enum values ('True', 'False', 'Unknown') match the domain types in
pkg/api/status_types.goand follow Kubernetes conventions.
863-874: Well-structured validation error details.The
detailsarray withfieldanderrorproperties enables clear field-level validation error reporting, improving API usability.
1190-1196: ResourcePhase enum properly defined.The enum values (NotReady, Ready, Failed) align with the domain constants in
pkg/api/status_types.go.
1197-1200: Security scheme correctly configured for bearer authentication.The
BearerAuthsecurity scheme undersecuritySchemesproperly usestype: httpwithscheme: bearer.
612-625: Verify if BearerAuth schema is referenced elsewhere in the specification.The
BearerAuthschema at lines 612-625 appears to define a data object withtypeandschemeproperties. The actual security scheme is defined at lines 1197-1200 undersecuritySchemes. Confirm whether this schema is referenced anywhere in the OpenAPI specification (via$ref,allOf, or other mechanisms). If it is not used, consider removing it as dead code per the coding guideline that TypeSpec definitions are maintained in the hyperfleet-api-spec repository.pkg/api/presenters/adapter_status_test.go (9)
1-11: Well-organized imports matching the cluster test file pattern.Consistent structure with the cluster presenter tests.
13-42: Good helper functions for test data creation.The
createTestAdapterStatusRequestprovides a complete test fixture, andstrPtris a useful utility for creating string pointers.
44-87: Comprehensive test for complete AdapterStatus conversion.Validates all fields including ResourceType, ResourceID, Adapter, ObservedGeneration, Conditions, Data, Metadata, and timestamps.
89-130: Good ObservedTime handling tests.Tests both explicit ObservedTime and zero-time fallback to
time.Now(), with appropriate time boundary checks.
132-184: Edge case tests for empty/nil fields.Proper coverage for empty conditions array, nil Data, and nil Metadata scenarios.
186-218: Table-driven test for ConditionStatus conversion.Clean implementation using test cases to verify all status value conversions (TRUE/FALSE/UNKNOWN) from OpenAPI to domain types.
220-284: Thorough presentation test with complete field verification.Tests PresentAdapterStatus with all fields populated, verifying correct conversion from domain to OpenAPI types.
286-350: Good edge case coverage for presentation.Tests nil timestamps, empty conditions JSON, and empty data JSON handling.
352-423: Excellent round-trip test ensuring bidirectional data integrity.The test validates that data survives the ConvertAdapterStatus → PresentAdapterStatus cycle, verifying all critical fields including nested conditions, data, and metadata.
yasun1
force-pushed
the
schema-validator
branch
from
fde7ccb to
1f434df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/integration/clusters_test.go (1)
332-335: Checkjson.Unmarshalerror.The error from
json.Unmarshalis being ignored, which could mask issues in the test.var errorResponse openapi.Error - json.Unmarshal(resp2.Body(), &errorResponse) + if err := json.Unmarshal(resp2.Body(), &errorResponse); err != nil { + t.Fatalf("failed to unmarshal error response: %v", err) + } Expect(errorResponse.Code).ToNot(BeNil())
🧹 Nitpick comments (4)
test/integration/adapter_status_test.go (1)
223-223: Inconsistent assertion style with lines 245 and 263.Line 223 uses
string()conversion on both sides of the comparison, but lines 245 and 263 compareopenapi.ConditionStatusvalues directly without conversion. For consistency, consider removing thestring()wrappers here since the types should be directly comparable.- Expect(string(status1.Conditions[0].Status)).To(Equal(string(openapi.FALSE))) + Expect(status1.Conditions[0].Status).To(Equal(openapi.FALSE))pkg/middleware/schema_validation.go (1)
26-26: Handle JSON encoding error.The error from
json.NewEncoder(w).Encode()is discarded. While unlikely to fail, ignoring it makes debugging harder if writes fail.- json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) + if encErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encErr != nil { + log.Errorf("Failed to encode error response: %v", encErr) + }test/integration/clusters_test.go (1)
435-435: Checkjson.Marshalerror.The error from
json.Marshalis discarded. While unlikely to fail for simple maps, consistency with other error handling in the file would be better.- body, _ := json.Marshal(invalidTypeRequest) + body, err := json.Marshal(invalidTypeRequest) + Expect(err).To(BeNil(), "Failed to marshal request")pkg/validators/schema_validator.go (1)
80-90: Consider removing deprecated methods in a follow-up.These wrapper methods are marked deprecated and simply delegate to
Validate(). If no external callers exist, consider removing them to reduce API surface.#!/bin/bash # Check if deprecated methods are used outside the validator rg -n 'ValidateClusterSpec|ValidateNodePoolSpec' --type go -g '!*_test.go' -g '!schema_validator.go'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
README.md(1 hunks)cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(7 hunks)pkg/api/adapter_status_types.go(0 hunks)pkg/api/adapter_status_types_test.go(1 hunks)pkg/api/cluster_types.go(3 hunks)pkg/api/cluster_types_test.go(1 hunks)pkg/api/node_pool_types.go(3 hunks)pkg/api/node_pool_types_test.go(1 hunks)pkg/api/presenters/adapter_status.go(1 hunks)pkg/api/presenters/adapter_status_test.go(1 hunks)pkg/api/presenters/cluster.go(1 hunks)pkg/api/presenters/cluster_test.go(1 hunks)pkg/api/presenters/node_pool.go(1 hunks)pkg/api/presenters/node_pool_test.go(1 hunks)pkg/api/status_types.go(1 hunks)pkg/api/status_types_test.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_nodepools.go(1 hunks)pkg/handlers/cluster_status.go(4 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(4 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/services/cluster.go(2 hunks)pkg/services/node_pool.go(2 hunks)pkg/services/types.go(1 hunks)pkg/services/types_test.go(1 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/api/adapter_status_types.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/api/adapter_status_types_test.go
- pkg/handlers/cluster.go
- pkg/api/status_types_test.go
- pkg/validators/schema_validator_test.go
- pkg/handlers/node_pool.go
- pkg/services/types.go
- pkg/api/cluster_types_test.go
- pkg/api/presenters/adapter_status.go
- pkg/middleware/schema_validation_test.go
- cmd/hyperfleet-api/server/routes.go
- pkg/services/node_pool.go
- pkg/services/types_test.go
- go.mod
- pkg/api/node_pool_types_test.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/presenters/cluster_test.gopkg/api/presenters/cluster.gotest/integration/integration_test.gopkg/api/presenters/node_pool.gopkg/errors/errors.gopkg/handlers/cluster_nodepools.gopkg/handlers/nodepool_status.gopkg/api/presenters/node_pool_test.gopkg/api/status_types.gopkg/handlers/cluster_status.gopkg/middleware/schema_validation.gopkg/api/presenters/adapter_status_test.gopkg/validators/schema_validator.gopkg/api/cluster_types.gopkg/api/node_pool_types.gopkg/services/cluster.gotest/integration/clusters_test.gotest/integration/adapter_status_test.go
pkg/api/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Files:
pkg/api/presenters/cluster_test.gopkg/api/presenters/cluster.gopkg/api/presenters/node_pool.gopkg/api/presenters/node_pool_test.gopkg/api/status_types.gopkg/api/presenters/adapter_status_test.gopkg/api/cluster_types.gopkg/api/node_pool_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/presenters/cluster_test.gopkg/api/presenters/cluster.gopkg/api/presenters/node_pool.gopkg/errors/errors.gopkg/handlers/cluster_nodepools.gopkg/handlers/nodepool_status.gopkg/api/presenters/node_pool_test.gopkg/api/status_types.gopkg/handlers/cluster_status.gopkg/middleware/schema_validation.gopkg/api/presenters/adapter_status_test.gopkg/validators/schema_validator.gopkg/api/cluster_types.gopkg/api/node_pool_types.gopkg/services/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/integration_test.gotest/integration/clusters_test.gotest/integration/adapter_status_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/cluster_nodepools.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/cluster.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/cluster_nodepools.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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/cluster_nodepools.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/cluster.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.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
Files:
openapi/openapi.yaml
🧠 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 test/integration/**/*.go : All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Applied to files:
pkg/api/presenters/cluster_test.gotest/integration/integration_test.gopkg/api/presenters/adapter_status_test.gotest/integration/clusters_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/integration/**/*.go : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/api/presenters/cluster_test.gotest/integration/integration_test.gotest/integration/clusters_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/factories/**/*.go : Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Applied to files:
pkg/api/presenters/cluster_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/api/**/*.go : Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Applied to files:
test/integration/integration_test.gopkg/handlers/nodepool_status.gopkg/api/status_types.gopkg/handlers/cluster_status.gopkg/api/presenters/adapter_status_test.gopkg/validators/schema_validator.gopkg/api/cluster_types.gopkg/services/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/db/migrations/**/*.go : Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Applied to files:
pkg/api/presenters/node_pool.gopkg/api/cluster_types.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 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:
README.mdpkg/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 cmd/hyperfleet/environments/**/*.go : Use OCM_ENV environment variable to select configuration environment (development, unit_testing, integration_testing, production)
Applied to files:
README.md
📚 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/errors/errors.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/middleware/schema_validation.gopkg/services/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/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.gopkg/api/status_types.gopkg/handlers/cluster_status.gopkg/api/presenters/adapter_status_test.gopkg/services/cluster.gotest/integration/adapter_status_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/handlers/nodepool_status.gopkg/api/status_types.gopkg/handlers/cluster_status.gopkg/api/presenters/adapter_status_test.gopkg/services/cluster.gotest/integration/adapter_status_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/**/*.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/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/**/*.go : Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Applied to files:
pkg/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/api/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/validators/schema_validator.gopkg/services/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/{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/services/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: Generate OpenAPI code by running make generate whenever the TypeSpec specification changes
Applied to files:
openapi/openapi.yaml
🧬 Code graph analysis (9)
pkg/api/presenters/cluster_test.go (4)
pkg/api/presenters/cluster.go (2)
ConvertCluster(13-48)PresentCluster(51-131)pkg/api/status_types.go (3)
ResourceCondition(28-37)ConditionTrue(20-20)ConditionFalse(21-21)pkg/api/cluster_types.go (1)
Cluster(11-34)pkg/errors/errors.go (1)
Href(182-184)
pkg/handlers/nodepool_status.go (2)
pkg/api/presenters/adapter_status.go (2)
PresentAdapterStatus(76-127)ConvertAdapterStatus(14-73)pkg/errors/errors.go (1)
GeneralError(190-192)
pkg/middleware/schema_validation.go (4)
pkg/errors/errors.go (3)
ServiceError(100-109)MalformedRequest(225-227)Validation(214-216)pkg/logger/logger.go (1)
NewOCMLogger(36-44)pkg/logger/operationid_middleware.go (1)
GetOperationID(38-43)pkg/validators/schema_validator.go (1)
SchemaValidator(19-22)
pkg/api/presenters/adapter_status_test.go (4)
pkg/api/metadata_types.go (1)
Metadata(28-34)pkg/api/presenters/adapter_status.go (2)
ConvertAdapterStatus(14-73)PresentAdapterStatus(76-127)pkg/api/status_types.go (5)
AdapterCondition(42-48)ConditionTrue(20-20)ConditionStatus(17-17)ConditionFalse(21-21)ConditionUnknown(22-22)pkg/api/adapter_status_types.go (1)
AdapterStatus(11-30)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (2)
ValidationWithDetails(219-223)ValidationDetail(68-71)
pkg/api/cluster_types.go (2)
pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/status_types.go (1)
PhaseNotReady(10-10)
pkg/api/node_pool_types.go (2)
pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/status_types.go (1)
PhaseNotReady(10-10)
pkg/services/cluster.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
test/integration/clusters_test.go (3)
test/registration.go (1)
RegisterIntegration(13-24)pkg/errors/errors.go (1)
Href(182-184)pkg/api/cluster_types.go (1)
Cluster(11-34)
🔇 Additional comments (45)
README.md (1)
263-304: Verify Helm configuration and runtime default path alignment with actual chart templates and codebase.The new Configuration section clearly documents OPENAPI_SCHEMA_PATH and provides practical examples. However, the following details require manual verification:
- Helm chart configuration (lines 298–301): Confirm that the Helm chart template uses
provider.schemaas the configuration key and that the volume mount path is/etc/hyperfleet/schemas/openapi.yaml.- Default path runtime behavior (line 271): Verify that
openapi/openapi.yamlis the actual default path used at runtime and confirm whether it's relative to the working directory or repository root.- Provider-specific schema constraints: The "How it works" section (lines 290–294) documents validation behavior but does not address the PR concern about provider-specific schemas potentially including constraints (e.g., mandatory headers, provider-specific pagination limits) that may not apply to hyperfleet-api. Consider adding a note about this limitation.
test/integration/adapter_status_test.go (1)
245-245: Good improvement: type-safe enum comparisons.Replacing string literals with
openapi.TRUEenum constants ensures compile-time detection of enum changes and improves type safety.Also applies to: 263-263
pkg/services/cluster.go (3)
133-134: Good architectural improvement: using domain types instead of OpenAPI types.The switch from
openapi.ResourceConditiontoapi.ResourceConditionproperly separates the domain layer from the API layer. This aligns with the new status types defined inpkg/api/status_types.go.
139-151: Correct type alignment for condition unmarshaling.The unmarshal to
api.AdapterConditionproperly aligns with the JSON tags defined inpkg/api/status_types.go, and the "Available" condition lookup logic is preserved.
159-178: ResourceCondition construction properly handles all fields.The construction correctly maps adapter condition data to the domain
ResourceConditiontype, including proper nil-checking for timestamp fields.pkg/errors/errors.go (4)
67-71: Well-structured validation detail type.The
ValidationDetailstruct cleanly captures field-level validation errors with appropriate JSON tags for API serialization.
107-108: Backward-compatible extension to ServiceError.Adding the optional
Detailsfield enables richer validation error responses without breaking existing error handling code.
153-176: Correct conversion of validation details to OpenAPI format.The conditional population of
Detailsonly when errors exist avoids cluttering API responses, and the conversion toopenapi.ErrorDetailsInnercorrectly uses pointer helpers.
218-223: Clean helper for validation errors with field details.The
ValidationWithDetailshelper provides a convenient way to create validation errors with field-level details attached.pkg/api/status_types.go (2)
25-48: Well-structured domain condition types.The separation between
ResourceCondition(resource-level with generation and timestamps) andAdapterCondition(simpler adapter-reported conditions) is appropriate. JSON tags correctly use snake_case for JSONB storage compatibility.
9-13: Verify ResourcePhase string values match OpenAPI constants.The string values "NotReady", "Ready", and "Failed" in the ResourcePhase constants should match the corresponding OpenAPI constants. If OpenAPI uses different formatting (e.g., snake_case like "not_ready"), mismatches could cause subtle bugs in status comparisons and serialization. Verify the actual OpenAPI constant definitions in
pkg/api/openapi/to ensure consistency.pkg/api/presenters/adapter_status_test.go (4)
13-42: Well-designed test helper functions.The
createTestAdapterStatusRequest()helper creates a comprehensive test fixture with all fields populated, andstrPtr()is a useful utility for pointer creation. Good test organization.
44-225: Comprehensive conversion test coverage.The tests thoroughly validate
ConvertAdapterStatusincluding:
- Complete field mapping
- ObservedTime handling (specific vs fallback to time.Now)
- Edge cases (empty conditions, nil data/metadata)
- All condition status values via table-driven tests
Good use of time tolerance checks for non-deterministic timestamps.
293-313: Good edge case coverage for nil timestamps.Testing nil timestamp handling ensures the presenter gracefully returns zero
time.Timevalues rather than panicking or producing unexpected behavior.
401-431: Excellent round-trip integrity test.The
TestConvertAndPresent_RoundTriptest validates that data survives the full conversion cycle (OpenAPI → domain → OpenAPI) without loss or corruption. This is valuable for catching subtle serialization issues.pkg/handlers/cluster_nodepools.go (1)
142-146: LGTM! Clean migration to presenters layer with proper error handling.The replacement of direct OpenAPI conversion with
presenters.ConvertNodePoolappropriately centralizes the conversion logic and includes proper error handling for conversion failures.test/integration/integration_test.go (1)
18-45: LGTM! Path resolution is now robust and safe.The schema path computation now uses proper
filepathoperations, checks theokreturn fromruntime.Caller, and verifies file existence before setting the environment variable. The warning logs provide clear diagnostics if setup fails. This addresses the fragile path computation concerns from the previous review.pkg/handlers/nodepool_status.go (2)
83-86: LGTM! Consistent use of presenters layer.The migration to
presenters.ConvertAdapterStatuswith proper error handling aligns with the broader refactoring and maintains consistency with similar changes incluster_status.go.
44-44: LGTM! Presentation layer properly integrated.The use of
presenters.PresentAdapterStatuscorrectly separates presentation logic from handler logic and maintains consistency across the codebase.Also applies to: 101-102
pkg/api/presenters/cluster_test.go (1)
1-400: LGTM! Comprehensive test coverage for cluster presenters.The test suite thoroughly validates:
- Complete conversion and presentation flows
- JSON marshaling/unmarshaling of Spec, Labels, and StatusConditions
- Default handling (StatusPhase, timestamps, Href generation)
- Multiple status conditions conversion
- Round-trip data integrity (ConvertCluster → PresentCluster)
The tests are well-structured and use appropriate assertions. This provides strong confidence in the presenters layer implementation.
pkg/api/presenters/node_pool_test.go (1)
1-428: LGTM! Comprehensive test coverage for node pool presenters.The test suite thoroughly validates:
- Complete conversion and presentation flows
- Kind defaulting and explicit Kind handling
- JSON marshaling/unmarshaling of Spec, Labels, and StatusConditions
- Default handling (StatusPhase, timestamps, Href/OwnerHref generation)
- OwnerReferences construction
- Multiple status conditions conversion
- Round-trip data integrity (ConvertNodePool → PresentNodePool)
The tests are well-structured and mirror the cluster test patterns, ensuring consistency across the presenters layer.
pkg/api/presenters/cluster.go (2)
12-48: LGTM! JSON marshaling errors now properly handled.The updated
ConvertClusternow returns errors for all JSON marshaling failures, preventing silent data loss. This addresses the concerns from the previous review. The error messages provide clear context with wrapped errors.
50-131: LGTM! Presentation logic properly handles defaults and conversions.The
PresentClusterfunction correctly:
- Unmarshals JSON fields (Spec, Labels, StatusConditions)
- Generates fallback Href when not set
- Applies default phase (NOT_READY) when StatusPhase is empty
- Handles nil timestamps by returning zero values
- Converts domain ResourceConditions to OpenAPI format
The silently ignored unmarshal errors in presentation are acceptable as they allow partial data display rather than failing the entire response.
pkg/api/presenters/node_pool.go (2)
12-54: LGTM! JSON marshaling errors now properly handled.The updated
ConvertNodePoolnow returns errors for all JSON marshaling failures, preventing silent data loss. This addresses the concerns from the previous review. The error messages provide clear context with wrapped errors.
56-148: LGTM! Presentation logic properly handles defaults, conversions, and owner references.The
PresentNodePoolfunction correctly:
- Unmarshals JSON fields (Spec, Labels, StatusConditions)
- Generates fallback Href and OwnerHref when not set
- Applies default phase (NOT_READY) when StatusPhase is empty
- Handles nil timestamps by returning zero values
- Converts domain ResourceConditions to OpenAPI format
- Builds OwnerReferences with proper fields
The silently ignored unmarshal errors in presentation are acceptable as they allow partial data display rather than failing the entire response.
pkg/handlers/cluster_status.go (2)
83-86: LGTM! Consistent use of presenters layer.The migration to
presenters.ConvertAdapterStatuswith proper error handling aligns with the broader refactoring and maintains consistency with similar changes innodepool_status.go.
44-44: LGTM! Presentation layer properly integrated.The use of
presenters.PresentAdapterStatuscorrectly separates presentation logic from handler logic and maintains consistency across the codebase.Also applies to: 101-102
pkg/middleware/schema_validation.go (2)
29-97: LGTM!The middleware correctly buffers the request body for reuse, extracts the spec field, and delegates validation to the SchemaValidator. Error handling properly distinguishes between
ServiceErrorwith details and generic errors.
99-121: LGTM!The function correctly prioritizes
/nodepoolspath matching before/clustersto handle the nested resource structure. Method filtering is properly scoped within this function.test/integration/clusters_test.go (3)
361-365: Verify skip logic handles all path variations.The check
strings.HasSuffix(schemaPath, "openapi/openapi.yaml")may not correctly identify the base schema if the path uses different separators or naming conventions (e.g.,openapi.yamlvsapi/openapi.yaml).Consider also checking for the filename alone or using
filepath.Base():+import "path/filepath" + // Check if we're using a provider schema or base schema -if schemaPath == "" || strings.HasSuffix(schemaPath, "openapi/openapi.yaml") { +baseSchemaName := filepath.Base(schemaPath) +if schemaPath == "" || baseSchemaName == "openapi.yaml" { t.Skip("Skipping provider schema validation test - using base schema")
476-525: LGTM!The default sorting test correctly verifies that clusters are returned in
created_time descorder. Using 100ms delays ensures distinct timestamps for reliable ordering verification.
527-624: LGTM!Good coverage of custom sorting by name in both ascending and descending order. The tests use unique prefixes to isolate test data and verify the expected order.
pkg/api/node_pool_types.go (2)
53-76: LGTM!The
BeforeCreatehook correctly initializes timestamps and uses the type-safePhaseNotReadyconstant instead of a string literal.
78-81: LGTM!The new
BeforeUpdatehook ensuresUpdatedTimeis refreshed on every update, maintaining consistency with theClustertype.pkg/api/cluster_types.go (1)
1-78: LGTM!The changes correctly address past review feedback by removing OpenAPI dependencies from domain types. The lifecycle hooks (
BeforeCreate,BeforeUpdate) are consistent withNodePoolimplementation, and usingstring(PhaseNotReady)provides compile-time safety for status constants.pkg/validators/schema_validator.go (3)
24-66: LGTM!The constructor properly loads and validates the OpenAPI spec, extracts the required schemas, and builds a map for efficient lookup. Good error messages for missing schemas.
92-108: LGTM!The unnecessary JSON marshal/unmarshal cycle has been removed per previous review feedback. The spec is now passed directly to
VisitJSONasinterface{}.
110-163: LGTM!The error conversion logic handles
MultiErrorrecursively and extracts field paths fromSchemaErrorusing multiple strategies (SchemaField, JSONPointer, message parsing). This provides detailed validation feedback to API consumers.openapi/openapi.yaml (7)
1190-1196: ResourcePhase enum definition looks good.The new
ResourcePhaseenum correctly defines the three resource phases (NotReady, Ready, Failed) for cluster and nodepool status tracking. The values align with the PR objectives for schema validation.
839-845: ConditionStatus enum aligns with Kubernetes conventions.The new
ConditionStatusenum correctly defines the standard Kubernetes condition statuses (True, False, Unknown) for conditions across adapter and resource statuses.
756-757: Phase properties now reference ResourcePhase via allOf.Both
ClusterStatus.phaseandNodePoolStatus.phasenow useallOfto referenceResourcePhaseinstead of direct enum definitions. This indirection is structurally valid and enables centralized enum management, supporting the PR goal of replacing string literals with schema-validated constants.Also applies to: 1110-1111
806-807: ConditionBase and ConditionRequest status fields now reference ConditionStatus.The updates to
ConditionBase.status(via allOf) andConditionRequest.status(direct reference) now consistently reference the newConditionStatusenum, supporting type-safe comparisons across adapter and resource conditions.Also applies to: 831-831
863-874: Error schema now includes field-level validation details.The new
detailsfield in theErrorschema enables the API to return structured, field-level validation errors (each withfieldanderrorproperties). This aligns with the PR objective to surface schema validation issues with better diagnostics.
1-1: Verify that openapi.yaml changes were generated from TypeSpec.Per the coding guidelines and prior review history, TypeSpec definitions are maintained in the separate hyperfleet-api-spec repository, and
openapi/openapi.yamlshould be regenerated by runningmake generateafter any TypeSpec changes. Please confirm that this file was generated (not manually edited) and thatmake generatewas executed as part of this PR.You can verify this by checking that the changes are consistent with TypeSpec definitions and by confirming the generation step completed successfully. If these changes were manually edited rather than generated, they may not align with the authoritative TypeSpec source.
Also applies to: 1-10
612-625: Remove the BearerAuth schema definition fromcomponents/schemas.BearerAuth is a security scheme (correctly defined at lines 1198-1200 in
components/securitySchemes), not a data type. Per OpenAPI 3.0 specification, security schemes should not have corresponding schema definitions incomponents/schemas. The schema entry at lines 612-625 appears redundant and should be removed unless there is a specific use case for it elsewhere in the specification.If this schema definition is necessary, clarify its purpose in the PR description or code comments.
yasun1
force-pushed
the
schema-validator
branch
from
1f434df to
5c87f98
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/integration/clusters_test.go (1)
332-335: Check json.Unmarshal error as flagged in past review.This was previously flagged but not addressed. The error from
json.Unmarshalshould be checked.var errorResponse openapi.Error - json.Unmarshal(resp2.Body(), &errorResponse) + err := json.Unmarshal(resp2.Body(), &errorResponse) + Expect(err).To(BeNil(), "Failed to unmarshal error response") Expect(errorResponse.Code).ToNot(BeNil())
🧹 Nitpick comments (13)
pkg/api/presenters/cluster.go (2)
52-68: Consider logging unmarshal failures instead of silently ignoring them.Lines 55, 61, and 67 discard
json.Unmarshalerrors. While the presentation layer shouldn't fail on read, silently ignoring unmarshal errors could mask data corruption issues in the database. Consider adding debug/warning logs to aid troubleshooting without failing the request.// Unmarshal Spec var spec map[string]interface{} if len(cluster.Spec) > 0 { - _ = json.Unmarshal(cluster.Spec, &spec) + if err := json.Unmarshal(cluster.Spec, &spec); err != nil { + // Log but don't fail - data was validated on write + // Consider adding: log.Warnf("failed to unmarshal cluster spec: %v", err) + } }
41-43: Use thePhaseNotReadyconstant for consistency.Line 42 uses the string literal
"NotReady"whilecluster_types.gousesstring(PhaseNotReady). Using the constant ensures consistency and safer refactoring.Generation: 1, - StatusPhase: "NotReady", + StatusPhase: string(api.PhaseNotReady), StatusObservedGeneration: 0,pkg/api/cluster_types.go (1)
11-12: Minor: Comment refers toDeletedTimebut field isDeletedAt.The embedded
Metacomment mentions "DeletedTime" but the actual field inMeta(frompkg/api/metadata_types.go) isDeletedAt.// Cluster database model type Cluster struct { - Meta // Contains ID, CreatedTime, UpdatedTime, DeletedTime + Meta // Contains ID, CreatedTime, UpdatedTime, DeletedAttest/integration/adapter_status_test.go (1)
223-223: Inconsistent assertion style compared to lines 245 and 263.This assertion uses
string()conversion on both sides, while the assertions on lines 245 and 263 compareopenapi.ConditionStatusvalues directly. Consider using the direct comparison for consistency:- Expect(string(status1.Conditions[0].Status)).To(Equal(string(openapi.FALSE))) + Expect(status1.Conditions[0].Status).To(Equal(openapi.FALSE))pkg/services/node_pool.go (1)
139-142: Silent skip on unmarshal error may hide data corruption.The current logic silently skips adapters when JSON unmarshalling fails. While this prevents a single corrupted record from breaking aggregation, consider logging the error for observability:
var conditions []api.AdapterCondition if err := json.Unmarshal(adapterStatus.Conditions, &conditions); err != nil { + log := logger.NewOCMLogger(ctx) + log.Extra("adapter", adapterStatus.Adapter).Extra("error", err).Warning("Failed to unmarshal adapter conditions") continue // Skip if can't unmarshal }pkg/services/types_test.go (1)
197-205: Use Gomega matchers consistently instead of reflect.DeepEqual.This test uses
reflect.DeepEqualandt.Errorfwhile all other tests in this file use Gomega matchers. For consistency and better error messages, useExpect().To(Equal()).for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { RegisterTestingT(t) listArgs := NewListArguments(tt.queryParams) - if !reflect.DeepEqual(listArgs.Fields, tt.expectedFields) { - t.Errorf("Fields = %v, want %v", listArgs.Fields, tt.expectedFields) - } + Expect(listArgs.Fields).To(Equal(tt.expectedFields), "Fields mismatch") }) }You can also remove the
"reflect"import after this change.pkg/api/presenters/node_pool_test.go (2)
126-131: Check json.Unmarshal error for test reliability.The error from
json.Unmarshalis ignored, which could mask test failures if the Labels JSON is malformed.var resultLabels map[string]string - json.Unmarshal(result.Labels, &resultLabels) + err = json.Unmarshal(result.Labels, &resultLabels) + Expect(err).To(BeNil(), "Failed to unmarshal labels") Expect(resultLabels["environment"]).To(Equal("production"))
146-148: Check json.Unmarshal error for consistency.Same issue as above - error is ignored.
var resultLabels map[string]string - json.Unmarshal(result.Labels, &resultLabels) + err = json.Unmarshal(result.Labels, &resultLabels) + Expect(err).To(BeNil(), "Failed to unmarshal labels") Expect(len(resultLabels)).To(Equal(0)) // Empty mappkg/middleware/schema_validation.go (2)
23-27: Consider handling JSON encoding error.The error from
json.NewEncoder(w).Encode()is ignored. While unlikely to fail after headers are written, a failure here would result in an incomplete response body.// Write JSON error response w.Header().Set("Content-Type", "application/json") w.WriteHeader(err.HttpCode) - json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) + if encErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encErr != nil { + log.Warningf("Failed to encode error response: %v", encErr) + }
107-118: Path matching could match unintended routes.The
strings.Contains(path, "/nodepools")check on line 110 could match unrelated paths like/api/v1/nodepoolsettingsor/api/v1/resources/nodepoolsmeta. Similarly, line 116's cluster matching could affect unexpected endpoints.Consider using more precise path patterns or leveraging the router's path variables:
- // Check nodepools first (more specific path) - // POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools - // PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} - if strings.Contains(path, "/nodepools") { - return true, "nodepool" - } - - // POST /api/hyperfleet/v1/clusters - // PATCH /api/hyperfleet/v1/clusters/{cluster_id} - if strings.HasSuffix(path, "/clusters") || strings.Contains(path, "/clusters/") { - return true, "cluster" - } + // Match nodepool paths more precisely + // POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools + // PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} + if strings.Contains(path, "/clusters/") && strings.Contains(path, "/nodepools") { + return true, "nodepool" + } + + // Match cluster paths - exclude nodepool sub-resources + // POST /api/hyperfleet/v1/clusters + // PATCH /api/hyperfleet/v1/clusters/{cluster_id} + if (strings.HasSuffix(path, "/clusters") || strings.Contains(path, "/clusters/")) && !strings.Contains(path, "/nodepools") { + return true, "cluster" + }test/integration/clusters_test.go (1)
435-435: Check json.Marshal error.The error from
json.Marshalis ignored, which could cause test failures to go unnoticed.- body, _ := json.Marshal(invalidTypeRequest) + body, err := json.Marshal(invalidTypeRequest) + Expect(err).To(BeNil(), "Failed to marshal request body")pkg/validators/schema_validator.go (2)
68-78: Silent skip on unknownresourceTypemay hide wiring mistakes.Right now an unrecognized
resourceTypejust returnsnil, which makes mis-typed keys or incomplete wiring indistinguishable from successful validation. Consider at least logging, or optionally returning a distinct error (or feature-flagging strict mode), so that adding new resource types or typos don’t quietly bypass validation.
92-163: Combine JSONPointer and SchemaField to improve nested validation error messages.The current
convertValidationErrorfunction usesSchemaFieldorJSONPointer(), but for nested objects kin-openapi populates both:JSONPointer()provides the data path context (e.g.,["gcp"]) whileSchemaFieldprovides the failing keyword/field name (e.g.,"region"for a missing required field). Using only one loses nesting information.The suggested refactor should combine both when available:
field := prefix + jsonPath := strings.Join(e.JSONPointer(), ".") + switch { + case jsonPath != "" && e.SchemaField != "": + field = prefix + "." + jsonPath + "." + e.SchemaField + case jsonPath != "": + field = prefix + "." + jsonPath + case e.SchemaField != "": + field = prefix + "." + e.SchemaField + } - if e.SchemaField != "" { - field = prefix + "." + e.SchemaField - } else if len(e.JSONPointer()) > 0 { - jsonPath := strings.Join(e.JSONPointer(), ".") - if jsonPath != "" { - field = prefix + "." + jsonPath - } - }This yields more precise
Fieldvalues for nested validation errors (e.g.,spec.gcp.regioninstead ofspec.region), making error messages actionable while preserving current behavior for top-level fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
README.md(1 hunks)cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(13 hunks)pkg/api/adapter_status_types.go(0 hunks)pkg/api/adapter_status_types_test.go(1 hunks)pkg/api/cluster_types.go(3 hunks)pkg/api/cluster_types_test.go(1 hunks)pkg/api/node_pool_types.go(3 hunks)pkg/api/node_pool_types_test.go(1 hunks)pkg/api/presenters/adapter_status.go(1 hunks)pkg/api/presenters/adapter_status_test.go(1 hunks)pkg/api/presenters/cluster.go(1 hunks)pkg/api/presenters/cluster_test.go(1 hunks)pkg/api/presenters/node_pool.go(1 hunks)pkg/api/presenters/node_pool_test.go(1 hunks)pkg/api/status_types.go(1 hunks)pkg/api/status_types_test.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_nodepools.go(1 hunks)pkg/handlers/cluster_status.go(4 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(4 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/services/cluster.go(2 hunks)pkg/services/node_pool.go(2 hunks)pkg/services/types.go(1 hunks)pkg/services/types_test.go(1 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/api/adapter_status_types.go
🚧 Files skipped from review as they are similar to previous changes (14)
- test/integration/integration_test.go
- pkg/handlers/cluster_nodepools.go
- README.md
- pkg/validators/schema_validator_test.go
- pkg/services/types.go
- pkg/api/cluster_types_test.go
- pkg/handlers/node_pool.go
- pkg/middleware/schema_validation_test.go
- pkg/api/status_types.go
- pkg/api/node_pool_types_test.go
- pkg/api/presenters/adapter_status.go
- cmd/hyperfleet-api/server/routes.go
- pkg/api/presenters/cluster_test.go
- pkg/api/status_types_test.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/presenters/node_pool_test.gopkg/api/presenters/node_pool.gopkg/services/types_test.gopkg/handlers/cluster.gopkg/api/adapter_status_types_test.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/node_pool.gotest/integration/adapter_status_test.gopkg/api/cluster_types.gotest/integration/clusters_test.gopkg/services/cluster.gopkg/api/node_pool_types.gopkg/errors/errors.gopkg/validators/schema_validator.gopkg/middleware/schema_validation.gopkg/api/presenters/adapter_status_test.gopkg/api/presenters/cluster.go
pkg/api/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Files:
pkg/api/presenters/node_pool_test.gopkg/api/presenters/node_pool.gopkg/api/adapter_status_types_test.gopkg/api/cluster_types.gopkg/api/node_pool_types.gopkg/api/presenters/adapter_status_test.gopkg/api/presenters/cluster.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/presenters/node_pool_test.gopkg/api/presenters/node_pool.gopkg/services/types_test.gopkg/handlers/cluster.gopkg/api/adapter_status_types_test.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/node_pool.gopkg/api/cluster_types.gopkg/services/cluster.gopkg/api/node_pool_types.gopkg/errors/errors.gopkg/validators/schema_validator.gopkg/middleware/schema_validation.gopkg/api/presenters/adapter_status_test.gopkg/api/presenters/cluster.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/types_test.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/node_pool.gopkg/services/cluster.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/types_test.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/node_pool.gopkg/services/cluster.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/types_test.gopkg/services/node_pool.gopkg/services/cluster.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/cluster.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_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/adapter_status_test.gotest/integration/clusters_test.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
Files:
openapi/openapi.yaml
🧠 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 pkg/db/migrations/**/*.go : Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Applied to files:
pkg/api/presenters/node_pool_test.gopkg/api/presenters/node_pool.gopkg/services/node_pool.gopkg/api/cluster_types.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 : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/api/presenters/node_pool_test.gotest/integration/clusters_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/integration/**/*.go : All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Applied to files:
pkg/api/presenters/node_pool_test.gopkg/services/types_test.gopkg/api/adapter_status_types_test.gotest/integration/clusters_test.gopkg/api/presenters/adapter_status_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_test.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/node_pool.gotest/integration/adapter_status_test.gopkg/services/cluster.gopkg/api/presenters/adapter_status_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/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_test.gopkg/handlers/nodepool_status.gopkg/handlers/cluster_status.gopkg/services/node_pool.gotest/integration/adapter_status_test.gopkg/services/cluster.gopkg/api/presenters/adapter_status_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/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.gopkg/handlers/cluster_status.gopkg/services/node_pool.gopkg/api/cluster_types.gopkg/services/cluster.gopkg/validators/schema_validator.gogo.modpkg/middleware/schema_validation.gopkg/api/presenters/adapter_status_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/**/*.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/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/{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.gopkg/handlers/cluster_status.gopkg/services/node_pool.gopkg/services/cluster.gopkg/errors/errors.gopkg/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/{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/services/node_pool.gopkg/services/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/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/services/node_pool.gopkg/services/cluster.gopkg/validators/schema_validator.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: Generate OpenAPI code by running make generate whenever the TypeSpec specification changes
Applied to files:
openapi/openapi.yaml
📚 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:
go.modpkg/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/**/*.go : Use PostgreSQL 14.2 with GORM ORM for all database operations
Applied to files:
go.mod
🧬 Code graph analysis (11)
pkg/api/presenters/node_pool_test.go (4)
pkg/api/presenters/node_pool.go (1)
PresentNodePool(57-148)pkg/api/status_types.go (2)
ResourceCondition(28-37)ConditionTrue(20-20)pkg/api/node_pool_types.go (1)
NodePool(12-40)pkg/errors/errors.go (1)
Href(182-184)
pkg/services/types_test.go (1)
pkg/services/types.go (2)
NewListArguments(25-82)MaxListSize(22-22)
pkg/handlers/cluster.go (2)
pkg/api/presenters/cluster.go (1)
ConvertCluster(13-48)pkg/errors/errors.go (1)
GeneralError(190-192)
pkg/api/adapter_status_types_test.go (1)
pkg/api/adapter_status_types.go (2)
AdapterStatusList(32-32)AdapterStatus(11-30)
pkg/handlers/nodepool_status.go (2)
pkg/api/presenters/adapter_status.go (2)
PresentAdapterStatus(76-127)ConvertAdapterStatus(14-73)pkg/errors/errors.go (1)
GeneralError(190-192)
pkg/services/node_pool.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
pkg/api/cluster_types.go (2)
pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/status_types.go (1)
PhaseNotReady(10-10)
test/integration/clusters_test.go (3)
test/registration.go (1)
RegisterIntegration(13-24)pkg/errors/errors.go (1)
Href(182-184)pkg/api/cluster_types.go (1)
Cluster(11-34)
pkg/services/cluster.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
pkg/api/presenters/adapter_status_test.go (3)
pkg/api/presenters/adapter_status.go (2)
ConvertAdapterStatus(14-73)PresentAdapterStatus(76-127)pkg/api/status_types.go (5)
AdapterCondition(42-48)ConditionTrue(20-20)ConditionStatus(17-17)ConditionFalse(21-21)ConditionUnknown(22-22)pkg/api/adapter_status_types.go (1)
AdapterStatus(11-30)
pkg/api/presenters/cluster.go (3)
pkg/api/cluster_types.go (1)
Cluster(11-34)pkg/api/status_types.go (4)
ResourceCondition(28-37)PhaseNotReady(10-10)ResourcePhase(7-7)ConditionStatus(17-17)pkg/errors/errors.go (1)
Href(182-184)
🔇 Additional comments (36)
go.mod (1)
13-20: LGTM! Dependency updates align with PR objectives and address prior security concerns.The
golang-jwt/jwt/v4upgrade to v4.5.2 addresses the previously flagged security vulnerabilities (GO-2024-3250 and GO-2025-3553). The addition ofkin-openapi v0.133.0supports the new schema validation functionality, andgorilla/muxupgrade to v1.8.0 is a reasonable maintenance update.pkg/api/presenters/cluster.go (1)
12-48: LGTM! Error handling for JSON marshaling is now properly implemented.The
ConvertClusterfunction now returns errors fromjson.Marshaloperations, addressing the previous review concern about silent data loss. The function signature change to return(*api.Cluster, error)is appropriate.pkg/api/cluster_types.go (1)
47-71: LGTM! Lifecycle hooks are well-implemented.The
BeforeCreateandBeforeUpdatehooks properly manage timestamps and defaults. Usingstring(PhaseNotReady)for the status phase default ensures type safety. The removal of OpenAPI dependencies from domain types addresses the previous review feedback about keeping business types isolated from the presentation layer.pkg/api/node_pool_types.go (1)
53-81: LGTM! NodePool lifecycle hooks are consistent with Cluster implementation.The
BeforeCreateandBeforeUpdatehooks follow the same pattern asCluster, properly managing timestamps and defaults. The use ofstring(PhaseNotReady)and the removal of OpenAPI dependencies maintain consistency with the architectural changes across the codebase.openapi/openapi.yaml (4)
740-745: LGTM! Provider-agnostic spec schemas support flexible validation.
ClusterSpecandNodePoolSpecas generic objects withadditionalProperties(implicit) allow the API to accept any provider-specific properties while still enabling base schema validation. This aligns with the PR objective of validating specs against the base OpenAPI schema while allowing provider customization.Also applies to: 1094-1099
1190-1196: LGTM! Centralized enums improve type safety.
ResourcePhaseandConditionStatusas shared enums ensure consistent phase and status values acrossClusterStatus,NodePoolStatus, and conditions. This enables compile-time detection of enum changes as noted in the PR objectives.Also applies to: 839-845
863-874: LGTM! Field-level validation error details enhance API usability.The
detailsarray in theErrorschema supports returning field-specific validation errors, which improves the developer experience when debugging schema validation failures.
612-625: Remove or clarify theBearerAuthschema object definition.The schema at lines 612-625 defines
type: objectwith enum properties fortypeandscheme. This does not conform to OpenAPI 3.0 security scheme specification, where security schemes should only usetype: httpandscheme: bearer(as correctly defined in lines 1197-1200). The object-based schema appears to model authentication configuration data rather than define a security scheme, creating redundancy and potential confusion. Verify if this schema is referenced elsewhere via$ref; if unused, remove it.test/integration/adapter_status_test.go (1)
245-245: LGTM!Direct comparison with
openapi.TRUEenum constant is the correct approach for type-safe assertions, aligning with the PR objective of replacing string literals with OpenAPI-generated enum constants.Also applies to: 263-263
pkg/api/adapter_status_types_test.go (2)
10-54: LGTM!Comprehensive test coverage for the
Index()method, including edge cases for empty lists, single/multiple items, and duplicate ID overwrite behavior.
56-94: LGTM!The
BeforeCreatetests appropriately verify ID auto-generation and field preservation. Passingnilfor the gorm.DB parameter is acceptable here since the hook only generates an ID and doesn't require database access.pkg/handlers/nodepool_status.go (2)
83-86: LGTM!The conversion to using
presenters.ConvertAdapterStatusfollows the established pattern and properly handles conversion errors witherrors.GeneralError.
101-102: LGTM!Returning a pointer to the presented status is safe here since the
handlefunction serializes the response before this closure returns.pkg/services/node_pool.go (1)
159-176: LGTM!The migration to internal
api.ResourceConditionandapi.AdapterConditiontypes is correctly implemented. The nil-safe population ofCreatedTimeandLastUpdatedTimeis good defensive coding that prevents panics from unexpected nil values.pkg/handlers/cluster.go (1)
39-47: Consider extracting the username from the authenticated request context instead of hardcoding "system".Per the coding guidelines, JWT authentication middleware extracts account ID and username from claims. The
createdByparameter should retrieve the actual authenticated user from context for proper audit trails, rather than using a hardcoded "system" value.pkg/services/types_test.go (4)
1-69: LGTM! Well-structured table-driven tests for OrderBy parsing.The test cases comprehensively cover default behavior, direction handling, multiple fields, whitespace trimming, and edge cases with empty/whitespace-only inputs.
71-80: LGTM! Default values test provides good baseline coverage.Validates the expected defaults for Page, Size, Search, and OrderBy when no query parameters are provided.
82-131: LGTM! PageSize tests cover important precedence and boundary scenarios.Good coverage of legacy
sizeparameter,pageSizeprecedence, negative values, and MaxListSize clamping.
133-165: LGTM! Search parameter tests are concise and complete.Covers empty, populated, and whitespace-trimming scenarios appropriately.
pkg/services/cluster.go (1)
133-176: LGTM! Type migration from OpenAPI to domain types is correctly implemented.The switch to
api.AdapterConditionandapi.ResourceConditionaligns with the broader refactor to separate domain types from OpenAPI presentation. Nil checks forCreatedTimeandLastReportTimeproperly guard against nil pointer dereferences.pkg/api/presenters/node_pool_test.go (3)
29-70: LGTM! Thorough test coverage for ConvertNodePool with all fields populated.The test validates Kind, Name, OwnerID, OwnerKind, CreatedBy, UpdatedBy, status defaults, and proper JSON marshaling of Spec, Labels, and StatusConditions.
151-234: LGTM! Comprehensive PresentNodePool test with full field verification.Excellent coverage of basic fields, Spec/Labels unmarshaling, OwnerReferences, Status with conditions, and timestamp handling.
386-428: LGTM! Round-trip test validates data integrity through convert/present cycle.This test ensures that data survives the ConvertNodePool → database simulation → PresentNodePool flow without corruption.
pkg/api/presenters/node_pool.go (3)
12-54: LGTM! ConvertNodePool properly handles marshaling errors.The function now returns errors from JSON marshaling operations, addressing the previous review feedback. Default values for Kind, OwnerKind, StatusPhase, and StatusObservedGeneration are appropriately set.
58-74: Silent unmarshal errors in presentation layer - verify this is intentional.Lines 61, 67, and 73 silently ignore
json.Unmarshalerrors. While this may be intentional for graceful degradation in the presentation layer (returning empty values rather than failing), corrupted JSONB data would go unnoticed.Consider whether logging these errors at debug/warn level would aid troubleshooting without breaking the API response:
if err := json.Unmarshal(nodePool.Spec, &spec); err != nil { // Log warning for debugging // logger.Warn("Failed to unmarshal nodepool spec", "id", nodePool.ID, "error", err) }
124-137: LGTM! Condition conversion correctly maps domain types to OpenAPI format.The loop properly converts each
api.ResourceConditiontoopenapi.ResourceConditionwith all fields mapped, including the type cast forConditionStatus.pkg/handlers/cluster_status.go (1)
83-102: LGTM! Proper error handling and presenter usage.The conversion to use
presenters.ConvertAdapterStatusandpresenters.PresentAdapterStatusfollows the established pattern. Error handling for the conversion properly returns a structuredServiceError. The handler correctly follows the pattern: parse request → call service/DAO → handle errors → send response.pkg/middleware/schema_validation.go (1)
40-50: LGTM! Request body buffering and restoration is correctly implemented.The body is read, closed, and restored using
io.NopCloser(bytes.NewBuffer(bodyBytes))so downstream handlers can read it again. This is the standard pattern for middleware that needs to inspect request bodies.test/integration/clusters_test.go (3)
284-351: LGTM! Schema validation integration test covers key scenarios.Good coverage for valid specs, invalid spec types (using raw JSON to bypass typed client), and empty specs. The conditional logging for base schema behavior is appropriate since the base schema may accept any object.
476-525: LGTM! Default sorting test validates created_time desc behavior.The test creates clusters with deliberate delays to ensure different timestamps, then verifies they appear in reverse creation order. The logic to find test clusters within the full list is sound.
527-624: LGTM! Custom sorting tests cover name asc and desc ordering.Both tests use unique prefixes to isolate test data and properly verify alphabetical ordering in both directions.
pkg/errors/errors.go (3)
67-71: LGTM! ValidationDetail struct provides clean field-level error representation.The struct cleanly captures field path and error message for validation failures, enabling detailed API error responses.
153-176: LGTM! AsOpenapiError correctly converts validation details to OpenAPI format.The conditional conversion of
Detailstoopenapi.ErrorDetailsInneris well-implemented, maintaining backward compatibility when no details are present.
218-223: LGTM! ValidationWithDetails provides a clean API for field-level validation errors.This helper follows the established pattern of other error constructors while adding the ability to attach detailed field information.
pkg/validators/schema_validator.go (1)
24-66: OpenAPI loading and core schema extraction look solid.The constructor cleanly loads the spec, validates it once, and fails fast if
ClusterSpec/NodePoolSpecare missing. The explicitschemasmap with"cluster"/"nodepool"keys and type names is straightforward and easy to extend later. I don’t see functional issues here.pkg/api/presenters/adapter_status_test.go (1)
13-431: Comprehensive adapter status conversion tests align well with the presenter API.This test suite does a good job exercising both directions of conversion (OpenAPI → domain and back), including:
- All
ConditionStatusvalues via the generatedopenapienums and the domain constants.- Presence/absence of
ObservedTime, and nil vs empty JSON forConditions,Data, andMetadata.- Round‑trip integrity checks over conditions, data, and metadata.
The time-based assertions are bounded to avoid flakiness, and expectations are consistent with the presenter implementation in
pkg/api/presenters/adapter_status.go. I don’t see issues that would block this from merging.
yasun1
force-pushed
the
schema-validator
branch
from
5c87f98 to
a05f543
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 (1)
pkg/services/cluster.go (1)
133-207:StatusObservedGenerationis computed as max, but the spec requires the lowest value.Here you track
maxObservedGenerationacross adapter statuses and assign it tocluster.StatusObservedGeneration. The OpenAPI description forClusterStatus.observed_generationexplicitly states this “will be the lowest value of each condition's observed_generation values; the phase value is based on this generation.” Using the maximum means the cluster can report a generation that some adapters haven’t actually reached.Consider tracking the minimum observed generation instead and only setting it when you have at least one adapter:
- // Build the list of ResourceCondition - adapters := []api.ResourceCondition{} - maxObservedGeneration := int32(0) + // Build the list of ResourceCondition + adapters := []api.ResourceCondition{} + var minObservedGeneration int32 + var hasObservedGeneration bool @@ - // Track max observed generation - if adapterStatus.ObservedGeneration > maxObservedGeneration { - maxObservedGeneration = adapterStatus.ObservedGeneration - } + // Track min observed generation across adapters + if !hasObservedGeneration || adapterStatus.ObservedGeneration < minObservedGeneration { + minObservedGeneration = adapterStatus.ObservedGeneration + hasObservedGeneration = true + } @@ - cluster.StatusObservedGeneration = maxObservedGeneration + if hasObservedGeneration { + cluster.StatusObservedGeneration = minObservedGeneration + } else { + cluster.StatusObservedGeneration = 0 + }You may also want to mirror this behavior anywhere else
StatusObservedGenerationis computed (e.g., nodepool services) to keep the semantics consistent.
♻️ Duplicate comments (1)
test/integration/clusters_test.go (1)
332-335: Handle JSON unmarshal error in test.Line 333 ignores the error from
json.Unmarshal, which could mask parsing issues and make test failures harder to diagnose.Apply this diff:
// Verify error response contains details var errorResponse openapi.Error - json.Unmarshal(resp2.Body(), &errorResponse) + err := json.Unmarshal(resp2.Body(), &errorResponse) + Expect(err).To(BeNil(), "Failed to unmarshal error response") Expect(errorResponse.Code).ToNot(BeNil())
🧹 Nitpick comments (11)
pkg/api/presenters/cluster_test.go (1)
88-91: Uncheckedjson.Unmarshalerror could mask test failures.The error from
json.Unmarshalis ignored here. If unmarshaling fails, the test would still pass with a nil map, potentially hiding issues.var resultLabels map[string]string -json.Unmarshal(result.Labels, &resultLabels) +err = json.Unmarshal(result.Labels, &resultLabels) +Expect(err).To(BeNil()) Expect(resultLabels["env"]).To(Equal("production"))The same pattern appears at lines 109 and 139—consider checking those as well.
pkg/validators/schema_validator_test.go (1)
350-351: Remove stale comment.The comment references importing the errors package, but it's already imported at line 9. This appears to be leftover from development.
func getServiceError(err error) *errors.ServiceError { if err == nil { return nil } - // Import errors package at the top - // This is a type assertion to check if err is a *errors.ServiceError if serviceErr, ok := err.(*errors.ServiceError); ok { return serviceErr } return nil }pkg/middleware/schema_validation.go (2)
23-27: Unchecked error from JSON encoding.If
json.NewEncoder(w).Encode()fails (e.g., client disconnects), the error is silently ignored. While unlikely to cause issues, logging the error would aid debugging.// Write JSON error response w.Header().Set("Content-Type", "application/json") w.WriteHeader(err.HttpCode) -json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) +if encErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encErr != nil { + log.Warningf("Failed to encode error response: %v", encErr) +}
110-118: Path matching may produce false positives.The
strings.Contains(path, "/nodepools")could match paths like/api/v1/some-nodepools-adminor other unintended routes. Similarly,strings.Contains(path, "/clusters/")could match/api/v1/some-clusters-report/.Consider using more precise pattern matching or relying on the router's path parameters.
-// Check nodepools first (more specific path) -// POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools -// PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} -if strings.Contains(path, "/nodepools") { - return true, "nodepool" -} - -// POST /api/hyperfleet/v1/clusters -// PATCH /api/hyperfleet/v1/clusters/{cluster_id} -if strings.HasSuffix(path, "/clusters") || strings.Contains(path, "/clusters/") { - return true, "cluster" -} +// Check nodepools first (more specific path) +// POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools +// PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} +if strings.Contains(path, "/clusters/") && strings.Contains(path, "/nodepools") { + return true, "nodepool" +} + +// POST /api/hyperfleet/v1/clusters +// PATCH /api/hyperfleet/v1/clusters/{cluster_id} +if strings.HasSuffix(path, "/clusters") || + (strings.Contains(path, "/clusters/") && !strings.Contains(path, "/nodepools")) { + return true, "cluster" +}This ensures nodepool paths are only matched when they follow the expected
/clusters/{id}/nodepoolspattern.pkg/validators/schema_validator.go (1)
68-78: Consider surfacing unknownresourceTypevalues for easier debugging.
ValidatereturnsnilwhenresourceSchemais missing, which is convenient but can also hide typos or routing bugs. You might consider logging a debug/warn when the type is unknown, or making the allowed resource types explicit at call sites so unexpected values are caught early.openapi/openapi.yaml (1)
612-625: Clarify or removecomponents.schemas.BearerAuthif it isn’t meant to be a data schema.You now define
BearerAuthboth undercomponents.schemas(as an object withtype/scheme) and undercomponents.securitySchemes(as the actual HTTP bearer auth scheme). If the schema form isn’t intentionally exposed in any request/response payloads, consider removing or renaming it to avoid confusion and potential codegen name clashes with the security scheme.test/integration/adapter_status_test.go (1)
223-224: You can drop the extrastring(...)casts in the first status assertion.In
Expect(string(status1.Conditions[0].Status)).To(Equal(string(openapi.FALSE))), both operands already share the same underlying string value; for consistency with the later checks you can comparestatus1.Conditions[0].Statusdirectly againstopenapi.FALSE.pkg/services/cluster.go (1)
139-143: Log condition unmarshal failures instead of silently skipping adapters.If
json.Unmarshal(adapterStatus.Conditions, &conditions)fails, the code currently justcontinues, which can hide malformed rows and make debugging harder. Emitting a structured warning (e.g., vialogger.NewOCMLogger(ctx).Warnfwith the adapter ID and error) would preserve behavior while improving observability, in line with the structured logging guidance.pkg/api/presenters/node_pool.go (1)
58-147: Small presentation tweaks to consider for kind, labels, and fallback hrefs.A few minor refinements you might want to make over time:
- If
nodePool.Kindis empty, default it to"NodePool"before taking its address so older rows still present a consistent kind.- When
len(nodePool.Labels) == 0, leavingLabelsasnilinstead of&labelspointing at a nil map would avoid serializing"labels": nulland instead omit the field.- For the fallback
Href/OwnerHref, consider guarding onOwnerID != ""so a row with an empty owner doesn’t render URLs like/api/hyperfleet/v1/clusters//nodepools/{id}.Behavior today is acceptable; these are just small output-shape improvements.
pkg/handlers/cluster_status.go (1)
83-86: Improve error handling specificity.While checking the conversion error is good, wrapping it in
errors.GeneralErrorloses the specific error context from JSON marshaling failures. Consider using a more specific error type that preserves the underlying error details for better debugging.Apply this diff:
- newStatus, convErr := presenters.ConvertAdapterStatus("Cluster", clusterID, &req) - if convErr != nil { - return nil, errors.GeneralError("Failed to convert adapter status: %v", convErr) - } + newStatus, err := presenters.ConvertAdapterStatus("Cluster", clusterID, &req) + if err != nil { + return nil, errors.BadRequest("Invalid adapter status: %v", err) + }pkg/api/presenters/cluster.go (1)
12-48: Use status phase constant for consistency.The error handling for JSON marshaling is excellent and addresses previous review concerns. However, Line 42 uses a string literal
"NotReady"for StatusPhase. For consistency with the domain model and to prevent typos, use the constant defined inpkg/api/status_types.go.Apply this diff:
Generation: 1, - StatusPhase: "NotReady", + StatusPhase: string(api.PhaseNotReady), StatusObservedGeneration: 0,Based on coding patterns in pkg/api/cluster_types.go Line 59.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
README.md(1 hunks)cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(13 hunks)pkg/api/adapter_status_types.go(0 hunks)pkg/api/adapter_status_types_test.go(1 hunks)pkg/api/cluster_types.go(3 hunks)pkg/api/cluster_types_test.go(1 hunks)pkg/api/node_pool_types.go(3 hunks)pkg/api/node_pool_types_test.go(1 hunks)pkg/api/presenters/adapter_status.go(1 hunks)pkg/api/presenters/adapter_status_test.go(1 hunks)pkg/api/presenters/cluster.go(1 hunks)pkg/api/presenters/cluster_test.go(1 hunks)pkg/api/presenters/node_pool.go(1 hunks)pkg/api/presenters/node_pool_test.go(1 hunks)pkg/api/status_types.go(1 hunks)pkg/api/status_types_test.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_nodepools.go(1 hunks)pkg/handlers/cluster_status.go(4 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(4 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/services/cluster.go(2 hunks)pkg/services/node_pool.go(2 hunks)pkg/services/types.go(1 hunks)pkg/services/types_test.go(1 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/api/adapter_status_types.go
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/api/node_pool_types_test.go
- README.md
- pkg/handlers/nodepool_status.go
- pkg/handlers/cluster_nodepools.go
- test/integration/integration_test.go
- pkg/api/cluster_types_test.go
- pkg/services/node_pool.go
- go.mod
- pkg/api/status_types_test.go
- pkg/services/types_test.go
- pkg/api/presenters/node_pool_test.go
- pkg/api/status_types.go
- pkg/api/adapter_status_types_test.go
- cmd/hyperfleet-api/server/routes.go
- pkg/errors/errors.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/services/types.gopkg/validators/schema_validator_test.gopkg/middleware/schema_validation_test.gopkg/api/presenters/node_pool.gopkg/api/presenters/cluster.gotest/integration/adapter_status_test.gopkg/api/cluster_types.gopkg/services/cluster.gopkg/api/presenters/adapter_status.gopkg/handlers/node_pool.gopkg/handlers/cluster.gopkg/api/node_pool_types.gopkg/middleware/schema_validation.gopkg/api/presenters/adapter_status_test.gotest/integration/clusters_test.gopkg/handlers/cluster_status.gopkg/api/presenters/cluster_test.gopkg/validators/schema_validator.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/types.gopkg/services/cluster.gopkg/handlers/node_pool.gopkg/handlers/cluster.gopkg/handlers/cluster_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/services/types.gopkg/services/cluster.gopkg/handlers/node_pool.gopkg/handlers/cluster.gopkg/handlers/cluster_status.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/types.gopkg/services/cluster.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/services/types.gopkg/validators/schema_validator_test.gopkg/middleware/schema_validation_test.gopkg/api/presenters/node_pool.gopkg/api/presenters/cluster.gopkg/api/cluster_types.gopkg/services/cluster.gopkg/api/presenters/adapter_status.gopkg/handlers/node_pool.gopkg/handlers/cluster.gopkg/api/node_pool_types.gopkg/middleware/schema_validation.gopkg/api/presenters/adapter_status_test.gopkg/handlers/cluster_status.gopkg/api/presenters/cluster_test.gopkg/validators/schema_validator.go
pkg/api/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Files:
pkg/api/presenters/node_pool.gopkg/api/presenters/cluster.gopkg/api/cluster_types.gopkg/api/presenters/adapter_status.gopkg/api/node_pool_types.gopkg/api/presenters/adapter_status_test.gopkg/api/presenters/cluster_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/adapter_status_test.gotest/integration/clusters_test.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/cluster.gopkg/handlers/cluster_status.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
Files:
openapi/openapi.yaml
🧠 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 test/integration/**/*.go : All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Applied to files:
pkg/middleware/schema_validation_test.gopkg/api/presenters/adapter_status_test.gotest/integration/clusters_test.gopkg/api/presenters/cluster_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/middleware/schema_validation_test.gopkg/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Applied to files:
pkg/api/presenters/node_pool.gopkg/api/cluster_types.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 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:
test/integration/adapter_status_test.gopkg/services/cluster.gopkg/api/presenters/adapter_status.gopkg/api/presenters/adapter_status_test.gopkg/handlers/cluster_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 : Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Applied to files:
test/integration/adapter_status_test.gopkg/services/cluster.gopkg/api/presenters/adapter_status.gopkg/api/presenters/adapter_status_test.gopkg/handlers/cluster_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/cluster_types.gopkg/services/cluster.gopkg/api/presenters/adapter_status.gopkg/middleware/schema_validation.gopkg/handlers/cluster_status.gopkg/validators/schema_validator.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:
pkg/api/cluster_types.gopkg/services/cluster.gopkg/validators/schema_validator.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/services/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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/services/cluster.gopkg/middleware/schema_validation.gopkg/handlers/cluster_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/middleware/schema_validation.gopkg/handlers/cluster_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:
test/integration/clusters_test.gopkg/api/presenters/cluster_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 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:
openapi/openapi.yaml
📚 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: Generate OpenAPI code by running make generate whenever the TypeSpec specification changes
Applied to files:
openapi/openapi.yaml
📚 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/factories/**/*.go : Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Applied to files:
pkg/api/presenters/cluster_test.go
🧬 Code graph analysis (13)
pkg/validators/schema_validator_test.go (2)
pkg/validators/schema_validator.go (2)
NewSchemaValidator(25-66)SchemaValidator(19-22)pkg/errors/errors.go (1)
ServiceError(100-109)
pkg/middleware/schema_validation_test.go (2)
pkg/middleware/schema_validation.go (1)
SchemaValidationMiddleware(30-97)pkg/validators/schema_validator.go (2)
SchemaValidator(19-22)NewSchemaValidator(25-66)
pkg/api/presenters/cluster.go (3)
pkg/api/cluster_types.go (1)
Cluster(11-34)pkg/api/status_types.go (4)
ResourceCondition(28-37)PhaseNotReady(10-10)ResourcePhase(7-7)ConditionStatus(17-17)pkg/errors/errors.go (1)
Href(182-184)
pkg/api/cluster_types.go (2)
pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/status_types.go (1)
PhaseNotReady(10-10)
pkg/services/cluster.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
pkg/api/presenters/adapter_status.go (3)
pkg/api/adapter_status_types.go (1)
AdapterStatus(11-30)pkg/api/status_types.go (2)
AdapterCondition(42-48)ConditionStatus(17-17)pkg/api/metadata_types.go (1)
Metadata(28-34)
pkg/handlers/node_pool.go (2)
pkg/api/presenters/node_pool.go (1)
ConvertNodePool(13-54)pkg/errors/errors.go (1)
GeneralError(190-192)
pkg/handlers/cluster.go (2)
pkg/api/presenters/cluster.go (1)
ConvertCluster(13-48)pkg/errors/errors.go (1)
GeneralError(190-192)
pkg/api/node_pool_types.go (2)
pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/status_types.go (1)
PhaseNotReady(10-10)
pkg/api/presenters/adapter_status_test.go (4)
pkg/api/metadata_types.go (1)
Metadata(28-34)pkg/api/presenters/adapter_status.go (2)
ConvertAdapterStatus(14-73)PresentAdapterStatus(76-127)pkg/api/status_types.go (5)
AdapterCondition(42-48)ConditionTrue(20-20)ConditionStatus(17-17)ConditionFalse(21-21)ConditionUnknown(22-22)pkg/api/adapter_status_types.go (1)
AdapterStatus(11-30)
test/integration/clusters_test.go (3)
test/registration.go (1)
RegisterIntegration(13-24)pkg/errors/errors.go (1)
Href(182-184)pkg/api/cluster_types.go (1)
Cluster(11-34)
pkg/api/presenters/cluster_test.go (1)
pkg/api/presenters/cluster.go (2)
ConvertCluster(13-48)PresentCluster(51-131)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (2)
ValidationWithDetails(219-223)ValidationDetail(68-71)
🔇 Additional comments (31)
pkg/api/presenters/cluster_test.go (2)
155-233: Comprehensive presenter test with excellent coverage.This test thoroughly validates the
PresentClusterfunction including:
- All basic fields and their mapping
- Spec and Labels unmarshaling
- Status phase and conditions conversion
- Timestamp preservation
The assertions against OpenAPI enum constants (
openapi.READY,openapi.TRUE) ensure type-safe comparisons as mentioned in PR objectives.
363-400: Good round-trip test validating data integrity.This test properly validates that data survives the
ConvertCluster→PresentClustercycle, including Spec and Labels preservation. The assertions confirm that defaults are correctly applied for Status fields.pkg/validators/schema_validator_test.go (2)
56-77: Well-structured validator initialization test.The test properly validates that the
SchemaValidatorinitializes with correct internal state, including verifying the schema map keys and TypeName mappings for both cluster and nodepool resources.
279-310: Good boundary testing for numeric constraints.Tests for
replicasproperly cover both upper bound (150 > max 100) and lower bound (0 < min 1) violations, ensuring the validator correctly enforces range constraints.pkg/middleware/schema_validation.go (2)
40-50: Body buffering implementation is correct.The pattern of reading the body, closing it, and restoring with
NopCloseris the standard approach for middleware that needs to inspect the request body while allowing downstream handlers to read it. The close on line 47 is safe but optional sinceReadAlldrains the reader.
76-91: Good error handling with proper type differentiation.The code correctly checks if the validation error is a
*errors.ServiceErrorwith field-level details before falling back to a generic validation error. This ensures rich error responses when available.As per coding guidelines, the structured
ServiceErrortype is properly used for error responses.pkg/api/node_pool_types.go (2)
53-76: Good lifecycle hook implementation with consistent timestamp handling.The
BeforeCreatehook properly initializes bothCreatedTimeandUpdatedTimeto the same instant, and uses thePhaseNotReadyconstant for type-safe status initialization.
78-81: BeforeUpdate hook added for timestamp consistency.This ensures
UpdatedTimeis refreshed on every update operation. However, verify that theMetastruct doesn't have GORM'sgorm:"autoUpdateTime"tag on theUpdatedTimefield, as this could cause conflicts with the manual update in the BeforeUpdate hook and result in unexpected behavior.pkg/services/types.go (1)
57-61: Clarify the review intent and verify column name against database schema.The original comment contains contradictory elements—it approves the changes with "LGTM" while simultaneously requesting verification. Additionally, the review lacks confirmation that the column name
created_timematches the actual database schema used in your GORM models. This verification step is essential before approval, as mismatched column names could cause runtime failures.Provide or verify:
- The actual GORM model definition showing the column name and its database tag
- Confirmation that the
created_timecolumn exists in your database schema- Clarification of whether this code change also requires structured logging per coding guidelines for
pkg/services/**/*.gopkg/validators/schema_validator.go (1)
24-105: SchemaValidator construction and error mapping look solid.Loading and validating the doc, extracting
ClusterSpec/NodePoolSpec, and then usingvalidateSpec+convertValidationErrorto turn kin-openapi errors intoValidationWithDetailsfits cleanly with the new error model and should give good field-level feedback without extra allocations or marshal cycles.openapi/openapi.yaml (1)
711-746: Condition/phase enums and validation error details are well aligned with the Go layer.Wiring
ClusterSpec/NodePoolSpecas simple object schemas, introducingConditionStatus/ResourcePhase, and referencing them fromConditionBase,ConditionRequest,ClusterStatus.phase, andNodePoolStatus.phase, plus addingError.details[field,error], all line up with the newValidationDetail/ServiceErrortypes and the schema validator. This should give clients strongly-typed phases and rich validation feedback without changing existing response shapes.pkg/handlers/cluster.go (1)
39-47: Cluster create now cleanly delegates conversion to the presenters layer.Routing the request through
presenters.ConvertClusterwith explicit error handling before callingh.cluster.Create, and then presenting viaPresentCluster, keeps the handler focused on orchestration and mirrors the nodepool flow nicely.test/integration/adapter_status_test.go (1)
223-264: Enum-based assertions make the idempotency test robust to spec changes.Comparing adapter condition statuses against
openapi.TRUE/openapi.FALSEties the test directly to the generated enum values, so if the ConditionStatus enum ever changes you’ll get a compile-time or test-time signal instead of silently drifting.pkg/middleware/schema_validation_test.go (1)
45-393: SchemaValidationMiddleware tests comprehensively cover happy and error paths.The small test schema plus
setupTestValidatorand the range of POST/PATCH/GET/DELETE, cluster vs nodepool, missing/invalid spec, and malformed JSON scenarios give very solid coverage of when validation should run, how failures surface (includingopenapi.Errordecoding), and that the handler still sees the original body. This makes future changes to the middleware or validator much safer.pkg/api/presenters/node_pool.go (1)
13-54: ConvertNodePool now robustly handles marshaling and sets sane defaults.Switching
ConvertNodePoolto return(*api.NodePool, error)and checking the marshal errors forSpec,Labels, and the emptyStatusConditionsslice avoids silent data loss and aligns with the updated handler error handling. The constructedapi.NodePoolalso has sensible defaults for kind, phase, observed generation, and created/updated metadata.pkg/handlers/node_pool.go (1)
41-44: EmptyownerIDinConvertNodePoolmay yield odd hrefs if this Create path is used.
Createcurrently callspresenters.ConvertNodePool(&req, "", "system"), so any nodepools created via this handler will carry an emptyOwnerID. GivenPresentNodePoolbuilds fallbackHref/OwnerHrefusingOwnerID, that would produce URLs like/api/hyperfleet/v1/clusters//nodepools/{id}. That's acceptable if this endpoint isn't wired or is truly "standalone," but once it's exposed you'll likely want to plumb a real owner ID. Verify whether this Create endpoint is actually routed and exposed.pkg/handlers/cluster_status.go (3)
9-9: LGTM: Presenter import aligns with architectural pattern.The addition of the presenters package import supports the architectural shift from OpenAPI-centric conversions to a presenter-based approach, improving separation of concerns between the presentation and domain layers.
44-44: LGTM: Presenter pattern consistently applied.The conversion to
presenters.PresentAdapterStatusis appropriate and aligns with the broader refactoring. Note that PresentAdapterStatus silently ignores JSON unmarshal errors internally, which is acceptable for presentation logic with sensible defaults.
101-102: LGTM: Consistent presenter usage.The presentation logic correctly converts the domain model to the OpenAPI representation and returns the appropriate pointer type.
pkg/api/presenters/cluster.go (2)
50-89: LGTM: Presentation logic with sensible defaults.The function appropriately unmarshals JSON fields and provides fallback values. The ignored unmarshal errors (lines 55, 61, 67) are acceptable for presentation logic since the function returns sensible defaults (empty maps/slices) if unmarshaling fails, and data corruption would already exist in the database at this point.
90-131: LGTM: Robust status presentation with proper defaults.The status construction properly handles nil timestamps, provides sensible phase defaults, and correctly converts domain conditions to OpenAPI format. The type casting between domain and OpenAPI enums is appropriate.
pkg/api/presenters/adapter_status_test.go (1)
1-431: Excellent test coverage and quality.This test suite provides comprehensive coverage of the adapter status conversion and presentation logic:
- Complete field validation for both directions
- Edge case handling (nil timestamps, empty collections, zero values)
- Enum conversion verification in both directions
- Round-trip data integrity tests
- Proper use of Gomega assertions
The tests ensure that the presenter functions maintain data integrity and handle all edge cases appropriately.
pkg/api/presenters/adapter_status.go (2)
13-73: Excellent error handling and data conversion.The function properly addresses the past review concern by checking all
json.Marshalerrors and returning wrapped errors with context. The timestamp handling logic correctly usesreq.ObservedTimewhen provided and falls back totime.Now(). All optional fields are handled appropriately with sensible defaults.
75-127: LGTM: Presentation logic with appropriate error handling.The function correctly unmarshals JSON fields and converts domain types to OpenAPI types. The ignored unmarshal errors (lines 80, 98, 104) are acceptable in the presentation layer since the function provides sensible defaults (empty slices/maps, nil metadata, zero times) and this is a read-only path.
pkg/api/cluster_types.go (2)
47-66: LGTM: Proper lifecycle hook implementation.The BeforeCreate hook correctly initializes timestamps and uses the
PhaseNotReadyconstant (line 59) instead of a string literal, ensuring consistency and preventing typos. The Href defaulting logic is preserved appropriately.
68-71: LGTM: Essential timestamp maintenance.The BeforeUpdate hook correctly refreshes the UpdatedTime field on every update, ensuring accurate audit trails. This is a standard and necessary GORM lifecycle hook.
test/integration/clusters_test.go (5)
353-418: LGTM: Comprehensive provider schema validation test.The test properly handles the provider vs. base schema distinction with appropriate skip logic (lines 361-365). The error handling for reading and unmarshaling the response body (lines 395-403) has been correctly addressed from the previous review. Field-level error detail verification is thorough.
420-474: LGTM: Thorough error response validation.The test comprehensively validates error response structure and includes proper error handling for JSON unmarshaling (line 453), addressing the previous review concern. The acceptance of multiple valid error codes (lines 464-465) is pragmatic and appropriate.
476-525: LGTM: Robust default sorting verification.The test properly creates clusters with distinct timestamps using delays (line 497) and verifies the default sort order (created_time desc). The verification logic correctly identifies test clusters and validates their ordering.
527-576: LGTM: Clean test of custom sorting.The test properly isolates test data using a unique prefix (line 535) and validates ascending name sorting. The verification logic is straightforward and effective.
578-624: LGTM: Complementary descending sort test.The test appropriately validates descending name sorting and follows a consistent structure with the ascending sort test. The verification correctly checks reverse alphabetical ordering.
update update update Enable default orderBy: created_time desc update update speccore to spec enhance error adpater status should use minObservedGeneration instead of maxObservedGeneration
yasun1
force-pushed
the
schema-validator
branch
from
a05f543 to
f91e7b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
go.mod (1)
16-16: Security update for golang-jwt addressed.The upgrade to v4.5.2 addresses the previously flagged vulnerabilities (GO-2024-3250 and GO-2025-3553).
pkg/api/presenters/node_pool.go (1)
58-74: Silent unmarshal errors match cluster presenter pattern.Same observation as
pkg/api/presenters/cluster.go: unmarshal errors are silently ignored. Consider adding logging for observability, consistent with the recommendation for the cluster presenter.
🧹 Nitpick comments (7)
pkg/handlers/nodepool_status.go (1)
101-102: Consider returning value instead of pointer for consistency.The
PresentAdapterStatusreturnsopenapi.AdapterStatus(value), and then you create a pointer to a local variable. While this works (Go allows returning addresses of local variables), check if other handlers return values directly. This may be intentional for interface satisfaction.If other
handle()calls return values directly, consider:- status := presenters.PresentAdapterStatus(adapterStatus) - return &status, nil + return presenters.PresentAdapterStatus(adapterStatus), nilpkg/services/node_pool.go (1)
120-243: Consider extracting shared status aggregation logic.
UpdateNodePoolStatusFromAdaptersclosely mirrorsUpdateClusterStatusFromAdaptersinpkg/services/cluster.go. The core logic (unmarshal conditions, find Available condition, build ResourceCondition, track min generation, compute phase, update timestamps) is duplicated. Consider extracting a shared helper to reduce maintenance burden.Example structure:
// In a shared status_helpers.go func aggregateAdapterStatuses(adapterStatuses []*api.AdapterStatus, requiredAdapters []string, currentGeneration int32) ( conditions []api.ResourceCondition, minGeneration int32, minLastUpdated *time.Time, newPhase string, )pkg/api/presenters/cluster.go (2)
42-42: Use constant for StatusPhase default.Line 42 uses the string literal
"NotReady"while line 102 in the same file usesapi.PhaseNotReady. Use the constant consistently for maintainability.- StatusPhase: "NotReady", + StatusPhase: string(api.PhaseNotReady),
52-68: Silent unmarshal errors in PresentCluster could mask data corruption.Lines 55, 61, and 67 ignore
json.Unmarshalerrors. While these fields originate from the database and should contain valid JSON, silently ignoring errors could mask data corruption issues. Consider logging unmarshal failures for observability.// Unmarshal Spec var spec map[string]interface{} if len(cluster.Spec) > 0 { - _ = json.Unmarshal(cluster.Spec, &spec) + if err := json.Unmarshal(cluster.Spec, &spec); err != nil { + // Log but don't fail - return partial data + // logger.Warningf("Failed to unmarshal cluster spec: %v", err) + } }pkg/middleware/schema_validation.go (2)
15-27: Check error from json.NewEncoder.The error from json.NewEncoder(w).Encode() on line 26 is not checked. While encoding errors are rare for our error types, checking would follow best practices.
Apply this diff:
// Write JSON error response w.Header().Set("Content-Type", "application/json") w.WriteHeader(err.HttpCode) - json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)) + if encodeErr := json.NewEncoder(w).Encode(err.AsOpenapiError(operationID)); encodeErr != nil { + log.Errorf("Failed to encode error response: %v", encodeErr) + }
99-121: Consider more robust path matching.The current implementation uses
strings.Containsandstrings.HasSuffixfor path matching, which works but could match unintended paths. For example,strings.Contains(path, "/clusters/")would match/api/v1/myclusters/foo.Consider using more precise path matching:
func shouldValidateRequest(method, path string) (bool, string) { // Only validate POST and PATCH requests if method != http.MethodPost && method != http.MethodPatch { return false, "" } // Check nodepools first (more specific path) if strings.Contains(path, "/nodepools") { return true, "nodepool" } // POST /api/hyperfleet/v1/clusters // PATCH /api/hyperfleet/v1/clusters/{cluster_id} // Use regexp or path parsing to avoid false positives if strings.HasPrefix(path, "/api/hyperfleet/v1/clusters") { // Exclude nodepool paths that were not caught above if !strings.Contains(path, "/nodepools") { return true, "cluster" } } return false, "" }Or use a router's pattern matching capabilities if available.
pkg/validators/schema_validator.go (1)
92-108: Simplify unnecessary type cast.Lines 94-95 cast
spectointerface{}via an intermediate variable, butmap[string]interface{}already satisfiesinterface{}. You can passspecdirectly toVisitJSON.Apply this diff:
func (v *SchemaValidator) validateSpec(spec map[string]interface{}, schemaRef *openapi3.SchemaRef, specTypeName string) error { - // Cast spec to interface{} for VisitJSON - var specData interface{} = spec - // Validate against schema - if err := schemaRef.Value.VisitJSON(specData); err != nil { + if err := schemaRef.Value.VisitJSON(spec); err != nil { // Convert validation error to our error format with details validationDetails := convertValidationError(err, "spec") return errors.ValidationWithDetails(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
README.md(1 hunks)cmd/hyperfleet-api/server/routes.go(3 hunks)go.mod(5 hunks)openapi/openapi.yaml(13 hunks)pkg/api/adapter_status_types.go(0 hunks)pkg/api/adapter_status_types_test.go(1 hunks)pkg/api/cluster_types.go(3 hunks)pkg/api/cluster_types_test.go(1 hunks)pkg/api/node_pool_types.go(3 hunks)pkg/api/node_pool_types_test.go(1 hunks)pkg/api/presenters/adapter_status.go(1 hunks)pkg/api/presenters/adapter_status_test.go(1 hunks)pkg/api/presenters/cluster.go(1 hunks)pkg/api/presenters/cluster_test.go(1 hunks)pkg/api/presenters/node_pool.go(1 hunks)pkg/api/presenters/node_pool_test.go(1 hunks)pkg/api/status_types.go(1 hunks)pkg/api/status_types_test.go(1 hunks)pkg/errors/errors.go(6 hunks)pkg/handlers/cluster.go(1 hunks)pkg/handlers/cluster_nodepools.go(1 hunks)pkg/handlers/cluster_status.go(4 hunks)pkg/handlers/node_pool.go(1 hunks)pkg/handlers/nodepool_status.go(4 hunks)pkg/middleware/schema_validation.go(1 hunks)pkg/middleware/schema_validation_test.go(1 hunks)pkg/services/cluster.go(5 hunks)pkg/services/node_pool.go(5 hunks)pkg/services/types.go(1 hunks)pkg/services/types_test.go(1 hunks)pkg/validators/schema_validator.go(1 hunks)pkg/validators/schema_validator_test.go(1 hunks)test/integration/adapter_status_test.go(3 hunks)test/integration/clusters_test.go(2 hunks)test/integration/integration_test.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/api/adapter_status_types.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/services/types_test.go
- pkg/api/adapter_status_types_test.go
- pkg/api/presenters/adapter_status_test.go
- pkg/api/cluster_types_test.go
- pkg/handlers/node_pool.go
- pkg/api/presenters/adapter_status.go
- pkg/api/presenters/cluster_test.go
- pkg/services/types.go
- cmd/hyperfleet-api/server/routes.go
- pkg/api/status_types.go
- test/integration/adapter_status_test.go
- README.md
- test/integration/clusters_test.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/integration_test.gopkg/services/node_pool.gopkg/middleware/schema_validation_test.gopkg/handlers/cluster_nodepools.gopkg/middleware/schema_validation.gopkg/api/cluster_types.gopkg/api/presenters/node_pool_test.gopkg/api/presenters/node_pool.gopkg/errors/errors.gopkg/handlers/cluster_status.gopkg/handlers/cluster.gopkg/services/cluster.gopkg/api/status_types_test.gopkg/api/node_pool_types.gopkg/validators/schema_validator_test.gopkg/api/node_pool_types_test.gopkg/validators/schema_validator.gopkg/api/presenters/cluster.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/integration_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/services/node_pool.gopkg/handlers/cluster_nodepools.gopkg/handlers/cluster_status.gopkg/handlers/cluster.gopkg/services/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/services/node_pool.gopkg/handlers/cluster_nodepools.gopkg/handlers/cluster_status.gopkg/handlers/cluster.gopkg/services/cluster.gopkg/handlers/nodepool_status.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/cluster.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/services/node_pool.gopkg/middleware/schema_validation_test.gopkg/handlers/cluster_nodepools.gopkg/middleware/schema_validation.gopkg/api/cluster_types.gopkg/api/presenters/node_pool_test.gopkg/api/presenters/node_pool.gopkg/errors/errors.gopkg/handlers/cluster_status.gopkg/handlers/cluster.gopkg/services/cluster.gopkg/api/status_types_test.gopkg/api/node_pool_types.gopkg/validators/schema_validator_test.gopkg/api/node_pool_types_test.gopkg/validators/schema_validator.gopkg/api/presenters/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/cluster_nodepools.gopkg/handlers/cluster_status.gopkg/handlers/cluster.gopkg/handlers/nodepool_status.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/cluster_types.gopkg/api/presenters/node_pool_test.gopkg/api/presenters/node_pool.gopkg/api/status_types_test.gopkg/api/node_pool_types.gopkg/api/node_pool_types_test.gopkg/api/presenters/cluster.go
openapi/openapi.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
Files:
openapi/openapi.yaml
🧠 Learnings (16)
📓 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 openapi/openapi.yaml : TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/api/**/*.go : Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
Applied to files:
test/integration/integration_test.gopkg/services/node_pool.gogo.modpkg/middleware/schema_validation.gopkg/api/cluster_types.gopkg/handlers/cluster_status.gopkg/services/cluster.gopkg/api/status_types_test.gopkg/validators/schema_validator.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 : All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Applied to files:
test/integration/integration_test.gopkg/middleware/schema_validation_test.gopkg/api/status_types_test.gopkg/validators/schema_validator_test.gopkg/api/node_pool_types_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 : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
test/integration/integration_test.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to 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/cluster_status.gopkg/services/cluster.gopkg/api/status_types_test.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/services/node_pool.gopkg/handlers/cluster_nodepools.gopkg/api/cluster_types.gopkg/api/presenters/node_pool.gopkg/api/node_pool_types.gopkg/api/node_pool_types_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/services/node_pool.gopkg/handlers/cluster_status.gopkg/services/cluster.gopkg/api/status_types_test.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/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/services/node_pool.gogo.modpkg/services/cluster.gopkg/validators/schema_validator.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/services/node_pool.gopkg/services/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/services/node_pool.gopkg/services/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/**/*.go : Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Applied to files:
pkg/middleware/schema_validation_test.gogo.modpkg/middleware/schema_validation.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/**/*.go : Use PostgreSQL 14.2 with GORM ORM for all database operations
Applied to files:
go.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/{handlers,services,dao}/**/*.go : Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Applied to files:
pkg/errors/errors.gopkg/handlers/cluster_status.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/**/*.go : Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Applied to files:
pkg/handlers/cluster_status.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 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:
openapi/openapi.yaml
📚 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: Generate OpenAPI code by running make generate whenever the TypeSpec specification changes
Applied to files:
openapi/openapi.yaml
🧬 Code graph analysis (13)
pkg/services/node_pool.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
pkg/middleware/schema_validation_test.go (2)
pkg/middleware/schema_validation.go (1)
SchemaValidationMiddleware(30-97)pkg/validators/schema_validator.go (2)
SchemaValidator(19-22)NewSchemaValidator(25-66)
pkg/handlers/cluster_nodepools.go (2)
pkg/api/presenters/node_pool.go (1)
ConvertNodePool(13-54)pkg/errors/errors.go (1)
GeneralError(190-192)
pkg/middleware/schema_validation.go (4)
pkg/errors/errors.go (3)
ServiceError(100-109)MalformedRequest(225-227)Validation(214-216)pkg/logger/logger.go (1)
NewOCMLogger(36-44)pkg/logger/operationid_middleware.go (1)
GetOperationID(38-43)pkg/validators/schema_validator.go (1)
SchemaValidator(19-22)
pkg/api/cluster_types.go (2)
pkg/api/metadata_types.go (1)
Meta(37-42)pkg/api/status_types.go (1)
PhaseNotReady(10-10)
pkg/api/presenters/node_pool_test.go (4)
pkg/api/presenters/node_pool.go (2)
ConvertNodePool(13-54)PresentNodePool(57-148)pkg/api/status_types.go (2)
ResourceCondition(28-37)ConditionTrue(20-20)pkg/api/node_pool_types.go (1)
NodePool(12-40)pkg/errors/errors.go (1)
Href(182-184)
pkg/handlers/cluster_status.go (2)
pkg/api/presenters/adapter_status.go (2)
PresentAdapterStatus(76-127)ConvertAdapterStatus(14-73)pkg/errors/errors.go (1)
GeneralError(190-192)
pkg/handlers/cluster.go (2)
pkg/api/presenters/cluster.go (1)
ConvertCluster(13-48)pkg/errors/errors.go (1)
GeneralError(190-192)
pkg/services/cluster.go (1)
pkg/api/status_types.go (2)
ResourceCondition(28-37)AdapterCondition(42-48)
pkg/api/node_pool_types_test.go (2)
pkg/api/node_pool_types.go (2)
NodePoolList(42-42)NodePool(12-40)pkg/errors/errors.go (1)
Href(182-184)
pkg/validators/schema_validator.go (1)
pkg/errors/errors.go (2)
ValidationWithDetails(219-223)ValidationDetail(68-71)
pkg/api/presenters/cluster.go (3)
pkg/api/cluster_types.go (1)
Cluster(11-34)pkg/api/status_types.go (4)
ResourceCondition(28-37)PhaseNotReady(10-10)ResourcePhase(7-7)ConditionStatus(17-17)pkg/errors/errors.go (1)
Href(182-184)
pkg/handlers/nodepool_status.go (2)
pkg/api/presenters/adapter_status.go (2)
PresentAdapterStatus(76-127)ConvertAdapterStatus(14-73)pkg/errors/errors.go (1)
GeneralError(190-192)
🔇 Additional comments (37)
pkg/api/status_types_test.go (1)
1-336: Comprehensive test coverage for status types.The tests thoroughly validate:
- Constant values and string conversions for
ResourcePhaseandConditionStatus- JSON serialization with proper
omitemptyhandling for optional fields- Round-trip integrity with appropriate second-precision timestamp comparisons
The test structure is clean and follows good practices for domain type validation.
pkg/api/node_pool_types.go (1)
53-81: GORM lifecycle hooks are well-implemented.The
BeforeCreateandBeforeUpdatehooks properly manage timestamps and default values. Usingstring(PhaseNotReady)on line 65 ensures type-safe enum handling, which aligns with the PR objective of compile-time detection of enum changes.test/integration/integration_test.go (1)
18-45: Robust path resolution for OPENAPI_SCHEMA_PATH.The implementation addresses the previous review feedback by:
- Using
filepath.Dirfor safe directory traversal instead of string slicing- Checking the
okreturn fromruntime.Caller- Using
filepath.Joinfor cross-platform compatibility- Validating file existence with
os.Statbefore setting the environment variable- Logging warnings instead of failing when path resolution fails
This enables schema validation middleware during integration tests while maintaining test reliability.
pkg/handlers/cluster.go (1)
39-48: Cluster creation refactored consistently with NodePool handler.The conversion is delegated to the presenters layer with appropriate error handling. The code properly distinguishes between conversion errors (wrapped in
GeneralError) and service errors (svcErr), maintaining clean error semantics.pkg/handlers/nodepool_status.go (1)
83-102: Adapter status handler refactored to use presenters.The conversion and presentation logic is properly delegated to the presenters layer. Error handling follows the established pattern, and the status aggregation failure is handled gracefully with a warning log rather than failing the request.
pkg/handlers/cluster_nodepools.go (1)
142-146: Verify whethercreatedByshould use authenticated user context.The review identifies a valid concern: the hardcoded
"system"value forcreatedBymay not align with the coding guidelines requiring JWT authentication middleware to extract account ID and username from claims. However, this cannot be verified without access to the codebase to confirm:
- Whether other handlers use user context for similar fields
- If authenticated user information is available in the current handler context
- Whether hardcoding to
"system"is an intentional design pattern for system-initiated operationsgo.mod (1)
13-13: Addition of kin-openapi for schema validation looks good.The
kin-openapilibrary is a well-established OpenAPI toolkit for Go. v0.133.0 is a secure release that includes fixes for CVE-2025-30153 and a Snyk-reported path-traversal vulnerability. This version aligns with the PR objective to implement schema validation.pkg/api/node_pool_types_test.go (3)
1-54: Good test coverage for NodePoolList.Index() method.The tests thoroughly cover edge cases: empty list, single item, multiple items, and duplicate ID overwrite behavior. The assertions are clear and well-structured.
226-245: Comprehensive integration test for BeforeCreate defaults.This test validates the complete default state after
BeforeCreate, ensuring all auto-generated fields (ID, Kind, OwnerKind, StatusPhase, Href, OwnerHref) are correctly set. Good holistic verification.
56-69: Verify nil tx parameter is acceptable for BeforeCreate.The test passes
nilfor the GORM transaction parameter. This works ifBeforeCreatedoesn't use thetxargument, but the implementation details could not be verified. Confirm that theBeforeCreatemethod signature accepts a nil transaction or update the test to provide a valid transaction context if required.pkg/middleware/schema_validation_test.go (3)
17-43: Well-designed test schema for validation scenarios.The embedded test schema effectively covers key validation scenarios: required fields (
region,replicas), enum constraints (us-central1,us-east1), and numeric range constraints (minimum: 1,maximum: 10).
378-393: Clean test helper implementation.The
setupTestValidatorhelper properly usest.TempDir()for automatic cleanup and fails fast witht.Fatalfon setup errors.
291-317: Verify whetherspecfield is required for cluster creation per API schema.The test allows requests without a
specfield to pass validation. Per OpenAPI/JSON Schema best practices, ifspecis required for cluster creation, it must be listed in the schema'srequiredarray and the middleware should validate and reject requests missing required fields. Clarify the intended behavior—either update the API schema to reflect thatspecis optional, or implement validation to enforce it as required.Additionally, verify that JWT authentication middleware is implemented in this file to validate tokens and extract account ID and username from claims, as required by the coding guidelines for
pkg/**/*.gofiles.pkg/services/node_pool.go (1)
134-186: Correct migration to internal domain types with proper min-generation semantics.The switch to
api.ResourceConditionandapi.AdapterConditionaligns with the domain type refactoring. The minimum observed generation tracking (lines 181-185) ensures the nodepool status only advances when all adapters have caught up—this is correct for distributed consistency. As per coding guidelines, this aligns with the adapter status aggregation requirement.pkg/api/presenters/cluster.go (1)
12-48: Error handling properly implemented in ConvertCluster.This addresses the past review feedback. All
json.Marshalcalls now return wrapped errors with context, preventing silent data loss.pkg/api/presenters/node_pool_test.go (3)
29-70: Thorough test coverage for ConvertNodePool.The test validates all aspects of conversion: basic fields, ownership, defaults, and proper JSON marshaling of Spec, Labels, and StatusConditions. The assertions are comprehensive.
151-234: Excellent PresentNodePool test with complete field verification.The test covers all presentation aspects including Status with conditions, timestamps, owner references, and proper type conversions between domain and OpenAPI models.
386-428: Good round-trip test validates data integrity.This test ensures that data survives the ConvertNodePool → PresentNodePool transformation, which is critical for API correctness.
pkg/api/presenters/node_pool.go (2)
12-54: Proper error handling in ConvertNodePool.All
json.Marshalcalls include error handling with descriptive error messages. The Kind defaulting logic (lines 36-39) correctly handles nil pointer.
118-145: Correct phase defaulting and condition conversion.The phase defaults to
api.PhaseNotReadywhen empty (lines 118-122), and the ResourceCondition to openapi conversion properly maps all fields including the ConditionStatus type conversion.pkg/handlers/cluster_status.go (2)
41-45: Clean migration to presenters layer for List.The change from
as.ToOpenAPI()topresenters.PresentAdapterStatus(as)properly decouples the domain model from OpenAPI conversion.
82-102: Proper error handling for presenter conversion.The conversion error is wrapped with context using
errors.GeneralError. The final presentation and return pattern is correct.pkg/services/cluster.go (2)
134-186: Correct domain type migration with min-generation semantics.The migration to
api.ResourceConditionandapi.AdapterConditionis implemented correctly. The minimum observed generation tracking ensures cluster status only advances when all adapters have caught up, which is the correct behavior for distributed status aggregation. As per coding guidelines, this implements the adapter status aggregation pattern properly.
206-213: Correct handling of empty adapter list.Setting
StatusObservedGenerationto 0 when no adapters exist (matching DB default) is appropriate and prevents themath.MaxInt32sentinel value from leaking into the status.pkg/validators/schema_validator_test.go (1)
1-357: LGTM! Comprehensive test coverage.The test suite thoroughly validates the SchemaValidator functionality across initialization, error handling, and spec validation scenarios for both ClusterSpec and NodePoolSpec. The use of a temporary schema file and helper functions (setupTestValidator, getServiceError) provides good test isolation and reusability.
pkg/errors/errors.go (2)
67-71: LGTM! Well-structured validation error support.The new ValidationDetail struct and Details field on ServiceError provide clean field-level error reporting that integrates well with the OpenAPI error model.
Also applies to: 107-108
218-223: LGTM! Helpful constructor for validation errors with details.The ValidationWithDetails helper provides a clean way to create validation errors with field-level details, used by the schema validator.
pkg/api/cluster_types.go (2)
47-66: LGTM! Proper initialization with time-based fields.The BeforeCreate hook correctly initializes ID, timestamps (CreatedTime, UpdatedTime), and default values (Kind, Generation, StatusPhase, Href).
68-71: LGTM! BeforeUpdate hook maintains UpdatedTime.The new BeforeUpdate hook ensures UpdatedTime is refreshed on every update, maintaining proper audit trails.
pkg/middleware/schema_validation.go (1)
32-97: LGTM! Well-structured middleware with proper body handling.The middleware correctly:
- Checks if validation is needed via shouldValidateRequest
- Buffers and restores the request body for downstream handlers
- Extracts and validates the spec field
- Returns detailed OpenAPI-style errors with field-level details
- Handles edge cases (no spec field, invalid JSON)
pkg/validators/schema_validator.go (3)
25-66: LGTM! Clean schema loading and initialization.The constructor properly:
- Loads and validates the OpenAPI document
- Extracts required ClusterSpec and NodePoolSpec schemas
- Returns descriptive errors for missing schemas
- Builds a schemas map for resource type lookup
68-78: LGTM! Graceful handling of unknown resource types.The Validate method correctly returns nil for unknown resource types, allowing the middleware to skip validation gracefully when encountering unsupported resources.
110-153: LGTM! Comprehensive error conversion with field paths.The convertValidationError function properly:
- Handles MultiError by recursively processing sub-errors
- Extracts field paths from SchemaError using JSONPointer()
- Preserves error reasons from the validation library
- Provides fallback for unknown error types
openapi/openapi.yaml (4)
740-745: LGTM! Provider-agnostic spec schemas.ClusterSpec and NodePoolSpec are intentionally defined as flexible object types without required properties, allowing provider-specific adapters to define and validate their own spec requirements. This aligns with the architecture described in the PR objectives.
Also applies to: 1094-1099
839-845: LGTM! Type-safe enums for status and phase.The new ConditionStatus and ResourcePhase enums provide type-safe values that are used throughout the schema via
allOfreferences, enabling compile-time detection of enum changes in generated code.Also applies to: 1190-1196
863-874: LGTM! Enhanced error details for field-level validation.The details field on Error properly supports field-level validation errors, aligning with the ServiceError.Details implementation in pkg/errors/errors.go.
1-1204: Verify TypeSpec consistency.As per coding guidelines, "TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes." These changes appear to be direct edits to openapi.yaml. Please confirm whether corresponding TypeSpec definitions have been updated in the hyperfleet-api-spec repository to keep both sources in sync.
Based on learnings, TypeSpec is the source of truth for API definitions.
| // Cluster database model | ||
| type Cluster struct { | ||
| Meta // Contains ID, CreatedAt, UpdatedAt, DeletedAt | ||
| Meta // Contains ID, CreatedTime, UpdatedTime, DeletedTime |
There was a problem hiding this comment.
Fix inconsistent comment.
The comment mentions "DeletedTime" but the Meta struct uses "DeletedAt" (gorm.DeletedAt for soft deletes).
Apply this diff:
- Meta // Contains ID, CreatedTime, UpdatedTime, DeletedTime
+ Meta // Contains ID, CreatedTime, UpdatedTime, DeletedAt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Meta // Contains ID, CreatedTime, UpdatedTime, DeletedTime | |
| Meta // Contains ID, CreatedTime, UpdatedTime, DeletedAt |
🤖 Prompt for AI Agents
In pkg/api/cluster_types.go around line 12, the field comment currently says
"DeletedTime" but the Meta struct uses gorm.DeletedAt; update the comment to use
"DeletedAt" (or explicitly "DeletedAt (gorm.DeletedAt for soft deletes)") so the
comment matches the actual struct field name and type.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin, yasun1 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 2c61fae
into
openshift-hyperfleet:main
|
@yasun1 Adding a post lgtm thought here In the This is a bit weird.... it is now a test to check if a constant has a certain value... The downside is that now |
|
@rh-amarin If |
4 participants
Summary
Enable schema validation in integration tests and implement type-safe enum handling to ensure the schema
validation middleware is properly tested and production-ready.
close HYPERFLEET-286
Motivation
The schema validation middleware was implemented to validate cluster and nodepool specifications against OpenAPI schemas, but integration tests were running without schema validation enabled. This meant the validation logic wasn't being exercised during CI/CD, creating a gap between test and production environments. Additionally, tests were using string literals to compare against OpenAPI-generated enum types, which could lead to type safety issues.
What Changed
Schema Validation Enabled for Tests
Modified the integration test setup to automatically configure schema validation by setting OPENAPI_SCHEMA_PATH based on the test file location. This ensures:
Type-Safe Enum Handling
Updated adapter status tests to use proper openapi.ConditionStatus enum constants instead of raw strings. For example, replaced "True" with openapi.TRUE and "False" with openapi.FALSE. This ensures:
Provider Schema Test Compatibility
Improved the skip condition for provider-specific schema tests to handle both relative and absolute file paths,
ensuring tests behave correctly whether run locally or with custom schema paths.
Impact
Verification
All 39 integration tests pass, including schema validation tests that were previously skipped.
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.