Skip to content

HYPERFLEET-978 - chore: Update adapter configs, docs, and tests to use put #113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
May 14, 2026
Merged

HYPERFLEET-978 - chore: Update adapter configs, docs, and tests to use put #113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion configs/adapter-task-config-template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ post:
# Report cluster status to HyperFleet API (always executed)
- name: "reportClusterStatus"
api_call:
method: "POST"
method: "PUT"
# NOTE: API path includes /api/hyperfleet/ prefix and ends with /statuses
url: "/clusters/{{ .clusterId }}/statuses"
body: "{{ .clusterStatusPayload }}"
Expand Down
12 changes: 6 additions & 6 deletions docs/adapter-authoring-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ sequenceDiagram
Sentinel->>Adapter: CloudEvent {id, generation: N+1}
Adapter->>API: GET /clusters/{id}
Adapter->>Adapter: Create/update resources
Adapter->>API: POST status {observed_generation: N+1}
Adapter->>API: PUT status {observed_generation: N+1}
API->>API: All adapters at N+1 → Reconciled=True
Comment thread
coderabbitai[bot] marked this conversation as resolved.
```

Expand Down Expand Up @@ -378,7 +378,7 @@ To implement time-based stability checks, you need to know how long a cluster ha
| Field | Updates when | Use for |
|-------|-------------|---------|
| **`last_transition_time`** | Condition status **changes** (True→False or False→True) | **Stability windows** — "cluster has been Reconciled for N minutes" |
| **`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" |

**Critical:** For stability windows, always use `last_transition_time`. The `last_updated_time` field has special aggregation behavior that makes it unsuitable for measuring state duration.

