[HYPERFLEET-321] feat: add user-friendly search syntax and lowercase Base32 ID encoding #16
Conversation
WalkthroughThis PR updates OpenAPI docs (SearchParams) and the name validation pattern to an RFC‑1123/DNS‑1123 style; changes KSUID output to a lowercase Base32 32-character ID and updates factories/tests accordingly; introduces field-name mapping (status.* → DB columns, labels.* → JSONB extracts) earlier in the search pipeline; adds DB indexes on status_last_updated_time for clusters and node_pools with rollbacks; and expands test factories and integration tests to cover status/labels search behavior. Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant API as API Handler
participant Val as Validation
participant Parser as TSL Parser
participant Mapper as Field Mapper
participant Builder as Search Builder
participant SQLizer as SQL Generator
participant DB
User->>API: HTTP search request (e.g. status.phase='NotReady' and labels.region='us-east')
API->>Val: Validate request & params (OpenAPI + name pattern)
Val-->>API: OK / errors
API->>Parser: Parse TSL → AST
Parser-->>API: AST
API->>Mapper: FieldNameWalk(AST) — map status.* → columns, labels.* → JSONB extracts
Mapper-->>API: Mapped AST
API->>Builder: buildSearchValues(mapped AST)
Builder->>SQLizer: treeWalkForSqlizer(mapped AST)
SQLizer->>DB: Execute generated SQL (e.g. WHERE status_phase = ?, labels->>'region' = ?)
DB-->>API: Results
API-->>User: HTTP response with filtered items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring 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: 6
♻️ Duplicate comments (1)
test/factories/factory.go (1)
12-22: Duplicate code - see comment on pkg/api/resource_id.go.This code duplicates the Base32 encoding logic from pkg/api/resource_id.go. Consider using
api.NewID()directly instead of reimplementing the encoding logic.
🧹 Nitpick comments (2)
pkg/api/resource_id.go (1)
9-22: Suggest extracting shared Base32 encoding constants.The uidAlphabet and uidEncoding definitions are duplicated in test/factories/factory.go (lines 18-22). Consider extracting these to a shared location to maintain a single source of truth.
Apply this approach:
Option 1 (Recommended): Keep in pkg/api/resource_id.go and import in test/factories/factory.go
In test/factories/factory.go:
package factories import ( - "encoding/base32" - + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/segmentio/ksuid" ) -// NewID generates a new unique identifier using KSUID with lowercase Base32 encoding. -// The resulting identifier is compatible with Kubernetes DNS-1123 subdomain naming requirements. func (f *Factories) NewID() string { - return uidEncoding.EncodeToString(ksuid.New().Bytes()) + return api.NewID() } - -// uidAlphabet is the lowercase alphabet used to encode unique identifiers. -const uidAlphabet = "0123456789abcdefghijklmnopqrstuv" - -// uidEncoding is the lowercase variant of Base32 used to encode unique identifiers. -var uidEncoding = base32.NewEncoding(uidAlphabet).WithPadding(base32.NoPadding)test/integration/search_field_mapping_test.go (1)
229-285: Consider splitting NodePool test into separate test functions.This test covers two distinct scenarios (status.phase and labels mappings) in a single function. While functionally correct, splitting into
TestSearchNodePoolStatusPhaseMappingandTestSearchNodePoolLabelsMappingwould improve test isolation and make failures easier to diagnose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
openapi/openapi.yaml(2 hunks)pkg/api/resource_id.go(1 hunks)pkg/api/resource_id_test.go(1 hunks)pkg/db/migrations/202511111044_add_clusters.go(1 hunks)pkg/db/migrations/202511111055_add_node_pools.go(2 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/handlers/validation.go(2 hunks)pkg/handlers/validation_test.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(3 hunks)test/factories/clusters.go(2 hunks)test/factories/factory.go(1 hunks)test/factories/node_pools.go(2 hunks)test/integration/search_field_mapping_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
pkg/db/migrations/202511111044_add_clusters.gopkg/services/generic_test.gopkg/db/migrations/202511111055_add_node_pools.gopkg/api/resource_id_test.gopkg/db/sql_helpers.gotest/integration/search_field_mapping_test.gopkg/services/generic.gopkg/handlers/validation_test.gotest/factories/factory.gotest/factories/clusters.gopkg/handlers/validation.gotest/factories/node_pools.gopkg/api/resource_id.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/migrations/202511111044_add_clusters.gopkg/db/migrations/202511111055_add_node_pools.gopkg/db/sql_helpers.go
pkg/db/migrations/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/db/migrations/**/*.go: Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Files:
pkg/db/migrations/202511111044_add_clusters.gopkg/db/migrations/202511111055_add_node_pools.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Files:
pkg/db/migrations/202511111044_add_clusters.gopkg/services/generic_test.gopkg/db/migrations/202511111055_add_node_pools.gopkg/api/resource_id_test.gopkg/db/sql_helpers.gopkg/services/generic.gopkg/handlers/validation_test.gopkg/handlers/validation.gopkg/api/resource_id.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/generic_test.gopkg/services/generic.gopkg/handlers/validation_test.gopkg/handlers/validation.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/generic_test.gopkg/services/generic.gopkg/handlers/validation_test.gopkg/handlers/validation.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/generic_test.gopkg/services/generic.go
pkg/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/resource_id_test.gopkg/api/resource_id.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/search_field_mapping_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
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/validation_test.gopkg/handlers/validation.go
test/factories/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Files:
test/factories/factory.gotest/factories/clusters.gotest/factories/node_pools.go
🧠 Learnings (8)
📚 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/db/migrations/202511111044_add_clusters.gopkg/db/migrations/202511111055_add_node_pools.gopkg/db/sql_helpers.gopkg/services/generic.gotest/factories/node_pools.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/db/migrations/202511111044_add_clusters.gopkg/db/migrations/202511111055_add_node_pools.gopkg/db/sql_helpers.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/dao/**/*.go : Increment generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/db/migrations/202511111044_add_clusters.gopkg/services/generic_test.gopkg/db/sql_helpers.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/integration/**/*.go : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/services/generic_test.gotest/integration/search_field_mapping_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/services/generic_test.gopkg/api/resource_id_test.gotest/integration/search_field_mapping_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:
test/factories/factory.gotest/factories/clusters.gotest/factories/node_pools.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to cmd/hyperfleet/environments/**/*.go : Use OCM_ENV environment variable to select configuration environment (development, unit_testing, integration_testing, production)
Applied to files:
test/factories/clusters.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/validation.go
🧬 Code graph analysis (6)
pkg/services/generic_test.go (1)
pkg/db/sql_helpers.go (1)
FieldNameWalk(129-220)
pkg/api/resource_id_test.go (1)
pkg/api/resource_id.go (1)
NewID(14-16)
test/integration/search_field_mapping_test.go (5)
test/registration.go (1)
RegisterIntegration(13-24)test/factories/clusters.go (3)
NewClusterWithStatus(55-77)NewClusterWithLabels(80-101)NewClusterWithStatusAndLabels(104-126)test/factories/factory.go (1)
Factories(9-10)pkg/api/resource_id.go (1)
NewID(14-16)test/factories/node_pools.go (2)
NewNodePoolWithStatus(75-97)NewNodePoolWithLabels(100-121)
pkg/services/generic.go (1)
pkg/db/sql_helpers.go (1)
FieldNameWalk(129-220)
pkg/handlers/validation.go (1)
pkg/errors/errors.go (1)
Validation(214-216)
test/factories/node_pools.go (2)
pkg/db/session.go (1)
SessionFactory(12-20)pkg/api/node_pool_types.go (1)
NodePool(12-40)
🔇 Additional comments (22)
pkg/db/migrations/202511111055_add_node_pools.go (1)
73-77: Index addition and rollback forstatus_last_updated_timelook correctThe new
idx_node_pools_status_last_updated_timeindex is created and dropped in a fully symmetric, idempotent way (IF NOT EXISTS/IF EXISTS), matches the existing index naming scheme, and fits the stated search use case. The migration still keepsspecandstatus_conditionsas JSONB, consistent with the JSONB storage guideline. Based on learnings, this is aligned with the expected migration shape.Also applies to: 99-101
pkg/db/migrations/202511111044_add_clusters.go (1)
70-75: Consistentstatus_last_updated_timeindexing and rollback for clustersThe
idx_clusters_status_last_updated_timeindex is added in line with existing index patterns and is cleanly removed in rollback usingIF EXISTS. This keeps the clusters migration symmetrical and idempotent while supporting the new search patterns that filter onstatus_last_updated_time. The JSONB-based spec/status storage remains intact, consistent with prior migration guidelines.Also applies to: 80-82
pkg/handlers/validation_test.go (1)
81-83: LGTM! Good test coverage for RFC 1123 edge cases.The added test cases properly validate that names cannot start or end with hyphens, ensuring compliance with the RFC 1123 pattern.
pkg/handlers/validation.go (2)
10-12: LGTM! RFC 1123 DNS subdomain pattern correctly implemented.The regex pattern
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$properly enforces Kubernetes-compatible naming: starts and ends with alphanumeric, allows internal hyphens.
76-76: LGTM! Clear validation error message.The updated error message accurately describes the RFC 1123 validation requirements.
openapi/openapi.yaml (2)
410-413: LGTM! Clear documentation of search syntax.The SearchParams description effectively documents the TSL query syntax with practical examples demonstrating
status.*,labels.*, andinoperator usage.
718-718: LGTM! Pattern matches validation logic.The ClusterBase.name pattern is consistent with the RFC 1123 validation implemented in pkg/handlers/validation.go line 12.
pkg/services/generic_test.go (2)
35-36: Verify the error message change.The error message changed from "clusters.id invalid field name" to "spec invalid field name" for the search query
spec = '{}'. While both indicate invalid fields, can you confirm this change is intentional and reflects the actual error behavior?
59-95: LGTM! Comprehensive field mapping test coverage.The new test cases thoroughly validate the field mapping functionality:
status.phase→status_phasestatus.last_updated_time→status_last_updated_timelabels.environment→labels->>'environment'idquery (should be allowed)The explicit FieldNameWalk call before SQL conversion correctly reflects the new pre-processing step.
pkg/api/resource_id_test.go (3)
8-30: LGTM! Thorough ID format and uniqueness validation.The test properly validates:
- 32-character length (correct for Base32 encoding of 20-byte KSUID)
- Character set restricted to
0-9a-v(matching uidAlphabet)- Uniqueness across 100 generated IDs
32-51: LGTM! Pragmatic approach to time-ordering validation.Good decision to verify consistency and uniqueness within the same second rather than introducing test delays. The comment clearly explains KSUID's second-level timestamp precision.
53-67: LGTM! Kubernetes compatibility validation.The test correctly validates DNS-1123 subdomain compatibility using the standard pattern, confirming that the generated IDs are suitable for Kubernetes resource names.
pkg/db/sql_helpers.go (2)
36-66: LGTM! Clean field mapping implementation.The field mapping logic is well-structured:
labels.*correctly maps to JSONB extraction:labels->>'xxx'status.*maps to database columns via statusFieldMappings- Mappings are applied before disallowed field validation (correct order)
The statusFieldMappings includes the core status fields (phase, last_updated_time, last_transition_time, observed_generation, conditions).
79-79: LGTM! More accurate comment.The comment change from "Check for allowed fields" to "Check for disallowed fields" better describes the actual logic (checking if field exists in disallowedFields map).
pkg/services/generic.go (3)
41-49: LGTM! Provider-specific field restrictions are well-structured.The
SearchDisallowedFieldsmap correctly blocksspecfields for both Cluster and NodePool resources, preventing queries on provider-specific JSONB data. This aligns with the coding guideline to store specifications as JSONB without making them searchable.
171-181: LGTM! Field name mapping correctly moved before related-table detection.Applying
FieldNameWalkbeforetreeWalkForRelatedTablespreventsstatusandlabelsprefixes from being misinterpreted as related table names. The comment clearly documents the rationale.
347-358: LGTM! Cleaned up redundant call with clear documentation.The comment accurately explains that
FieldNameWalkis now called earlier in the pipeline, and the function correctly delegates tosqlFilter.Walk.test/integration/search_field_mapping_test.go (5)
16-54: LGTM! Solid integration test forstatus.phasemapping.The test correctly creates clusters with different status phases, queries using user-friendly syntax, and validates both inclusion and exclusion of expected results. As per coding guidelines, this provides good coverage of the search functionality.
56-99: LGTM! Time-based filtering test is well-structured.Good use of relative time offsets to test
status.last_updated_timemapping. The RFC3339 format in the search query aligns with standard timestamp handling.
101-145: LGTM! Labels JSONB query mapping test is thorough.Correctly validates that
labels.environment='production'maps to the appropriate JSONB query and returns only matching clusters.
147-163: LGTM! Negative test for disallowedspecfield.Important test that validates the
SearchDisallowedFieldsconfiguration correctly rejects queries on thespecfield with a 400 status.
165-227: LGTM! Combined AND query test validates complex search expressions.Good coverage of multi-condition queries combining status and labels fields, with proper verification that only matching resources are returned.
yasun1
force-pushed
the
search-capabilities
branch
from
9aded6e to
b3fa8cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/factories/factory.go (1)
14-22: Consider reusingapi.NewID()to avoid duplicating encoding logic.The
uidAlphabetanduidEncodingdefinitions are duplicated frompkg/api/resource_id.go. If the encoding logic changes, both files must be updated in sync.package factories import ( - "encoding/base32" - - "github.com/segmentio/ksuid" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" ) type Factories struct { } // NewID generates a new unique identifier using KSUID with lowercase Base32 encoding. // The resulting identifier is compatible with Kubernetes DNS-1123 subdomain naming requirements. func (f *Factories) NewID() string { - return uidEncoding.EncodeToString(ksuid.New().Bytes()) + return api.NewID() } - -// uidAlphabet is the lowercase alphabet used to encode unique identifiers. -const uidAlphabet = "0123456789abcdefghijklmnopqrstuv" - -// uidEncoding is the lowercase variant of Base32 used to encode unique identifiers. -var uidEncoding = base32.NewEncoding(uidAlphabet).WithPadding(base32.NoPadding)test/factories/clusters.go (1)
114-139: Consider reusingNewClusterWithLabelsto reduce duplication.
NewClusterWithStatusAndLabelsduplicates the label update logic fromNewClusterWithLabels. This could be simplified by delegating to the existing helper.func NewClusterWithStatusAndLabels(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time, labels map[string]string) (*api.Cluster, error) { cluster, err := NewClusterWithStatus(f, dbFactory, id, phase, lastUpdatedTime) if err != nil { return nil, err } if labels != nil { - labelsJSON, err := json.Marshal(labels) - if err != nil { - return nil, err - } - - dbSession := dbFactory.New(context.Background()) - err = dbSession.Model(cluster).Update("labels", labelsJSON).Error + labelsJSON, marshalErr := json.Marshal(labels) + if marshalErr != nil { + return nil, marshalErr + } + dbSession := dbFactory.New(context.Background()) + err = dbSession.Model(cluster).Update("labels", labelsJSON).Error if err != nil { return nil, err } - if err := reloadCluster(dbSession, cluster); err != nil { return nil, err } } return cluster, nil }Alternatively, consider calling the label update logic through a shared helper to maintain consistency if this pattern grows.
test/factories/node_pools.go (1)
134-159: Same duplication pattern as clusters.go.The label update logic in lines 141-155 duplicates
NewNodePoolWithLabels. Consider applying the same refactor suggested for the cluster factories if you address that.test/integration/search_field_mapping_test.go (1)
229-285: LGTM! Good parity test for NodePool field mapping.This test ensures that NodePool resources support the same field mapping as Clusters. Testing both
status.phaseandlabels.xxxin a single test function is acceptable, though splitting them could improve test isolation and error diagnosis.Consider splitting into two separate test functions (
TestSearchNodePoolStatusPhaseMappingandTestSearchNodePoolLabelsMapping) for better isolation and clearer failure diagnosis. This is a minor suggestion and the current approach is also fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
openapi/openapi.yaml(2 hunks)pkg/api/resource_id.go(1 hunks)pkg/api/resource_id_test.go(1 hunks)pkg/db/migrations/202511111044_add_clusters.go(1 hunks)pkg/db/migrations/202511111055_add_node_pools.go(2 hunks)pkg/db/sql_helpers.go(3 hunks)pkg/handlers/validation.go(2 hunks)pkg/handlers/validation_test.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(3 hunks)test/factories/clusters.go(2 hunks)test/factories/factory.go(1 hunks)test/factories/node_pools.go(2 hunks)test/integration/search_field_mapping_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/db/migrations/202511111055_add_node_pools.go
- pkg/api/resource_id_test.go
- pkg/handlers/validation.go
- pkg/handlers/validation_test.go
- pkg/services/generic_test.go
🧰 Additional context used
📓 Path-based instructions (11)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
test/factories/clusters.gotest/factories/node_pools.gopkg/api/resource_id.gotest/integration/search_field_mapping_test.gotest/factories/factory.gopkg/db/migrations/202511111044_add_clusters.gopkg/services/generic.gopkg/db/sql_helpers.go
test/factories/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Files:
test/factories/clusters.gotest/factories/node_pools.gotest/factories/factory.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
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/resource_id.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/resource_id.gopkg/db/migrations/202511111044_add_clusters.gopkg/services/generic.gopkg/db/sql_helpers.go
test/integration/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
test/integration/**/*.go: Create integration tests using Testcontainers for isolated PostgreSQL instances per test
All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases
Files:
test/integration/search_field_mapping_test.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/migrations/202511111044_add_clusters.gopkg/db/sql_helpers.go
pkg/db/migrations/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/db/migrations/**/*.go: Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Files:
pkg/db/migrations/202511111044_add_clusters.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/generic.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/services/generic.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/generic.go
🧠 Learnings (10)
📚 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:
test/factories/clusters.gotest/factories/node_pools.gotest/factories/factory.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:
test/factories/clusters.gotest/factories/node_pools.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:
test/factories/clusters.gotest/factories/node_pools.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Applied to files:
test/factories/clusters.gotest/factories/node_pools.gopkg/db/migrations/202511111044_add_clusters.gopkg/services/generic.gopkg/db/sql_helpers.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Always use the context-based database session from db.NewContext(ctx) instead of creating new connections to participate in HTTP request transactions
Applied to files:
test/factories/clusters.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to cmd/hyperfleet/environments/**/*.go : Use OCM_ENV environment variable to select configuration environment (development, unit_testing, integration_testing, production)
Applied to files:
test/factories/clusters.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/search_field_mapping_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/search_field_mapping_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/db/migrations/202511111044_add_clusters.gopkg/db/sql_helpers.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/dao/**/*.go : Increment generation field on each spec update to enable optimistic concurrency control
Applied to files:
pkg/services/generic.gopkg/db/sql_helpers.go
🧬 Code graph analysis (4)
test/factories/clusters.go (4)
pkg/api/cluster_types.go (1)
Cluster(11-34)test/factories/factory.go (1)
Factories(9-10)pkg/db/session.go (1)
SessionFactory(12-20)pkg/db/migrations/migration_structs.go (1)
Model(36-41)
test/integration/search_field_mapping_test.go (2)
test/factories/clusters.go (3)
NewClusterWithStatus(62-86)NewClusterWithLabels(89-112)NewClusterWithStatusAndLabels(115-139)test/factories/node_pools.go (2)
NewNodePoolWithStatus(82-106)NewNodePoolWithLabels(109-132)
test/factories/factory.go (1)
pkg/api/resource_id.go (1)
NewID(14-16)
pkg/services/generic.go (1)
pkg/db/sql_helpers.go (1)
FieldNameWalk(129-220)
🔇 Additional comments (18)
pkg/db/migrations/202511111044_add_clusters.go (2)
70-74: Index onstatus_last_updated_timeis well‑placed and consistent.The new
idx_clusters_status_last_updated_timeindex matches the existing indexing style (IF NOT EXISTSon a single column) and targets the field described as hot in sentinel queries; no changes needed.
80-82: Rollback cleanly removes the new index.Dropping
idx_clusters_status_last_updated_timewithIF EXISTSand doing so before the other drops keeps migrate/rollback symmetric and safe; this is good as‑is.pkg/api/resource_id.go (1)
9-22: LGTM!The implementation correctly encodes KSUID bytes to a 32-character lowercase Base32 string. The custom alphabet ensures DNS-1123 compatibility (lowercase alphanumeric only), and the documentation clearly explains the design rationale.
test/factories/clusters.go (2)
55-86: LGTM! The reload helper and error handling are well implemented.The
reloadClusterhelper properly propagates errors, addressing previous review feedback. Error handling throughoutNewClusterWithStatusis correct.
88-112: LGTM!Proper error handling for JSON marshaling and DB operations. Based on learnings, this follows the factory pattern for consistent test data.
pkg/db/sql_helpers.go (1)
36-43: LGTM! Static mapping is a safe approach.The
statusFieldMappingsuses a static map lookup, which safely restricts user input to known column names without injection risk.openapi/openapi.yaml (2)
410-413: LGTM! Clear documentation with practical examples.The TSL query syntax examples (
status.phase,labels.region) align with the new field mappings implemented insql_helpers.go.
716-719: LGTM! RFC 1123 compliant pattern.The updated pattern
^[a-z0-9]([-a-z0-9]*[a-z0-9])?$correctly enforces DNS-1123 label constraints: starts and ends with alphanumeric, allows internal hyphens. Combined withminLength: 3, this ensures valid Kubernetes resource names.test/factories/node_pools.go (2)
75-106: LGTM! Error handling addresses previous review feedback.The
reloadNodePoolhelper properly propagates errors, and all DB operations inNewNodePoolWithStatuscorrectly handle errors.
108-132: LGTM!Proper error handling for JSON marshaling and DB operations. Based on learnings, this follows the factory pattern for consistent test data.
pkg/services/generic.go (3)
41-49: LGTM! Appropriate disallowed field mappings for provider-specific data.The
specfield is correctly blocked from search queries for both Cluster and NodePool, which aligns with the learning that cluster and nodepool specifications are stored as JSONB and are provider-specific. This prevents potentially expensive or meaningless searches on unindexed JSON blobs.
171-178: LGTM! Correct ordering of field name mapping before related table detection.Moving
FieldNameWalkto execute beforetreeWalkForRelatedTablesis the right architectural decision. This ensures that user-friendly field names likestatus.phaseandlabels.xxxare transformed to their database equivalents (status_phase,labels->>'xxx') before the related table detection logic runs, preventing false positives where "status" or "labels" could be mistakenly identified as related resources.
347-357: LGTM! Clean separation of concerns.The function is now more focused on its single responsibility of converting the TSL tree to a SQLizer. The comment clearly documents that field name mapping happens earlier in the pipeline, which aids future maintainability.
test/integration/search_field_mapping_test.go (5)
16-54: LGTM! Comprehensive test for status.phase field mapping.Good coverage verifying both that the correct cluster is found and that clusters with non-matching status are excluded. The assertions properly validate the status field value after retrieval.
56-99: LGTM! Solid time-based filtering test.Good test coverage for the
status.last_updated_timefield mapping with proper time comparison logic. Using RFC3339 format for the timestamp in the query is the correct approach.
101-145: LGTM! Good JSONB labels query test with appropriate nil checks.The test properly validates the
labels.xxxtolabels->>'xxx'mapping for JSONB queries. The nil check onitem.Labels(line 137) is a good defensive practice.
147-163: LGTM! Important validation test for blocked fields.This test correctly verifies that queries on the
specfield are rejected with a 400 status code, aligning with theSearchDisallowedFieldsconfiguration.
165-227: LGTM! Excellent combined query test coverage.This test provides thorough coverage of AND query combinations, verifying that both status and label conditions must match. The test data setup with three distinct clusters (matching, wrong region, wrong status) effectively validates the query logic.
yasun1
force-pushed
the
search-capabilities
branch
from
b3fa8cb to
1950219
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
pkg/api/resource_id.go (1)
18-22: Consider moving declarations before the function.Conventionally, constants and package-level variables are placed before the functions that use them for improved readability. This is a minor style preference.
package api import ( "encoding/base32" "github.com/segmentio/ksuid" ) +// uidAlphabet is the lowercase alphabet used to encode unique identifiers. +const uidAlphabet = "0123456789abcdefghijklmnopqrstuv" + +// uidEncoding is the lowercase variant of Base32 used to encode unique identifiers. +var uidEncoding = base32.NewEncoding(uidAlphabet).WithPadding(base32.NoPadding) + // NewID generates a new unique identifier using KSUID with lowercase Base32 encoding. // The resulting 32-character lowercase string is compatible with Kubernetes DNS-1123 // subdomain naming requirements, making it suitable for use as Kubernetes resource names // and labels. The KSUID provides time-based ordering (second precision) and global // uniqueness without requiring a central server. func NewID() string { return uidEncoding.EncodeToString(ksuid.New().Bytes()) } - -// uidAlphabet is the lowercase alphabet used to encode unique identifiers. -const uidAlphabet = "0123456789abcdefghijklmnopqrstuv" - -// uidEncoding is the lowercase variant of Base32 used to encode unique identifiers. -var uidEncoding = base32.NewEncoding(uidAlphabet).WithPadding(base32.NoPadding)pkg/db/migrations/202511111055_add_node_pools.go (1)
73-77: Node poolstatus_last_updated_timeindex matches search use case; consider partial index if you always filter out deleted rowsThe index is well‑placed for sentinel “staleness” queries on
status_last_updated_timeand follows the existing indexing pattern. If your queries typically includeWHERE deleted_at IS NULLalongside this column, a partial index likeON node_pools(status_last_updated_time) WHERE deleted_at IS NULLcould shrink index size and improve performance, but it’s purely an optimization, not a blocker.pkg/db/migrations/202511111044_add_clusters.go (1)
70-75: Clustersstatus_last_updated_timeindex is consistent and useful; optional scope tighteningThe added index nicely aligns with the sentinel “find stale clusters” pattern and mirrors the node_pools migration. As with node_pools, if your real queries always add
deleted_at IS NULL, you might get better performance with a partial index scoped to non‑deleted rows, but the current general index is perfectly valid and safe. Based on learnings, this also keepsspecas JSONB, which is in line with our migration guidelines.pkg/handlers/validation.go (1)
46-46: Stale comment references old pattern.The comment still mentions
^[a-z0-9-]+$but the actual pattern is now RFC 1123-compliant.-// validateName validates that a name field matches the pattern ^[a-z0-9-]+$ and length constraints +// validateName validates that a name field matches RFC 1123 DNS subdomain pattern and length constraintstest/factories/node_pools.go (1)
134-159: Consider reusing DB session in combined factory.
NewNodePoolWithStatusAndLabelscreates a new DB session on line 147 even thoughNewNodePoolWithStatusalready created one. While this works correctly for tests, you could pass the session through for efficiency:func NewNodePoolWithStatusAndLabels(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time, labels map[string]string) (*api.NodePool, error) { nodePool, err := NewNodePoolWithStatus(f, dbFactory, id, phase, lastUpdatedTime) if err != nil { return nil, err } if labels != nil { + // Reuse session from factory labelsJSON, err := json.Marshal(labels)This is a minor optimization and acceptable as-is for test code.
test/integration/search_field_mapping_test.go (5)
3-24: Avoid doubleRegisterTestingTand consider dropping dot-import
test.RegisterIntegration(t)already callsgomega.RegisterTestingT(t), so the extraRegisterTestingT(t)in each test is redundant and can be removed. Also consider importing Gomega with a named alias instead of.for clearer call sites and to avoid polluting the package namespace.
56-99: Make time-based search test more deterministicThe relative
time.Now()offsets are reasonable, but you can make this test more robust and environment-independent by anchoring everything to a fixed time in UTC (e.g.base := time.Date(..., time.UTC)) or callingtime.Now().UTC()and formatting withthreshold.UTC().Format(time.RFC3339). That avoids any surprises from local timezone configuration.
101-145: Tighten label assertions to guarantee mapping to responseCurrently the assertion on
item.Labelsonly runs whenitem.Labels != nil, so the test would still pass if the server filtered bylabels.environmentcorrectly but omitted labels from the response. It would be stronger to assert non-nil labels for the matching cluster and then check the key/value, e.g.Expect(item.Labels).NotTo(BeNil())before dereferencing (or drop theifguard).
165-227: Consider also asserting label content in combined query testThe combined-conditions test correctly ensures that only the
NotReady+us-eastcluster is returned and that the other two variants are excluded. To fully validate label mapping here, you could also assert that the returned cluster’s labels includeregion=us-east, mirroring the label assertions inTestSearchLabelsMapping.
229-285: NodePool mapping tests are good; label assertions could be stricterThe NodePool
status.phasemapping is well covered and mirrors the cluster test nicely. For the label-mapping part, you currently only assert that the labeled node pool appears in the results; to ensure response mapping as well as filtering, consider asserting thatitem.Labelsis non-nil and containsenvironment=testfor the matched node pool.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
openapi/openapi.yaml(2 hunks)pkg/api/resource_id.go(1 hunks)pkg/api/resource_id_test.go(1 hunks)pkg/db/migrations/202511111044_add_clusters.go(1 hunks)pkg/db/migrations/202511111055_add_node_pools.go(2 hunks)pkg/db/sql_helpers.go(5 hunks)pkg/handlers/validation.go(2 hunks)pkg/handlers/validation_test.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(3 hunks)test/factories/clusters.go(2 hunks)test/factories/factory.go(1 hunks)test/factories/node_pools.go(2 hunks)test/integration/search_field_mapping_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/factories/factory.go
- pkg/api/resource_id_test.go
🧰 Additional context used
📓 Path-based instructions (12)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Files:
pkg/db/migrations/202511111055_add_node_pools.gopkg/handlers/validation.gopkg/services/generic_test.gopkg/db/sql_helpers.gopkg/handlers/validation_test.gopkg/api/resource_id.gotest/factories/node_pools.gopkg/db/migrations/202511111044_add_clusters.gopkg/services/generic.gotest/factories/clusters.gotest/integration/search_field_mapping_test.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/migrations/202511111055_add_node_pools.gopkg/db/sql_helpers.gopkg/db/migrations/202511111044_add_clusters.go
pkg/db/migrations/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/db/migrations/**/*.go: Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Files:
pkg/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202511111044_add_clusters.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Files:
pkg/db/migrations/202511111055_add_node_pools.gopkg/handlers/validation.gopkg/services/generic_test.gopkg/db/sql_helpers.gopkg/handlers/validation_test.gopkg/api/resource_id.gopkg/db/migrations/202511111044_add_clusters.gopkg/services/generic.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/handlers/validation.gopkg/services/generic_test.gopkg/handlers/validation_test.gopkg/services/generic.go
pkg/handlers/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response
Files:
pkg/handlers/validation.gopkg/handlers/validation_test.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/validation.gopkg/services/generic_test.gopkg/handlers/validation_test.gopkg/services/generic.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/generic_test.gopkg/services/generic.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/resource_id.go
test/factories/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Files:
test/factories/node_pools.gotest/factories/clusters.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
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/search_field_mapping_test.go
🧠 Learnings (10)
📚 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/db/migrations/202511111055_add_node_pools.gopkg/services/generic_test.gotest/factories/node_pools.gopkg/db/migrations/202511111044_add_clusters.gopkg/services/generic.gotest/factories/clusters.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/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202511111044_add_clusters.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/generic_test.gopkg/services/generic.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/integration/**/*.go : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/services/generic_test.gotest/integration/search_field_mapping_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/services/generic_test.gotest/integration/search_field_mapping_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/services/generic_test.gotest/factories/node_pools.gotest/factories/clusters.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/**/*.go : Use PostgreSQL 14.2 with GORM ORM for all database operations
Applied to files:
pkg/db/sql_helpers.gotest/factories/node_pools.gotest/factories/clusters.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:
test/factories/node_pools.gotest/factories/clusters.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: Always use the context-based database session from db.NewContext(ctx) instead of creating new connections to participate in HTTP request transactions
Applied to files:
test/factories/clusters.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to cmd/hyperfleet/environments/**/*.go : Use OCM_ENV environment variable to select configuration environment (development, unit_testing, integration_testing, production)
Applied to files:
test/factories/clusters.go
🧬 Code graph analysis (6)
pkg/handlers/validation.go (1)
pkg/errors/errors.go (1)
Validation(214-216)
pkg/services/generic_test.go (1)
pkg/db/sql_helpers.go (1)
FieldNameWalk(154-245)
pkg/db/sql_helpers.go (1)
pkg/errors/errors.go (2)
ServiceError(100-109)BadRequest(229-231)
test/factories/node_pools.go (4)
pkg/api/node_pool_types.go (1)
NodePool(12-40)test/factories/factory.go (1)
Factories(9-10)pkg/db/session.go (1)
SessionFactory(12-20)pkg/db/migrations/migration_structs.go (1)
Model(36-41)
pkg/services/generic.go (1)
pkg/db/sql_helpers.go (1)
FieldNameWalk(154-245)
test/integration/search_field_mapping_test.go (4)
test/registration.go (1)
RegisterIntegration(13-24)test/factories/clusters.go (3)
NewClusterWithStatus(62-86)NewClusterWithLabels(89-112)NewClusterWithStatusAndLabels(115-139)pkg/api/resource_id.go (1)
NewID(14-16)test/factories/node_pools.go (2)
NewNodePoolWithStatus(82-106)NewNodePoolWithLabels(109-132)
🔇 Additional comments (27)
pkg/api/resource_id.go (2)
3-7: LGTM!Imports are minimal and appropriate for the base32-encoded KSUID generation.
9-16: LGTM!The implementation correctly encodes KSUID's 20 bytes to a 32-character lowercase string using base32. The resulting IDs are DNS-1123 compliant and maintain KSUID's time-ordering properties.
pkg/db/migrations/202511111055_add_node_pools.go (1)
99-101: Rollback correctly drops the new node_pools index firstDropping
idx_node_pools_status_last_updated_timeexplicitly and before the table keeps the rollback path clean and idempotent; this is consistent with your other index cleanup logic.pkg/db/migrations/202511111044_add_clusters.go (1)
80-82: Rollback cleans up the new clusters index in a safe orderDropping
idx_clusters_status_last_updated_timeup front withDROP INDEX IF EXISTSis correct and consistent with the rest of the rollback logic. No issues here.pkg/handlers/validation.go (2)
10-12: LGTM! RFC 1123-compliant name pattern.The updated regex correctly enforces Kubernetes DNS Subdomain naming: must start and end with alphanumeric characters, allowing hyphens only in the middle.
74-77: LGTM! Error message correctly describes the updated validation rules.pkg/handlers/validation_test.go (1)
81-83: Good test coverage for RFC 1123 boundary constraints.These test cases properly validate that names with leading/trailing hyphens are rejected per the updated pattern.
pkg/db/sql_helpers.go (4)
18-30: SQL injection mitigation is properly implemented.The validation approach correctly prevents SQL injection by allowlisting safe characters before interpolating the key into the JSONB query expression. This addresses the previous review concern.
54-61: LGTM! Status field mappings enable user-friendly search syntax.The mapping from
status.xxxto database column names is clear and complete for the documented status fields.
74-86: LGTM! Labels handling with proper validation before query construction.The flow correctly validates the label key before constructing the JSONB expression, preventing injection attacks.
15-16: Label key pattern is too restrictive for common label formats.The pattern
^[a-z0-9_]+$rejects Kubernetes-style labels with hyphens (e.g.,my-label), dots (e.g.,app.kubernetes.io/name), and uppercase characters. According to Kubernetes specifications, label names allow alphanumerics, hyphens, underscores, and dots. Consider expanding the pattern to:-var labelKeyPattern = regexp.MustCompile(`^[a-z0-9_]+$`) +var labelKeyPattern = regexp.MustCompile(`^[a-zA-Z0-9._\-]+$`)This supports the full range of valid label key characters while maintaining SQL injection protection. Update the validation error message accordingly.
openapi/openapi.yaml (2)
410-413: Helpful search syntax documentation with examples.The TSL query examples clearly demonstrate the supported search patterns for status, name filtering, and labels.
718-718: Pattern correctly aligns with backend validation.The OpenAPI pattern matches the RFC 1123-compliant regex in
pkg/handlers/validation.go, ensuring consistent validation at both API spec and runtime levels.pkg/services/generic.go (3)
41-49: LGTM! Properly initialized disallowed fields for both resource types.The
specfield is correctly marked as non-searchable for both Cluster and NodePool, preventing provider-specific field queries.
171-181: Good architectural change: field mapping before related table detection.Moving
FieldNameWalkearlier in the pipeline correctly preventsstatusandlabelsprefixes from being misinterpreted as related table references. The comment clearly explains the rationale.
347-349: Clear documentation of the pipeline change.The comment appropriately notes why
FieldNameWalkis no longer called here, preventing future confusion.pkg/services/generic_test.go (3)
35-37: Test case correctly validates spec field rejection.Updated to test the actual disallowed field (
spec) rather than a manually injected test field.
59-81: Comprehensive test coverage for field name mappings.The test cases properly validate:
status.phase→status_phasestatus.last_updated_time→status_last_updated_timelabels.environment→labels->>'environment'- Direct
idqueries remain unchanged
92-95: Test flow correctly mirrors production pipeline.The explicit
FieldNameWalkcall beforetreeWalkForSqlizermatches the updated production code flow inbuildSearchValues.test/factories/node_pools.go (2)
75-78: Good helper extraction for reload logic.The
reloadNodePoolhelper properly encapsulates the reload pattern and returns errors as requested in previous reviews.
80-106: LGTM! Clean factory for status-based node pool creation.Properly creates the base node pool, updates status fields, and reloads to ensure consistent state.
test/factories/clusters.go (4)
55-58: Good helper extraction for reload logic.Consistent with
node_pools.go, properly encapsulates reload and returns errors.
60-86: LGTM! Factory correctly creates clusters with status fields.The implementation properly handles optional
lastUpdatedTimeand ensures the cluster is reloaded after updates.
88-112: LGTM! Labels factory with proper JSON marshaling.Correctly converts the labels map to JSON before persisting.
114-139: LGTM! Combined factory composes the individual helpers correctly.The implementation reuses
NewClusterWithStatusand adds labels handling when provided. As per coding guidelines, test factories provide consistent test data. This pattern is clear and maintainable.test/integration/search_field_mapping_test.go (2)
16-54: Clusterstatus.phasemapping test looks solidThis test cleanly exercises the
status.phase→status_phasemapping, verifies filtering (excludes the Ready cluster), and asserts the enum value. No issues from a correctness standpoint.
147-163: Spec field rejection test is clear and sufficientThis test cleanly validates that
specis not searchable and that the API responds with400 Bad Request. The behavior under test is straightforward and well covered.
yasun1
force-pushed
the
search-capabilities
branch
from
1950219 to
3eadee6
Compare
…Base32 ID encoding update update update update
yasun1
force-pushed
the
search-capabilities
branch
from
3eadee6 to
61892d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pkg/db/sql_helpers.go (1)
15-30: Label key validation properly closes the SQL injection hole; charset is intentionally narrowThe
validateLabelKey+labelKeyPatternflow now ensures only[a-z0-9_]keys make it intolabels->>'%s', which removes the earlier injection risk from interpolating raw user input into the JSONB path. This looks solid and addresses the prior concern from earlier reviews.The trade‑off is that perfectly valid labels containing characters outside this set (e.g.
-or uppercase) will now be rejected at search time with a 400. If that’s acceptable for your API surface, this is fine; if you expect to support a broader label key charset later, you may want to relax the regex (while still keeping it free of quotes and operators).Also applies to: 74-86
test/factories/node_pools.go (1)
75-79: Reload helper and error propagation look correct and address earlier feedbackThe
reloadNodePoolhelper plus the updated reload calls in all three factory functions now checkFirst(...).Errorand propagate failures, which fixes the earlier issue where reload errors were silently ignored.This makes DB issues during tests much more visible and keeps the factories consistent with the analogous cluster helpers.
Also applies to: 101-105, 127-131, 153-155
🧹 Nitpick comments (7)
README.md (2)
11-16: Consider aligning documented Go/Postgres versions with your actual support policyThe README now advertises “Go 1.24 or higher” and “PostgreSQL 13+”. Internally you’re targeting a specific toolchain (Go 1.24.9 with FIPS crypto) and Postgres 14.x for migrations/ORM behavior.
If you truly depend on those baselines, it may be clearer to state them explicitly (e.g. “Go 1.24.9+” and “PostgreSQL 14+”) to avoid users trying to run against older minor versions that haven’t been exercised.
Also applies to: 212-215
184-204: TSL search docs match the implementation; you may want to mention label-key restrictionsThe examples for
status.phaseandstatus.last_updated_timereflect the newstatus.* → status_*mappings, andlabels.environmentmatches thelabels->>'environment'translation, so the documented queries are consistent with the backend.Given that the implementation now enforces a strict label key regex (
[a-z0-9_]+after thelabels.prefix), it might be helpful to briefly call out that only “simple” lowercase label keys are currently searchable, so users don’t get surprised when querying labels with-or uppercase characters.Also applies to: 564-570
test/factories/factory.go (1)
3-7: Test ID generation now matches production IDs; consider centralizing the encoderUsing
uidEncoding.EncodeToString(ksuid.New().Bytes())with the custom lowercase Base32 alphabet ensures factory IDs have the same 32‑character, DNS‑1123‑compatible format as the mainpkg/apiNewID()implementation, which is exactly what you want in tests.The only downside is that
uidAlphabet/uidEncodingare now defined in bothpkg/api/resource_id.goand here. If you expect the ID format to evolve, it might be worth centralizing these constants in a small shared internal package so tests and production code can’t drift apart over time.Also applies to: 12-16, 18-22
test/factories/clusters.go (3)
60-86: Consider accepting a context parameter for flexibility.The function uses
context.Background()when creating the DB session. While acceptable for test factories, accepting acontext.Contextparameter would provide more flexibility for tests that need specific context values or timeouts.Example signature:
func NewClusterWithStatus(f *Factories, dbFactory db.SessionFactory, ctx context.Context, id string, phase string, lastUpdatedTime *time.Time) (*api.Cluster, error) { // ... dbSession := dbFactory.New(ctx) // ... }If not provided by the caller,
context.Background()can be used as a default.
88-112: LGTM with same context parameter suggestion.The implementation correctly marshals labels to JSON and updates the database. The same recommendation about accepting a context parameter (from the previous comment) applies here for consistency.
114-139: LGTM! Consider session reuse for efficiency.The function correctly combines status and labels updates. It creates two separate DB sessions (one in
NewClusterWithStatusat line 69, another at line 127), which is acceptable for test code but could be optimized by reusing a single session if performance becomes a concern.test/integration/search_field_mapping_test.go (1)
231-285: LGTM! Consider splitting for better test isolation.The test correctly verifies that NodePools support the same field mapping as Clusters for both status and labels. The test combines two searches (status.phase and labels.environment) in a single test function. While functional, splitting into separate tests would provide better isolation and clearer failure messages.
Consider splitting into:
TestSearchNodePoolStatusMappingTestSearchNodePoolLabelsMapping
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(4 hunks)openapi/openapi.yaml(2 hunks)pkg/api/resource_id.go(1 hunks)pkg/api/resource_id_test.go(1 hunks)pkg/db/migrations/202511111044_add_clusters.go(1 hunks)pkg/db/migrations/202511111055_add_node_pools.go(2 hunks)pkg/db/sql_helpers.go(5 hunks)pkg/handlers/validation.go(2 hunks)pkg/handlers/validation_test.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(3 hunks)test/factories/clusters.go(2 hunks)test/factories/factory.go(1 hunks)test/factories/node_pools.go(2 hunks)test/integration/search_field_mapping_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/handlers/validation.go
- pkg/handlers/validation_test.go
- pkg/api/resource_id.go
- pkg/api/resource_id_test.go
- openapi/openapi.yaml
🧰 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/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202511111044_add_clusters.gopkg/db/sql_helpers.gopkg/services/generic_test.gotest/factories/node_pools.gopkg/services/generic.gotest/factories/factory.gotest/integration/search_field_mapping_test.gotest/factories/clusters.go
pkg/db/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use PostgreSQL 14.2 with GORM ORM for all database operations
Files:
pkg/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202511111044_add_clusters.gopkg/db/sql_helpers.go
pkg/db/migrations/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/db/migrations/**/*.go: Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Use polymorphic adapter_statuses table with owner_type and owner_id columns instead of separate status tables per resource type
Files:
pkg/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202511111044_add_clusters.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims
Files:
pkg/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202511111044_add_clusters.gopkg/db/sql_helpers.gopkg/services/generic_test.gopkg/services/generic.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Always retrieve database session from context via db.NewContext() instead of creating new connections
Files:
pkg/services/generic_test.gopkg/services/generic.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields
Files:
pkg/services/generic_test.gopkg/services/generic.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Implement adapter status aggregation: phase is Ready if all adapters report Ready=True, Failed if any report Ready=False, NotReady otherwise
Files:
pkg/services/generic_test.gopkg/services/generic.go
test/factories/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Files:
test/factories/node_pools.gotest/factories/factory.gotest/factories/clusters.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/search_field_mapping_test.go
🧠 Learnings (14)
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Applied to files:
pkg/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202511111044_add_clusters.gopkg/services/generic_test.gotest/factories/node_pools.gotest/factories/clusters.goREADME.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/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/db/migrations/202511111055_add_node_pools.gopkg/db/migrations/202511111044_add_clusters.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/db/migrations/202511111044_add_clusters.gopkg/services/generic_test.goREADME.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 test/integration/**/*.go : Create integration tests using Testcontainers for isolated PostgreSQL instances per test
Applied to files:
pkg/services/generic_test.gotest/integration/search_field_mapping_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/services/generic_test.gotest/integration/search_field_mapping_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:
test/factories/node_pools.gotest/factories/factory.gotest/factories/clusters.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:
test/factories/node_pools.gotest/factories/clusters.goREADME.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}/**/*.go : Always retrieve database session from context via db.NewContext() instead of creating new connections
Applied to files:
test/factories/node_pools.gotest/factories/clusters.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: Always use the context-based database session from db.NewContext(ctx) instead of creating new connections to participate in HTTP request transactions
Applied to files:
test/factories/clusters.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to cmd/hyperfleet/environments/**/*.go : Use OCM_ENV environment variable to select configuration environment (development, unit_testing, integration_testing, production)
Applied to files:
test/factories/clusters.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to **/*.go : Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Applied to files:
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/api/**/*.go : Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
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/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:
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 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.md
🧬 Code graph analysis (5)
pkg/db/sql_helpers.go (1)
pkg/errors/errors.go (2)
ServiceError(100-109)BadRequest(229-231)
pkg/services/generic.go (1)
pkg/db/sql_helpers.go (1)
FieldNameWalk(154-245)
test/factories/factory.go (1)
pkg/api/resource_id.go (1)
NewID(14-16)
test/integration/search_field_mapping_test.go (5)
test/registration.go (1)
RegisterIntegration(13-24)test/factories/clusters.go (3)
NewClusterWithStatus(62-86)NewClusterWithLabels(89-112)NewClusterWithStatusAndLabels(115-139)test/factories/factory.go (1)
Factories(9-10)pkg/api/resource_id.go (1)
NewID(14-16)test/factories/node_pools.go (2)
NewNodePoolWithStatus(82-106)NewNodePoolWithLabels(109-132)
test/factories/clusters.go (4)
pkg/api/cluster_types.go (1)
Cluster(11-34)test/factories/factory.go (1)
Factories(9-10)pkg/db/session.go (1)
SessionFactory(12-20)pkg/db/migrations/migration_structs.go (1)
Model(36-41)
🔇 Additional comments (14)
pkg/db/sql_helpers.go (1)
54-61: status. mapping and disallowed-field handling are coherent with search semantics*The
statusFieldMappingsmap and the earlystatus.xxx → status_xxxrewrite ingetFieldensure that user-friendly queries likestatus.phaseandstatus.last_updated_timeare consistently mapped to the flattened columns before the disallowed-field check, which is exactly what the migrations/indexes expect.Also worth noting:
labels.*short‑circuits before the disallowed‑field logic, whilestatus.*flows through it via the mapped name. That matches the currentSearchDisallowedFieldscontents (onlyspecis blocked) but is something to keep in mind if you ever decide certain label keys should be non‑searchable.Also applies to: 88-92, 104-108
pkg/services/generic.go (2)
41-49: SearchDisallowedFields now cleanly expresses spec-only blockingRestricting the disallowed map to just
"spec"forClusterandNodePool, withallFieldsAllowedas the default, makes the behavior explicit and easy to extend per resource type later. ThenewListContextfallback logic toallFieldsAllowedkeeps other resource types fully searchable by default.
171-176: FieldNameWalk-before-related-tables ordering is correct and avoids misclassificationRunning
db.FieldNameWalkimmediately after parsing the TSL tree (and beforetreeWalkForRelatedTablesandtreeWalkForAddingTableName) is the right call: it ensuresstatus.*andlabels.*are normalized to their SQL forms before any related-table detection kicks in, sostatus/labelsaren’t accidentally treated as relation prefixes.The corresponding comment in
treeWalkForSqlizermakes it clear that field mapping is a prior step, which should help prevent future duplication ofFieldNameWalkin that stage.Also applies to: 178-181, 347-349
pkg/db/migrations/202511111055_add_node_pools.go (1)
73-77: status_last_updated_time index for node_pools is well-scoped and rollback-safeThe
idx_node_pools_status_last_updated_timeindex onstatus_last_updated_timematches the search pattern described in comments (sentinel queries for stale resources), and theIF NOT EXISTSplus explicit drop in rollback keeps the migration idempotent and reversible. This is consistent with the clusters migration and looks good.Also applies to: 99-101
pkg/db/migrations/202511111044_add_clusters.go (1)
70-74: Clusters status_last_updated_time index mirrors node_pools strategy appropriatelyThe
idx_clusters_status_last_updated_timeindex complements the node_pools index and matches the flattenedstatus_last_updated_timecolumn used by the new search mappings. Rollback ordering is correct (drop index before table), so this migration is safe and consistent.Also applies to: 80-82
pkg/services/generic_test.go (1)
35-37: Tests accurately cover disallowed spec and new field mappingsThe updated bad-input case (
spec = '{}') now correctly asserts thespec is not a valid field nameerror, matching theSearchDisallowedFieldsbehavior.The additional SQL translation tests for:
status.phase→status_phase = ?status.last_updated_time→status_last_updated_time < ?labels.environment→labels->>'environment' = ?id→id = ?combined with the explicit
FieldNameWalkcall beforetreeWalkForSqlizer, give good coverage of the new mapping pipeline. This is a solid, focused test expansion.Also applies to: 59-81, 92-95
test/factories/node_pools.go (1)
80-106: New NodePool status/label factories are well-structured for integration tests
NewNodePoolWithStatus,NewNodePoolWithLabels, andNewNodePoolWithStatusAndLabelsbuild onNewNodePool, perform focused updates (status fields and/or labels JSON), and then reload via the shared helper. This keeps test setup concise while ensuring you always exercise the real DB schema and GORM mapping.Using a
db.SessionFactoryargument instead of constructing sessions internally is also a nice touch for test control.Also applies to: 108-132, 134-159
test/factories/clusters.go (2)
5-12: LGTM!The new imports support JSON marshaling of labels, timestamp handling, GORM DB operations, and the db package for session management. All imports are appropriate for the new factory functionality.
55-58: LGTM!The
reloadClusterhelper consolidates the reload pattern and properly handles errors, addressing the previous review comments about uncheckeddbSession.Firstcalls.test/integration/search_field_mapping_test.go (5)
18-54: LGTM!The test correctly verifies that
status.phaseuser-friendly syntax maps to thestatus_phasedatabase field. It includes both positive validation (NotReady cluster found) and negative validation (Ready cluster not found), following integration testing best practices.Based on coding guidelines, integration tests use Testcontainers for isolated PostgreSQL instances.
59-99: LGTM!The test correctly verifies time-based filtering with
status.last_updated_timemapping. The test uses sufficient time margins (2 hours vs 30 minutes with a 1-hour threshold) to prevent flakiness from timestamp precision issues.
103-145: LGTM!The test correctly verifies JSONB label mapping with
labels.xxxsyntax. Good defensive programming with the nil check foritem.Labelsat line 137 before asserting on the map contents.
149-163: LGTM!The test correctly verifies that attempts to query the
specfield are rejected with a 400 Bad Request, which is important for preventing arbitrary JSONB queries and maintaining search field constraints.
167-227: LGTM!The test provides excellent coverage of combined AND queries, verifying that both status and labels field mappings work correctly together. It includes comprehensive negative validation by ensuring clusters that match only one condition are properly excluded.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
pkg/handlers/validation.go (1)
10-12: Align validation comments with the new DNS‑1123 label regexThe behavior of
namePatternand the error message is correct, but the comments are now misleading:
- Line 10–11: the pattern is for a single DNS‑1123 label (no dots), while the comment says “DNS Subdomain Names”; consider clarifying that it matches a label suitable for use as a DNS‑1123 subdomain component.
- Line 46: still references the old pattern
^[a-z0-9-]+$; update this to describe the current^[a-z0-9]([-a-z0-9]*[a-z0-9])?$rule (start/end alnum, hyphens only in the middle).This is purely documentation, but tightening it will avoid future confusion when someone cross‑checks the regex and tests.
Also applies to: 46-47, 74-77
README.md (1)
11-11: Search docs look good; consider clarifying supported operators/fields with aninexampleThe new TSL search documentation and examples are clear and align with the implemented field mapping (
name,status.phase,labels.<key>). Two optional tweaks that might help users:
- Add a concrete
inoperator example (e.g.,labels.env in ('dev','staging')) alongside the AND/OR examples so all listed operators are demonstrated.- Optionally call out that only the documented fields are searchable (e.g.,
specis intentionally not supported) to avoid users guessing unsupported fields.These are small doc improvements; behavior and examples are otherwise consistent and easy to follow.
Also applies to: 184-201, 561-567
test/factories/factory.go (1)
3-7: Avoid duplicating ID encoding logic; delegate toapi.NewIDThe KSUID+Base32 logic here mirrors
pkg/api/resource_id.go, which is good for now but risks drift if the production ID format changes.Given factories already depend on
pkg/apitypes, you can simplify and guarantee consistency by delegating:-import ( - "encoding/base32" - - "github.com/segmentio/ksuid" -) +import ( + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" +) @@ -// NewID generates a new unique identifier using KSUID with lowercase Base32 encoding. -// The resulting identifier is compatible with Kubernetes DNS-1123 subdomain naming requirements. -func (f *Factories) NewID() string { - return uidEncoding.EncodeToString(ksuid.New().Bytes()) -} - -// uidAlphabet is the lowercase alphabet used to encode unique identifiers. -const uidAlphabet = "0123456789abcdefghijklmnopqrstuv" - -// uidEncoding is the lowercase variant of Base32 used to encode unique identifiers. -var uidEncoding = base32.NewEncoding(uidAlphabet).WithPadding(base32.NoPadding) +// NewID delegates to the production API ID generator so tests use the same format. +func (f *Factories) NewID() string { + return api.NewID() +}This keeps test IDs forever aligned with the runtime behavior and removes redundant encoding constants here.
Also applies to: 12-16, 18-22
pkg/api/resource_id_test.go (1)
8-30: Solid NewID tests; consider precompiling regexes to avoid duplication and overheadThe test coverage for
NewID(length, charset, uniqueness, and DNS‑1123 compatibility) looks very good.Two optional refinements:
- In
TestNewID,regexp.MustCompile("^[0-9a-v]{32}$")is called inside the loop; consider compiling it once (either at package scope or at the start of the test) and reusing it.- The DNS‑1123 pattern in
TestNewID_K8sCompatibleduplicates the validation regex used in the HTTP layer; if you ever expose that regex as a shared helper/const, using it here would reduce the risk of the tests silently diverging from the actual validation rule.These are small test‑hygiene improvements; the current tests are already functionally strong.
Also applies to: 32-51, 53-67
pkg/api/resource_id.go (1)
3-7: NewID implementation matches requirements; use it as the single source of truthThe KSUID+lowercase Base32 implementation here cleanly satisfies the 32‑char,
[0-9a-v]charset and DNS‑1123‑compatible requirements described in the comments and tests.Given this is the canonical ID generator:
- It’s a good idea for other code (notably test factories) to delegate to
api.NewID()rather than duplicatinguidAlphabet/uidEncodinglogic elsewhere. That keeps all callers aligned if the encoding ever needs to change.- Keeping
uidAlphabet/uidEncodingunexported here is fine; callers only needNewID().No functional issues found; just recommend making this the single source of truth for ID generation across the codebase.
Also applies to: 9-22
test/factories/node_pools.go (1)
5-8: Node pool factory helpers correctly update DB state and handle reload errorsThe new helpers look well‑structured:
reloadNodePoolcentralizes the reload logic and now properly checksFirst(...).Error, addressing the earlier concern about ignored reload errors.NewNodePoolWithStatusandNewNodePoolWithStatusAndLabelsupdatestatus_phaseand optionalstatus_last_updated_timein the DB, then reload, which ensures tests see the same shape as real persisted data.NewNodePoolWithLabelsmirrors the same pattern forlabelsand correctly propagates JSON marshal and DB errors.One small nuance: if
NewNodePoolWithLabelsis ever called withlabels == nil, it will store JSONnullin the column rather than an empty object; if that matters for any tests, you could special‑case nil to write{}instead. Otherwise this looks solid for factory usage.Also applies to: 13-15, 75-79, 80-106, 108-132, 134-159
test/factories/clusters.go (2)
114-139: Minor optimization opportunity: redundant reload when labels are provided.When
labels != nil, the cluster is reloaded twice: once insideNewClusterWithStatusand again after updating labels. For test factories this is acceptable, but could be optimized by extracting an internal helper that skips the intermediate reload.
62-62: Consider API consistency with existing factory methods.The new functions are standalone (
factories.NewClusterWithStatus(f, ...)) while existing functions are methods (f.NewCluster(...)). This inconsistency exists becausedbFactoryis needed. Consider addingdb.SessionFactoryto theFactoriesstruct to enable a consistent method-based API, or document this design choice.test/integration/search_field_mapping_test.go (1)
231-285: Consider splitting into separate tests for status and labels.This test validates two distinct behaviors (status.phase mapping and labels mapping) for NodePools. While functional, splitting into
TestSearchNodePoolStatusPhaseMappingandTestSearchNodePoolLabelsMappingwould provide better test isolation and clearer failure diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.md(4 hunks)openapi/openapi.yaml(2 hunks)pkg/api/resource_id.go(1 hunks)pkg/api/resource_id_test.go(1 hunks)pkg/db/migrations/202511111044_add_clusters.go(1 hunks)pkg/db/migrations/202511111055_add_node_pools.go(2 hunks)pkg/db/sql_helpers.go(5 hunks)pkg/handlers/validation.go(2 hunks)pkg/handlers/validation_test.go(1 hunks)pkg/services/generic.go(3 hunks)pkg/services/generic_test.go(3 hunks)test/factories/clusters.go(2 hunks)test/factories/factory.go(1 hunks)test/factories/node_pools.go(2 hunks)test/integration/search_field_mapping_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/db/sql_helpers.go
- pkg/db/migrations/202511111044_add_clusters.go
- pkg/db/migrations/202511111055_add_node_pools.go
- openapi/openapi.yaml
- pkg/services/generic_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/validation_test.gopkg/services/generic.gotest/factories/clusters.gopkg/handlers/validation.gotest/factories/factory.gopkg/api/resource_id.gotest/factories/node_pools.gopkg/api/resource_id_test.gotest/integration/search_field_mapping_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/validation_test.gopkg/services/generic.gopkg/handlers/validation.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/validation_test.gopkg/handlers/validation.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/validation_test.gopkg/services/generic.gopkg/handlers/validation.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/validation_test.gopkg/services/generic.gopkg/handlers/validation.gopkg/api/resource_id.gopkg/api/resource_id_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/generic.go
test/factories/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Files:
test/factories/clusters.gotest/factories/factory.gotest/factories/node_pools.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/resource_id.gopkg/api/resource_id_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/search_field_mapping_test.go
🧠 Learnings (13)
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to test/factories/**/*.go : Use test data factories in test/factories/ to provide consistent test data instead of hardcoding values
Applied to files:
test/factories/clusters.gotest/factories/factory.gotest/factories/node_pools.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:
test/factories/clusters.goREADME.mdtest/factories/node_pools.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:
test/factories/clusters.gotest/factories/node_pools.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and nodepool specifications as JSONB in PostgreSQL schema, never as separate columns
Applied to files:
test/factories/clusters.goREADME.mdtest/factories/node_pools.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: Always use the context-based database session from db.NewContext(ctx) instead of creating new connections to participate in HTTP request transactions
Applied to files:
test/factories/clusters.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to cmd/hyperfleet/environments/**/*.go : Use OCM_ENV environment variable to select configuration environment (development, unit_testing, integration_testing, production)
Applied to files:
test/factories/clusters.go
📚 Learning: 2025-12-03T15:47:39.989Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T15:47:39.989Z
Learning: Applies to **/*.go : Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)
Applied to files:
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/api/**/*.go : Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml
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/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:
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/dao/**/*.go : Increment generation field on each spec update to enable optimistic concurrency control
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 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.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 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/resource_id_test.gotest/integration/search_field_mapping_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/resource_id_test.gotest/integration/search_field_mapping_test.go
🧬 Code graph analysis (5)
pkg/services/generic.go (1)
pkg/db/sql_helpers.go (1)
FieldNameWalk(154-245)
test/factories/clusters.go (4)
pkg/api/cluster_types.go (1)
Cluster(11-34)test/factories/factory.go (1)
Factories(9-10)pkg/db/session.go (1)
SessionFactory(12-20)pkg/db/migrations/migration_structs.go (1)
Model(36-41)
pkg/handlers/validation.go (1)
pkg/errors/errors.go (1)
Validation(214-216)
test/factories/node_pools.go (4)
pkg/api/node_pool_types.go (1)
NodePool(12-40)test/factories/factory.go (1)
Factories(9-10)pkg/db/session.go (1)
SessionFactory(12-20)pkg/db/migrations/migration_structs.go (1)
Model(36-41)
pkg/api/resource_id_test.go (1)
pkg/api/resource_id.go (1)
NewID(14-16)
🔇 Additional comments (11)
pkg/handlers/validation_test.go (1)
72-94: Good coverage for boundary hyphen casesThe added
invalidNames("-test","test-","-test-") correctly exercise the new “must start and end alphanumeric” rule and keep the assertion tied to the updated error message substring. This nicely closes the gap on common DNS‑1123 boundary mistakes.pkg/services/generic.go (1)
41-50: Field mapping ordering and disallowedspecfields look correctThe refactor here tightens the search pipeline nicely:
- Centralizing
SearchDisallowedFieldsper resource type and defaulting toallFieldsAllowedmakes it explicit thatspecis intentionally non‑searchable/non‑sortable for bothClusterandNodePool.- Running
db.FieldNameWalkbeforetreeWalkForRelatedTablesis the right fix to keepstatus.*andlabels.*from being treated as related resources while still allowing true related fields (e.g.,creator.username) to be resolved bytreeWalkForRelatedTables.- Updating
treeWalkForSqlizerto assume the mapping is already applied keeps responsibilities clear and avoids double‑walking the tree.Given the existing tests around field mapping and search, this ordering change looks sound. No functional issues spotted.
Also applies to: 67-88, 160-201, 347-355
test/factories/clusters.go (3)
55-58: LGTM! Clean helper extraction.The
reloadClusterhelper nicely consolidates the reload logic across all factory functions, addressing the past review feedback about code duplication.
60-86: LGTM! Well-structured status update factory.The conditional update logic for
lastUpdatedTimeand proper error handling look correct. Based on learnings, this factory provides consistent test data.
88-112: LGTM! Clean labels factory implementation.JSON marshaling and database update are properly error-handled.
test/integration/search_field_mapping_test.go (6)
1-14: LGTM! Appropriate imports for integration testing.The use of gomega with dot import follows common BDD-style testing patterns. As per coding guidelines, this test uses factories for consistent test data.
18-54: LGTM! Comprehensive status phase mapping test.The test correctly verifies both positive (NotReady cluster found) and negative (Ready cluster excluded) conditions for the
status.phasefield mapping.
59-99: LGTM! Good coverage of time-based filtering.The test properly validates timestamp comparison using RFC3339 format and verifies correct filtering behavior for
status.last_updated_time.
103-145: LGTM! Well-structured labels mapping test.The defensive nil check on
item.Labels(line 137) and the verification of both cluster inclusion and exclusion provide good coverage.
149-163: LGTM! Important negative test case.Testing that
specfield queries are rejected with 400 Bad Request validates the field access control mechanism.
167-227: LGTM! Thorough combined query test.The test matrix (matching, wrong region, wrong status) provides excellent coverage for verifying AND query behavior with both status and label conditions.
|
/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 2ee9190
into
openshift-hyperfleet:main
2 participants
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.