HYPERFLEET-545 - feat: add PATCH operations for clusters and nodepools#32
Conversation
WalkthroughBumps API version to 1.0.7 and adds PATCH endpoints for clusters and nodepools with new request models ClusterPatchRequest and NodePoolPatchRequest (both require ≥1 top-level property and disallow additionalProperties) and attached examples. OpenAPI/Swagger specs updated for core and GCP (including spec update variants for platform-specific patch shapes). Service interfaces add patchClusterById and patchNodePoolById with Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "API Gateway / HyperFleet API"
participant Auth as "Auth (BearerAuth)"
participant Service as "HyperFleet Service"
participant Store as "DataStore"
Client->>API: PATCH /api/hyperfleet/v1/clusters/{cluster_id}\nBody: ClusterPatchRequest (Bearer token)
API->>Auth: Validate token
Auth-->>API: token OK
API->>Service: patchClusterById(cluster_id, body)
Service->>Store: Read & apply partial updates
Store-->>Service: Updated Cluster
Service-->>API: 200 (Cluster) or 400/404/Error
API-->>Client: 200 (Cluster) or Error
sequenceDiagram
participant Client
participant API as "API Gateway / HyperFleet API"
participant Auth as "Auth (BearerAuth)"
participant Service as "HyperFleet Service"
participant Store as "DataStore"
Client->>API: PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}\nBody: NodePoolPatchRequest (Bearer token)
API->>Auth: Validate token
Auth-->>API: token OK
API->>Service: patchNodePoolById(cluster_id, nodepool_id, body)
Service->>Store: Read & apply partial updates to NodePool
Store-->>Service: Updated NodePool
Service-->>API: 200 (NodePool) or 400/404/Error
API-->>Client: 200 (NodePool) or Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@schemas/gcp/openapi.yaml`:
- Around line 863-867: Update the six PATCH-related schemas to reject empty
objects and unknown keys by adding "additionalProperties: false" and
"minProperties: 1" to each schema: ClusterPatchRequest,
ClusterPlatformSpecUpdate, ClusterSpecUpdate, NodePoolPatchRequest,
NodePoolPlatformSpecUpdate, and NodePoolSpecUpdate; ensure these constraints are
applied inside each schema object so PATCH requestBody validation fails for
empty payloads or typos while keeping requestBody.required: true on the
endpoints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb207614-e9f4-4dee-b8ac-b8705d2e078c
📒 Files selected for processing (9)
main.tspmodels/clusters/model.tspmodels/nodepools/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/clusters.tspservices/nodepools.tsp
| model ClusterPatchRequest { | ||
| spec?: ClusterSpec; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think that labels should be also possible to update in a PATCH request
Same for nodepools
The JIRA ticket says:
Not in scope: Labels in PATCH — out of scope per epic. Handler/service implementation — already exists.
But that means that the implementation may be already in the code, but is not in the API spec, so IMO it should be added to the API spec.
There was a problem hiding this comment.
The spec may also benefit from having an example,
take a look at the create operation
@example(exampleClusterCreateRequest)
model ClusterCreateRequest {
...ClusterBase;
}
There was a problem hiding this comment.
Labels were already in the model but not applied by the PATCH handlers. I have added the handling and exposed labels in both ClusterPatchRequest and NodePoolPatchRequest in the api spec.
Mischulee
force-pushed
the
HYPERFLEET-545
branch
from
bbd86ec to
d317c12
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
models-core/nodepool/example_patch.tsp (1)
4-6: Avoid shipping a no-op PATCH example.
models/nodepools/model.tsp(Lines 60-62) uses this value as the publicNodePoolPatchRequestexample.spec: #{}doesn't demonstrate any mutation and makes a no-op request look like the intended client payload. If core can't show a concrete change here, I'd omit the example and keep the real example in provider-specific specs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models-core/nodepool/example_patch.tsp` around lines 4 - 6, The public example exampleNodePoolPatchRequest currently sets spec: #{} which is a no-op; update the NodePoolPatchRequest example (exampleNodePoolPatchRequest) to either show a concrete, meaningful mutation in spec (e.g., changing a field that is commonly patched) or remove/omit the example entirely from models-core so provider-specific specs can supply real examples; ensure references to the exampleNodePoolPatchRequest and the spec field in the patch request are adjusted accordingly.models-core/cluster/example_patch.tsp (1)
4-6: Avoid shipping a no-op PATCH example.
models/clusters/model.tsp(Lines 62-65) binds this constant to the publicClusterPatchRequest, sospec: #{}becomes the documented example for the new PATCH endpoint. That doesn't show an actual update and makes a no-op payload look intentional. If core has no stable field to mutate, I'd omit the core example and let provider-specific specs carry the concrete PATCH examples.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models-core/cluster/example_patch.tsp` around lines 4 - 6, The exampleClusterPatchRequest constant currently contains an empty no-op payload (spec: #{}) which gets published as the public ClusterPatchRequest example; remove this misleading core example by deleting the exampleClusterPatchRequest constant (or replace it with a comment stating core has no stable patch fields) so that provider-specific modules supply real PATCH examples instead; locate the symbol exampleClusterPatchRequest in the diff and remove or neutralize it rather than shipping an empty spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@models-core/cluster/example_patch.tsp`:
- Around line 4-6: The exampleClusterPatchRequest constant currently contains an
empty no-op payload (spec: #{}) which gets published as the public
ClusterPatchRequest example; remove this misleading core example by deleting the
exampleClusterPatchRequest constant (or replace it with a comment stating core
has no stable patch fields) so that provider-specific modules supply real PATCH
examples instead; locate the symbol exampleClusterPatchRequest in the diff and
remove or neutralize it rather than shipping an empty spec.
In `@models-core/nodepool/example_patch.tsp`:
- Around line 4-6: The public example exampleNodePoolPatchRequest currently sets
spec: #{} which is a no-op; update the NodePoolPatchRequest example
(exampleNodePoolPatchRequest) to either show a concrete, meaningful mutation in
spec (e.g., changing a field that is commonly patched) or remove/omit the
example entirely from models-core so provider-specific specs can supply real
examples; ensure references to the exampleNodePoolPatchRequest and the spec
field in the patch request are adjusted accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6590a4c-b023-45c6-9108-502039358a71
📒 Files selected for processing (15)
aliases-core.tspaliases-gcp.tspmain.tspmodels-core/cluster/example_patch.tspmodels-core/nodepool/example_patch.tspmodels-gcp/cluster/example_patch.tspmodels-gcp/nodepool/example_patch.tspmodels/clusters/model.tspmodels/nodepools/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/clusters.tspservices/nodepools.tsp
✅ Files skipped from review due to trivial changes (5)
- aliases-core.tsp
- aliases-gcp.tsp
- models-gcp/cluster/example_patch.tsp
- main.tsp
- schemas/gcp/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- models/nodepools/model.tsp
- models/clusters/model.tsp
- services/nodepools.tsp
- services/clusters.tsp
- schemas/gcp/swagger.yaml
- schemas/core/openapi.yaml
| } | Error | ||
| | BadRequestResponse; | ||
|
|
||
| @route("/{cluster_id}") |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
The nodepool PATCH has a JSDoc comment that generates a description field in OpenAPI, but the cluster PATCH doesn't. This causes an inconsistency in the generated specs where patchNodePoolById has a description but patchClusterById does not.
Consider adding a JSDoc comment before @route:
/**
* Patch cluster spec fields by cluster ID
*/
@route("/{cluster_id}")
@patch(#{implicitOptionality: true})
@summary("Patch cluster by ID")There was a problem hiding this comment.
Description added.
| import "./models-gcp/nodepool/example_nodepool.tsp"; | ||
| import "./models-gcp/nodepool/example_post.tsp"; No newline at end of file | ||
| import "./models-gcp/nodepool/example_post.tsp"; | ||
| import "./models-gcp/nodepool/example_patch.tsp"; No newline at end of file |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Pattern
Missing trailing newline at end of file. Git shows \ No newline at end of file for this line. Adding a newline after the last import keeps the file POSIX-compliant and avoids noisy diffs if another import is appended later.
Mischulee
force-pushed
the
HYPERFLEET-545
branch
4 times, most recently
from
b9fcfdb to
19f6056
Compare
Mischulee
force-pushed
the
HYPERFLEET-545
branch
from
19f6056 to
f54f2b1
Compare
| ...ClusterBase; | ||
| } | ||
|
|
||
| @extension("minProperties", 1) |
There was a problem hiding this comment.
Warning
Blocking
Category: Inconsistency
The additionalProperties: false fix discussed in the CodeRabbit thread doesn't appear to have been applied. Neither the TypeSpec models nor the generated schemas contain it.
Current state (models/clusters/model.tsp):
@extension("minProperties", 1)
@example(exampleClusterPatchRequest)
model ClusterPatchRequest {
spec?: ClusterSpec;
labels?: Record<string>;
}Suggested fix — add the decorator to both ClusterPatchRequest and NodePoolPatchRequest:
@extension("minProperties", 1)
@extension("additionalProperties", false)
@example(exampleClusterPatchRequest)
model ClusterPatchRequest {
spec?: ClusterSpec;
labels?: Record<string>;
}Then regenerate the schemas (npm run build:all) so the YAML files pick up the change.
| ): Cluster | ||
| | Error | ||
| | NotFoundResponse | ||
| | BadRequestResponse; |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Standards
The BadRequestResponse used here generates a 400 response without a body schema. Per the HyperFleet error-model standard (RFC 9457), error responses should include a Problem Details body (application/problem+json).
This is a pre-existing pattern across all endpoints, so I wouldn't fix it just for PATCH — but it might be worth a follow-up ticket to define a custom BadRequestResponse model with a Problem Details body and apply it consistently across the API.
Mischulee
force-pushed
the
HYPERFLEET-545
branch
from
f54f2b1 to
5c00354
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
schemas/gcp/openapi.yaml (1)
1088-1121:⚠️ Potential issue | 🟠 MajorGCP PATCH models still admit empty sub-patches
The top-level
minProperties: 1is not enough here.{"labels": {}},{"spec": {}}, and{"spec": {"platform": {}}}still validate because the label maps and the*Updateobjects all allow empty objects, and the nested*Updateschemas still accept unknown keys.Suggested contract tightening
ClusterPatchRequest: type: object properties: spec: $ref: '#/components/schemas/ClusterSpecUpdate' labels: type: object + minProperties: 1 additionalProperties: type: string @@ ClusterPlatformSpecUpdate: type: object + additionalProperties: false + minProperties: 1 properties: type: type: string @@ ClusterSpecUpdate: type: object + additionalProperties: false + minProperties: 1 properties: infraID: type: string @@ NodePoolPatchRequest: type: object properties: spec: $ref: '#/components/schemas/NodePoolSpecUpdate' labels: type: object + minProperties: 1 additionalProperties: type: string @@ NodePoolPlatformSpecUpdate: type: object + additionalProperties: false + minProperties: 1 properties: type: type: string @@ NodePoolSpecUpdate: type: object + additionalProperties: false + minProperties: 1 properties: platform: $ref: '#/components/schemas/NodePoolPlatformSpecUpdate'Also applies to: 1147-1183, 1557-1590, 1635-1665
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/gcp/openapi.yaml` around lines 1088 - 1121, The ClusterPatchRequest and other PATCH schemas (e.g., the ClusterSpecUpdate and nested platform/update schemas referenced by spec and labels) currently allow empty sub-patches; fix by tightening the schemas: add minProperties: 1 to the labels object, and in each "*Update" schema referenced (ClusterSpecUpdate and its nested platform/ gcp/ networking/etc. update types) set additionalProperties: false and minProperties: 1 so empty objects are rejected; ensure nested platform (e.g., platform/gcp) update schemas also require at least one field and disallow unknown keys. Apply the same changes to the other patch model blocks noted (lines referenced in the review: the other Cluster*PatchRequest equivalents).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@schemas/core/openapi.yaml`:
- Around line 1126-1140: ClusterPatchRequest currently allows no-op patches
because minProperties: 1 only forces a top-level key and accepts {"spec": {}} or
{"labels": {}}; update ClusterPatchRequest to require at least one non-empty
field by replacing the loose properties rule with an anyOf/oneOf that enforces
non-empty nested objects (e.g., a branch where "spec" is present and its schema
requires minProperties: 1 or references ClusterSpec-with-minProperties, and
another branch where "labels" is present and has minProperties: 1), and apply
the same pattern to the other patch schemas mentioned (lines ~1483-1497) so a
PATCH must change at least one actual field rather than an empty object.
---
Duplicate comments:
In `@schemas/gcp/openapi.yaml`:
- Around line 1088-1121: The ClusterPatchRequest and other PATCH schemas (e.g.,
the ClusterSpecUpdate and nested platform/update schemas referenced by spec and
labels) currently allow empty sub-patches; fix by tightening the schemas: add
minProperties: 1 to the labels object, and in each "*Update" schema referenced
(ClusterSpecUpdate and its nested platform/ gcp/ networking/etc. update types)
set additionalProperties: false and minProperties: 1 so empty objects are
rejected; ensure nested platform (e.g., platform/gcp) update schemas also
require at least one field and disallow unknown keys. Apply the same changes to
the other patch model blocks noted (lines referenced in the review: the other
Cluster*PatchRequest equivalents).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf0d1374-75b2-4d94-95b0-ac7ab32b54e8
📒 Files selected for processing (15)
aliases-core.tspaliases-gcp.tspmain.tspmodels-core/cluster/example_patch.tspmodels-core/nodepool/example_patch.tspmodels-gcp/cluster/example_patch.tspmodels-gcp/nodepool/example_patch.tspmodels/clusters/model.tspmodels/nodepools/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/clusters.tspservices/nodepools.tsp
✅ Files skipped from review due to trivial changes (4)
- aliases-gcp.tsp
- models-core/nodepool/example_patch.tsp
- main.tsp
- aliases-core.tsp
🚧 Files skipped from review as they are similar to previous changes (7)
- models/nodepools/model.tsp
- models-gcp/cluster/example_patch.tsp
- models-gcp/nodepool/example_patch.tsp
- services/clusters.tsp
- services/nodepools.tsp
- models-core/cluster/example_patch.tsp
- models/clusters/model.tsp
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin 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 fe59c7a
into
openshift-hyperfleet:main
3 participants
Summary
Adds PATCH operations for clusters and nested nodepools.
Test Plan
Summary by CodeRabbit
New Features
Chores