HYPERFLEET-978 - feat: Update adapter configs, docs, and tests to use put endpoint for status updates#113
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 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes systematically update the HTTP method used for status reporting from POST to PUT across the codebase. This includes modifying the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@ma-hill: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/integration/executor/executor_k8s_integration_test.go (1)
86-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign the mock expectation with the configured post-action method.
At Line 86 the mock only accepts
PUT, butcreateK8sTestConfigstill sendsPOSTfor/statuses(Line 298). That mismatch makes status calls fall through to 404 and breaks assertions that expect a captured status payload.Suggested fix
- Method: "POST", + Method: "PUT",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/executor/executor_k8s_integration_test.go` around lines 86 - 94, The test HTTP mock only handles http.MethodPut for status updates but createK8sTestConfig sends POST to "/statuses", so update the mock handler to accept the configured method (either change the condition to r.Method == http.MethodPost or allow both POST and PUT) and still unmarshal the body into statusBody and append to mock.statusResponses; ensure the handler returns the same 200+{"status":"accepted"} response so assertions that inspect mock.statusResponses (from the test helper that calls createK8sTestConfig) capture the payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adapter-authoring-guide.md`:
- Around line 1262-1264: Several examples still document status-reporting as
POST while the canonical example uses "method": "PUT" for the status endpoint
(urlPattern: "/api/hyperfleet/v1/clusters/.*/statuses"); update every example
and sample that posts cluster statuses to use "method": "PUT" (and adjust any
surrounding narrative/sample payloads to match the PUT flow) so all references
to status-reporting consistently target the same HTTP verb and URL pattern, then
validate the changed examples against the HyperFleet architecture standards to
ensure conformity.
In `@test/integration/executor/executor_integration_test.go`:
- Around line 1302-1304: The test currently hardcodes http.MethodPut when
locating the /error-report request, but the test configuration defines that
endpoint as POST; update the condition that sets errorReportRequest to use the
configured method instead of a hardcoded PUT — e.g., replace the http.MethodPut
check with the test's expected method variable (such as errorReportMethod or
cfg.ErrorReportMethod) so the lookup uses requests[i].Method ==
expectedErrorReportMethod && strings.Contains(requests[i].Path, "/error-report")
and then assign errorReportRequest = &requests[i].
---
Outside diff comments:
In `@test/integration/executor/executor_k8s_integration_test.go`:
- Around line 86-94: The test HTTP mock only handles http.MethodPut for status
updates but createK8sTestConfig sends POST to "/statuses", so update the mock
handler to accept the configured method (either change the condition to r.Method
== http.MethodPost or allow both POST and PUT) and still unmarshal the body into
statusBody and append to mock.statusResponses; ensure the handler returns the
same 200+{"status":"accepted"} response so assertions that inspect
mock.statusResponses (from the test helper that calls createK8sTestConfig)
capture the payload.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: dbaef7cc-0e25-4dfc-bf42-dd60f23e98e3
📒 Files selected for processing (7)
configs/adapter-task-config-template.yamldocs/adapter-authoring-guide.mdinternal/configloader/validator_test.gointernal/executor/README.mdinternal/executor/utils.gotest/integration/executor/executor_integration_test.gotest/integration/executor/executor_k8s_integration_test.go
| "method": "PUT", | ||
| "urlPattern": "/api/hyperfleet/v1/clusters/.*/statuses" | ||
| }, |
There was a problem hiding this comment.
Complete the method migration in remaining examples within this guide.
These sections now document PUT, but the same file still has status-reporting references to POST (e.g., Line 1102 and Line 381). That leaves conflicting guidance for adapter authors.
Suggested fix
- method: "POST"
+ method: "PUT"-| **`last_updated_time`** | Adapter **reports status** (every POST, even if unchanged) | **Liveness checks** — "adapter reported recently" |
+| **`last_updated_time`** | Adapter **reports status** (every PUT, even if unchanged) | **Liveness checks** — "adapter reported recently" |As per coding guidelines, Validate changes against HyperFleet architecture standards from the linked architecture repository.
Also applies to: 1330-1331, 1417-1419
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adapter-authoring-guide.md` around lines 1262 - 1264, Several examples
still document status-reporting as POST while the canonical example uses
"method": "PUT" for the status endpoint (urlPattern:
"/api/hyperfleet/v1/clusters/.*/statuses"); update every example and sample that
posts cluster statuses to use "method": "PUT" (and adjust any surrounding
narrative/sample payloads to match the PUT flow) so all references to
status-reporting consistently target the same HTTP verb and URL pattern, then
validate the changed examples against the HyperFleet architecture standards to
ensure conformity.
| if requests[i].Method == http.MethodPut && strings.Contains(requests[i].Path, "/error-report") { | ||
| errorReportRequest = &requests[i] | ||
| break |
There was a problem hiding this comment.
Use the configured method in the /error-report request assertion.
Line 1302 now looks for PUT, but this test config still defines /error-report as POST (Line 1257). The assertion can fail even when the call is correctly made.
Suggested fix
- if requests[i].Method == http.MethodPut && strings.Contains(requests[i].Path, "/error-report") {
+ if requests[i].Method == http.MethodPost && strings.Contains(requests[i].Path, "/error-report") {📝 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.
| if requests[i].Method == http.MethodPut && strings.Contains(requests[i].Path, "/error-report") { | |
| errorReportRequest = &requests[i] | |
| break | |
| if requests[i].Method == http.MethodPost && strings.Contains(requests[i].Path, "/error-report") { | |
| errorReportRequest = &requests[i] | |
| break |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/integration/executor/executor_integration_test.go` around lines 1302 -
1304, The test currently hardcodes http.MethodPut when locating the
/error-report request, but the test configuration defines that endpoint as POST;
update the condition that sets errorReportRequest to use the configured method
instead of a hardcoded PUT — e.g., replace the http.MethodPut check with the
test's expected method variable (such as errorReportMethod or
cfg.ErrorReportMethod) so the lookup uses requests[i].Method ==
expectedErrorReportMethod && strings.Contains(requests[i].Path, "/error-report")
and then assign errorReportRequest = &requests[i].
1 participant
Summary
This change updates adapter documentation, configs, and testing to use the
which handles the idempotent upserts.
Changes made:
Related changes from these two PRs:
Test Plan
Jira Issue