HYPERFLEET-545 - feat: add PATCH operations for clusters and nodepools#32
HYPERFLEET-545 - feat: add PATCH operations for clusters and nodepools#32Mischulee wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAPI version bumped to 1.0.7. Added PATCH endpoints for clusters and nodepools with new request models 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🎯 3 (Moderate) | ⏱️ ~25 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.
@rh-amarin labels are not implemented yet in PATCH handler (https://github.com/openshift-hyperfleet/hyperfleet-api/blob/main/pkg/handlers/cluster.go#L63), including it would be misleading. I think thats for separate ticket.
There was a problem hiding this comment.
Ticket for labels: https://redhat.atlassian.net/browse/HYPERFLEET-972
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")
| 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
3 times, most recently
from
fc0efbf to
b9fcfdb
Compare
Mischulee
force-pushed
the
HYPERFLEET-545
branch
from
b9fcfdb to
19f6056
Compare
3 participants
Summary
Adds PATCH operations for clusters and nested nodepools.
Test Plan
Summary by CodeRabbit
New Features
Chores