HYPERFLEET-860 - feat: add delete and update lifecycle tests for resources#85
HYPERFLEET-860 - feat: add delete and update lifecycle tests for resources#85kuudori wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Conversation
…ters and nodepools Implement Tier0 E2E tests covering soft-delete, hard-delete, cascade deletion, 409-Conflict on PATCH after soft-delete, and PATCH-triggered reconciliation workflows. - Add cluster delete, update, nodepool delete, update test suites - Add client methods: DeleteCluster, DeleteNodePool, PatchClusterRaw, PatchNodePoolRaw - Add cluster-patch.json and nodepool-patch.json test payloads - Add WaitFor* helper functions for async condition polling - Update test-design docs to align with hard-delete terminology
…custom matchers Migrate all E2E tests from WaitFor* helper functions to the pollers+matchers pattern, keeping Eventually visible at call sites. - Add pollers.go with PollCluster, PollNodePool, PollClusterHTTPStatus, etc. - Add matchers.go with HaveResourceCondition, HaveAllAdaptersWithCondition, HaveAllAdaptersAtGeneration - Delete wait.go containing WaitFor* wrapper functions - Update all 8 test files to use Eventually(poller).Should(matcher) - Add hasReconciled guard in nodepool/update.go before observedGeneration loop - Enhance delete test AfterEach with API delete before K8s cleanup - Update CLAUDE.md and docs/development.md with pollers+matchers convention
|
[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 |
WalkthroughThe pull request introduces a significant refactoring of test-writing patterns and expands client/test infrastructure. It replaces blocking Sequence Diagram(s)sequenceDiagram
participant Test
participant Client as HTTP Client
participant API as Hyperfleet API
participant Resources as Cluster Resource
Test->>Client: CreateCluster (fixture)
Client->>API: POST /clusters
API->>Resources: Provision cluster
API-->>Client: 202 Created
Client-->>Test: *Cluster
loop Eventually (poll until reconciled)
Test->>Client: GetCluster(id)
Client->>API: GET /clusters/{id}
API-->>Client: *Cluster
Client-->>Test: *Cluster with Reconciled=True
end
Test->>Client: PatchCluster(id, patch)
Client->>API: PATCH /clusters/{id}
API->>Resources: Apply patch
API-->>Client: 200 OK, generation incremented
Client-->>Test: *Cluster (gen N+1)
Test->>Test: Verify generation incremented by 1
loop Eventually (poll adapter statuses)
Test->>Client: GetClusterAdapterStatuses(id)
Client->>API: GET /clusters/{id}/adapters
API-->>Client: AdapterStatusList
Client-->>Test: All adapters at generation N+1
end
loop Eventually (poll cluster reconciliation)
Test->>Client: GetCluster(id)
Client->>API: GET /clusters/{id}
API-->>Client: *Cluster
Client-->>Test: *Cluster with Reconciled=True at gen N+1
end
Test->>Test: Assert Reconciled condition observedGen matches
sequenceDiagram
participant Test
participant Client as HTTP Client
participant API as Hyperfleet API
participant Resources as NodePool Resource
Test->>Client: DeleteNodePool(clusterId, npId)
Client->>API: DELETE /clusters/{cid}/nodepools/{npid}
API->>Resources: Soft-delete (set DeletedTime)
API-->>Client: 202 Accepted
Client-->>Test: *NodePool (DeletedTime set)
Test->>Test: Verify DeletedTime non-nil, Generation incremented
loop Eventually (poll adapter status)
Test->>Client: GetNodePoolAdapterStatuses(cid, npid)
Client->>API: GET /clusters/{cid}/nodepools/{npid}/adapters
API-->>Client: AdapterStatusList
Client-->>Test: All adapters with Finalized=True
end
Test->>Test: Verify each adapter: Applied=False, Available=False, Health=True
loop Eventually (poll HTTP status until 404)
Test->>Client: PollNodePoolHTTPStatus(cid, npid)
Client->>API: GET /clusters/{cid}/nodepools/{npid}
alt Resource exists
API-->>Client: 200 OK
Client-->>Test: 200
else Resource deleted
API-->>Client: 404 Not Found
Client-->>Test: 404 (hard-delete confirmed)
end
end
Test->>Client: GetCluster (verify parent unaffected)
Client->>API: GET /clusters/{cid}
API-->>Client: *Cluster
Client-->>Test: Cluster generation unchanged, Reconciled=True
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/nodepool/creation.go (1)
247-256:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let the pre-cleanup readiness wait block teardown.
If the cluster never becomes Ready, this
Eventually(...).Should(...)fails beforeCleanupTestClusterruns, which can leave the test cluster/nodepool behind and pollute later runs. Make the readiness check best-effort, or ensure cleanup is guaranteed through a separate always-run path. As per coding guidelines, always implement resource cleanup in AfterEach blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nodepool/creation.go` around lines 247 - 256, The readiness wait must not block teardown: change the hard assertion using Eventually(h.PollCluster(ctx, clusterID), ...).Should(...) into a best-effort check that logs failures but does not fail the test, and ensure CleanupTestCluster(clusterID) always runs (move/duplicate cleanup into an AfterEach or a deferred/always-run path). Concretely, replace the strict Eventually(...).Should(helper.HaveResourceCondition(...)) with a non-fatal check that captures errors/results and logs them (or uses Ginkgo’s By + framework log) then proceed, and guarantee invocation of h.CleanupTestCluster(ctx, clusterID) from an AfterEach or a finally-style block so cleanup runs even when the readiness check times out; reference PollCluster and CleanupTestCluster to locate the changes.
🧹 Nitpick comments (7)
e2e/nodepool/update.go (2)
15-16: ⚡ Quick winAlign suite title with the required naming format.
Use
[Suite: nodepool] NodePool Update Lifecycle(without the extra[update]token) to match the mandated suite-title convention.As per coding guidelines
e2e/**/*.go: Test name must be formatted as[Suite: component] Description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nodepool/update.go` around lines 15 - 16, Update the ginkgo.Describe suite title string used in the test declaration so it matches the mandated format: replace the current title passed to ginkgo.Describe (the string starting with "[Suite: nodepool][update] NodePool Update Lifecycle") with "[Suite: nodepool] NodePool Update Lifecycle"; locate the ginkgo.Describe call in this file (the var _ = ginkgo.Describe(...) declaration) and adjust only the title string to remove the extra "[update]" token.
69-70: ⚡ Quick winUse
helper.HaveResourceConditiondirectly for final condition assertions.These checks currently use
HasResourceConditionbooleans; switch to matcher-based assertions for consistency and clearer failures.Proposed change
- hasReconciled := h.HasResourceCondition(finalNP.Status.Conditions, client.ConditionTypeReconciled, openapi.ResourceConditionStatusTrue) - Expect(hasReconciled).To(BeTrue(), "nodepool should have Reconciled=True") + Expect(finalNP).To(helper.HaveResourceCondition(client.ConditionTypeReconciled, openapi.ResourceConditionStatusTrue), + "nodepool should have Reconciled=True") - hasParentReconciled := h.HasResourceCondition(parentCluster.Status.Conditions, client.ConditionTypeReconciled, openapi.ResourceConditionStatusTrue) - Expect(hasParentReconciled).To(BeTrue(), "parent cluster should remain Reconciled=True") + Expect(parentCluster).To(helper.HaveResourceCondition(client.ConditionTypeReconciled, openapi.ResourceConditionStatusTrue), + "parent cluster should remain Reconciled=True")As per coding guidelines
e2e/**/*.go: Usehelper.HaveResourceCondition()custom matcher to verify resource conditions instead of inline assertions.Also applies to: 83-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nodepool/update.go` around lines 69 - 70, Replace the boolean-style checks that call h.HasResourceCondition with the matcher-based assertion helper.HaveResourceCondition to get clearer failures: instead of computing hasReconciled := h.HasResourceCondition(finalNP.Status.Conditions, client.ConditionTypeReconciled, openapi.ResourceConditionStatusTrue) and Expect(hasReconciled).To(BeTrue(), ...), call Expect(finalNP.Status.Conditions).To(helper.HaveResourceCondition(client.ConditionTypeReconciled, openapi.ResourceConditionStatusTrue)) (and do the same replacement for the similar check around lines 83-84) so the assertions use the custom matcher directly against finalNP.Status.Conditions.e2e/cluster/delete.go (2)
16-17: ⚡ Quick winUpdate suite titles to the required format.
Both suite names should follow
[Suite: cluster] Descriptionwithout the extra[delete]bracket token.As per coding guidelines
e2e/**/*.go: Test name must be formatted as[Suite: component] Description.Also applies to: 111-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cluster/delete.go` around lines 16 - 17, The test suite titles include an extra bracketed token; update the ginkgo.Describe calls (e.g., the top-level ginkgo.Describe string currently "[Suite: cluster][delete] Cluster Deletion Lifecycle") to follow the required format "[Suite: cluster] Cluster Deletion Lifecycle" (remove the extra "[delete]" token) and apply the same change to the other ginkgo.Describe invocation referenced (lines 111-112) so all suite names use the pattern "[Suite: component] Description".
49-59: ⚡ Quick winPrefer adapter custom matchers over manual adapter-condition loops.
Use
HaveAllAdaptersWithConditionassertions here for the post-finalization condition checks to keep adapter verification consistent and centralized.As per coding guidelines
e2e/**/*.go: Usehelper.HaveAllAdaptersWithCondition()andhelper.HaveAllAdaptersAtGeneration()matchers for adapter status verification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cluster/delete.go` around lines 49 - 59, Replace the manual loop over statuses.Items with the centralized matcher-based checks: after calling h.Client.GetClusterStatuses(ctx, clusterID) use Expect(statuses).To(helper.HaveAllAdaptersWithCondition(client.ConditionTypeApplied, openapi.AdapterConditionStatusFalse)), Expect(statuses).To(helper.HaveAllAdaptersWithCondition(client.ConditionTypeAvailable, openapi.AdapterConditionStatusFalse)), and Expect(statuses).To(helper.HaveAllAdaptersWithCondition(client.ConditionTypeHealth, openapi.AdapterConditionStatusTrue)); also add an Expect(statuses).To(helper.HaveAllAdaptersAtGeneration(...)) if generation verification is required. This removes the explicit for-loop and uses helper.HaveAllAdaptersWithCondition and helper.HaveAllAdaptersAtGeneration matchers for centralized adapter status checks.e2e/nodepool/delete.go (3)
16-17: ⚡ Quick winNormalize suite title to the required format.
Rename to
[Suite: nodepool] NodePool Deletion Lifecycleto match the enforced suite naming pattern.As per coding guidelines
e2e/**/*.go: Test name must be formatted as[Suite: component] Description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nodepool/delete.go` around lines 16 - 17, Update the ginkgo.Describe suite title string used in the var _ = ginkgo.Describe(...) declaration: change the title from "[Suite: nodepool][delete] NodePool Deletion Lifecycle" to the normalized format "[Suite: nodepool] NodePool Deletion Lifecycle" so it matches the enforced `[Suite: component] Description` pattern; locate the ginkgo.Describe call in this file and replace the combined tag string accordingly.
60-70: ⚡ Quick winUse adapter custom matchers instead of manual condition loops.
This section should rely on
HaveAllAdaptersWithConditionforApplied=False,Available=False, andHealth=Truechecks.As per coding guidelines
e2e/**/*.go: Usehelper.HaveAllAdaptersWithCondition()andhelper.HaveAllAdaptersAtGeneration()matchers for adapter status verification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nodepool/delete.go` around lines 60 - 70, Replace the manual loop that iterates over statuses.Items and calls h.HasAdapterCondition with the helper matchers: after calling h.Client.GetNodePoolStatuses(ctx, clusterID, nodepoolID) assert that statuses.Items satisfy helper.HaveAllAdaptersWithCondition for Applied=False, Available=False and Health=True respectively (three Expect calls), and optionally use helper.HaveAllAdaptersAtGeneration if generation checks are needed; remove the for loop and any direct uses of HasAdapterCondition so the test uses helper.HaveAllAdaptersWithCondition() (and HaveAllAdaptersAtGeneration()) for adapter status verification.
82-84: ⚡ Quick winSwitch parent-cluster condition check to
HaveResourceCondition.Using the matcher here keeps condition verification consistent with the rest of the suite pattern.
As per coding guidelines
e2e/**/*.go: Usehelper.HaveResourceCondition()custom matcher to verify resource conditions instead of inline assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/nodepool/delete.go` around lines 82 - 84, Replace the inline boolean check using h.HasResourceCondition and Expect(hasReconciled).To(BeTrue()) with the suite's custom matcher helper.HaveResourceCondition: call Expect(parentCluster).To(helper.HaveResourceCondition(client.ConditionTypeReconciled, openapi.ResourceConditionStatusTrue)) (removing the temporary hasReconciled variable and the manual BeTrue assertion) so condition verification matches the rest of the e2e suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/cluster/delete.go`:
- Around line 157-167: The two Eventually checks that call h.Client.GetNodePool
for nodepoolID1 and nodepoolID2 are race-prone because GetNodePool may return a
404 if the child nodepool hard-deletes quickly; update each Eventually lambda
(the calls invoking h.Client.GetNodePool and asserting on npX.DeletedTime) to
treat either a successful response with np.DeletedTime != nil OR an HTTP 404
(not-found) error as a success: call GetNodePool, if err is a not-found/404
treat the check as passed, otherwise assert no error and that np.DeletedTime is
not nil.
In `@e2e/nodepool/concurrent_creation.go`:
- Around line 177-183: The readiness wait
(Eventually(h.PollCluster(...)).Should(HaveResourceCondition(...))) must not
block cleanup; change the test so the readiness check is non-fatal and cleanup
always runs: either move the cleanup call into an AfterEach/teardown hook that
always invokes h.CleanupTestCluster(ctx, clusterID), or make the
Immediately-executed readiness probe only log the result (capture the Eventually
result from h.PollCluster(ctx, clusterID) and if it times out, call
processLogger or GinkgoWriter to report the failure but do not call
Should/Fail), then always call h.CleanupTestCluster(ctx, clusterID) afterwards;
reference: PollCluster, HaveResourceCondition, Eventually, and
CleanupTestCluster to locate the code to change.
In `@pkg/client/client.go`:
- Around line 54-60: When checking resp.StatusCode != expectedStatus in the
client code, don't ignore the error from io.ReadAll(resp.Body); capture the
(body, err) result and if err != nil include a clear read-failure message in the
returned HTTPError.Body (e.g., "failed to read response body: <err>") or combine
any partial body with the read error so callers get diagnostic info; update the
return that constructs the HTTPError (referencing HTTPError, resp.Body,
io.ReadAll, expectedStatus and action) to surface the read error instead of
silencing it.
---
Outside diff comments:
In `@e2e/nodepool/creation.go`:
- Around line 247-256: The readiness wait must not block teardown: change the
hard assertion using Eventually(h.PollCluster(ctx, clusterID), ...).Should(...)
into a best-effort check that logs failures but does not fail the test, and
ensure CleanupTestCluster(clusterID) always runs (move/duplicate cleanup into an
AfterEach or a deferred/always-run path). Concretely, replace the strict
Eventually(...).Should(helper.HaveResourceCondition(...)) with a non-fatal check
that captures errors/results and logs them (or uses Ginkgo’s By + framework log)
then proceed, and guarantee invocation of h.CleanupTestCluster(ctx, clusterID)
from an AfterEach or a finally-style block so cleanup runs even when the
readiness check times out; reference PollCluster and CleanupTestCluster to
locate the changes.
---
Nitpick comments:
In `@e2e/cluster/delete.go`:
- Around line 16-17: The test suite titles include an extra bracketed token;
update the ginkgo.Describe calls (e.g., the top-level ginkgo.Describe string
currently "[Suite: cluster][delete] Cluster Deletion Lifecycle") to follow the
required format "[Suite: cluster] Cluster Deletion Lifecycle" (remove the extra
"[delete]" token) and apply the same change to the other ginkgo.Describe
invocation referenced (lines 111-112) so all suite names use the pattern
"[Suite: component] Description".
- Around line 49-59: Replace the manual loop over statuses.Items with the
centralized matcher-based checks: after calling h.Client.GetClusterStatuses(ctx,
clusterID) use
Expect(statuses).To(helper.HaveAllAdaptersWithCondition(client.ConditionTypeApplied,
openapi.AdapterConditionStatusFalse)),
Expect(statuses).To(helper.HaveAllAdaptersWithCondition(client.ConditionTypeAvailable,
openapi.AdapterConditionStatusFalse)), and
Expect(statuses).To(helper.HaveAllAdaptersWithCondition(client.ConditionTypeHealth,
openapi.AdapterConditionStatusTrue)); also add an
Expect(statuses).To(helper.HaveAllAdaptersAtGeneration(...)) if generation
verification is required. This removes the explicit for-loop and uses
helper.HaveAllAdaptersWithCondition and helper.HaveAllAdaptersAtGeneration
matchers for centralized adapter status checks.
In `@e2e/nodepool/delete.go`:
- Around line 16-17: Update the ginkgo.Describe suite title string used in the
var _ = ginkgo.Describe(...) declaration: change the title from "[Suite:
nodepool][delete] NodePool Deletion Lifecycle" to the normalized format "[Suite:
nodepool] NodePool Deletion Lifecycle" so it matches the enforced `[Suite:
component] Description` pattern; locate the ginkgo.Describe call in this file
and replace the combined tag string accordingly.
- Around line 60-70: Replace the manual loop that iterates over statuses.Items
and calls h.HasAdapterCondition with the helper matchers: after calling
h.Client.GetNodePoolStatuses(ctx, clusterID, nodepoolID) assert that
statuses.Items satisfy helper.HaveAllAdaptersWithCondition for Applied=False,
Available=False and Health=True respectively (three Expect calls), and
optionally use helper.HaveAllAdaptersAtGeneration if generation checks are
needed; remove the for loop and any direct uses of HasAdapterCondition so the
test uses helper.HaveAllAdaptersWithCondition() (and
HaveAllAdaptersAtGeneration()) for adapter status verification.
- Around line 82-84: Replace the inline boolean check using
h.HasResourceCondition and Expect(hasReconciled).To(BeTrue()) with the suite's
custom matcher helper.HaveResourceCondition: call
Expect(parentCluster).To(helper.HaveResourceCondition(client.ConditionTypeReconciled,
openapi.ResourceConditionStatusTrue)) (removing the temporary hasReconciled
variable and the manual BeTrue assertion) so condition verification matches the
rest of the e2e suite.
In `@e2e/nodepool/update.go`:
- Around line 15-16: Update the ginkgo.Describe suite title string used in the
test declaration so it matches the mandated format: replace the current title
passed to ginkgo.Describe (the string starting with "[Suite: nodepool][update]
NodePool Update Lifecycle") with "[Suite: nodepool] NodePool Update Lifecycle";
locate the ginkgo.Describe call in this file (the var _ = ginkgo.Describe(...)
declaration) and adjust only the title string to remove the extra "[update]"
token.
- Around line 69-70: Replace the boolean-style checks that call
h.HasResourceCondition with the matcher-based assertion
helper.HaveResourceCondition to get clearer failures: instead of computing
hasReconciled := h.HasResourceCondition(finalNP.Status.Conditions,
client.ConditionTypeReconciled, openapi.ResourceConditionStatusTrue) and
Expect(hasReconciled).To(BeTrue(), ...), call
Expect(finalNP.Status.Conditions).To(helper.HaveResourceCondition(client.ConditionTypeReconciled,
openapi.ResourceConditionStatusTrue)) (and do the same replacement for the
similar check around lines 83-84) so the assertions use the custom matcher
directly against finalNP.Status.Conditions.
🪄 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: e06f554e-da24-414d-9a2f-e86b1294dd37
📒 Files selected for processing (24)
CLAUDE.mddocs/development.mde2e/cluster/concurrent_creation.goe2e/cluster/creation.goe2e/cluster/delete.goe2e/cluster/update.goe2e/nodepool/concurrent_creation.goe2e/nodepool/creation.goe2e/nodepool/delete.goe2e/nodepool/update.gopkg/client/client.gopkg/client/cluster.gopkg/client/constants.gopkg/client/nodepool.gopkg/helper/helper.gopkg/helper/matchers.gopkg/helper/pollers.gopkg/helper/wait.gotest-design/testcases/delete-cluster.mdtest-design/testcases/delete-nodepool.mdtest-design/testcases/update-cluster.mdtest-design/testcases/update-nodepool.mdtestdata/payloads/clusters/cluster-patch.jsontestdata/payloads/nodepools/nodepool-patch.json
💤 Files with no reviewable changes (1)
- pkg/helper/wait.go
| Eventually(func(g Gomega) { | ||
| np1, err := h.Client.GetNodePool(ctx, clusterID, nodepoolID1) | ||
| g.Expect(err).NotTo(HaveOccurred(), "first nodepool should still be accessible") | ||
| g.Expect(np1.DeletedTime).NotTo(BeNil(), "first nodepool should have deleted_time set via cascade") | ||
| }, h.Cfg.Timeouts.Adapter.Processing, h.Cfg.Polling.Interval).Should(Succeed()) | ||
|
|
||
| Eventually(func(g Gomega) { | ||
| np2, err := h.Client.GetNodePool(ctx, clusterID, nodepoolID2) | ||
| g.Expect(err).NotTo(HaveOccurred(), "second nodepool should still be accessible") | ||
| g.Expect(np2.DeletedTime).NotTo(BeNil(), "second nodepool should have deleted_time set via cascade") | ||
| }, h.Cfg.Timeouts.Adapter.Processing, h.Cfg.Polling.Interval).Should(Succeed()) |
There was a problem hiding this comment.
Cascade check is race-prone when child nodepools hard-delete quickly.
These Eventually blocks require GetNodePool to succeed. If a nodepool reaches hard-delete before this assertion runs, you’ll get repeated 404s and a false failure even though cascade deletion succeeded. Accepting either DeletedTime != nil or HTTP 404 in this phase will remove this flake path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/cluster/delete.go` around lines 157 - 167, The two Eventually checks that
call h.Client.GetNodePool for nodepoolID1 and nodepoolID2 are race-prone because
GetNodePool may return a 404 if the child nodepool hard-deletes quickly; update
each Eventually lambda (the calls invoking h.Client.GetNodePool and asserting on
npX.DeletedTime) to treat either a successful response with np.DeletedTime !=
nil OR an HTTP 404 (not-found) error as a success: call GetNodePool, if err is a
not-found/404 treat the check as passed, otherwise assert no error and that
np.DeletedTime is not nil.
| ginkgo.By("Verify final cluster state to ensure Ready before cleanup") | ||
| err := h.WaitForClusterCondition( | ||
| ctx, | ||
| clusterID, | ||
| client.ConditionTypeReady, | ||
| openapi.ResourceConditionStatusTrue, | ||
| h.Cfg.Timeouts.Cluster.Ready, | ||
| ) | ||
| if err != nil { | ||
| ginkgo.GinkgoWriter.Printf("WARNING: cluster %s did not reach Ready state before cleanup: %v\n", clusterID, err) | ||
| } | ||
| Eventually(h.PollCluster(ctx, clusterID), h.Cfg.Timeouts.Cluster.Ready, h.Cfg.Polling.Interval). | ||
| Should(helper.HaveResourceCondition(client.ConditionTypeReady, openapi.ResourceConditionStatusTrue)) | ||
|
|
||
| ginkgo.By("cleaning up test cluster " + clusterID) | ||
| err = h.CleanupTestCluster(ctx, clusterID) | ||
| err := h.CleanupTestCluster(ctx, clusterID) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to cleanup cluster %s", clusterID) |
There was a problem hiding this comment.
Don't let the pre-cleanup readiness wait block teardown.
If the cluster never becomes Ready, this Eventually(...).Should(...) can fail before CleanupTestCluster runs, which leaves the cluster/nodepool behind and increases test pollution. Keep the readiness check non-fatal, or move cleanup onto an always-executed path. As per coding guidelines, always implement resource cleanup in AfterEach blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/nodepool/concurrent_creation.go` around lines 177 - 183, The readiness
wait (Eventually(h.PollCluster(...)).Should(HaveResourceCondition(...))) must
not block cleanup; change the test so the readiness check is non-fatal and
cleanup always runs: either move the cleanup call into an AfterEach/teardown
hook that always invokes h.CleanupTestCluster(ctx, clusterID), or make the
Immediately-executed readiness probe only log the result (capture the Eventually
result from h.PollCluster(ctx, clusterID) and if it times out, call
processLogger or GinkgoWriter to report the failure but do not call
Should/Fail), then always call h.CleanupTestCluster(ctx, clusterID) afterwards;
reference: PollCluster, HaveResourceCondition, Eventually, and
CleanupTestCluster to locate the code to change.
| if resp.StatusCode != expectedStatus { | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unexpected status code %d for %s (failed to read response body: %w)", | ||
| resp.StatusCode, action, err) | ||
| body, _ := io.ReadAll(resp.Body) | ||
| return nil, &HTTPError{ | ||
| StatusCode: resp.StatusCode, | ||
| Action: action, | ||
| Body: string(body), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: confirm the new HTTP error path is used by the client/poller code.
rg -nP 'handleHTTPResponse\(|HTTPError|errors\.As\(' --type goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 559
🏁 Script executed:
sed -n '50,65p' pkg/client/client.goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 656
🏁 Script executed:
sed -n '45,70p' pkg/helper/pollers.goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 786
🏁 Script executed:
rg 'HTTPError\{|\.Body|httpErr\.Body' --type go -B2 -A2Repository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 7426
🏁 Script executed:
sed -n '1,30p' pkg/client/client.goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 840
🏁 Script executed:
sed -n '35,47p' pkg/client/client.goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 463
🏁 Script executed:
rg 'handleHTTPResponse' --type goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 1681
Handle io.ReadAll errors instead of silencing them.
When io.ReadAll fails at line 55, the error is discarded and an HTTPError is created with an empty Body field. This loses critical diagnostic information about transport failures or malformed responses. The Body field is used in the error message (line 44-45), so a read failure results in degraded error diagnostics for callers and the polling code.
🔧 Suggested fix
if resp.StatusCode != expectedStatus {
- body, _ := io.ReadAll(resp.Body)
+ body, err := io.ReadAll(resp.Body)
+ if err != nil {
+ return nil, fmt.Errorf("failed to read %s error response: %w", action, err)
+ }
return nil, &HTTPError{
StatusCode: resp.StatusCode,
Action: action,
Body: string(body),
}
}📝 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 resp.StatusCode != expectedStatus { | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("unexpected status code %d for %s (failed to read response body: %w)", | |
| resp.StatusCode, action, err) | |
| body, _ := io.ReadAll(resp.Body) | |
| return nil, &HTTPError{ | |
| StatusCode: resp.StatusCode, | |
| Action: action, | |
| Body: string(body), | |
| } | |
| if resp.StatusCode != expectedStatus { | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read %s error response: %w", action, err) | |
| } | |
| return nil, &HTTPError{ | |
| StatusCode: resp.StatusCode, | |
| Action: action, | |
| Body: string(body), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/client/client.go` around lines 54 - 60, When checking resp.StatusCode !=
expectedStatus in the client code, don't ignore the error from
io.ReadAll(resp.Body); capture the (body, err) result and if err != nil include
a clear read-failure message in the returned HTTPError.Body (e.g., "failed to
read response body: <err>") or combine any partial body with the read error so
callers get diagnostic info; update the return that constructs the HTTPError
(referencing HTTPError, resp.Body, io.ReadAll, expectedStatus and action) to
surface the read error instead of silencing it.
1 participant
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Summary by CodeRabbit
Release Notes
New Features
DeletedTimeandFinalizedconditions.Reconciledcondition type for improved resource state visibility alongside existing status indicators.Bug Fixes