fix: paginate ListNotCompletedDeployments in pipedv1#6727
fix: paginate ListNotCompletedDeployments in pipedv1#6727sridhar-panigrahi wants to merge 5 commits into
Conversation
|
@khanhtc1202 updated the patch at #6727 — went with the proto approach as originally proposed. Added cursor and page_size to the request, forwarded them server-side, and the pipedv1 store now loops until it gets an empty cursor. v0 store untouched. Let me know if anything needs changing! |
piped was calling ListNotCompletedDeployments once per sync tick and throwing away the cursor on the response. If the datastore returned a partial page, every deployment past the first page would be silently skipped until the next tick — showing up as stuck PENDING or PLANNED. Fix: - Add cursor and page_size to ListNotCompletedDeploymentsRequest proto - Forward them in PipedAPI.ListNotCompletedDeployments into the datastore ListOptions so the server honours the client's position - Replace the single RPC call in the pipedv1 deployment store with a loop that follows the cursor until the server returns an empty one, accumulating all deployments before classifying them - Add table-driven tests for the v1 store covering single-page, multi-page, and all deployment status classifications v0 store is intentionally untouched (frozen). Fixes pipe-cd#6696 Signed-off-by: Shridhar Panigrahi <sridharpanigrahi2006@gmail.com>
sridhar-panigrahi
force-pushed
the
fix/pipedv1-list-not-completed-deployments-pagination
branch
from
0dc66c0 to
fb7eff4
Compare
sridhar-panigrahi
changed the title
|
@sridhar-panigrahi please follow the PR template used for pipecd. It is hard to tell why this PR/Change is needed. Look at previous PRs for any idea. |
|
@mohammedfirdouss thanks for the nudge — you're right, the original description didn't make the motivation clear. I've rewritten it to follow the PR template (using #6724 as the reference). Let me know if it reads better now. |
@sridhar-panigrahi This is good, but try and remove the extra details beneath the template "A bit more detail on the changes" |
|
@mohammedfirdouss got it — dropped the extra section, just the template now. Thanks for the review! |
Remove the sign-off as well, at the bottom. |
|
@mohammedfirdouss , removed the signoff part too . |
Drop page_size from the proto for ListNotCompletedDeployments. Nothing
was reading the field — the pipedv1 store only forwards the cursor —
so adding it to the wire format was a speculative API surface cost. A
follow-up PR can add it back when a real use case appears.
Tighten the head-deployment assertions in store_test.go to compare the
full map of application id to deployment rather than just checking
that the application ids are present. While writing this I noticed the
sync() function's write order makes the running deployment win over a
pending or planned one for the same application, which contradicts
the Lister docstring ("head = oldest uncompleted"). Filed separately
as pipe-cd#6780 — not fixing it in this PR to keep the diff focused on
pagination.
Signed-off-by: Cyrus <sridharpanigrahi2006@gmail.com>
|
Just did a fresh self-review pass and pushed updates.
@khanhtc1202 @Warashi @ffjlabo @t-kikuc when one of you has a minute — would appreciate a maintainer look. |
2 participants
What this PR does:
Makes
pipedv1's deployment store actually paginate when it callsListNotCompletedDeployments. The store now loops on the cursor returned by the server until the cursor comes back empty, instead of taking only the first page and discarding the cursor.Why we need it:
pipedv1's deployment store was callingListNotCompletedDeploymentsonce per sync tick and ignoring the cursor on the response. If the datastore returned a partial page, anything past that boundary got dropped — those deployments looked stuck inPENDING/PLANNEDuntil the overall volume dropped enough to fit in a single page. The TODO at the top ofsync()had been there since the RPC was introduced.Scope — why
pipedv0is not touched here:Issue #6696 calls out the same bug in
pkg/app/piped/apistore/deploymentstore/store.go.pipedv0is on the deprecation path per RFC 0015 (support intended through end of 2025), so this PR is scoped topipedv1to keep the diff small. Happy to mirror the loop intopipedv0in a follow-up if maintainers prefer it covered.Which issue(s) this PR fixes:
Fixes #6696
Does this PR introduce a user-facing change?:
pipedv1under heavier deployment volume should see every pending/planned/running deployment picked up each sync tick.