HYPERFLEET-1238 - fix: paginate API responses to fetch all resources#185
Open
rafabene wants to merge 2 commits into
Open
HYPERFLEET-1238 - fix: paginate API responses to fetch all resources#185rafabene wants to merge 2 commits into
rafabene wants to merge 2 commits into
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR fixes a bug where Sentinel only fetched the first page (20 items) of clusters and nodepools from the HyperFleet API. Sequence Diagram(s)sequenceDiagram
participant FetchResources
participant HyperFleetAPI as HyperFleet API
participant fetchClusters
participant convertCluster as convertCluster Helper
FetchResources->>fetchClusters: call with Page=0, PageSize=20
fetchClusters->>HyperFleetAPI: GET /clusters?page=0&size=20
HyperFleetAPI-->>fetchClusters: {items:[...], total:65, page:0, size:20}
fetchClusters->>convertCluster: convert 20 items
convertCluster-->>fetchClusters: [Resource...]
fetchClusters->>HyperFleetAPI: GET /clusters?page=1&size=20
HyperFleetAPI-->>fetchClusters: {items:[...], total:65, page:1, size:20}
fetchClusters->>convertCluster: convert 20 items
convertCluster-->>fetchClusters: [Resource...]
fetchClusters->>HyperFleetAPI: GET /clusters?page=2&size=20
HyperFleetAPI-->>fetchClusters: {items:[...], total:65, page:2, size:20}
fetchClusters->>convertCluster: convert 25 items (final page)
convertCluster-->>fetchClusters: [Resource...]
fetchClusters-->>FetchResources: accumulated 65 Resources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 800 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Tests cover changed packages | +0 |
Computed by hyperfleet-risk-scorer
rafabene
force-pushed
the
HYPERFLEET-1238-paginate-api-results
branch
from
19c5ad2 to
f93ef03
Compare
…nings Replace repeated string literals in client test fixtures with named constants and helper functions (createMockCondition, createMockNodePool).
rafabene
force-pushed
the
HYPERFLEET-1238-paginate-api-results
branch
from
f93ef03 to
80e704c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/client_test.go (1)
471-505:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
TestFetchResources_NilStatusis not exercising a nil-status path (CWE-398).Line [475] creates only a valid cluster, so this test currently validates a happy path while claiming degradation behavior for nil
status.Proposed test-fix diff
func TestFetchResources_NilStatus(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Note: With v1.0.0 spec, status is required, so this test will fail validation - // This test might need to be removed or modified since status is now required - cluster2 := createMockCluster("cluster-2") + cluster1 := createMockCluster("cluster-1") + delete(cluster1, keyStatus) // malformed payload to exercise graceful degradation + cluster2 := createMockCluster("cluster-2") response := createMockClusterList([]map[string]interface{}{ + cluster1, cluster2, }) @@ - // Should skip cluster-1 with nil status, return only cluster-2 + // Should skip cluster-1 with nil status, return only cluster-2 if len(resources) != 1 { t.Fatalf("Expected 1 resource (nil status skipped), got %d", len(resources)) }As per coding guidelines, “Error paths SHOULD be tested, not just happy paths.”
🤖 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 `@internal/client/client_test.go` around lines 471 - 505, The TestFetchResources_NilStatus test is not actually exercising the nil-status path it claims to test. The test currently creates only a valid cluster using createMockCluster("cluster-2"), which has a valid status by default. To properly test graceful degradation, modify the mock response to include two clusters: one with a valid status and one with an explicitly nil status field. This way the test will verify that the FetchResources method correctly skips the cluster with nil status while returning the valid cluster, matching the test's assertion that only 1 resource should be returned.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/client/client_test.go (1)
1132-1275: ⚡ Quick winPagination coverage is cluster-only; nodepool pagination path is unverified (CWE-398).
These new tests validate only
ResourceTypeClusters, but the same PR changes pagination behavior for nodepools too (fetchNodePoolsloop +page/pageSizecontract). Add a mirrored nodepool pagination case (ideally table-driven by resource type) to prevent one-sided regressions.As per coding guidelines, “New exported functions and critical logic paths SHOULD have tests” and “Table-driven tests with t.Run() for repeated patterns.”
🤖 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 `@internal/client/client_test.go` around lines 1132 - 1275, The pagination tests currently only validate ResourceTypeClusters, but the code changes also affect nodepool pagination with the same page/pageSize contract. Refactor the existing pagination tests (TestFetchResources_Pagination, TestFetchResources_PaginationSinglePage, TestFetchResources_PaginationErrorOnSecondPage, and TestFetchResources_PaginationSendsPageSize) to use table-driven test patterns with t.Run() that cover both ResourceTypeClusters and ResourceTypeNodePools to ensure pagination behavior is properly verified for all affected resource types and prevent one-sided regressions.Source: Coding guidelines
🤖 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 `@internal/client/client_test.go`:
- Around line 1218-1248: The test function
TestFetchResources_PaginationErrorOnSecondPage verifies that an error is
returned when pagination fails on the second page, but it does not validate that
no partial results are included in the response. After the FetchResources call
returns with an error, add an assertion to verify that the resources value is
nil or empty, ensuring that partial data is not returned alongside the error.
This hardening prevents the test from passing even if the implementation
incorrectly returns partial results on pagination failure.
In `@internal/client/client.go`:
- Around line 295-309: The code currently marks all network errors as retriable,
but context.Canceled and context.DeadlineExceeded errors should not be retriable
as they indicate intentional shutdown or timeout paths. In the error handling
blocks around lines 295-309 and 423-437, add explicit checks for
context.Canceled and context.DeadlineExceeded using errors.Is before the
existing url.Error checks. For these context errors, return an APIError with
Retriable set to false to prevent retries during shutdown or deadline scenarios.
Keep the existing logic for actual network timeouts as retriable.
- Around line 332-334: The code uses resourceList.Total directly as a capacity
hint in make() calls without validation, which can cause panics if the value is
negative or excessive memory allocation if extremely large. Add validation to
check that resourceList.Total is non-negative and within acceptable bounds
before using it for slice capacity in the make() statements. Apply this
validation at all locations where resourceList.Total is used for slice
allocation: the initial allResources assignment at line 333, the additional
assignments at lines 343-344, and the similar patterns at lines 460-462 and
471-472. If the Total value fails validation, use a safe default capacity (such
as 0) instead of the untrusted value.
- Around line 511-523: Add a nil guard before dereferencing ownerRef since it is
assigned from item.OwnerReferences which is a pointer type that could be nil
from an API response. Wrap the entire block that accesses ownerRef.Id,
ownerRef.Href, and ownerRef.Kind (the three conditional checks that dereference
the pointer fields) inside an if ownerRef != nil check to prevent a panic on
null pointer dereference.
---
Outside diff comments:
In `@internal/client/client_test.go`:
- Around line 471-505: The TestFetchResources_NilStatus test is not actually
exercising the nil-status path it claims to test. The test currently creates
only a valid cluster using createMockCluster("cluster-2"), which has a valid
status by default. To properly test graceful degradation, modify the mock
response to include two clusters: one with a valid status and one with an
explicitly nil status field. This way the test will verify that the
FetchResources method correctly skips the cluster with nil status while
returning the valid cluster, matching the test's assertion that only 1 resource
should be returned.
---
Nitpick comments:
In `@internal/client/client_test.go`:
- Around line 1132-1275: The pagination tests currently only validate
ResourceTypeClusters, but the code changes also affect nodepool pagination with
the same page/pageSize contract. Refactor the existing pagination tests
(TestFetchResources_Pagination, TestFetchResources_PaginationSinglePage,
TestFetchResources_PaginationErrorOnSecondPage, and
TestFetchResources_PaginationSendsPageSize) to use table-driven test patterns
with t.Run() that cover both ResourceTypeClusters and ResourceTypeNodePools to
ensure pagination behavior is properly verified for all affected resource types
and prevent one-sided regressions.
🪄 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: CHILL
Plan: Enterprise
Run ID: 2304dd11-31c7-463a-bc54-e80a70835099
📒 Files selected for processing (3)
CHANGELOG.mdinternal/client/client.gointernal/client/client_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
rafabene
force-pushed
the
HYPERFLEET-1238-paginate-api-results
branch
from
80e704c to
7fe0dd1
Compare
Sentinel was only fetching the first page of API results (default size 20), leaving clusters beyond that window unreconciled. Now iterates through all pages using the page/pageSize query parameters and the total field from the response.
rafabene
force-pushed
the
HYPERFLEET-1238-paginate-api-results
branch
from
7fe0dd1 to
5059e6a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fetchClusters()andfetchNodePools()now iterate through all pages usingpage/pageSizequery parameters, collecting every resource before returningcreateMockCondition,createMockNodePool) to resolve all goconst lint warningsAcceptance Criteria
E2E test covers environments with more than one page of resources— Covered by unit tests:TestFetchResources_Paginationexercises 54 clusters across 3 pages against a real HTTP mock server, validating the full pagination loop. The pagination logic is entirely within the client layer, making E2E unnecessary for this fix.Test Plan
TestFetchResources_Pagination— 54 clusters across 3 pages, verifies all resources collected and correct page requests madeTestFetchResources_PaginationSinglePage— verifies no extra requests when total fits in one pageTestFetchResources_PaginationErrorOnSecondPage— verifies errors on subsequent pages propagate correctlyTestFetchResources_PaginationSendsPageSize— verifiespageSizequery parameter is sentmake test-unit)make lint— 0 issues