feat: introduce generation field for NodePool management #22
Conversation
|
Warning Rate limit exceeded@rh-amarin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe PR adds generation tracking across API and backend: OpenAPI schemas gain a required integer Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API_Handler as API Handler (Patch/Update)
participant Service as NodePool Service
participant DAO as NodePool DAO
participant DB as Database
note right of Client: Update NodePool spec (PATCH)
Client->>API_Handler: PATCH /nodepools/{id} with new spec
API_Handler->>DAO: Get existing nodepool by id
DAO->>DB: SELECT nodepool
DB-->>DAO: existing nodepool (with Spec, Generation)
API_Handler->>DAO: Replace nodepool with updated Spec
DAO->>DAO: compare existing.Spec vs new Spec
alt spec changed
DAO->>DAO: nodepool.Generation++ (increment)
else spec unchanged
DAO->>DAO: preserve existing.Generation
end
DAO->>DB: UPDATE nodepool (new Spec, Generation)
DB-->>DAO: OK
DAO-->>API_Handler: updated nodepool
API_Handler-->>Client: 200 OK (nodepool includes Generation)
sequenceDiagram
autonumber
participant Adapter as Adapter(s)
participant Service as NodePool Service
participant DAO as NodePool DAO
participant DB as Database
note right of Adapter: Adapter statuses reported asynchronously
Adapter->>Service: Report adapter status (observedGeneration, phase, etc.)
Service->>DAO: Fetch nodepool (to read Generation/compute phase)
DAO->>DB: SELECT nodepool
DB-->>DAO: nodepool (with Generation)
DAO-->>Service: nodepool
Service->>Service: aggregate adapter statuses, use nodepool.Generation in phase computation
Service->>DB: UPDATE nodepool.status (phase, observedGeneration, adapters)
DB-->>Service: OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/api/node_pool_types.go (1)
87-90: Remove Generation from NodePoolPatchRequest to enforce server-side control.Including Generation in the patch request allows clients to include it in the API contract, which violates the principle that generation should be server-controlled. Kubernetes registry detects spec changes and increments generation automatically, and it would confuse controllers if clients were allowed to change generation freely. The DAO layer correctly increments generation server-side when the spec changes (see
pkg/dao/node_pool.golines 60-66), but the API type should not expose this field to clients to prevent misuse and align with Kubernetes design patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openapi/openapi.yaml(7 hunks)pkg/api/node_pool_types.go(3 hunks)pkg/api/presenters/node_pool.go(1 hunks)pkg/dao/node_pool.go(2 hunks)pkg/services/node_pool.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
pkg/{dao,services}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use context.Context as the first parameter in all DAO and service method signatures
Files:
pkg/dao/node_pool.gopkg/services/node_pool.go
pkg/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use structured logging via pkg/logger/logger.go with Extra() method for adding context fields
Files:
pkg/dao/node_pool.gopkg/api/presenters/node_pool.gopkg/services/node_pool.gopkg/api/node_pool_types.go
pkg/{db/migrations,dao}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use polymorphic status tables with owner_type and owner_id columns to support multiple resource types
Files:
pkg/dao/node_pool.go
pkg/{dao,handlers}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Support pagination in list operations via ListArgs parameter with configurable limit and offset
Files:
pkg/dao/node_pool.go
pkg/dao/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/dao/**/*.go: Implement DAO interfaces with Create, Get, List, Update, Delete methods following CRUD pattern
Accept ListArgs parameter in DAO List methods to support pagination with limit and offset
Query JSONB columns using GORM's native operators for filtering on nested properties
Files:
pkg/dao/node_pool.go
pkg/{handlers,services,dao}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Return ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields in error handling
Files:
pkg/dao/node_pool.gopkg/services/node_pool.go
pkg/{handlers,services}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Retrieve database sessions from context via db.NewContext() rather than creating new GORM connections
Files:
pkg/services/node_pool.go
pkg/services/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
pkg/services/**/*.go: Calculate aggregate status phase from adapter conditions using Ready/Failed/NotReady states
Implement service layer between handlers and DAOs for business logic like status aggregation and generation tracking
Files:
pkg/services/node_pool.go
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/{cluster_service,nodepool_service}.go : Increment generation field on each cluster or node pool spec update to enable optimistic concurrency control
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{db/migrations,dao}/adapter_status.go : Store adapter statuses with observed_generation field to track which resource generation each adapter has processed
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/{cluster_service,nodepool_service}.go : Increment generation field on each cluster or node pool spec update to enable optimistic concurrency control
Applied to files:
pkg/dao/node_pool.gopkg/api/presenters/node_pool.gopkg/services/node_pool.gopkg/api/node_pool_types.goopenapi/openapi.yaml
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and node pool specifications as JSONB columns to allow flexible cloud provider configurations
Applied to files:
pkg/dao/node_pool.gopkg/api/presenters/node_pool.gopkg/api/node_pool_types.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Enforce foreign key relationships between node pools and clusters via owner_references constraint
Applied to files:
pkg/dao/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/dao/**/*.go : Query JSONB columns using GORM's native operators for filtering on nested properties
Applied to files:
pkg/dao/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/dao/**/*.go : Implement DAO interfaces with Create, Get, List, Update, Delete methods following CRUD pattern
Applied to files:
pkg/dao/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{dao,services}/**/*.go : Use context.Context as the first parameter in all DAO and service method signatures
Applied to files:
pkg/dao/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/dao/**/*.go : Accept ListArgs parameter in DAO List methods to support pagination with limit and offset
Applied to files:
pkg/dao/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{dao,handlers}/**/*.go : Support pagination in list operations via ListArgs parameter with configurable limit and offset
Applied to files:
pkg/dao/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{handlers,services}/**/*.go : Retrieve database sessions from context via db.NewContext() rather than creating new GORM connections
Applied to files:
pkg/dao/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/api/openapi/model_*.go : Support JSON request/response serialization with proper struct tags on generated OpenAPI models
Applied to files:
pkg/api/presenters/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/**/*.go : Calculate aggregate status phase from adapter conditions using Ready/Failed/NotReady states
Applied to files:
pkg/services/node_pool.go
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{db/migrations,dao}/adapter_status.go : Store adapter statuses with observed_generation field to track which resource generation each adapter has processed
Applied to files:
pkg/services/node_pool.go
🧬 Code graph analysis (2)
pkg/dao/node_pool.go (1)
pkg/db/context.go (1)
MarkForRollback(48-57)
pkg/services/node_pool.go (1)
pkg/services/status_aggregation.go (1)
ComputePhase(99-169)
🔇 Additional comments (10)
pkg/api/presenters/node_pool.go (1)
90-96: LGTM!The Generation field is correctly mapped from the GORM model to the OpenAPI response structure, aligning with the broader changes to introduce generation tracking for NodePool resources.
pkg/dao/node_pool.go (2)
4-4: LGTM!The
bytesimport is appropriately used for comparing JSONB spec fields to determine generation changes.
52-68: LGTM! Generation increment logic correctly implements optimistic concurrency control.The logic correctly increments generation only when the spec changes, preserving it otherwise. This aligns with the learning that generation should be incremented on spec updates to enable optimistic concurrency control.
Based on learnings, this implementation correctly tracks spec changes while preserving generation when only non-spec fields (like labels) are updated.
pkg/api/node_pool_types.go (2)
22-23: LGTM!The Generation field is correctly defined with appropriate database constraints (default 1, not null) to support optimistic concurrency control.
61-63: LGTM!The BeforeCreate hook correctly initializes Generation to 1 when not already set, ensuring all NodePools start with a valid generation value.
pkg/services/node_pool.go (1)
189-189: LGTM! Correctly uses actual NodePool generation for status computation.This change enables proper generation tracking in status aggregation by passing the actual
nodePool.GenerationtoComputePhaseinstead of a hardcoded 0. This allows the system to correctly determine if adapters are processing stale generations.openapi/openapi.yaml (4)
635-654: LGTM! Generation field correctly added to Cluster schema.The generation field is properly defined with appropriate constraints (int32, minimum 1, required) and clear documentation explaining it tracks customer intent versions.
907-943: LGTM! Generation field correctly added to NodePool schema.The generation field is properly defined and included as a required field in the NodePool schema, with appropriate constraints and documentation. The example correctly shows
generation: 1.
1047-1069: LGTM! Generation field correctly added to NodePoolCreateResponse schema.The generation field is properly included in the create response, ensuring clients receive the initialized generation value (1) when creating NodePools.
1213-1214: The server URL was not changed in this commit.The current production server URL is
https://hyperfleet.redhat.com. This URL appears in the initial commit of the repository and has not been updated from a previous value. No change fromapi.hyperfleet.redhat.comoccurred.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
openapi.yaml (6)
1-1: Consider using OpenAPI 3.0.3.Based on learnings, the project should use OpenAPI 3.0.3 specification. Using
3.0.0may miss bug fixes and clarifications in later patch versions.-openapi: 3.0.0 +openapi: 3.0.3
773-780: Missingminimum: 1constraint onobserved_generation.
ClusterStatus.observed_generationlacks theminimum: 1constraint that's applied toCluster.generation(line 656),NodePool.generation(line 929), andNodePoolStatus.observed_generation(line 1137). For consistency, all generation-related fields should have the same constraint.observed_generation: type: integer format: int32 + minimum: 1 description: |-
500-503: Missingminimum: 1constraint onobserved_generation.
AdapterStatusBase.observed_generationshould haveminimum: 1for consistency with other generation fields in the spec.observed_generation: type: integer format: int32 + minimum: 1 description: Which generation of the resource this status reflects
1187-1190: Missingminimum: 1constraint onobserved_generation.
ResourceCondition.observed_generationshould haveminimum: 1for consistency with other generation fields.observed_generation: type: integer format: int32 + minimum: 1 description: Generation of the spec that this condition reflects
435-441: Consider addingmaxItemsconstraint toconditionsarray.Static analysis flags this array as unbounded. While the number of conditions is typically bounded by the number of adapters, adding an explicit
maxItemsconstraint provides defense-in-depth against malformed payloads.conditions: type: array + maxItems: 50 items: $ref: '#/components/schemas/AdapterCondition'
1043-1100:NodePoolCreateResponseduplicates fields fromNodePool.Similar to the previous comment, this schema duplicates many fields. Consider using
allOfwithNodePoolor restructuring the schemas to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openapi.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/{cluster_service,nodepool_service}.go : Increment generation field on each cluster or node pool spec update to enable optimistic concurrency control
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{db/migrations,dao}/adapter_status.go : Store adapter statuses with observed_generation field to track which resource generation each adapter has processed
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and node pool specifications as JSONB columns to allow flexible cloud provider configurations
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Use OpenAPI 3.0.3 specification generated from TypeSpec for API contract definition
Applied to files:
openapi.yaml
🪛 Checkov (3.2.334)
openapi.yaml
[high] 1-1220: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-1220: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 436-443: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (1)
openapi.yaml (1)
653-657: Generation field implementation looks good.The
generationfield is correctly added toClusterandNodePoolwith appropriate constraints (integer,int32,minimum: 1). The field is properly excluded from create requests since it's server-managed. This aligns with the learnings about incrementing generation on spec updates for optimistic concurrency control.Also applies to: 926-930
openapi.yaml
Outdated
| BearerAuth: | ||
| type: object | ||
| required: | ||
| - type | ||
| - scheme | ||
| properties: | ||
| type: | ||
| type: string | ||
| enum: | ||
| - http | ||
| scheme: | ||
| type: string | ||
| enum: | ||
| - bearer |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd openapi.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 112
🏁 Script executed:
wc -l openapi.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 96
🏁 Script executed:
# First examine the section with BearerAuth (lines 617-630 and surrounding context)
sed -n '600,650p' openapi.yaml
# Then search for key structural elements
echo "=== Searching for BearerAuth ==="
grep -n "BearerAuth" openapi.yaml
echo "=== Searching for securitySchemes ==="
grep -n "securitySchemes" openapi.yaml
echo "=== Searching for global security field ==="
grep -n "^security:" openapi.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1594
🏁 Script executed:
# Check which section BearerAuth is under
sed -n '580,625p' openapi.yaml | head -50
# Check for components section
echo "=== Checking components section ==="
grep -n "^components:" openapi.yaml
echo "=== Checking if schemas section exists ==="
grep -n " schemas:" openapi.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1613
🏁 Script executed:
# Verify the structure - what's between components: and BearerAuth
sed -n '363,420p' openapi.yaml
# Check what comes after BearerAuth to confirm it's in schemas
sed -n '617,680p' openapi.yaml | head -70Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 3135
🏁 Script executed:
# Check if securitySchemes section exists anywhere in the file
grep -n "securitySchemes" openapi.yaml
# Check for any security field at root level or paths level
echo "=== Root level security ==="
sed -n '1,370p' openapi.yaml | grep "^security:"
echo "=== Checking around openapi, info sections ==="
head -50 openapi.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1908
🏁 Script executed:
# Check the entire structure around components to understand it better
sed -n '363,1218p' openapi.yaml | grep -A 5 "^ [a-z]" | head -80Repository: openshift-hyperfleet/hyperfleet-api
Length of output: 292
🏁 Script executed:
# Check for securitySchemes anywhere in the entire file
cat openapi.yaml | grep -i "securityscheme"
# Check the structure from 363 to end to see all top-level keys under components
sed -n '363,430p' openapi.yamlRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 1896
🏁 Script executed:
# Confirm the full structure - list all keys directly under components
python3 << 'EOF'
import yaml
with open('openapi.yaml', 'r') as f:
spec = yaml.safe_load(f)
print("Keys under components:")
if 'components' in spec:
for key in spec['components'].keys():
print(f" - {key}")
if key == 'schemas' and 'BearerAuth' in spec['components']['schemas']:
print(f" (BearerAuth found in schemas)")
print("\nRoot-level security:")
if 'security' in spec:
print(f" security: {spec['security']}")
else:
print(" No global security field found")
print("\nBearerAuth structure in schemas:")
if 'components' in spec and 'schemas' in spec['components'] and 'BearerAuth' in spec['components']['schemas']:
ba = spec['components']['schemas']['BearerAuth']
print(f" type: {ba.get('type')}")
print(f" required: {ba.get('required')}")
print(f" properties: {list(ba.get('properties', {}).keys())}")
EOFRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 330
🏁 Script executed:
# Check if BearerAuth is referenced anywhere in paths or operations
grep -n "BearerAuth" openapi.yaml
# Check if any operations have security defined
grep -n "security:" openapi.yaml
# Verify the structure of a typical securityScheme - show what it SHOULD look like
python3 << 'EOF'
# Show what the correct structure should be
print("=== Current (INCORRECT) structure ===")
print("components:")
print(" schemas:")
print(" BearerAuth:")
print(" type: object")
print(" required: ['type', 'scheme']")
print(" properties:")
print(" type:")
print(" type: string")
print(" enum: ['http']")
print(" scheme:")
print(" type: string")
print(" enum: ['bearer']")
print()
print("=== Correct structure should be ===")
print("components:")
print(" securitySchemes:")
print(" BearerAuth:")
print(" type: http")
print(" scheme: bearer")
print()
print("# Then applied at root or operation level:")
print("security:")
print(" - BearerAuth: []")
EOFRepository: openshift-hyperfleet/hyperfleet-api
Length of output: 570
BearerAuth is defined as a schema, not a security scheme.
This schema object defines authentication properties but is placed under components.schemas rather than components.securitySchemes. As a result, no security is actually enforced on any endpoint.
If authentication is intentionally removed for this PR, remove this orphaned schema. If authentication is required, move it to securitySchemes and apply it globally or per-operation.
🤖 Prompt for AI Agents
In openapi.yaml around lines 617 to 630, the BearerAuth definition is a schema
placed under components.schemas instead of a security scheme, so no
authentication is enforced; either delete this orphaned schema if auth is
intentionally removed, or move it to components.securitySchemes as a security
scheme (use type: http and scheme: bearer, optionally bearerFormat) and then
reference it where needed by adding a security requirement (e.g. global
security: - BearerAuth: [] or per-operation security entries).
openapi.yaml
Outdated
| NodePoolCreateRequest: | ||
| type: object | ||
| required: | ||
| - name | ||
| - spec | ||
| properties: | ||
| labels: | ||
| type: object | ||
| additionalProperties: | ||
| type: string | ||
| description: labels for the API resource as pairs of name:value strings | ||
| id: | ||
| type: string | ||
| description: Resource identifier | ||
| kind: | ||
| type: string | ||
| description: Resource kind | ||
| href: | ||
| type: string | ||
| description: Resource URI | ||
| name: | ||
| type: string | ||
| description: NodePool name (unique in a cluster) | ||
| spec: | ||
| allOf: | ||
| - $ref: '#/components/schemas/NodePoolSpec' | ||
| description: |- | ||
| Cluster specification | ||
| CLM doesn't know how to unmarshall the spec, it only stores and forwards to adapters to do their job | ||
| But CLM will validate the schema before accepting the request | ||
| example: | ||
| name: worker-pool-1 | ||
| labels: | ||
| environment: production | ||
| pooltype: worker | ||
| spec: {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
NodePoolCreateRequest duplicates NodePoolBase fields.
This schema redefines all fields from NodePoolBase instead of using allOf composition like ClusterCreateRequest does (line 725-726). This violates DRY and risks schema drift.
NodePoolCreateRequest:
type: object
- required:
- - name
- - spec
- properties:
- labels:
- type: object
- additionalProperties:
- type: string
- description: labels for the API resource as pairs of name:value strings
- id:
- type: string
- description: Resource identifier
- kind:
- type: string
- description: Resource kind
- href:
- type: string
- description: Resource URI
- name:
- type: string
- description: NodePool name (unique in a cluster)
- spec:
- allOf:
- - $ref: '#/components/schemas/NodePoolSpec'
- description: |-
- Cluster specification
- CLM doesn't know how to unmarshall the spec, it only stores and forwards to adapters to do their job
- But CLM will validate the schema before accepting the request
+ allOf:
+ - $ref: '#/components/schemas/NodePoolBase'
example:
name: worker-pool-1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NodePoolCreateRequest: | |
| type: object | |
| required: | |
| - name | |
| - spec | |
| properties: | |
| labels: | |
| type: object | |
| additionalProperties: | |
| type: string | |
| description: labels for the API resource as pairs of name:value strings | |
| id: | |
| type: string | |
| description: Resource identifier | |
| kind: | |
| type: string | |
| description: Resource kind | |
| href: | |
| type: string | |
| description: Resource URI | |
| name: | |
| type: string | |
| description: NodePool name (unique in a cluster) | |
| spec: | |
| allOf: | |
| - $ref: '#/components/schemas/NodePoolSpec' | |
| description: |- | |
| Cluster specification | |
| CLM doesn't know how to unmarshall the spec, it only stores and forwards to adapters to do their job | |
| But CLM will validate the schema before accepting the request | |
| example: | |
| name: worker-pool-1 | |
| labels: | |
| environment: production | |
| pooltype: worker | |
| spec: {} | |
| NodePoolCreateRequest: | |
| type: object | |
| allOf: | |
| - $ref: '#/components/schemas/NodePoolBase' | |
| example: | |
| name: worker-pool-1 | |
| labels: | |
| environment: production | |
| pooltype: worker | |
| spec: {} |
🤖 Prompt for AI Agents
In openapi.yaml around lines 1007 to 1042, the NodePoolCreateRequest schema
duplicates fields from NodePoolBase; change it to use allOf composition
referencing NodePoolBase and the NodePoolSpec (similar to ClusterCreateRequest
at lines ~725-726) so the schema inherits id, kind, href, name, labels, etc.,
and then add/specify the required spec property. Update the example to match the
composed schema (keep name, labels, spec) and remove the duplicated property
definitions to avoid schema drift.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
openapi/openapi.yaml (1)
710-712: Name constraint changes already under discussion.The changes to
minLength: 1and the less restrictive pattern^[a-z0-9-]+$have been discussed in previous comments and acknowledged by the developer. These will be addressed in HYPERFLEET-364. The inconsistency with Go validation code (which still enforces minLength: 3) was also previously flagged.
🧹 Nitpick comments (1)
openapi/openapi.yaml (1)
617-630: Consider removing unused BearerAuth schema.The AI summary indicates BearerAuth was removed from
components.securitySchemes, but this schema definition remains. If it's no longer referenced anywhere, consider removing it to avoid confusion.Apply this diff if the schema is confirmed unused:
- BearerAuth: - type: object - required: - - type - - scheme - properties: - type: - type: string - enum: - - http - scheme: - type: string - enum: - - bearer Cluster:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openapi/openapi.yaml(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/{cluster_service,nodepool_service}.go : Increment generation field on each cluster or node pool spec update to enable optimistic concurrency control
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/{db/migrations,dao}/adapter_status.go : Store adapter statuses with observed_generation field to track which resource generation each adapter has processed
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/db/migrations/**/*.go : Store cluster and node pool specifications as JSONB columns to allow flexible cloud provider configurations
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Use OpenAPI 3.0.3 specification generated from TypeSpec for API contract definition
Applied to files:
openapi/openapi.yaml
📚 Learning: 2025-12-12T11:04:29.971Z
Learnt from: CR
Repo: openshift-hyperfleet/hyperfleet-api PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T11:04:29.971Z
Learning: Applies to pkg/services/{cluster_service,nodepool_service}.go : Increment generation field on each cluster or node pool spec update to enable optimistic concurrency control
Applied to files:
openapi/openapi.yaml
🔇 Additional comments (6)
openapi/openapi.yaml (6)
4-4: LGTM: Version bump for traceability.Good practice incrementing the version to 1.0.1. This aligns with the developer's comment about maintaining traceability when the OpenAPI contract changes.
402-404: LGTM: Helpful TSL documentation added.The description with examples improves API usability by clarifying the Tree Search Language syntax for filtering results.
638-638: LGTM: Generation field properly added to Cluster.The generation field is correctly configured:
- Marked as required for existing clusters
- Properly typed (int32, minimum: 1)
- Description clearly explains it tracks customer intent changes
- Initial value of 1 in examples is appropriate
- Correctly excluded from ClusterCreateRequest (server-initialized)
This aligns with the learning that generation enables optimistic concurrency control.
Based on learnings: "Increment generation field on each cluster or node pool spec update to enable optimistic concurrency control"
Also applies to: 653-657, 673-673
910-910: LGTM: Generation field properly added to NodePool.The generation field for NodePool is correctly configured:
- Marked as required in the schema
- Properly typed (int32, minimum: 1) with clear description
- Initial value of 1 shown in example
- Correctly excluded from NodePoolCreateRequest (server-initialized)
- Included in NodePoolCreateResponse (lines 1050, 1068-1072)
Structure is consistent with the Cluster generation field implementation.
Based on learnings: "Increment generation field on each cluster or node pool spec update to enable optimistic concurrency control"
Also applies to: 926-930, 946-946
1050-1050: LGTM: Generation field in NodePoolCreateResponse.The generation field is correctly included in NodePoolCreateResponse as a required field with proper typing (int32, minimum: 1). This ensures clients receive the server-initialized generation value after creating a NodePool.
Also applies to: 1068-1072
1216-1216: This appears to be a newly added API specification file with no prior version in git history. The URLhttps://hyperfleet.redhat.comwas not changed from a previous value—it was established when the openapi.yaml file was first committed today. There is no evidence of a priorapi.hyperfleet.redhat.comURL in the repository.If you have concerns about the choice of this specific URL or want to verify it's the correct production endpoint, that's a separate discussion from a breaking change to existing clients.
Likely an incorrect or invalid review comment.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rafabene 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 bb50b29
into
openshift-hyperfleet:main
2 participants
https://issues.redhat.com/browse/HYPERFLEET-278
v1.0.1from thisRefactor NodePool models to include generation field hyperfleet-api-spec#20
generationfield missing in NodePoolsnamefield from cluster and nodePool patchRequests(even though this is not for MVP, the code is in the repository already)
namefield for cluster and nodepoolSummary by CodeRabbit
New Features
Breaking Changes
Security
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.