feat: consistent lease filtering with -a/--all and -A/--all-clients flags#332
Open
raballew wants to merge 13 commits intojumpstarter-dev:mainfrom
Open
feat: consistent lease filtering with -a/--all and -A/--all-clients flags#332raballew wants to merge 13 commits intojumpstarter-dev:mainfrom
raballew wants to merge 13 commits intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds short CLI flags, enables multi-name deletion and client-scoped filtering for leases, adds LeaseList.filter_by_client, updates get/delete command signatures and logic, and introduces unit tests covering new CLI flags and deletion/get behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant CLI as jumpstarter-cli command
participant Config as Config client (list/delete)
User->>CLI: invoke delete_leases(names/--selector/--all/--all-clients)
CLI->>Config: if selector/list needed -> list_leases(filter)
Config-->>CLI: LeaseList
CLI->>CLI: if not all_clients -> leases = leases.filter_by_client(client_name)
CLI->>CLI: build to_delete from names/selector/all
alt no targets
CLI->>User: error "One of NAMES, --selector, --all or --all-clients must be specified"
else targets found
loop for each lease in to_delete
CLI->>Config: delete_lease(lease_name)
Config-->>CLI: deletion result
CLI->>User: print deletion status
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
python/packages/jumpstarter/jumpstarter/client/grpc.py (1)
347-349: Consider adding a docstring for consistency.The new
filter_by_clientmethod follows the same pattern asfilter_by_selectorabove, but lacks a docstring explaining its purpose and the pagination token clearing behavior.📝 Suggested docstring addition
def filter_by_client(self, client_name: str) -> LeaseList: + """Filter leases client-side by matching against their client name. + + Note: Pagination token is cleared since server pagination becomes invalid after client-side filtering. + """ filtered = [lease for lease in self.leases if lease.client == client_name] return LeaseList(leases=filtered, next_page_token=None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/grpc.py` around lines 347 - 349, Add a docstring to the filter_by_client method explaining that it returns a LeaseList containing only leases whose client matches the given client_name and that it clears pagination by setting next_page_token to None (mirroring the behavior of filter_by_selector); update the docstring to reference parameters (client_name: str) and return type (LeaseList) and note the pagination token clearing so callers understand why paging info is dropped.python/packages/jumpstarter-cli/jumpstarter_cli/get.py (1)
71-72: Consider validating client name before filtering.If
config.metadata.nameis an empty string (which can occur from misconfigured environment variables or malformed YAML),filter_by_clientwill silently return no leases instead of warning the user. Adding validation to ensure the name is non-empty would improve error visibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/get.py` around lines 71 - 72, Validate that config.metadata.name is a non-empty string before calling leases.filter_by_client when all_clients is False; if it is empty or whitespace, log or raise a user-facing error (or print a warning) explaining that the client name is missing/misconfigured and abort/skip filtering to avoid silently returning no leases. Update the conditional that currently uses all_clients and config.metadata.name so that filter_by_client is only invoked after this validation (referencing config.metadata.name, all_clients, and the leases.filter_by_client call). Ensure the message clearly instructs the user to check environment variables or YAML configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/cli_test.py`:
- Around line 24-31: The test TestDeleteLeasesShortFlags is looking for a
parameter named "all" but the click command delete_leases defines the flag
parameter as "delete_all"; update the test in
test_delete_leases_accepts_short_a_flag to look up param.name == "delete_all"
(i.e., change the param lookup from "all" to "delete_all") so the next(...)
finds the correct click Parameter and the assertion on "-a" in all_option.opts
passes.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/delete_test.py`:
- Around line 167-174: The failing test has a line exceeding 120 chars in
test_delete_no_args_raises_error; split the long pytest.raises(...) call and its
subsequent _delete_leases(...) invocation into multiple shorter lines so no line
exceeds 120 chars (break the with pytest.raises(...) and the _delete_leases(...)
argument list across lines), referencing the test function
test_delete_no_args_raises_error and the helper functions _make_config and
_delete_leases to locate the code to reformat.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/delete.py`:
- Line 25: The function signature for delete_leases is longer than 120
characters; break the long signature into multiple lines to satisfy Ruff (e.g.,
place the parameters each on their own line or wrap the return/annotations) so
that the line length is <=120. Edit the delete_leases(...) definition to wrap
parameters like names: tuple[str, ...], selector: str | None, delete_all: bool,
all_clients: bool, output: OutputType across lines (or use a shorter
alias/import for tuple typing) so the single line is within the linter limit
while preserving the same parameter names and types.
---
Nitpick comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/get.py`:
- Around line 71-72: Validate that config.metadata.name is a non-empty string
before calling leases.filter_by_client when all_clients is False; if it is empty
or whitespace, log or raise a user-facing error (or print a warning) explaining
that the client name is missing/misconfigured and abort/skip filtering to avoid
silently returning no leases. Update the conditional that currently uses
all_clients and config.metadata.name so that filter_by_client is only invoked
after this validation (referencing config.metadata.name, all_clients, and the
leases.filter_by_client call). Ensure the message clearly instructs the user to
check environment variables or YAML configuration.
In `@python/packages/jumpstarter/jumpstarter/client/grpc.py`:
- Around line 347-349: Add a docstring to the filter_by_client method explaining
that it returns a LeaseList containing only leases whose client matches the
given client_name and that it clears pagination by setting next_page_token to
None (mirroring the behavior of filter_by_selector); update the docstring to
reference parameters (client_name: str) and return type (LeaseList) and note the
pagination token clearing so callers understand why paging info is dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf89b687-05fd-4148-86b5-6dae7c6c27f3
📒 Files selected for processing (8)
.gitignorepython/packages/jumpstarter-cli/jumpstarter_cli/auth.pypython/packages/jumpstarter-cli/jumpstarter_cli/cli_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/delete.pypython/packages/jumpstarter-cli/jumpstarter_cli/delete_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/get.pypython/packages/jumpstarter-cli/jumpstarter_cli/get_test.pypython/packages/jumpstarter/jumpstarter/client/grpc.py
raballew
added a commit
to raballew/jumpstarter
that referenced
this pull request
Mar 17, 2026
Fix param name lookup in cli_test.py ("all" -> "delete_all"), wrap
long function signature in delete.py, and wrap long pytest.raises
line in delete_test.py to satisfy ruff E501.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/delete.py`:
- Around line 37-49: The delete command silently succeeds when filtering yields
zero leases; after calling config.list_leases(...) and
LeaseList.filter_by_selector()/filter_by_client() in delete.py (where you build
to_delete from leases.leases), add a guard that checks whether to_delete is
empty and raise a click.ClickException with a clear message (e.g. "No leases
matched for given NAMES/--selector/--all/--all-clients") so that
list_leases/filter_by_client returning an empty LeaseList fails loudly instead
of becoming a no-op.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 407099d5-421a-423f-8081-234c7881d0c5
📒 Files selected for processing (3)
python/packages/jumpstarter-cli/jumpstarter_cli/cli_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/delete.pypython/packages/jumpstarter-cli/jumpstarter_cli/delete_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-cli/jumpstarter_cli/delete_test.py
- python/packages/jumpstarter-cli/jumpstarter_cli/cli_test.py
Member
|
Wouldn't this change the behavior of "--all" that currently lists all leases, not only yours? |
Contributor
Author
|
@mangelajo, yeah it would. do you say inverting -A and -a would be better? is this logical? |
Member
but also --all changes behavior from today? at least reading at the description, I haven't tried. |
Contributor
Author
|
yes, it does change behavior from today. |
raballew
added a commit
to raballew/jumpstarter
that referenced
this pull request
Mar 18, 2026
Fix param name lookup in cli_test.py ("all" -> "delete_all"), wrap
long function signature in delete.py, and wrap long pytest.raises
line in delete_test.py to satisfy ruff E501.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
force-pushed
the
008-short-flag-aliases
branch
from
7133e93 to
bb8762a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
python/packages/jumpstarter-cli/jumpstarter_cli/delete.py (1)
33-55: Deduplicate delete targets before issuing RPCs.Repeated names can trigger duplicate delete calls and fail on subsequent attempts. Deduping once before the loop makes deletion idempotence safer.
Proposed patch
- if not to_delete: + to_delete = list(dict.fromkeys(to_delete)) + + if not to_delete: raise click.ClickException("no leases found matching the criteria")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/delete.py` around lines 33 - 55, The code collects deletion targets in the to_delete list (from names, selector, delete_all/all_clients) and may contain duplicates causing repeated config.delete_lease(name=...) RPCs; before iterating callsite for name in to_delete, deduplicate the list (preserving order) and assign back to to_delete so each unique lease name is deleted once — e.g., transform to_delete into an ordered-unique sequence (using an ordered set pattern) and then loop over it to call config.delete_lease(name=...).python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py (1)
363-364: Useinspect.unwrap()instead of hard-coded__wrapped__.__wrapped__for accessing the underlying function.The current approach
get_leases.callback.__wrapped__.__wrapped__works correctly but relies on specific decorator implementation details.inspect.unwrap(get_leases.callback)is more idiomatic and resilient to future refactoring of the decorator stack.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py` around lines 363 - 364, Replace the brittle manual unwrapping get_leases.callback.__wrapped__.__wrapped__ with inspect.unwrap(get_leases.callback): import inspect if not present, then set _unwrapped_get_leases = inspect.unwrap(get_leases.callback) so the underlying function is resolved robustly regardless of decorator stacking; target the assignment of _unwrapped_get_leases and the get_leases.callback symbol when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/delete.py`:
- Around line 21-22: The CLI reuses the --all flag (delete_all) for a new
client-scoped behavior which is a breaking change; preserve current global
behavior for now but add a migration guardrail: when delete_all is True and
all_clients is False, emit a clear deprecation/transition warning via click.echo
or process logger (mentioning that --all will be reassigned and recommending
--all-clients) and continue to perform the existing global-delete behavior to
avoid breaking automation; update the click help text to note the deprecation,
add a new explicit --all-clients (all_clients) branch for the new behavior, and
add a small unit/integration test that asserts the warning is emitted when
delete_all is used so callers get notified during migration.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py`:
- Around line 406-422: The test test_all_flag_includes_expired_own_only
currently doesn't create an expired lease, so update the fixture setup: use
_make_lease to create at least one lease with an expiration in the past (e.g.,
set an expired flag or end_time/past timestamp on the "my-lease" object returned
by _make_lease), keep the other lease active, and keep calling
_unwrapped_get_leases(config=..., show_all=True, all_clients=False); then assert
that the expired "my-lease" is included in printed_leases and that
config.list_leases was called with only_active=False as before. Reference
symbols: test_all_flag_includes_expired_own_only, _make_lease, _make_config,
_unwrapped_get_leases, config.list_leases.
---
Nitpick comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/delete.py`:
- Around line 33-55: The code collects deletion targets in the to_delete list
(from names, selector, delete_all/all_clients) and may contain duplicates
causing repeated config.delete_lease(name=...) RPCs; before iterating callsite
for name in to_delete, deduplicate the list (preserving order) and assign back
to to_delete so each unique lease name is deleted once — e.g., transform
to_delete into an ordered-unique sequence (using an ordered set pattern) and
then loop over it to call config.delete_lease(name=...).
In `@python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py`:
- Around line 363-364: Replace the brittle manual unwrapping
get_leases.callback.__wrapped__.__wrapped__ with
inspect.unwrap(get_leases.callback): import inspect if not present, then set
_unwrapped_get_leases = inspect.unwrap(get_leases.callback) so the underlying
function is resolved robustly regardless of decorator stacking; target the
assignment of _unwrapped_get_leases and the get_leases.callback symbol when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71a6cf84-7e38-4f00-af08-e9ada9632dc4
📒 Files selected for processing (8)
.gitignorepython/packages/jumpstarter-cli/jumpstarter_cli/auth.pypython/packages/jumpstarter-cli/jumpstarter_cli/cli_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/delete.pypython/packages/jumpstarter-cli/jumpstarter_cli/delete_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/get.pypython/packages/jumpstarter-cli/jumpstarter_cli/get_test.pypython/packages/jumpstarter/jumpstarter/client/grpc.py
🚧 Files skipped from review as they are similar to previous changes (6)
- python/packages/jumpstarter-cli/jumpstarter_cli/cli_test.py
- python/packages/jumpstarter/jumpstarter/client/grpc.py
- .gitignore
- python/packages/jumpstarter-cli/jumpstarter_cli/get.py
- python/packages/jumpstarter-cli/jumpstarter_cli/auth.py
- python/packages/jumpstarter-cli/jumpstarter_cli/delete_test.py
Comment on lines
+21
to
+22
| @click.option("-a", "--all", "delete_all", is_flag=True, help="Delete all your active leases") | ||
| @click.option("-A", "--all-clients", "all_clients", is_flag=True, help="Delete active leases from all clients") |
Contributor
There was a problem hiding this comment.
--all semantic change is a breaking CLI contract and needs migration guardrails.
Reassigning --all from global scope to client-scoped behavior can silently change existing automation outcomes. Please add a compatibility/deprecation path (or explicit transition warning) before release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/delete.py` around lines 21 -
22, The CLI reuses the --all flag (delete_all) for a new client-scoped behavior
which is a breaking change; preserve current global behavior for now but add a
migration guardrail: when delete_all is True and all_clients is False, emit a
clear deprecation/transition warning via click.echo or process logger
(mentioning that --all will be reassigned and recommending --all-clients) and
continue to perform the existing global-delete behavior to avoid breaking
automation; update the click help text to note the deprecation, add a new
explicit --all-clients (all_clients) branch for the new behavior, and add a
small unit/integration test that asserts the warning is emitted when delete_all
is used so callers get notified during migration.
Comment on lines
+406
to
+422
| def test_all_flag_includes_expired_own_only(self): | ||
| from unittest.mock import patch | ||
|
|
||
| my_lease = self._make_lease("my-lease", client="my-client") | ||
| other_lease = self._make_lease("other-lease", client="other-client") | ||
| config = self._make_config([my_lease, other_lease]) | ||
|
|
||
| with patch("jumpstarter_cli.get.model_print") as mock_print: | ||
| _unwrapped_get_leases( | ||
| config=config, selector=None, output=None, show_all=True, all_clients=False, | ||
| ) | ||
|
|
||
| printed_leases = mock_print.call_args[0][0] | ||
| assert len(printed_leases.leases) == 1 | ||
| assert printed_leases.leases[0].name == "my-lease" | ||
| config.list_leases.assert_called_once_with(filter=None, only_active=False) | ||
|
|
Contributor
There was a problem hiding this comment.
Test intent doesn’t match fixture data for “includes expired”.
The test name says expired leases are included, but both fixtures are default leases and no expired lease is modeled. This currently validates call-arguments/client filtering, not expired inclusion behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/get_test.py` around lines 406
- 422, The test test_all_flag_includes_expired_own_only currently doesn't create
an expired lease, so update the fixture setup: use _make_lease to create at
least one lease with an expiration in the past (e.g., set an expired flag or
end_time/past timestamp on the "my-lease" object returned by _make_lease), keep
the other lease active, and keep calling _unwrapped_get_leases(config=...,
show_all=True, all_clients=False); then assert that the expired "my-lease" is
included in printed_leases and that config.list_leases was called with
only_active=False as before. Reference symbols:
test_all_flag_includes_expired_own_only, _make_lease, _make_config,
_unwrapped_get_leases, config.list_leases.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- jmp get leases: shows only your leases by default - jmp get leases -a/--all: shows all leases including other clients and expired - jmp delete leases: accepts multiple lease names - jmp delete leases -a/--all: deletes only your active leases - Add LeaseList.filter_by_client() method Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- jmp get leases: your active leases - jmp get leases -a/--all: your active + expired leases - jmp get leases --all-clients: everyone's active leases - jmp get leases -a --all-clients: everyone's active + expired leases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… get Make delete leases flags match get leases: -a/--all deletes only your own active leases, -A/--all-clients includes leases from all clients. Both flags can be combined. Updated error message and tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix param name lookup in cli_test.py ("all" -> "delete_all"), wrap
long function signature in delete.py, and wrap long pytest.raises
line in delete_test.py to satisfy ruff E501.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
force-pushed
the
008-short-flag-aliases
branch
from
bb8762a to
66b7d2d
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test verifies that --all passes only_active=False and filters by client, not that expired leases are modeled in the fixture data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
2 participants
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
-a/--alland-A/--all-clientsas orthogonal flags for bothget leasesanddelete leasescommands-a/--allincludes expired leases (get) or deletes your own active leases (delete)-A/--all-clientsshows/deletes leases from all clients, not just your ownjmp get leases -a -Ashows all leases including expired from all clientsdelete leasesaccepts multiple names:jmp delete leases <id1> <id2> ...LeaseList.filter_by_client()for reusable client-side filteringTest plan
get leasesdefaults to showing only current client's active leasesget leases -ashows current client's active + expired leasesget leases -Ashows all clients' active leasesget leases -a -Ashows all clients' active + expired leasesdelete leases -aonly deletes current client's leasesdelete leases -Adeletes all clients' leasesdelete leases -a -Adeletes all clients' leasesdelete leases <name1> <name2>deletes specified leases by namedelete leaseswith no args raises error🤖 Generated with Claude Code