HYPERFLEET-978 - feat: (1/2) Include routes in API for PUT status updates#121
HYPERFLEET-978 - feat: (1/2) Include routes in API for PUT status updates#121ma-hill wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
[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 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/retest |
| schema: | ||
| $ref: '#/components/schemas/AdapterStatusCreateRequest' | ||
| put: | ||
| operationId: putNodePoolStatuses |
There was a problem hiding this 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: 2
🧹 Nitpick comments (1)
test/integration/adapter_status_test.go (1)
73-75: ⚡ Quick winKeep a compatibility test for the retained POST endpoints.
The PR explicitly keeps the POST status routes alive for this rollout, but this file swaps the integration coverage entirely to PUT. That removes the end-to-end guardrail for the temporary compatibility path. A small smoke test for cluster and nodepool POST would preserve that contract until the follow-up removal PR lands.
Also applies to: 199-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/adapter_status_test.go` around lines 73 - 75, Add a small compatibility smoke test that calls the retained POST status endpoints in addition to the new PUT tests: after the existing PutClusterStatusesWithResponse call (and similarly around the nodepool test near the area referenced at lines ~199-201) add assertions that client.PostClusterStatusesWithResponse(ctx, cluster.ID, openapi.PostClusterStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx)) succeeds and returns the expected status, and likewise call client.PostNodePoolStatusesWithResponse(ctx, cluster.ID, nodepool.ID, openapi.PostNodePoolStatusesJSONRequestBody(statusInput), test.WithAuthToken(ctx)). Ensure you reuse the same statusInput, cluster.ID and nodepool.ID variables and mirror the same response assertions used for the PUT tests so the POST compatibility path remains covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openapi/openapi.yaml`:
- Around line 533-551: The OpenAPI spec omits the documented 204 No Content
success response for the PUT upsert endpoints; update the responses block for
the PUT endpoints that currently reference
AdapterStatus/AdapterStatusCreateRequest to include a '204' response entry
(description: successful no-op/discarded update, content: none) so the generated
client/docs reflect the actual behavior (e.g., where AdapterStatus and
AdapterStatusCreateRequest are used); apply the same change to the other
occurrence noted (around the second block at lines 637-656).
- Around line 518-551: The new PUT operation (operationId putNodePoolStatuses)
is missing the OpenAPI security declaration; add a security block referencing
the existing BearerAuth scheme (e.g., security: - BearerAuth: []) to the
operation so the generated docs/clients show it requires authentication and
aligns with the middleware protecting the /clusters routes.
---
Nitpick comments:
In `@test/integration/adapter_status_test.go`:
- Around line 73-75: Add a small compatibility smoke test that calls the
retained POST status endpoints in addition to the new PUT tests: after the
existing PutClusterStatusesWithResponse call (and similarly around the nodepool
test near the area referenced at lines ~199-201) add assertions that
client.PostClusterStatusesWithResponse(ctx, cluster.ID,
openapi.PostClusterStatusesJSONRequestBody(statusInput),
test.WithAuthToken(ctx)) succeeds and returns the expected status, and likewise
call client.PostNodePoolStatusesWithResponse(ctx, cluster.ID, nodepool.ID,
openapi.PostNodePoolStatusesJSONRequestBody(statusInput),
test.WithAuthToken(ctx)). Ensure you reuse the same statusInput, cluster.ID and
nodepool.ID variables and mirror the same response assertions used for the PUT
tests so the POST compatibility path remains covered.
🪄 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: Enterprise
Run ID: 75408a9d-5108-49a2-8554-a18baea0260c
📒 Files selected for processing (4)
cmd/hyperfleet-api/server/api_server.goopenapi/openapi.yamlplugins/clusters/plugin.gotest/integration/adapter_status_test.go
| put: | ||
| operationId: putNodePoolStatuses | ||
| summary: Adapter creates or updates resource nodepool status | ||
| parameters: | ||
| - name: cluster_id | ||
| in: path | ||
| required: true | ||
| description: Cluster ID | ||
| schema: | ||
| type: string | ||
| - name: nodepool_id | ||
| in: path | ||
| required: true | ||
| schema: | ||
| type: string | ||
| responses: | ||
| '201': | ||
| description: The request has succeeded and a new resource has been created as a result. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/AdapterStatus' | ||
| '400': | ||
| description: The server could not understand the request due to invalid syntax. | ||
| '404': | ||
| description: The server cannot find the requested resource. | ||
| '409': | ||
| description: The request conflicts with the current state of the server. | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/AdapterStatusCreateRequest' |
There was a problem hiding this comment.
Add BearerAuth to the new nodepool status PUT operation.
plugins/clusters/plugin.go, Lines 89-90, protects all /clusters routes with auth middleware, but this new operation is published without security. That makes the OpenAPI contract and generated docs/client surface claim the endpoint is unauthenticated when it is not.
Suggested spec fix
put:
operationId: putNodePoolStatuses
summary: Adapter creates or updates resource nodepool status
parameters:
- name: cluster_id
in: path
required: true
description: Cluster ID
schema:
type: string
- name: nodepool_id
in: path
required: true
schema:
type: string
responses:
'201':
description: The request has succeeded and a new resource has been created as a result.
content:
application/json:
schema:
$ref: '#/components/schemas/AdapterStatus'
'400':
description: The server could not understand the request due to invalid syntax.
'404':
description: The server cannot find the requested resource.
'409':
description: The request conflicts with the current state of the server.
requestBody:
required: true
content:
application/json:
schema:
$ref: '#/components/schemas/AdapterStatusCreateRequest'
+ security:
+ - BearerAuth: []📝 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.
| put: | |
| operationId: putNodePoolStatuses | |
| summary: Adapter creates or updates resource nodepool status | |
| parameters: | |
| - name: cluster_id | |
| in: path | |
| required: true | |
| description: Cluster ID | |
| schema: | |
| type: string | |
| - name: nodepool_id | |
| in: path | |
| required: true | |
| schema: | |
| type: string | |
| responses: | |
| '201': | |
| description: The request has succeeded and a new resource has been created as a result. | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/AdapterStatus' | |
| '400': | |
| description: The server could not understand the request due to invalid syntax. | |
| '404': | |
| description: The server cannot find the requested resource. | |
| '409': | |
| description: The request conflicts with the current state of the server. | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/AdapterStatusCreateRequest' | |
| put: | |
| operationId: putNodePoolStatuses | |
| summary: Adapter creates or updates resource nodepool status | |
| parameters: | |
| - name: cluster_id | |
| in: path | |
| required: true | |
| description: Cluster ID | |
| schema: | |
| type: string | |
| - name: nodepool_id | |
| in: path | |
| required: true | |
| schema: | |
| type: string | |
| responses: | |
| '201': | |
| description: The request has succeeded and a new resource has been created as a result. | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/AdapterStatus' | |
| '400': | |
| description: The server could not understand the request due to invalid syntax. | |
| '404': | |
| description: The server cannot find the requested resource. | |
| '409': | |
| description: The request conflicts with the current state of the server. | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/AdapterStatusCreateRequest' | |
| security: | |
| - BearerAuth: [] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openapi/openapi.yaml` around lines 518 - 551, The new PUT operation
(operationId putNodePoolStatuses) is missing the OpenAPI security declaration;
add a security block referencing the existing BearerAuth scheme (e.g., security:
- BearerAuth: []) to the operation so the generated docs/clients show it
requires authentication and aligns with the middleware protecting the /clusters
routes.
| responses: | ||
| '201': | ||
| description: The request has succeeded and a new resource has been created as a result. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/AdapterStatus' | ||
| '400': | ||
| description: The server could not understand the request due to invalid syntax. | ||
| '404': | ||
| description: The server cannot find the requested resource. | ||
| '409': | ||
| description: The request conflicts with the current state of the server. | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/AdapterStatusCreateRequest' |
There was a problem hiding this comment.
Document the 204 No Content success case for these PUT upserts.
These endpoints already return 204 for valid no-op/discarded updates; test/integration/adapter_status_test.go, Lines 495-501 and 566-572, asserts that behavior. Leaving 204 out here makes a normal successful outcome undocumented in the generated client and API docs.
Also applies to: 637-656
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openapi/openapi.yaml` around lines 533 - 551, The OpenAPI spec omits the
documented 204 No Content success response for the PUT upsert endpoints; update
the responses block for the PUT endpoints that currently reference
AdapterStatus/AdapterStatusCreateRequest to include a '204' response entry
(description: successful no-op/discarded update, content: none) so the generated
client/docs reflect the actual behavior (e.g., where AdapterStatus and
AdapterStatusCreateRequest are used); apply the same change to the other
occurrence noted (around the second block at lines 637-656).
| put: | ||
| operationId: putNodePoolStatuses | ||
| summary: Adapter creates or updates resource nodepool status |
| put: | ||
| operationId: putClusterStatuses | ||
| summary: Adapter creates or updates resource cluster status | ||
| parameters: |
There was a problem hiding this comment.
The new PUT operations only have a summary and no description. Since PUT
will eventually replace POST, it should carry the same (or better) documentation.
| // Use nodePool.OwnerID as the cluster_id parameter | ||
| resp, err := client.PostNodePoolStatusesWithResponse( | ||
| resp, err := client.PutNodePoolStatusesWithResponse( |
There was a problem hiding this comment.
The function name still says TestNodePoolStatusPost but now tests PUT. Same for all the other Post test functions in this file. Consider renaming to Put (or Upsert) so test output matches what's actually being tested.
3 participants
Summary
1 of 2 changes that will properly register "PUT" routes for idempotent upserts of statuses coming from the adapter.
Does not remove current post route.
Changes in Step 1 of 2:
Test Plan
make test-allpassesmake lintpassesJira Issue
Summary by CodeRabbit
New Features
API Updates