feat: support label selectors for list operations - #362
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthyx
left a comment
There was a problem hiding this comment.
Thanks for this — the pagination + label/field selector matching logic looks correct, and the new TestStorageImpl_GetList_LabelSelectorWithPagination test does a good job exercising filtering across multiple pages of the underlying store.
A couple of things before this is ready to merge:
- The PR is still marked draft, and the description leaves an open question about whether
GeneratedNetworkPolicyand other generated/aggregated resources belong in this PR or a follow-up. That should be resolved (and the PR marked ready) before merging. - The branch is currently behind
main— please rebase/update before merge. - Left one minor nit inline about code duplication between the two fetch branches.
No functional blockers found in the diff itself — holding off on approval only because of the draft status and the branch being behind base. Happy to approve once those are addressed.
0c81c2c to
9580f49
Compare
Signed-off-by: ystkfujii <ystk.fujii0731@gmail.com>
9580f49 to
d7a157a
Compare
|
Thank you for the quick review! When you have time, could you please take another look? |
matthyx
left a comment
There was a problem hiding this comment.
Looks good now — thanks for the update.
- The duplication between the two fetch branches is gone;
GetListWithConnnow runs a single unified loop, andnormalizeSelectionPredicatecentralizes the nil-safety/GetAttrscheck (fails fast with a clear error instead of silently dropping items). - The scope question is resolved: aggregated resources (
GeneratedNetworkPolicy, config-scan/vulnerability summaries) explicitly forceLabel/FieldtoEverything()before paging their source objects, with aTODO+ link to the follow-up (#363) rather than silently misapplying the selector. - Pagination logic re-checked and still correct against the test.
Only remaining nit: the branch shows as behind main — worth updating before merge, but not a blocker from a code standpoint. Approving.
Overview
This PR adds label selector support to storage list operations.
Label selector support for
GeneratedNetworkPolicyand other generated or aggregated resources will be addressed in the following issue: