cli: add support for requesting custom lease name#763
cli: add support for requesting custom lease name#763bennyz merged 2 commits intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds an optional lease_id parameter to lease creation and threads it from the CLI through the config layer into the gRPC CreateLease request and lease creation code; also includes minor code formatting and test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI (create.py)
participant Config as ClientConfigV1Alpha1
participant Client as ClientService (grpc.py)
participant Server as Backend Service
Note over CLI,Server: Lease creation with optional lease_id
CLI->>Config: create_lease(selector, duration, begin_time, lease_id)
Config->>Client: CreateLease(selector, duration, begin_time, lease_id)
Client->>Server: CreateLeaseRequest { selector, duration, begin_time, lease_id }
Server-->>Client: CreateLeaseResponse (lease metadata)
Client-->>Config: response
Config-->>CLI: created lease info
Note right of CLI: CLI prints/returns lease info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
63-126: Test helpers now cover broader lease shapes
create_test_exporterandcreate_test_leaseprovide clearer, keyword-based setup for exporter/lease mocks, includingeffective_begin_time,effective_duration,begin_time,duration, andeffective_end_time. This makes the later tests easier to understand and keeps them aligned with the Lease model fields.If you want closer realism with production semantics, consider defaulting
effective_durationto a partial elapsed time wheneffective_begin_timeis set, mirroring how it behaves inLease.monitor_async. Based on learnings, effective_duration is “elapsed so far”, not total.packages/jumpstarter/jumpstarter/client/lease.py (1)
65-75: Passing lease_id=self.name cleanly supports custom lease IDsIn
_create, forwardinglease_id=self.nametoCreateLeaseallows a pre-setname(from env flag or CLI-supplied ID) to drive the requested lease ID, while the existing “no name” path still results inlease_id=""and server-generated IDs. Combined with the fallback that resetsself.name = Nonewhen selector mismatches, this looks correct and backward compatible.You might consider logging the requested
lease_idwhen it’s non-empty to aid debugging around custom lease-name collisions.packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
25-35: CLI wiring for --lease-id looks correct and backwards compatibleThe new
@click.option("--lease-id", ...)plus the updatedcreate_leasesignature order (config, selector, duration, begin_time, lease_id, output) match the decorator ordering and correctly forwardlease_idintoconfig.create_lease. When--lease-idis omitted,Nonepropagates and the backend will generate an ID as before.Consider adding a short CLI test that invokes
jmp create lease ... --lease-id fooand asserts that the resulting lease name/id contains or equalsfoo, to lock in this new behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-cli/jumpstarter_cli/create.py(2 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(3 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(4 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(12 hunks)packages/jumpstarter/jumpstarter/client/lease.py(2 hunks)packages/jumpstarter/jumpstarter/client/lease_test.py(15 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/client/client.pypackages/jumpstarter/jumpstarter/client/decorators.pypackages/jumpstarter/jumpstarter/client/lease.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter-cli/jumpstarter_cli/create.pypackages/jumpstarter/jumpstarter/client/grpc_test.pypackages/jumpstarter/jumpstarter/client/lease_test.pypackages/jumpstarter/jumpstarter/config/client.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/client/client.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/client/lease.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter/jumpstarter/client/grpc_test.pypackages/jumpstarter/jumpstarter/config/client.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_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter/jumpstarter/client/lease_test.py
🧬 Code graph analysis (6)
packages/jumpstarter/jumpstarter/client/client.py (2)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver.py (1)
report(42-45)packages/jumpstarter/jumpstarter/driver/base.py (1)
report(198-213)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)
packages/jumpstarter-cli/jumpstarter_cli/create.py (2)
packages/jumpstarter/jumpstarter/config/client.py (2)
create_lease(199-212)lease(135-143)packages/jumpstarter-cli/jumpstarter_cli/config.py (1)
config(8-12)
packages/jumpstarter/jumpstarter/client/grpc_test.py (5)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
Exporter(79-118)packages/jumpstarter/jumpstarter/common/metadata.py (1)
name(13-14)packages/jumpstarter-cli/jumpstarter_cli/get_test.py (1)
create_test_lease(248-263)packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
status(167-186)packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)
packages/jumpstarter/jumpstarter/client/lease_test.py (1)
packages/jumpstarter/jumpstarter/client/lease.py (2)
LeaseAcquisitionSpinner(326-396)_is_terminal_available(343-350)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
CreateLease(380-409)
⏰ 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 (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- 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.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (17)
packages/jumpstarter/jumpstarter/client/client.py (1)
62-63: Field access remains safe and backward compatibleUsing
getattrwith defaults fordescriptionandmethods_descriptionkeeps the client resilient to older reports that may not define these fields and normalizes empty values toNone/{}as before. The change to double-quoted attribute names is style-only and consistent with Ruff/formatter usage.packages/jumpstarter/jumpstarter/client/decorators.py (3)
38-52: Help text resolution and click param transfer remain correctLogic for deriving
helpfromkwargs, docstring, andclient.description, plus copying__click_params__, is unchanged apart from style; behavior looks correct and backwards compatible.
77-81: Single-command decorator description override is intact
driver_click_commandstill correctly letsclient.descriptionoverride Click’s default help; style-only change.
91-101: Command name normalization and server help override unchangedThe group
commanddecorator still normalizes names vialower().replace("_", "-")and overrides help frommethods_descriptionwhen present; this remains consistent and safe.packages/jumpstarter/jumpstarter/client/grpc.py (2)
268-289: ExporterList dump helpers still apply excludes correctlyThe comprehension-based construction for
"exporters"still honorsexclude_fieldsforlease/onlineand preserves the JSON and dict shapes; this is a safe readability improvement.
380-407: lease_id propagation into CreateLeaseRequest looks correctAdding
lease_id: str | None = Noneand forwarding it aslease_id=lease_id or ""ensures the field is always a string and remains optional. This aligns with the config andLease._createcallers, and keeps the API backward compatible whenlease_idis omitted. Verify there are no legacy direct stub usages ofCreateLease()elsewhere in the codebase.packages/jumpstarter/jumpstarter/client/grpc_test.py (4)
137-207: Exporter lease table tests remain consistent with display behaviorThe refactored Exporter constructions and assertions (with and without
lease, and withshow_leases=True/False) still validate column counts and basic content, including the “Available” fallback when no lease is attached. These tests continue to exercise the key branches inExporter.rich_add_rows.
210-283: Online status and combined-feature tests match rich_add_rows outputTests around online/offline exporters and the “all features” scenario now use concise
Exporter(...)calls and verify presence ofyes/no, labels, and lease-related fields. This gives good coverage of theshow_onlineandshow_leasesoptions together.
285-321: Ended lease info extraction aligns with exporter display logic
test_exporter_lease_info_extractionmirrors the logic inExporter.rich_add_rowsfor choosingeffective_end_timewhen present, and checks that the resultinglease_infotuple contains the expected client, status, and"2023-01-01 11:00:00"end time. This correctly exercises the “Ended” path.
323-376: Scheduled and no-lease scenarios correctly exercise expected release computationThe tests for no-lease exporters and scheduled leases validate that “Available” status is used when
leaseisNone, and that scheduled leases displaybegin_time + durationas the expected release time. This matches the intended semantics of using configureddurationrather thaneffective_durationfor future end times (consistent with earlier learnings).packages/jumpstarter/jumpstarter/client/lease.py (1)
371-388: Spinner log throttling condition remains equivalent and clearerThe consolidated
should_logexpression preserves the previous behavior: always log whenforce=True, on the first message, or when the throttle interval has elapsed. Tests inlease_test.pycover first/within/after-interval and forced cases, so this refactor looks safe.packages/jumpstarter/jumpstarter/client/lease_test.py (4)
33-59: TTY detection tests unchanged apart from formattingThe multi-line
withcontexts patchingsys.stdout.isatty/sys.stderr.isattyare just style changes; the tests still correctly cover all TTY combinations (True/True,False/False, mixed) for_is_terminal_available().
60-88: Context manager tests still validate spinner start/stop behaviorRefactored
with patch.object(...)blocks around_is_terminal_availableandconsole.statusmaintain the same assertions: spinner starts/stops only when a terminal is available, andstart_timeis set in both console and non-console paths. Behavior is preserved.
90-205: Console vs non-console status and tick behavior remain well coveredAll tests that patch
_is_terminal_availableforupdate_status,tick, elapsed time formatting, async integration, and message preservation still match the implementation branches (spinner path vs logging path). The expectations around_current_messageandspinner.updatecall counts look consistent with the currentLeaseAcquisitionSpinnerlogic.
238-336: Throttling tests align with new should_log conditionThe throttling suite—first update logged, within-interval suppressed, after-interval logged, forced updates, multiple rapid updates, and “no throttling when console is available”—all exercise the new
should_loglogic in the logging branch. This gives strong regression protection for the refactor inupdate_status.packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
59-64: Documentation clearly introduces custom lease ID usageThe added example for
--lease-id my-custom-lease-idintegrates well into the existing help text and accurately reflects the new option.packages/jumpstarter/jumpstarter/config/client.py (1)
197-212: Unable to verify create_lease signature extension without repository accessThe review approves threading
lease_id: str | None = Nonethrough toClientService.CreateLeasewith proper decorator handling. However, verification of call sites, signature compatibility, and code style adherence could not be completed due to repository access failure. Manual verification is needed to confirm all callers handle the new optional parameter correctly and that the code meets Ruff formatting standards.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
custom-lease-name
branch
from
e9a02db to
6d6c9fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
25-35:--lease-idCLI wiring and docs look correct; consider documenting constraints if anyThe new
--lease-idoption, extendedcreate_leasesignature, doc snippet, and forwardinglease_idintoconfig.create_leaseare all consistent and keep the default behavior (server‑generated IDs) when the flag is omitted. If the server enforces any particular format/length/charset for lease IDs, it might be worth hinting at that in thehelp=text or docs to avoid users hitting opaque server errors.Also applies to: 59-64, 67-68
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-cli/jumpstarter_cli/create.py(2 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/decorators.py(3 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(4 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(12 hunks)packages/jumpstarter/jumpstarter/client/lease.py(2 hunks)packages/jumpstarter/jumpstarter/client/lease_test.py(15 hunks)packages/jumpstarter/jumpstarter/config/client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter/jumpstarter/client/lease_test.py
- packages/jumpstarter/jumpstarter/client/lease.py
- packages/jumpstarter/jumpstarter/client/decorators.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/client/client.pypackages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter/jumpstarter/client/grpc_test.pypackages/jumpstarter-cli/jumpstarter_cli/create.py
🧠 Learnings (2)
📚 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/client/client.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/jumpstarter/client/grpc_test.py
🧬 Code graph analysis (4)
packages/jumpstarter/jumpstarter/client/client.py (2)
packages/jumpstarter/jumpstarter/driver/base.py (1)
report(199-214)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver.py (1)
report(42-45)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
ClientService(316-457)CreateLease(380-409)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
Exporter(79-118)
packages/jumpstarter-cli/jumpstarter_cli/create.py (3)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
handle_exceptions_with_reauthentication(89-106)packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)packages/jumpstarter/jumpstarter/config/client.py (2)
create_lease(199-212)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 (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter/jumpstarter/client/client.py (1)
55-64: Backwards-compatible handling ofdescriptionandmethods_descriptionlooks goodUsing
getattr(report, "description", None) or None/getattr(report, "methods_description", {}) or {}safely copes with older reports missing these fields while normalizing empty values. This keeps client initialization robust without changing semantics.Based on relevant code snippets.
packages/jumpstarter/jumpstarter/client/grpc.py (2)
268-289: ExporterListmodel_dump_json/model_dumpremain consistent while simplifying structureThe new single-expression construction of
{"exporters": [...]}keeps the previous behavior (respectinginclude_leases/include_onlineviaexclude_fields) and still usesmode="json"for exporter dumps, which matches existing YAML/JSON usage patterns. No issues from a correctness or API-shape standpoint.
380-407:lease_idpropagation inClientService.CreateLeaseis well-shaped; all call sites verifiedAdding
lease_id: str | None = Noneas a kw-only, last parameter and sendinglease_id=lease_id or ""inCreateLeaseRequestcleanly supports optional custom IDs while preserving backwards compatibility for existing callers and servers that treat empty string as "no requested ID". All in-repo usages rely on keyword arguments:
packages/jumpstarter/jumpstarter/config/client.py:207passesselector=,duration=,begin_time=,lease_id=packages/jumpstarter/jumpstarter/client/lease.py:72passesselector=,duration=,lease_id=No positional parameter dependencies detected.
packages/jumpstarter/jumpstarter/client/grpc_test.py (3)
63-67: Test helpercreate_test_exporteris clear and side‑effect freeReturning an
Exporterbuilt with explicit keyword arguments keeps the helper straightforward and makes test fixtures easy to reason about. No change requested.
116-135: Lease test helper and release‑time assertions align with lease semanticsExtending
create_test_leasewitheffective_end_timeand wiring it into tests that cover ended, active, and scheduled leases correctly reflects the display logic (prefereffective_end_time, otherwiseeffective_begin_time + durationorbegin_time + duration). The assertions for"2023-01-01 11:00:00"in the various tests match that contract and guard against regressions aroundeffective_durationmisuse.Based on learnings.
Also applies to: 285-376
137-283: Exporter display tests give good coverage of columns, online status, and lease visibilityThe updated tests constructing
Exporterinstances inline, checking column counts, and verifying rendered content ("Available","yes","no", lease client/status, etc.) provide solid regression coverage for the Rich table rendering paths with differentWithOptionscombinations. The new explicitonlineflags and string checks make the expectations very clear.packages/jumpstarter/jumpstarter/config/client.py (1)
199-212: Config clientcreate_leasecorrectly threadslease_idto the serviceAdding
lease_id: str | None = Noneas the final parameter and passing it through tosvc.CreateLeasekeeps the config API in sync with the underlying gRPC client while preserving backwards compatibility. The existing call site in the CLI (create.py:67) uses keyword arguments, confirming no positional argument dependencies exist.
2 participants
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.