Add a backwards compatible only_active flag on list leases#172
Conversation
WalkthroughListLeases now builds a variadic Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as ClientService
participant K as Controller/List API
C->>S: ListLeases(req: namespace, selector, pageSize, pageToken, OnlyActive)
S->>S: Assemble listOptions: namespace, selector, pageSize, pageToken
alt OnlyActive is nil or true
S->>S: Append controller.MatchingActiveLeases()
Note right of S #DFF0D8: Active-only filter appended
else OnlyActive is false
Note right of S #F0F0F0: Active filter omitted
end
S->>K: List(...listOptions)
K-->>S: LeaseList (paged)
S-->>C: Return LeaseList
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/client/v1/client_service.go (1)
138-167: Consider adding tests for the new filter parsing logic.The filter parsing and conditional application logic is well-implemented. To ensure robustness, consider adding tests for edge cases:
- Empty filter string
- "all" token only
- "all" with other selectors (e.g., "all,board=foo")
- Multiple "all" tokens (e.g., "all,all,key=value")
- Leading/trailing whitespace and commas
- Invalid label selectors after removing "all"
If test files exist, search for the test suite:
#!/bin/bash # Description: Find test files for ClientService to add coverage for the new logic fd -e go -e test.go | xargs rg -l "TestClientService|Test.*ListLeases"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/client/v1/client_service.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/client/v1/client_service.go (2)
internal/controller/lease.go (1)
MatchingActiveLeases(12-26)api/v1alpha1/lease_types.go (1)
LeaseList(91-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: lint-go
- GitHub Check: deploy-kind
- GitHub Check: tests
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
🔇 Additional comments (3)
internal/service/client/v1/client_service.go (3)
22-22: LGTM!The
stringsimport is necessary for the new filter parsing logic.
123-126: LGTM!The documentation clearly explains the default behavior and the special "all" token usage.
138-147: No changes needed for empty filter handlinglabels.Parse("") already returns an “everything” selector without error, so the existing logic correctly handles the “all” token and empty filters.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/service/client/v1/client_service.go (1)
161-164: Default active-only behavior preserved; “all” opt-out is clearThis keeps the existing active-only default and makes including ended leases explicit via “all”. Good.
🧹 Nitpick comments (2)
internal/service/client/v1/client_service.go (2)
154-159: Optional: add pagination options only when setAvoid sending zero/empty values. No behavior change, just cleaner options.
- listOptions := []kclient.ListOption{ - kclient.InNamespace(namespace), - kclient.MatchingLabelsSelector{Selector: selector}, - kclient.Limit(int64(req.PageSize)), - kclient.Continue(req.PageToken), - } + listOptions := []kclient.ListOption{ + kclient.InNamespace(namespace), + kclient.MatchingLabelsSelector{Selector: selector}, + } + if req.PageSize > 0 { + listOptions = append(listOptions, kclient.Limit(int64(req.PageSize))) + } + if req.PageToken != "" { + listOptions = append(listOptions, kclient.Continue(req.PageToken)) + }
123-127: Doc tweak: clarify “all” token and case-sensitivitySuggest mentioning that “all” is a special, case-insensitive token and can be combined with standard selectors; parentheses in label selectors are supported.
-// By default, only active leases (without the ended label) are returned. -// To include ended leases, add "all" to the comma-separated filter list as a special case +// By default, only active leases (without the ended label) are returned. +// To include ended leases, add the special token "all" (case-insensitive) to the comma-separated filter list, +// e.g., `all,board=foo` or `env in (a,b),ALL`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/client/v1/client_service.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/client/v1/client_service.go (2)
internal/controller/lease.go (1)
MatchingActiveLeases(12-26)api/v1alpha1/lease_types.go (1)
LeaseList(91-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: deploy-kind
- GitHub Check: lint-go
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
- GitHub Check: e2e-tests (ubuntu-24.04)
by setting the new optional active_only bool to false. Defaults to true.
using the upstream jumpstarter-protocol. This finally doesn't mess up imports.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
internal/protocol/jumpstarter/client/v1/client.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/v1/common.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/v1/jumpstarter.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/v1/kubernetes.pb.gois excluded by!**/*.pb.gointernal/protocol/jumpstarter/v1/router.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
Makefile(1 hunks)buf.gen.yaml(2 hunks)internal/service/client/v1/client_service.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/client/v1/client_service.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: lint-go
- GitHub Check: e2e-tests (ubuntu-24.04-arm)
- GitHub Check: e2e-tests (ubuntu-24.04)
- GitHub Check: deploy-kind
- GitHub Check: e2e-tests-28d6b1cc3b49ab9ae176918ab9709a2e2522c97e
mangelajo
changed the title
2 participants
it skips the default filter for active leases only. E.g.: jmp get leases -l board=foo --all will use only_active=false to list leases matching the board=foo selector including the ended ones
Summary by CodeRabbit
New Features
Chores