cli: support lease transfer #765
Conversation
📝 WalkthroughWalkthroughAdds lease-owner transfer support via a new CLI option Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Jumpstarter CLI
participant Config as ClientConfigV1Alpha1
participant Service as ClientService (gRPC)
Note over CLI,Config: User runs update with --to-client (optional) and/or duration/begin_time
User->>CLI: run update --name ... [--duration/--begin-time/--to-client]
CLI->>Config: update_lease(name, duration?, begin_time?, client?=namespace/to-client)
Config->>Service: UpdateLease(name, duration?, begin_time?, client?)
alt client provided
Note right of Service: service sets lease.client and adds "client" to field mask
else client not provided
Note right of Service: service updates only provided fields (duration/begin_time)
end
Service-->>Config: UpdateLease response
Config-->>CLI: formatted response / status
CLI-->>User: display result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mangelajo
added
usability
user-requested
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-cli/jumpstarter_cli/update.py(1 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(2 hunks)packages/jumpstarter/jumpstarter/config/client.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter-cli/jumpstarter_cli/update.py
🧠 Learnings (3)
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter-cli/jumpstarter_cli/update.py
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.788Z
Learning: In the Jumpstarter client lease model (packages/jumpstarter/jumpstarter/client/grpc.py), `effective_duration` represents the elapsed time for an active lease so far, not the total duration. To calculate expected release time, use `effective_begin_time + duration` (where `duration` is the configured/requested duration), not `effective_begin_time + effective_duration`.
Applied to files:
packages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter-cli/jumpstarter_cli/update.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-cli/jumpstarter_cli/update.py
🧬 Code graph analysis (2)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
ClientService(316-462)UpdateLease(411-454)
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
packages/jumpstarter/jumpstarter/config/client.py (2)
update_lease(244-252)lease(135-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
417-417: LGTM: Parameter addition follows existing pattern.The
clientparameter is added consistently with the existing optional parameters (durationandbegin_time), maintaining a clean and predictable API.packages/jumpstarter/jumpstarter/config/client.py (1)
244-252: LGTM: Clean pass-through implementation.The
update_leasemethod correctly forwards theclientparameter to the underlying gRPC service, maintaining consistency with the existing parameter handling pattern.packages/jumpstarter-cli/jumpstarter_cli/update.py (5)
25-30: LGTM: Well-documented CLI option.The
--clientoption is properly defined with clear help text that specifies the expected identifier format. This helps users understand how to use the lease transfer feature correctly.
33-35: LGTM: Function signature updated correctly.The function signature properly includes the new
clientparameter with appropriate type annotation.
39-44: LGTM: Comprehensive documentation.The docstring is well-written and provides clear guidance on:
- What fields can be updated
- The requirement for at least one parameter
- The expected format for client identifiers with a concrete example
48-49: LGTM: Validation logic correctly updated.The validation ensures at least one of the three parameters is provided, and the error message clearly communicates the requirement to users.
51-51: LGTM: Parameter correctly forwarded.The
clientparameter is properly passed through to the underlyingconfig.update_leasecall.
| if client is not None: | ||
| lease_pb.client = client | ||
| update_fields.append("client") | ||
|
|
||
| if not update_fields: | ||
| raise ValueError("At least one of duration or begin_time must be provided") | ||
| raise ValueError("At least one of duration, begin_time, or client must be provided") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for client identifier format validation patterns
# Search for parse_client_identifier usage
echo "=== Searching for parse_client_identifier usage ==="
rg -n "parse_client_identifier" --type py -C 3
# Search for validation patterns with client identifiers
echo -e "\n=== Searching for namespaces.*clients pattern ==="
rg -n "namespaces.*clients" --type py -C 2
# Search for client field validation in grpc file
echo -e "\n=== Searching for client validation in grpc.py ==="
rg -n "client.*=" packages/jumpstarter/jumpstarter/client/grpc.py -C 1
# Search for CLI help text
echo -e "\n=== Checking CLI help text ==="
rg -n "namespaces.*client" packages/jumpstarter-cli/jumpstarter_cli/update.py -C 2Repository: jumpstarter-dev/jumpstarter
Length of output: 3850
🏁 Script executed:
#!/bin/bash
# Find parse_identifier function implementation
echo "=== Searching for parse_identifier function definition ==="
rg -n "def parse_identifier" packages/jumpstarter/jumpstarter/client/grpc.py -A 10
# Also check what happens when parse_identifier is called with invalid format
echo -e "\n=== Searching for error handling around client parsing ==="
rg -n "parse_client_identifier\|ValueError\|InvalidArgument" packages/jumpstarter/jumpstarter/client/grpc.py -B 2 -A 2Repository: jumpstarter-dev/jumpstarter
Length of output: 801
🏁 Script executed:
#!/bin/bash
# Check CLI validation for client parameter
echo "=== Checking CLI update.py for validation ==="
cat packages/jumpstarter-cli/jumpstarter_cli/update.py | head -100
# Check if there's any validation of the client parameter before calling grpc
echo -e "\n=== Searching for client parameter validation in update.py ==="
rg -n "parse_client\|ValueError\|client.*format" packages/jumpstarter-cli/jumpstarter_cli/update.py -C 2Repository: jumpstarter-dev/jumpstarter
Length of output: 1881
🏁 Script executed:
#!/bin/bash
# Find the config.update_lease implementation
echo "=== Searching for config.update_lease implementation ==="
rg -n "def update_lease" packages/jumpstarter-cli/ -A 5
# Also check the flow from CLI to grpc
echo -e "\n=== Checking where parse_client_identifier is used ==="
rg -n "parse_client_identifier" packages/jumpstarter/jumpstarter/client/grpc.py -B 3 -A 3Repository: jumpstarter-dev/jumpstarter
Length of output: 1175
Add client identifier format validation before sending to server.
The client parameter is passed directly to the gRPC request without format validation. While parse_identifier() already exists in this file to validate the format (namespaces/{namespace}/clients/{client}), it's currently only used to parse server responses. Consider either validating the client parameter using parse_identifier(client, "clients") before the gRPC call, or confirm that the server-side validation in the dependent controller PR (jumpstarter-controller#199) comprehensively validates both the format and authorization for lease transfers.
🤖 Prompt for AI Agents
In packages/jumpstarter/jumpstarter/client/grpc.py around lines 437-442, the
client value is assigned to the gRPC request without validating its format; call
parse_identifier(client, "clients") before setting lease_pb.client and appending
"client" to update_fields, and if parse_identifier raises/returns an invalid
result then raise a ValueError with a clear message (e.g., "client must be a
valid identifier: namespaces/{namespace}/clients/{client}") so only validated
client identifiers are sent to the server.
| "--client", | ||
| type=str, | ||
| default=None, | ||
| help="Transfer lease to a different client (format: namespaces/{namespace}/clients/{client})", |
There was a problem hiding this comment.
We don't support cross namespace on the server anymore, it was removed after a few calls on the community meeting, and to make it straight forward with the operator.
We should probably just take "{client}"
There was a problem hiding this comment.
right right, i knew that 😅
| client: str | None = None, | ||
| ): | ||
| lease_pb = client_pb2.Lease( | ||
| name="namespaces/{}/leases/{}".format(self.namespace, name), |
There was a problem hiding this comment.
we should probably construct it as we do here :)
depends on jumpstarter-dev/jumpstarter-controller#199 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
lease-transfer
branch
from
09b7515 to
8ba9b40
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
437-439: Client identifier passed without format validation.The
clientvalue is assigned directly tolease_pb.clientwithout validating its format. Given thatparse_client_identifier()exists (lines 67-68) to validate thenamespaces/{namespace}/clients/{client}format, consider validating before sending to the server to fail fast with a clear error message.However, per the past review discussion, the server-side validation in the controller PR (jumpstarter-controller#199) may handle this. If server validation is comprehensive, this is acceptable—but a client-side check would improve UX by providing immediate feedback.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-cli/jumpstarter_cli/update.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter/jumpstarter/config/client.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter-cli/jumpstarter_cli/update.py
🧠 Learnings (3)
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.788Z
Learning: In the Jumpstarter client lease model (packages/jumpstarter/jumpstarter/client/grpc.py), `effective_duration` represents the elapsed time for an active lease so far, not the total duration. To calculate expected release time, use `effective_begin_time + duration` (where `duration` is the configured/requested duration), not `effective_begin_time + effective_duration`.
Applied to files:
packages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter-cli/jumpstarter_cli/update.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/grpc.py
📚 Learning: 2025-09-15T08:19:27.412Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 610
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:666-691
Timestamp: 2025-09-15T08:19:27.412Z
Learning: Bearer tokens in the jumpstarter flasher client should be validated for format, length, and character safety similar to HTTP header validation using _validate_bearer_token method.
Applied to files:
packages/jumpstarter/jumpstarter/client/grpc.py
🧬 Code graph analysis (2)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
ClientService(316-462)UpdateLease(411-454)
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
packages/jumpstarter/jumpstarter/config/client.py (2)
update_lease(244-252)lease(135-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (3)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
411-442: LGTM on the UpdateLease signature and logic.The conditional handling of the
clientparameter follows the same pattern asdurationandbegin_time, and the updated validation message is consistent.packages/jumpstarter/jumpstarter/config/client.py (1)
244-252: LGTM!The
clientparameter is correctly added to the method signature and passed through to the underlyingClientService.UpdateLeasecall, maintaining consistency with the existing parameter handling pattern.packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
55-57: LGTM on client path construction.The
client_pathis correctly constructed using the current namespace fromconfig.metadata.namespace, ensuring leases can only be transferred within the same namespace. This aligns with the past review discussion about removing cross-namespace support.
| Update the duration, begin time, and/or owner of an existing lease. | ||
| At least one of --duration, --begin-time, or --to_client must be specified. | ||
| To transfer a lease to another client in the same namespace, use the --to_client option. |
There was a problem hiding this comment.
Docstring uses underscore instead of hyphen for option name.
The docstring references --to_client (underscore) but the CLI option is --to-client (hyphen).
🔎 Proposed fix
- At least one of --duration, --begin-time, or --to_client must be specified.
+ At least one of --duration, --begin-time, or --to-client must be specified.
- To transfer a lease to another client in the same namespace, use the --to_client option.
+ To transfer a lease to another client in the same namespace, use the --to-client option.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Update the duration, begin time, and/or owner of an existing lease. | |
| At least one of --duration, --begin-time, or --to_client must be specified. | |
| To transfer a lease to another client in the same namespace, use the --to_client option. | |
| Update the duration, begin time, and/or owner of an existing lease. | |
| At least one of --duration, --begin-time, or --to-client must be specified. | |
| To transfer a lease to another client in the same namespace, use the --to-client option. |
🤖 Prompt for AI Agents
In packages/jumpstarter-cli/jumpstarter_cli/update.py around lines 44 to 47, the
docstring incorrectly references the CLI option as --to_client (underscore);
update the docstring text to use the correct option name --to-client (hyphen) so
it matches the actual CLI flag, and scan nearby docstring lines in the same
function to ensure no other option names use underscores.
| if duration is None and begin_time is None and to_client is None: | ||
| raise click.UsageError("At least one of --duration, --begin-time, or --client must be specified") |
There was a problem hiding this comment.
Error message references incorrect option name.
The error message says --client but the actual CLI option is --to-client. This inconsistency would confuse users.
🔎 Proposed fix
if duration is None and begin_time is None and to_client is None:
- raise click.UsageError("At least one of --duration, --begin-time, or --client must be specified")
+ raise click.UsageError("At least one of --duration, --begin-time, or --to-client must be specified")🤖 Prompt for AI Agents
In packages/jumpstarter-cli/jumpstarter_cli/update.py around lines 52-53 the
UsageError message references the wrong CLI option name `--client`; change the
error text to reference the actual option `--to-client` (e.g., "At least one of
--duration, --begin-time, or --to-client must be specified") so the message
matches the implemented flag and avoids user confusion.
2 participants
depends on jumpstarter-dev/jumpstarter-controller#199
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.