Expand Down Expand Up @@ -896,7 +896,7 @@ sequenceDiagram
Workload->>K8s: Update status (conditions, phase)
Adapter->>K8s: Discover resource (read status back)
Note over Adapter: Evaluate CEL expressions against<br/>discovered resource status
Adapter->>API: POST /statuses {Applied, Available, Health}
Adapter->>API: PUT /statuses {Applied, Available, Health}
```

The adapter does **not** wait for the workload to complete. It reads whatever status is available at discovery time and reports it. If the object is still pending, the adapter reports `Available=False`. The Sentinel will trigger another reconciliation cycle later, and the adapter will read the updated status then.
Expand Down Expand Up @@ -1259,7 +1259,7 @@ Mock responses matched by HTTP method and URL regex. Supports sequential respons
},
{
"match": {
"method": "POST",
"method": "PUT",
"urlPattern": "/api/hyperfleet/v1/clusters/.*/statuses"
},
Comment thread
ma-hill marked this conversation as resolved.
"responses": [
Expand Down Expand Up @@ -1327,7 +1327,7 @@ Phase 3.5: Discovery Results ................. (available as resources.* in payl

Phase 4: Post Actions ..................... SUCCESS
[1/1] update-status EXECUTED
API Call: POST /api/hyperfleet/v1/clusters/abc123/statuses -> 200
API Call: PUT /api/hyperfleet/v1/clusters/abc123/statuses -> 200

Result: SUCCESS
```
Expand Down Expand Up @@ -1414,7 +1414,7 @@ Post-actions target the NodePool status endpoint instead of the cluster one:
post_actions:
- name: "reportNodepoolStatus"
api_call:
method: "POST"
method: "PUT"
url: "/api/hyperfleet/v1/clusters/{{ .clusterId }}/nodepools/{{ .nodepoolId }}/statuses"
body: "{{ .nodepoolStatusPayload }}"
```
Expand Down
2 changes: 1 addition & 1 deletion internal/configloader/validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func TestValidateActionBaseName(t *testing.T) {
PostActions: []PostAction{{
ActionBase: ActionBase{
Name: "update-status",
APICall: &APICall{Method: "POST", URL: "/status"},
APICall: &APICall{Method: "PUT", URL: "/status"},
},
}},
}
Expand Down
4 changes: 2 additions & 2 deletions internal/executor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ post:
post_actions:
- name: "reportStatus"
api_call:
method: "POST"
method: "PUT"
url: "{{ .apiBaseUrl }}/clusters/{{ .clusterId }}/statuses"
body: "{{ .statusPayload }}"
```
Expand Down Expand Up @@ -446,7 +446,7 @@ post:
post_actions:
- name: "reportStatus"
api_call:
method: "POST"
method: "PUT"
url: "{{ .apiBaseUrl }}/clusters/{{ .clusterId }}/statuses"
body: "{{ .statusPayload }}"
```
Expand Down
15 changes: 13 additions & 2 deletions internal/executor/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func ExecuteAPICall(
}
log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body))
resp, err = apiClient.Post(ctx, url, body, opts...)
// Log body on failure for debugging
// Log error message on failure for debugging purposes
if err != nil || (resp != nil && !resp.IsSuccess()) {
var logErr error
if err != nil {
Expand All @@ -158,7 +158,7 @@ func ExecuteAPICall(
logErr = fmt.Errorf("POST %s returned non-success status: %d", url, resp.StatusCode)
}
errCtx := logger.WithErrorField(ctx, logErr)
log.Error(errCtx, "Request failed")
log.Error(errCtx, "POST Request failed")
}
case http.MethodPut:
body := []byte(apiCall.Body)
Expand All @@ -170,6 +170,17 @@ func ExecuteAPICall(
}
log.Debugf(ctx, "API call payload: %s %s payload=%s", apiCall.Method, url, string(body))
resp, err = apiClient.Put(ctx, url, body, opts...)
// Log error message on failure for debugging purposes
if err != nil || (resp != nil && !resp.IsSuccess()) {
var logErr error
if err != nil {
logErr = err
} else {
logErr = fmt.Errorf("PUT %s returned non-success status: %d", url, resp.StatusCode)
}
errCtx := logger.WithErrorField(ctx, logErr)
log.Error(errCtx, "PUT Request failed")
}
case http.MethodPatch:
body := []byte(apiCall.Body)
if apiCall.Body != "" {
Expand Down
26 changes: 13 additions & 13 deletions test/integration/executor/executor_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func createTestConfig(apiBaseURL string) *configloader.Config {
ActionBase: configloader.ActionBase{
Name: "reportClusterStatus",
APICall: &configloader.APICall{
Method: "POST",
Method: "PUT",
URL: "{{ .hyperfleetApiBaseUrl }}/api/{{ .hyperfleetApiVersion }}/clusters/{{ .clusterID }}/statuses",
Comment on lines +145 to 146
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add integration build tags to this integration test file.

This integration test should be explicitly gated to avoid running in default unit-test paths.

Suggested patch 
+//go:build integration
+// +build integration
+
 package executorintegrationtest

As per coding guidelines, **/*_test.go: Integration tests tagged with //go:build integration.

🤖 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 145 -
146, Add the Go integration build tag to the top of executor_integration_test.go
so it doesn't run as a normal unit test: insert a build constraint lines
(//go:build integration and the legacy // +build integration) immediately before
the package declaration in executor_integration_test.go, followed by a blank
line, ensuring the package line and imports remain unchanged; this explicitly
gates the tests (e.g., the file containing Method: "PUT" URL: "{{
.hyperfleetApiBaseUrl }}/api/{{ .hyperfleetApiVersion }}/clusters/{{ .clusterID
}}/statuses") to the integration build.

Copy link
Copy Markdown
Contributor

@tirthct tirthct May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a mandatory tag? A lot of the test files under https://github.com/openshift-hyperfleet/hyperfleet-adapter/tree/main/test/integration are missing the tag.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== Build tags in test/integration ==="
grep -rn "//go:build" test/integration/ || echo "NONE FOUND"

echo ""
echo "=== Package declarations in test/integration ==="
grep -rn "^package " test/integration/ | head -30

echo ""
echo "=== Makefile targets related to test/integration ==="
grep -n -A5 "integration" Makefile | head -60

echo ""
echo "=== List all test files under test/integration ==="
fd "_test.go" test/integration/

Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 3351

🏁 Script executed:

#!/bin/bash
echo "=== run-integration-tests.sh ==="
cat scripts/run-integration-tests.sh

echo ""
echo "=== make test target ==="
grep -n -A10 "^test:" Makefile | head -30

Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 2945

@tirthct — you're right to push back. Looking at the actual setup:

  1. make test (unit tests) uses grep -v /test/ to exclude the entire /test/ directory, so integration files are already isolated regardless of build tags.
  2. make test-integration explicitly targets ./test/integration/... with -tags=integration, so both the path filter and the tag are passed together.

This means the //go:build integration tag is not strictly mandatory in your current project. It's only truly necessary if someone runs go test ./... directly — outside the Makefile — without path filtering. The Makefile already provides the isolation.

The inconsistency in the repo confirms this is not an enforced convention: only config-loader/config_criteria_integration_test.go and k8sclient/client_integration_test.go have the tag, while the executor, maestroclient, and loader template tests don't.

I'll retract this as a blocking concern given the current project conventions. If the team wants to standardize on the build tag for defense-in-depth (e.g., enabling go test ./... to work safely without Makefile wrappers), that would be a separate cleanup tracked as a follow-up issue rather than a change in this PR. Would you like me to open one?

📝 Learnings were identified and not saved (knowledge base disabled). Enable

Body: "{{ .clusterStatusPayload }}",
Timeout: "5s",
Expand Down Expand Up @@ -1142,21 +1142,21 @@ func TestExecutor_PostActionAPIFailure(t *testing.T) {
// Verify the phase is post_actions
assert.Equal(t, executor.PhasePostActions, result.CurrentPhase, "Expected failure in post_actions phase")

// Verify precondition API was called, but status POST failed
// Verify precondition API was called, but status PUT failed
requests := mockAPI.GetRequests()
assert.GreaterOrEqual(t, len(requests), 2, "Expected at least 2 API calls (GET cluster + POST status)")
assert.GreaterOrEqual(t, len(requests), 2, "Expected at least 2 API calls (GET cluster + PUT status)")

// Find the status POST request
var statusPostFound bool
var statusPutFound bool
for _, req := range requests {
if req.Method == http.MethodPost && strings.Contains(req.Path, "/statuses") {
statusPostFound = true
t.Logf("Status POST was attempted: %s %s", req.Method, req.Path)
if req.Method == http.MethodPut && strings.Contains(req.Path, "/statuses") {
statusPutFound = true
t.Logf("Status PUT was attempted: %s %s", req.Method, req.Path)
}
}
assert.True(t, statusPostFound, "Expected status POST to be attempted")
assert.True(t, statusPutFound, "Expected status PUT to be attempted")

// No status should be successfully stored since POST failed
// No status should be successfully stored since PUT failed
statusResponses := mockAPI.GetStatusResponses()
assert.Empty(t, statusResponses, "Expected no successful status responses due to API failure")

Expand Down Expand Up @@ -1254,7 +1254,7 @@ func TestExecutor_ExecutionError_CELAccess(t *testing.T) {
ActionBase: configloader.ActionBase{
Name: "reportError",
APICall: &configloader.APICall{
Method: "POST",
Method: "PUT",
URL: "{{ .hyperfleetApiBaseUrl }}/api/{{ .hyperfleetApiVersion }}/clusters/{{ .clusterID }}/error-report",
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Body: "{{ .errorReportPayload }}",
Timeout: "5s",
Expand Down Expand Up @@ -1299,7 +1299,7 @@ func TestExecutor_ExecutionError_CELAccess(t *testing.T) {
requests := mockAPI.GetRequests()
var errorReportRequest *testutil.MockRequest
for i := range requests {
if requests[i].Method == http.MethodPost && strings.Contains(requests[i].Path, "/error-report") {
if requests[i].Method == http.MethodPut && strings.Contains(requests[i].Path, "/error-report") {
errorReportRequest = &requests[i]
break
}
Expand Down Expand Up @@ -1391,7 +1391,7 @@ func TestExecutor_PayloadBuildFailure(t *testing.T) {
ActionBase: configloader.ActionBase{
Name: "shouldNotExecute",
APICall: &configloader.APICall{
Method: "POST",
Method: "PUT",
URL: "{{ .hyperfleetApiBaseUrl }}/api/{{ .hyperfleetApiVersion }}/clusters/{{ .clusterID }}/statuses",
Body: "{{ .badPayload }}",
Timeout: "5s",
Expand Down Expand Up @@ -1457,7 +1457,7 @@ func TestExecutor_PayloadBuildFailure(t *testing.T) {
// Verify NO API call was made to the post action endpoint (blocked)
requests := mockAPI.GetRequests()
for _, req := range requests {
if req.Method == http.MethodPost && strings.Contains(req.Path, "/statuses") {
if req.Method == http.MethodPut && strings.Contains(req.Path, "/statuses") {
t.Errorf("Post action API call should NOT have been made (blocked by payload build failure)")
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}
Expand Down
4 changes: 2 additions & 2 deletions test/integration/executor/executor_k8s_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func newK8sTestAPIServer(t *testing.T) *k8sTestAPIServer {

switch {
case strings.Contains(r.URL.Path, "/clusters/") && strings.HasSuffix(r.URL.Path, "/statuses"):
if r.Method == http.MethodPost {
if r.Method == http.MethodPut {
var statusBody map[string]interface{}
Comment thread
ma-hill marked this conversation as resolved.
if err := json.Unmarshal([]byte(bodyStr), &statusBody); err == nil {
mock.statusResponses = append(mock.statusResponses, statusBody)
Expand Down Expand Up @@ -295,7 +295,7 @@ func createK8sTestConfig(testNamespace string) *configloader.Config {
ActionBase: configloader.ActionBase{
Name: "reportClusterStatus",
APICall: &configloader.APICall{
Method: "POST",
Method: "PUT",
URL: "{{ .hyperfleetApiBaseUrl }}/api/{{ .hyperfleetApiVersion }}/clusters/" +
Comment on lines +298 to 299
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 14, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add integration build tags to this integration test file.

This test sits in the integration suite and should be gated with build tags so default unit-test runs don’t include it.

Suggested patch 
+//go:build integration
+// +build integration
+
 package executorintegrationtest

As per coding guidelines, **/*_test.go: Integration tests tagged with //go:build integration.

📝 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.

Suggested change
Method: "PUT",
URL: "{{ .hyperfleetApiBaseUrl }}/api/{{ .hyperfleetApiVersion }}/clusters/" +
//go:build integration
// +build integration
package executorintegrationtest
🤖 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 298
- 299, This integration test file is missing the integration build tag so it
runs during normal `go test`; add the standard build constraint at the top of
the file by inserting both the new Go build line and the legacy tag before the
package declaration: "//go:build integration" followed by "// +build
integration", ensuring they are placed at the very top of
test/integration/executor/executor_k8s_integration_test.go so the tests are only
included when built with the "integration" tag.

Copy link
Copy Markdown
Contributor

@tirthct tirthct May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a mandatory tag? A lot of the test files under https://github.com/openshift-hyperfleet/hyperfleet-adapter/tree/main/test/integration are missing the tag.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find all test files under test/integration and check for build tags
echo "=== Files WITH integration build tag ==="
grep -rl "//go:build integration" test/integration/ 2>/dev/null || echo "none"

echo ""
echo "=== Files WITHOUT integration build tag ==="
fd "_test.go" test/integration/ --exec grep -rL "//go:build integration" {} \; 2>/dev/null || echo "none"

echo ""
echo "=== All test files under test/integration/ ==="
fd "_test.go" test/integration/

Repository: openshift-hyperfleet/hyperfleet-adapter
Length of output: 1677

@tirthct, you're right to push back. The results confirm the tag is almost entirely absent across the integration suite — 13 out of 14 test files are missing it, including all of executor/, maestroclient/, and k8sclient/. Only config-loader/config_criteria_integration_test.go has it.

This is a pre-existing gap in the codebase, not something introduced by this PR. Flagging it here is unfair to this change. I'll withdraw this comment as a blocker.

That said, the coding guideline exists for a good reason — without the build tag, these tests run during go test ./... and can fail in environments without K8s/Maestro dependencies available. Would you like me to open a follow-up issue to add //go:build integration consistently across all files under test/integration/?

📝 Learnings were identified and not saved (knowledge base disabled). Enable

"{{ .clusterID }}/statuses",
Body: "{{ .clusterStatusPayload }}",
Expand Down