Conversation
WalkthroughThis pull request overhauls the CLI commands in the Jumpstarter client. It removes the old Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CreateLease Command
participant Config as ClientConfig
U->>CLI: Run "create lease" with selector & duration
CLI->>Config: Call create_lease(selector, duration)
Config->>Config: Validate input and process lease creation
Config-->>CLI: Return lease details
CLI-->>U: Output formatted lease information
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
NickCao
force-pushed
the
client-svc-lease
branch
from
1286919 to
fef3d35
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: 2
🧹 Nitpick comments (17)
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (1)
19-21: Consider handling malformed selector inputsThe current implementation will fail if a selector term doesn't contain "=" character. Consider adding error handling for malformed inputs.
def selector_to_labels(selector: str): # TODO: support complex selectors (e.g. !=) - return dict([term.split("=") for term in selector.split(",")]) + labels = {} + for term in selector.split(","): + if "=" not in term: + raise ValueError(f"Invalid selector format: {term}. Expected format: key=value") + key, value = term.split("=", 1) + labels[key] = value + return labelspackages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (1)
26-39: Consider adding confirmation for bulk deletionWhen using the
--allflag, multiple leases can be deleted without confirmation. Consider adding a confirmation prompt to prevent accidental deletion.elif all: leases = config.list_leases() + lease_names = [] for lease in leases.leases: if lease.client == config.metadata.name: - names.append(lease.name) + lease_names.append(lease.name) + + if not lease_names: + click.echo("No leases found for the current client") + return + + click.echo(f"You are about to delete {len(lease_names)} lease(s): {', '.join(lease_names)}") + if not click.confirm("Do you want to continue?"): + click.echo("Operation cancelled") + return + + names.extend(lease_names)packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py (2)
18-27: Add examples of duration format in help textThe command requires a duration parameter but doesn't specify the expected format in the help text.
@update.command(name="lease") @opt_context @click.argument("name") -@click.option("--duration", "duration", type=str, required=True) +@click.option("--duration", "duration", type=str, required=True, + help="Duration in format accepted by pydantic's timedelta (e.g., '1d', '24h', '86400s')") @opt_output_all @handle_exceptions
44-51: Consider enhancing selector formatting in table outputThe current implementation joins selector key-value pairs with commas, which could be ambiguous if values contain commas. Consider using a different delimiter or format.
{ "NAME": lease.name, - "SELECTOR": ",".join("{}={}".format(k, v) for (k, v) in lease.selector.match_labels.items()), + "SELECTOR": ";".join("{}={}".format(k, v) for (k, v) in lease.selector.match_labels.items()) if lease.selector.match_labels else "<none>", "DURATION": str(lease.duration.total_seconds()), "CLIENT": lease.client, "EXPORTER": lease.exporter, }packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py (1)
32-36: Consider reordering commands for clarity.
Currently, commands are registered in the sequence “create, get, delete, update, config.” For a more intuitive flow, you might place “update” before “delete” (e.g., CRUD order).- client.add_command(create) - client.add_command(get) - client.add_command(delete) - client.add_command(update) - client.add_command(config) + client.add_command(create) + client.add_command(get) + client.add_command(update) + client.add_command(delete) + client.add_command(config)packages/jumpstarter-cli-client/jumpstarter_cli_client/config.py (4)
30-30: Function name mismatch with subcommand name.
For consistency, consider renamingcreate_client_configto match the “create” subcommand.- @config.command("create", short_help="Create a client config.") - def create_client_config(...): + @config.command("create", short_help="Create a client config.") + def create(...):
125-125: Function name mismatch with subcommand name.
Consider renamingdelete_client_configto match the “delete” subcommand.- @config.command("delete", short_help="Delete a client config.") - def delete_client_config(...): + @config.command("delete", short_help="Delete a client config.") + def delete(...):
137-137: Function name mismatch with subcommand name.
Consider renaminglist_client_configsto match the “list” subcommand.- @config.command("list", short_help="List available client configurations.") - def list_client_configs(...): + @config.command("list", short_help="List available client configurations.") + def list(...):
171-171: Function name mismatch with subcommand name.
Consider renaminguse_client_configto match the “use” subcommand.- @config.command("use", short_help="Select the current client config.") - def use_client_config(...): + @config.command("use", short_help="Select the current client config.") + def use(...):packages/jumpstarter/jumpstarter/client/grpc.py (1)
70-89: Deserialization logic forLease.
Theexporterfield is set to an empty string if absent. Consider usingNoneto indicate missing exporter more clearly.- exporter = "" + exporter = Nonepackages/jumpstarter/jumpstarter/config/client.py (7)
86-93: Consider avoiding the built-infiltername.
Usingfilteras a parameter name overshadows the built-in functionfilter. Although not critical, renaming it (e.g.,filter_expr) might improve clarity.def list_leases( self, page_size: int | None = None, page_token: str | None = None, - filter: str | None = None, + filter_expr: str | None = None, ): with start_blocking_portal() as portal: - return portal.call(self.list_leases_async, page_size, page_token, filter) + return portal.call(self.list_leases_async, page_size, page_token, filter_expr)
95-102: Optionally handle exceptions or validation forselectorandduration.
While the synchronous method simply delegates tocreate_lease_async, consider validating parameters here or catching known exceptions to provide more user-friendly feedback before raising.
103-109: Ensure consistent error handling.
Like the create_lease pattern, consider either returning a status or error message after the delete request completes, or continuing to rely solely on exceptions.
110-112: Add type annotation forname.
For clarity and consistency with other methods, annotatename: strin the signature.-def update_lease(self, name, duration: timedelta): +def update_lease(self, name: str, duration: timedelta): with start_blocking_portal() as portal: return portal.call(self.update_lease_async, name, duration)
135-149: Validate the selector format to prevent ValueError.
Assess whetherselectormight be empty or missing an=character. Currently,term.split("=")can trigger errors. You may gracefully handle malformed selectors or log a clear error.match_labels = dict([term.split("=") for term in selector.split(",")]) +if "" in match_labels.keys(): + # Provide a user-friendly error or ignore blank terms + ...
179-190: Consider renamingfilterto avoid overshadowing the built-in.
As withlist_leases, renaming it (e.g.,filter_expr) can prevent confusion.
191-194: Add a type annotation for thenameargument.
For consistency withdelete_lease_asyncand other methods, usename: str.-async def update_lease_async(self, name, duration: timedelta): +async def update_lease_async(self, name: str, duration: timedelta): svc = ClientService(channel=await self.channel()) with translate_grpc_exceptions(): return await svc.UpdateLease(namespace=self.metadata.namespace, name=name, duration=duration)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.github/workflows/e2e.yaml(1 hunks)docs/source/cli/clients.md(1 hunks)docs/source/config/cli.md(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(2 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py(3 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/config.py(4 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(3 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/__init__.py(0 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2.py(0 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2_grpc.py(0 hunks)packages/jumpstarter/conftest.py(0 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(4 hunks)packages/jumpstarter/jumpstarter/client/lease.py(5 hunks)packages/jumpstarter/jumpstarter/config/client.py(6 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(0 hunks)packages/jumpstarter/jumpstarter/listener_test.py(0 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (8)
- packages/jumpstarter/jumpstarter/config/exporter_test.py
- packages/jumpstarter-protocol/jumpstarter_protocol/init.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py
- packages/jumpstarter/jumpstarter/listener_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py
- packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/client_pb2_grpc.py
- packages/jumpstarter/conftest.py
🧰 Additional context used
🪛 Ruff (0.8.2)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py
25-25: Module level import not at top of file
(E402)
25-25: google.api.annotations_pb2 imported but unused
Remove unused import: google.api.annotations_pb2
(F401)
26-26: Module level import not at top of file
(E402)
26-26: google.api.client_pb2 imported but unused
Remove unused import: google.api.client_pb2
(F401)
27-27: Module level import not at top of file
(E402)
27-27: google.api.field_behavior_pb2 imported but unused
Remove unused import: google.api.field_behavior_pb2
(F401)
28-28: Module level import not at top of file
(E402)
28-28: google.api.resource_pb2 imported but unused
Remove unused import: google.api.resource_pb2
(F401)
29-29: Module level import not at top of file
(E402)
29-29: google.protobuf.duration_pb2 imported but unused
Remove unused import: google.protobuf.duration_pb2
(F401)
30-30: Module level import not at top of file
(E402)
30-30: google.protobuf.empty_pb2 imported but unused
Remove unused import: google.protobuf.empty_pb2
(F401)
31-31: Module level import not at top of file
(E402)
31-31: google.protobuf.field_mask_pb2 imported but unused
Remove unused import: google.protobuf.field_mask_pb2
(F401)
32-32: Module level import not at top of file
(E402)
32-32: google.protobuf.timestamp_pb2 imported but unused
Remove unused import: google.protobuf.timestamp_pb2
(F401)
33-33: Module level import not at top of file
(E402)
33-33: ...v1.kubernetes_pb2 imported but unused
Remove unused import: ...v1.kubernetes_pb2
(F401)
36-36: Line too long (6089 > 120)
(E501)
43-43: Line too long (265 > 120)
(E501)
49-49: Line too long (159 > 120)
(E501)
65-65: Line too long (121 > 120)
(E501)
69-69: Line too long (142 > 120)
(E501)
71-71: Line too long (130 > 120)
(E501)
73-73: Line too long (136 > 120)
(E501)
81-81: Line too long (124 > 120)
(E501)
83-83: Line too long (130 > 120)
(E501)
91-91: Line too long (131 > 120)
(E501)
101-101: Line too long (127 > 120)
(E501)
103-103: Line too long (159 > 120)
(E501)
105-105: Line too long (163 > 120)
(E501)
107-107: Line too long (154 > 120)
(E501)
109-109: Line too long (158 > 120)
(E501)
111-111: Line too long (181 > 120)
(E501)
113-113: Line too long (182 > 120)
(E501)
115-115: Line too long (154 > 120)
(E501)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest-matrix (3.13)
🔇 Additional comments (64)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (2)
16-17: Preserve Metadata in Async Exception Decorator
Adding@wraps(func)to the async decorator is a best practice because it retains the original function's metadata (name, docstring, etc.), which aids in debugging and introspection.
33-34: Preserve Metadata in Blocking Exception Decorator
The use of@wraps(func)for the synchronous decorator similarly ensures that the wrapped function keeps its original identity—this is beneficial for logging and testing.pyproject.toml (1)
70-72: Extend Typos Configuration Properly
The addition of the[tool.typos.default.extend-words]section withser = "ser"is a purposeful improvement to prevent false positives from the typo-checker. This small enhancement aligns well with our overall configuration strategy..github/workflows/e2e.yaml (1)
19-21: Update Workflow to Use Lease-Specific References
The update replacing the action version and parameter (switching frommaintoclient-svc-lease) ensures that the E2E tests run against the correct branch/reference. This change properly aligns the workflow with the new lease-related methods under the client service.docs/source/cli/clients.md (1)
82-89: Document Updated Client Configuration Command
The documentation now correctly refers to the new commandjmp client config create my-client. The link reference and command example align with the revised CLI structure, providing clear guidance to the user.docs/source/config/cli.md (1)
96-118: Reflect Updated Client Configuration Commands
The documentation updates here clearly illustrate the new client configuration commands for switching (jmp client config use <name>), listing (jmp client config list), and deletion (jmp client config delete <name>). This update enhances consistency with the consolidated command group and improves user understanding.packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py (2)
15-16: Command structure updated for better organizationThe commands have been standardized to use a hierarchical structure with "config" as the main command group and actions like "create", "list", "use", and "delete" as subcommands. This makes the CLI more intuitive and aligned with modern CLI design practices.
36-37: Consistent command restructuring throughout testsAll command invocations have been updated to follow the new hierarchical structure. The test assertions remain unchanged, preserving the test functionality while adapting to the new command format.
Also applies to: 56-56, 62-62, 68-68, 72-72, 78-78, 82-82
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (2)
8-16: Well-defined CLI options for context and selectorThe defined options follow good CLI design practices with clear help text for the selector option, which will improve user experience.
24-34: Robust context loading with clear error messagingThe implementation correctly handles both explicit context and default client context, with appropriate error messaging when no valid context is available.
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (2)
8-13: Well-structured command group for resource deletionThe command structure follows good CLI design practices with a clear hierarchy and descriptive docstring.
15-24: Comprehensive decorator setup for lease deletion commandThe command is well-decorated with options for context, output formatting, and exception handling.
packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py (2)
11-16: Clear command group structure for resource updatesThe command structure follows good CLI design practices with a clear hierarchy and descriptive docstring.
35-53: Comprehensive output formatting optionsThe implementation supports multiple output formats (JSON, YAML, name-only, table) with appropriate formatting for each, providing good flexibility for users.
packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py (4)
16-21: LGTM - Clean command group definitionThe command group is well-structured with clear documentation on its purpose.
23-53: LGTM - Well-documented lease creation commandThe command is properly decorated with required options and includes comprehensive documentation with practical examples, making it user-friendly.
54-57: LGTM - Good parameter validationThe code correctly loads the context and validates the duration parameter using TypeAdapter, ensuring that input is properly handled before proceeding.
58-79: LGTM - Comprehensive output formattingThe output handling is well-implemented with support for multiple formats (JSON, YAML, NAME, and tabular), giving users flexibility in how they consume the command output.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (3)
6-6: LGTM - Updated importsAppropriate imports from the common module to support the updated function signature and implementation.
12-16: LGTM - Improved function signature and optionsThe command has been updated to use context and selector pattern, which is more consistent with the other commands in the CLI.
18-24: LGTM - Simplified lease acquisitionClean implementation that uses the context loading pattern and creates a lease with the selector-based metadata filter.
packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py (4)
7-13: LGTM - Well-defined selector optionThe selector option has a clear help message explaining its purpose and usage format.
15-20: LGTM - Clear command group definitionThe command group is well-structured with appropriate documentation.
22-53: LGTM - Comprehensive exporters commandThe command has proper parameter handling and output formatting with support for multiple output types. The tabular output includes the essential information (name and labels).
55-87: LGTM - Well-implemented leases commandThe command follows the same pattern as the exporters command, maintaining consistency throughout the CLI. The tabular output includes the relevant lease information.
packages/jumpstarter/jumpstarter/client/lease.py (5)
35-35: LGTM - Added namespace parameterThe namespace field addition supports the updated lease service operations.
48-48: LGTM - ClientService initializationGood initialization of the ClientService using the existing channel.
52-62: LGTM - Simplified duration handlingThe code now uses timedelta directly instead of converting through Protocol Buffers, making the implementation cleaner. The lease creation has been updated to use the ClientService.
99-99: LGTM - Updated lease retrievalCorrectly updated to use the ClientService's GetLease method with the namespace parameter.
133-133: LGTM - Updated lease deletionCorrectly updated to use the ClientService's DeleteLease method with the namespace parameter.
packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py (1)
9-13: Restructured imports align with the new CLI approach.
No issues found, and the changes are consistent with the refactored command structure.packages/jumpstarter-cli-client/jumpstarter_cli_client/config.py (1)
23-28: Adoption of a new command group “config.”
The new grouped CLI approach improves maintainability and organization of related commands.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (1)
1-143: Auto-generated Protobuf file.
Manual edits might be overwritten. It’s recommended to exclude this file from lint checks due to typical style warnings in generated code.🧰 Tools
🪛 Ruff (0.8.2)
25-25: Module level import not at top of file
(E402)
25-25:
google.api.annotations_pb2imported but unusedRemove unused import:
google.api.annotations_pb2(F401)
26-26: Module level import not at top of file
(E402)
26-26:
google.api.client_pb2imported but unusedRemove unused import:
google.api.client_pb2(F401)
27-27: Module level import not at top of file
(E402)
27-27:
google.api.field_behavior_pb2imported but unusedRemove unused import:
google.api.field_behavior_pb2(F401)
28-28: Module level import not at top of file
(E402)
28-28:
google.api.resource_pb2imported but unusedRemove unused import:
google.api.resource_pb2(F401)
29-29: Module level import not at top of file
(E402)
29-29:
google.protobuf.duration_pb2imported but unusedRemove unused import:
google.protobuf.duration_pb2(F401)
30-30: Module level import not at top of file
(E402)
30-30:
google.protobuf.empty_pb2imported but unusedRemove unused import:
google.protobuf.empty_pb2(F401)
31-31: Module level import not at top of file
(E402)
31-31:
google.protobuf.field_mask_pb2imported but unusedRemove unused import:
google.protobuf.field_mask_pb2(F401)
32-32: Module level import not at top of file
(E402)
32-32:
google.protobuf.timestamp_pb2imported but unusedRemove unused import:
google.protobuf.timestamp_pb2(F401)
33-33: Module level import not at top of file
(E402)
33-33:
...v1.kubernetes_pb2imported but unusedRemove unused import:
...v1.kubernetes_pb2(F401)
36-36: Line too long (6089 > 120)
(E501)
43-43: Line too long (265 > 120)
(E501)
49-49: Line too long (159 > 120)
(E501)
65-65: Line too long (121 > 120)
(E501)
69-69: Line too long (142 > 120)
(E501)
71-71: Line too long (130 > 120)
(E501)
73-73: Line too long (136 > 120)
(E501)
81-81: Line too long (124 > 120)
(E501)
83-83: Line too long (130 > 120)
(E501)
91-91: Line too long (131 > 120)
(E501)
101-101: Line too long (127 > 120)
(E501)
103-103: Line too long (159 > 120)
(E501)
105-105: Line too long (163 > 120)
(E501)
107-107: Line too long (154 > 120)
(E501)
109-109: Line too long (158 > 120)
(E501)
111-111: Line too long (181 > 120)
(E501)
113-113: Line too long (182 > 120)
(E501)
115-115: Line too long (154 > 120)
(E501)
packages/jumpstarter/jumpstarter/client/grpc.py (21)
4-4: Addition oftimedeltaimport.
This is appropriate for handling lease durations in the new methods.
7-7: Expanded protobuf imports.
These are essential for the lease functionality (duration and field masking).
9-9: Import ofkubernetes_pb2.
Needed for theLabelSelectortype references in lease operations.
10-10: Import ofclient_pb2andclient_pb2_grpc.
Enables the new gRPC stubs and messages for lease management.
14-21: Refactoredparse_identifierfor dynamic resource type validation.
This single function consolidates logic for clients, exporters, and leases efficiently.
25-26: Dedicated function for parsing client identifiers.
A straightforward wrapper aroundparse_identifierfor “clients.”
29-30: Dedicated function for parsing exporter identifiers.
Consistent with the approach used for client resources.
33-34: Dedicated function for parsing lease identifiers.
Further reduces duplication, ensuring the “leases” kind is enforced.
48-56: Introduction ofLeasemodel.
Defines a Pydantic model for lease data with relevant fields. Looks correct for the new lease feature.
57-60: Custom Pydantic model configuration.
Allowing arbitrary types and float-based timedelta serialization is a valid design choice.
62-65: Serializer forselectorfield.
ConvertsLabelSelectorto a dictionary, simplifying JSON output.
66-69: Serializer forconditionsfield.
Transforms each protobufConditioninto a dictionary for JSON consistency.
90-95: JSON and YAML dump methods forLease.
Providing multiple export formats is beneficial. No issues found.
115-118: Introduction ofLeaseListmodel.
Mirrors theExporterListstructure, enabling consolidated list functionality for leases.
119-124: Deserialization logic forLeaseList.
Aggregates multiple lease entries and stores the next page token as expected.
126-131: Export methods forLeaseList.
Enables easy JSON/YAML output with consistent formatting.
167-173:GetLeasemethod inClientService.
Fetches a single lease by name. This implementation appears correct.
175-191:ListLeasesmethod inClientService.
Lists leases with pagination and optional filtering. Matches the established pattern for listing.
193-213:CreateLeasemethod inClientService.
Constructs the creation request using durations and label selectors. Straightforward code flow.
214-237:UpdateLeasemethod inClientService.
Applies a field mask for partial updates, focusing onduration. Good approach.
238-243:DeleteLeasemethod inClientService.
Implements a direct gRPC call for lease removal. Looks clear.packages/jumpstarter/jumpstarter/config/client.py (4)
3-3: Import looks good.
No immediate concerns regarding importingtimedeltafromdatetime. This addition aligns with the new lease duration functionality.
150-157: No return value fromdelete_lease_async.
This is fine if you intend deletion to be a fire-and-forget call. Otherwise, consider returning a status or confirmation message.
197-199: Release lease method updated correctly.
Switching from a controller reference toClientServiceis a consistent approach with the other methods.
218-218: Reusing the passedlease_name.
Passinglease_nameto theLeaseconstructor aligns well with the new approach. Ensure that external code expects this naming behavior.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2_grpc.py (6)
1-3: Auto-generated file header recognized.
This file is likely generated by the gRPC Python plugin. Avoid manual edits unless absolutely necessary.
5-7: Protobuf imports appear standard.
All imports for protobuf messages seem correctly referenced.
9-52:ClientServiceStubclass is correctly generated.
No concerns; standard gRPC stub definitions are in place.
55-99:ClientServiceServiceris unimplemented.
Methods raiseNotImplementedError, which is acceptable if the server-side functionality is not yet ready.
101-143:add_ClientServiceServicer_to_servermethod setup is correct.
The registration of RPC method handlers appears consistent with best practices.
145-337:ClientServicestatic methods are standard experimental gRPC calls.
No issues. If you plan to implement them, ensure the service addresses are correct and integrated properly.
packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/config.py (1)
16-99: Consider updating function names to match command namesWhile the CLI reorganization is well-implemented, there's a minor inconsistency between function names and their associated commands. For example, the
create_exporter_configfunction is now associated with thecreatecommand. Consider renaming these functions to match their command names for better code maintainability.- def create_exporter_config(alias, namespace, name, endpoint, token, output: PathOutputType): + def create(alias, namespace, name, endpoint, token, output: PathOutputType): - def delete_exporter_config(alias, output: PathOutputType): + def delete(alias, output: PathOutputType): - def edit_exporter_config(alias): + def edit(alias): - def list_exporter_configs(output: OutputType): + def list(output: OutputType):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py(2 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/config.py(4 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (13)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py (6)
15-16: Command structure update looks goodThe command format has been updated to follow a more structured, hierarchical approach with
config createinstead of the previous flat command structure. This change is consistent with the broader CLI restructuring mentioned in the PR summary.
34-35: Consistent command structure in duplicate test caseThe duplicate exporter test case correctly uses the same updated command structure, maintaining consistency throughout the test suite.
51-51: Interactive test properly updatedThe interactive test correctly uses the new command structure while maintaining the same input format. This ensures that both interactive and non-interactive modes are properly tested with the new command hierarchy.
56-56: List command properly updatedThe command to list exporters has been updated to use the new hierarchical structure, ensuring consistency with the other command changes.
62-62: Delete command properly updatedThe command for deleting exporters has been updated to match the new command structure. The test still validates the same behavior as before.
66-66: List command consistently updatedThe second list command is also properly updated, maintaining consistency across the test file.
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/__init__.py (2)
7-7: Good module organizationImporting the
configcommand from a dedicated module improves code organization and maintains separation of concerns. This aligns with the command restructuring approach seen in the test file.
22-22: Properly registered config commandThe config command is correctly added to the exporter command group. This restructuring consolidates related commands (create, list, delete) under a common namespace, improving the CLI's organization and discoverability.
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/config.py (5)
16-21: Good command grouping structure implementationAdding a dedicated command group for config-related operations improves the CLI organization and creates a more intuitive hierarchical command structure. The descriptive docstring clearly communicates the purpose of this command group.
23-23: Command refactoring looks goodChanging from a standalone command to a subcommand under the 'config' group simplifies the command structure. Users will now use
config createinstead ofcreate-config.
51-51: Command refactoring looks goodThe delete command has been properly refactored under the config group. This change is consistent with the overall CLI reorganization pattern.
65-65: Command refactoring looks goodThe edit command has been properly refactored under the config group, maintaining consistency with the other command changes.
76-76: Command refactoring looks goodThe list command has been properly refactored under the config group, completing the consistent reorganization of all configuration commands.
NickCao
force-pushed
the
client-svc-lease
branch
4 times, most recently
from
935eae6 to
19cb070
Compare
NickCao
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (2)
27-29: Enhance selector_to_labels for error handling and edge cases.The current implementation could be more robust in handling edge cases like empty selectors, malformed selectors, or selectors without equals signs.
def selector_to_labels(selector: str): # TODO: support complex selectors (e.g. !=) - return dict([term.split("=") for term in selector.split(",")]) + if not selector: + return {} + + labels = {} + for term in selector.split(","): + if not term: + continue + if "=" not in term: + raise ValueError(f"Invalid selector term: {term}. Expected format: key=value") + key, value = term.split("=", 1) # Split on first equals sign only + labels[key] = value + return labels
48-58: Enhance DurationParamType with more descriptive error messages.The current error message doesn't provide users with guidance on expected duration formats.
def convert(self, value, param, ctx): if isinstance(value, timedelta): return value try: return TypeAdapter(timedelta).validate_python(value) except ValueError: - self.fail(f"{value!r} is not a valid duration", param, ctx) + self.fail( + f"{value!r} is not a valid duration. " + f"Expected formats: '1d', '3h', '30m', '1d12h', etc.", + param, + ctx + )packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (1)
27-42: Clarify behavior when multiple options are provided.
Currently, if both--allandnameorselectorare supplied, the code processes only the first condition. Consider validating these arguments or clarifying that giving more than one option is not supported.packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py (2)
15-45: Provide feedback when no exporters are found.
Ifexporters.exportersis empty, consider displaying a user-friendly message (e.g., "No exporters found") to inform the user rather than printing nothing or an empty table.
47-78: Provide feedback when no leases are found.
Ifleases.leasesis empty, consider echoing a message like "No leases found" so users know the command completed successfully but returned zero resources.packages/jumpstarter/jumpstarter/config/client.py (8)
86-89: Validate filters and confirm usage.
Thelist_leasesmethod accepts afilterparameter but does not enforce or validate its usage. Consider checking for non-empty strings or clarifying whether an empty filter is allowed.
90-97: Consider input validation for lease creation.
Check for negative or zero durations or empty selectors before creating a lease. Doing so helps avoid unintended lease configurations.
98-104: Ensurenameis validated for deletion.
Ifnameis empty or invalid, the call may fail with a less descriptive error. Validatingnameupfront could improve user feedback.
105-107: Reinforce parameter checks for lease updates.
Similar to lease creation, ensuredurationis valid (not negative) and thatnamemeets any required constraints.
128-139: Add error handling for invalid arguments increate_lease_async.
Sinceselectoranddurationdirectly govern lease creation, consider additional validation or specialized error messages.
140-146: Surface lease-not-found scenarios with clearer exception handling.
A specific lease-not-found flow would help provide a better error message than a generic exception, guiding the user to correct the lease name.
168-172: Confirm correct filter usage forlist_leases_async.
Unclear if a None or empty string is accepted as a valid filter. Consider clarifying or validating to avoid ambiguous server-side behavior.
173-176: Validate inputs forupdate_lease_async.
Similar to creation, consider verifying lease existence and ensuringdurationis positive, improving reliability and user feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/e2e.yaml(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(2 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(3 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)
💤 Files with no reviewable changes (2)
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/e2e.yaml
- packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
🔇 Additional comments (11)
packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py (2)
23-28: Implement user-friendly duration validation error handling.The implementation uses a custom
DURATIONtype but doesn't provide additional error handling or clear guidance on expected duration formats when validation fails.While the
DurationParamTypeincommon.pydoes catchValueErrorand provide a basic error message, consider adding:
- A more descriptive error message with examples of valid formats
- Documentation in the help text for the duration option
-@click.option("--duration", "duration", type=DURATION, required=True) +@click.option("--duration", "duration", type=DURATION, required=True, + help="Duration of the lease (e.g., '1d', '3h', '30m', '1d12h')")
28-49: LGTM: Well-structured command implementation.The implementation of the
update_leasecommand follows a consistent pattern with other CLI commands, providing appropriate output formatting options and error handling.packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (1)
32-46: LGTM: Well-designed ClientParamType parameter type.The implementation handles different input scenarios appropriately, including conversion from existing objects, loading from configuration files, and falling back to default configurations.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (4)
6-6: LGTM: Clean implementation of imports from common module.The import statement brings in the necessary decorators and utility functions from the common module, which improves code organization.
13-15: LGTM: Good use of common decorators for configuration and selector.Replacing direct parameter handling with decorators from the common module simplifies the code and ensures consistent behavior across commands.
16-16: LGTM: Updated function signature is cleaner and more specific.The new signature with
config, selector: str, lease_nameparameters is clearer and more intuitive than the previous version.
21-23: LGTM: Simplified lease acquisition with MetadataFilter.The updated implementation simplifies lease acquisition by leveraging the common
selector_to_labelsfunction for converting the selector string to labels.packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py (3)
22-52: LGTM: Comprehensive command documentation and structure.The command implementation provides excellent documentation including a clear description, explanation of use cases, and practical examples.
25-25: Implement user-friendly duration validation error handling.Similar to the update command, the duration option would benefit from more descriptive help text explaining the expected formats.
-@click.option("--duration", "duration", type=DURATION, required=True) +@click.option("--duration", "duration", type=DURATION, required=True, + help="Duration of the lease (e.g., '1d', '3h', '30m', '1d12h')")
54-74: LGTM: Clean implementation with consistent output formatting.The implementation follows the established pattern for output formatting, providing flexible options (JSON, YAML, NAME, or tabular) based on user preference.
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (1)
44-50: Non-existent lease handling duplicated comment.
This logic does not account for the case when the lease does not exist. Although exceptions will be translated by@handle_exceptions, providing a custom, user-friendly message (e.g., "Lease not found") could improve user experience.
|
e2e side changes: jumpstarter-dev/jumpstarter-e2e#9 |
NickCao
force-pushed
the
client-svc-lease
branch
from
e10765a to
a2bc435
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (2)
44-50: Add confirmation prompt for dangerous operationsDeletion is a potentially dangerous operation, especially when using
--allor selectors that might match multiple leases. Consider adding a confirmation prompt unless a--forceflag is provided.@delete.command(name="leases") @opt_config @click.argument("name", required=False) @opt_selector @click.option("--all", "all", is_flag=True) +@click.option("--force", "force", is_flag=True, help="Skip confirmation prompt") @opt_output_name_only @handle_exceptions -def delete_leases(config, name: str, selector: str | None, all: bool, output: OutputType): +def delete_leases(config, name: str, selector: str | None, all: bool, force: bool, output: OutputType): # ... # Add before the deletion loop + if not force and len(names) > 1: + click.confirm(f"Are you sure you want to delete {len(names)} leases?", abort=True) + elif not force and len(names) == 1: + click.confirm(f"Are you sure you want to delete lease '{names[0]}'?", abort=True)
50-50: Use f-strings for improved readabilityConsider using f-strings instead of the old-style string formatting for better readability.
- click.echo('lease "{}" deleted'.format(name)) + click.echo(f'lease "{name}" deleted')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/e2e.yaml(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(9 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(2 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)
💤 Files with no reviewable changes (2)
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py
🚧 Files skipped from review as they are similar to previous changes (12)
- .github/workflows/e2e.yaml
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/init.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py
- packages/jumpstarter/jumpstarter/config/client.py
🧰 Additional context used
🧬 Code Definitions (1)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (3)
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (1) (1)
selector_to_labels(27:29)packages/jumpstarter/jumpstarter/common/utils.py (1) (1)
launch_shell(83:104)packages/jumpstarter/jumpstarter/config/client.py (1) (1)
lease(64:67)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
🔇 Additional comments (6)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (3)
6-6: Improved imports with reusable componentsThe addition of decorator imports from a common module demonstrates good code organization and reusability. The
selector_to_labelsutility function centralizes label parsing logic, which helps maintain consistency across the codebase.
12-16: Good refactoring using decoratorsThe refactoring to use decorators
@opt_configand@opt_selector_simpleimproves code organization by moving the parameter handling logic outside the function body. This follows the principle of separation of concerns and makes the code more maintainable.
21-23: Improved lease creation with structured metadata filteringThe lease creation logic has been improved by using a more structured approach with
MetadataFilterand theselector_to_labelsfunction. This change makes the code more robust and easier to understand.packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (3)
15-25: Good command structure with clear documentationThe command structure follows CLI best practices with a clear hierarchy and good documentation. The use of decorators for common functionality keeps the code DRY and maintainable.
29-42: Good implementation of multiple selection methodsThe implementation provides three flexible ways to select leases (by name, selector, or all), with appropriate validation to ensure at least one method is specified.
44-50: Non-existent lease handling neededThe current implementation doesn't handle cases where a lease doesn't exist. This can lead to confusing error messages for users.
Consider adding specific error handling for non-existent leases. Let's check how lease deletion is implemented:
#!/bin/bash # Check how delete_lease is implemented rg "def delete_lease" -A 10
NickCao
force-pushed
the
client-svc-lease
branch
from
a2bc435 to
cfd59ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (1)
44-50:⚠️ Potential issueNon-existent Lease Handling Needed
Although exception handling is applied via the
@handle_exceptionsdecorator, the deletion flow does not specifically distinguish when a lease doesn't exist. This can lead to a generic exception without a clear, user-friendly message.Consider updating the deletion logic to verify if a lease exists before attempting deletion, or catch and handle the specific exception for non-existent leases:
for name in names: - config.delete_lease(name=name) + try: + config.delete_lease(name=name) + match output: + case OutputMode.NAME: + click.echo(name) + case _: + click.echo('lease "{}" deleted'.format(name)) + except Exception as e: + if "not found" in str(e).lower(): + click.echo('lease "{}" not found'.format(name), err=True) + else: + raise - match output: - case OutputMode.NAME: - click.echo(name) - case _: - click.echo('lease "{}" deleted'.format(name))
🧹 Nitpick comments (2)
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (2)
29-42: Consider improving the error message clarityThe error message uses "NAME" to refer to the positional argument, which doesn't match how a user would typically see it in help documentation or usage examples.
- raise click.ClickException("One of NAME, --selector or --all must be specified") + raise click.ClickException("One of <lease_name>, --selector or --all must be specified")
36-41: Consider adding a confirmation prompt for destructive operationsWhen using the
--allflag, multiple leases could be deleted at once without any confirmation. This could be risky, especially in production environments.Consider adding a confirmation prompt for destructive operations, especially when using
--all:elif all: leases = config.list_leases(filter=None) + count = 0 for lease in leases.leases: if lease.client == config.metadata.name: names.append(lease.name) + count += 1 + + if count > 0 and not click.confirm(f"Are you sure you want to delete {count} leases?"): + click.echo("Operation cancelled") + return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/e2e.yaml(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(9 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(2 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)
💤 Files with no reviewable changes (2)
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py
🚧 Files skipped from review as they are similar to previous changes (13)
- .github/workflows/e2e.yaml
- packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/init.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
🔇 Additional comments (6)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (3)
6-6: Improved import organization using dedicated common moduleThe new import from
.commonbrings in helper functions and decorators that standardize parameter handling across CLI commands, enhancing consistency and reducing code duplication.
12-16: Well-structured decorator pattern for parameter handlingThe function now uses
@opt_configand@opt_selector_simpledecorators which standardize how configuration and selector parameters are handled. This approach:
- Reduces boilerplate code in each command implementation
- Ensures consistent parameter handling across commands
- Makes the function signature clearer
The use of decorators is a good practice for CLI applications as it centralizes common parameter processing logic.
16-27: Improved lease management with context managersThe updated implementation:
- Uses a cleaner context manager pattern for lease management
- Centralizes label handling through the
selector_to_labelsfunction- Focuses on the core functionality (spawning a shell) rather than configuration handling
This refactoring aligns with the "kubectl style CLI" objective, making the code more maintainable and consistent with modern CLI patterns.
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (3)
8-14: Good command group structure following CLI conventionsThe command group organization with
deleteas the main command and subcommands for specific resources follows well-established CLI conventions (similar to kubectl, git, etc.). This structure allows for intuitive command navigation and future extensibility to delete other resource types.
15-22: Well-structured command with appropriate decoratorsThe command definition:
- Uses a consistent naming convention (
leasesrather than legacylease)- Leverages decorators for common parameter handling
- Supports multiple selection methods (by name, selector, or all)
- Includes output formatting options
This structure aligns well with the kubectl-style CLI pattern and promotes code reuse through decorators.
46-50: Good use of pattern matching for output formattingThe use of Python 3.10+ pattern matching for output formatting makes the code clean and easy to understand. It also provides a clear path for adding additional output modes in the future.
Note: Ensure that your project's minimum Python version requirement is documented if you're using Python 3.10+ features.
NickCao
force-pushed
the
client-svc-lease
branch
from
c2388ab to
d837d39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (1)
44-46:⚠️ Potential issueImprove handling of non-existent leases.
The function doesn't handle the scenario where a lease doesn't exist, which was already flagged in a previous review.
Add explicit error handling for non-existent leases:
for name in names: - config.delete_lease(name=name) + try: + config.delete_lease(name=name) + except Exception as e: + if "not found" in str(e).lower(): + click.echo(f'Error: Lease "{name}" not found', err=True) + continue + raise match output:
🧹 Nitpick comments (5)
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (2)
27-29: Implement the TODO for complex selector support.The current implementation only supports simple equality checks. As noted in the TODO comment, supporting more complex selectors would enhance functionality.
Consider implementing support for inequality operators as mentioned in the TODO:
def selector_to_labels(selector: str): # TODO: support complex selectors (e.g. !=) - return dict([term.split("=") for term in selector.split(",")]) + result = {} + for term in selector.split(","): + if "!=" in term: + key, value = term.split("!=", 1) + result[key] = {"operator": "!=", "value": value} + elif "==" in term: + key, value = term.split("==", 1) + result[key] = {"operator": "==", "value": value} + elif "=" in term: + key, value = term.split("=", 1) + result[key] = {"operator": "=", "value": value} + return result
55-58: Improve error messaging for duration validation.The current error message for invalid duration inputs is not very helpful for users.
Enhance the error message with examples of valid formats:
try: return TypeAdapter(timedelta).validate_python(value) except ValueError: - self.fail(f"{value!r} is not a valid duration", param, ctx) + self.fail(f"{value!r} is not a valid duration. Examples of valid formats: '1d', '2h30m', '90s'", param, ctx)packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (3)
15-22: Consider aligning command name with function name.The function name
delete_leasesdoes not match the command name "leases", which could lead to confusion in maintenance.Either rename the function to match the command name or update the command name to match the function:
-@delete.command(name="leases") +@delete.command(name="lease") def delete_leases(config, name: str, selector: str | None, all: bool, output: OutputType):or
@delete.command(name="leases") -def delete_leases(config, name: str, selector: str | None, all: bool, output: OutputType): +def leases(config, name: str, selector: str | None, all: bool, output: OutputType):
29-41: Consider optimizing lease filtering.The current implementation fetches all leases and then filters them in memory, which might not be efficient for large numbers of leases.
Consider passing the filter to the backend service to reduce data transfer and processing:
elif selector: - leases = config.list_leases(filter=selector) - for lease in leases.leases: - if lease.client == config.metadata.name: - names.append(lease.name) + leases = config.list_leases(filter=f"{selector},client={config.metadata.name}") + names.extend([lease.name for lease in leases.leases]) elif all: - leases = config.list_leases(filter=None) - for lease in leases.leases: - if lease.client == config.metadata.name: - names.append(lease.name) + leases = config.list_leases(filter=f"client={config.metadata.name}") + names.extend([lease.name for lease in leases.leases])
46-50: Enhanced output formatting could improve UX.The current output formatting is limited to either the name or a simple deletion message.
Consider adding more comprehensive output options, such as JSON or YAML format for programmatic consumption:
match output: case OutputMode.NAME: click.echo(name) + case OutputMode.JSON: + click.echo(json.dumps({"name": name, "status": "deleted"})) + case OutputMode.YAML: + click.echo(yaml.dump({"name": name, "status": "deleted"})) case _: click.echo('lease "{}" deleted'.format(name))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/e2e.yaml(1 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(9 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(2 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)
💤 Files with no reviewable changes (2)
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py
🚧 Files skipped from review as they are similar to previous changes (11)
- .github/workflows/e2e.yaml
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/init.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py
- packages/jumpstarter/jumpstarter/config/client.py
🧰 Additional context used
🧬 Code Definitions (3)
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (1)
packages/jumpstarter/jumpstarter/config/client.py (2) (2)
ClientConfigV1Alpha1(38-309)load(255-260)
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1) (1)
handle_exceptions(30-44)packages/jumpstarter/jumpstarter/config/client.py (4) (4)
delete(303-309)list_leases(87-89)lease(65-68)delete_lease(99-104)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (2)
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (1) (1)
selector_to_labels(27-29)packages/jumpstarter/jumpstarter/config/client.py (1) (1)
lease(65-68)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (8)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (3)
20-22: Well-structured selector class addition.The introduction of
V1Alpha1LeaseSelectorwithmatch_labelsfield properly aligns with Kubernetes API conventions, where selectors typically have amatchLabelsproperty.
27-27: Appropriate type update for selector field.Changing the selector field type from a direct dictionary to the structured
V1Alpha1LeaseSelectorclass improves type safety and makes the code more maintainable.
75-75: Updated from_dict implementation for new selector structure.The
from_dictmethod has been properly updated to handle the new selector structure, ensuring backward compatibility with existing data.packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (2)
11-16: Good option definition with clear help text.The
opt_selectoroption is well-defined, with clear help text explaining the supported operators and format.
18-24: Clear distinction between selector options.Making
opt_selector_simplerequired and limiting its operation to equality checks only is a good design choice that helps prevent user errors.packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (2)
13-14: Effective use of decorator pattern.Using the
@opt_configand@opt_selector_simpledecorators simplifies the function signature and makes the code more maintainable.
21-23: Clean implementation of selector conversion.The use of
selector_to_labels(selector)to convert the selector string into a dictionary for the metadata filter is clean and maintainable.packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (1)
8-13: Clear group command structure.The
deletecommand group is well-defined with clear docstring explaining its purpose.
NickCao
force-pushed
the
client-svc-lease
branch
from
f3d9635 to
fa18287
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (1)
75-75: Add error handling for missing dictionary keys.The current implementation assumes that
dict["spec"]["selector"]["matchLabels"]will always exist, but this could raise aKeyErrorif any part of this path is missing. Consider adding defensive error handling.- selector=V1Alpha1LeaseSelector(match_labels=dict["spec"]["selector"]["matchLabels"]), + selector=V1Alpha1LeaseSelector( + match_labels=dict["spec"].get("selector", {}).get("matchLabels", {}) + ),packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py (1)
38-48: Consider enhancing lease representation in table output.The duration is currently displayed as total seconds, which may not be the most user-friendly format. Consider converting it to a more readable format like "1d 2h 30m" for better readability.
- "DURATION": str(lease.duration.total_seconds()), + "DURATION": str(lease.duration),packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (1)
27-29: Implement more robust selector parsing.The current implementation has a TODO to support complex selectors. As noted, it only handles simple equality selectors and would fail on more complex expressions.
Consider implementing support for additional operators as mentioned in the TODO:
def selector_to_labels(selector: str): # TODO: support complex selectors (e.g. !=) - return dict([term.split("=") for term in selector.split(",")]) + if not selector: + return {} + + result = {} + terms = selector.split(",") + for term in terms: + if "=" not in term: + continue + key, value = term.split("=", 1) + result[key] = value + return result
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py(9 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(2 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py(0 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py(1 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py(1 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(3 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/__init__.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(4 hunks)
💤 Files with no reviewable changes (2)
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_exporter.py
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/get_test.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli-admin/jumpstarter_cli_admin/print.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/init.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/create.py
- packages/jumpstarter-kubernetes/jumpstarter_kubernetes/test_leases.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/get.py
- packages/jumpstarter/jumpstarter/config/client.py
🧰 Additional context used
🧠 Learnings (1)
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (1)
Learnt from: NickCao
PR: jumpstarter-dev/jumpstarter#338
File: packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py:39-43
Timestamp: 2025-03-20T13:50:36.218Z
Learning: In click/asyncclick, when a parameter is not provided (None), the conversion function is skipped entirely rather than being called with None. The `isinstance(value, bool)` check in ClientParamType is used as a workaround to handle cases where no client configuration is explicitly provided.
🧬 Code Definitions (3)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (2)
packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (1) (1)
selector_to_labels(27-29)packages/jumpstarter/jumpstarter/config/client.py (1) (1)
lease(65-68)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/json.py (1) (1)
JsonBaseModel(5-14)
packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (2)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1) (1)
handle_exceptions(30-44)packages/jumpstarter/jumpstarter/config/client.py (4) (4)
delete(303-309)list_leases(87-89)lease(65-68)delete_lease(99-104)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
🔇 Additional comments (12)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (2)
20-22: Good addition of the structured V1Alpha1LeaseSelector class.The introduction of this class aligns with Kubernetes API conventions, where selectors typically have a
matchLabelsfield. This provides better type safety and clearer structure than the previous dictionary approach.
27-27: LGTM on updating the selector field type.Properly updating the field type to use the new
V1Alpha1LeaseSelectorclass maintains type consistency throughout the codebase.packages/jumpstarter-cli-client/jumpstarter_cli_client/update.py (2)
1-49: LGTM! Clean implementation of the update lease command.The implementation follows the kubectl-style CLI pattern with proper command structure, argument handling, and output formatting options. The code is well-organized with clear separation between command definition and execution logic.
23-29: Duration validation is improved with error handling.The implementation now uses the custom
DURATIONtype which includes error handling for invalid duration formats as defined in common.py.packages/jumpstarter-cli-client/jumpstarter_cli_client/common.py (2)
32-46: Use of boolean check for default config is well-reasoned.As noted in the retrieved learning, the
isinstance(value, bool)check is necessary because click skips the conversion function entirely when a parameter is not provided (None).
48-59: LGTM! Good error handling in DurationParamType.The implementation properly handles validation errors and provides clear error messages when invalid duration formats are encountered.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (3)
6-6: LGTM! Clean import of common utilities.Efficiently imports the necessary decorators and functions from the common module.
13-16: LGTM! Properly applied new command structure.The function signature has been effectively updated to use the new command structure with
@opt_configand@opt_selector_simpledecorators, simplifying the implementation by encapsulating configuration and selector handling.
21-23: LGTM! Correctly uses the new selector handling.The implementation properly uses
selector_to_labels(selector)for label filtering, aligning with the new approach to selector handling.packages/jumpstarter-cli-client/jumpstarter_cli_client/delete.py (3)
1-51: LGTM! Well-structured delete command implementation.The implementation follows the kubectl-style CLI pattern with a clear group and subcommand structure. The code provides flexible options for specifying which leases to delete (by name, selector, or all) and handles output formatting appropriately.
27-42: LGTM! Clear and robust parameter handling.The code correctly prioritizes different specification methods and raises a clear error message when none are provided. The logic for filtering leases by the current client is particularly valuable.
44-50: Add explicit handling for non-existent leases.The current implementation doesn't specifically handle the case where a lease doesn't exist, which could result in generic error messages.
Consider adding explicit handling:
for name in names: - config.delete_lease(name=name) + try: + config.delete_lease(name=name) + match output: + case OutputMode.NAME: + click.echo(name) + case _: + click.echo('lease "{}" deleted'.format(name)) + except Exception as e: + if "not found" in str(e).lower(): + click.echo('lease "{}" not found'.format(name), err=True) + else: + raise - match output: - case OutputMode.NAME: - click.echo(name) - case _: - click.echo('lease "{}" deleted'.format(name))
3 participants
See jumpstarter-dev/jumpstarter-protocol#17
Summary by CodeRabbit
New Features
Refactor