cli: add support for requesting custom lease name #763
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 <[email protected]>
bennyz
force-pushed
the
custom-lease-name
branch
from
e9a02db to
6d6c9fc
Compare
1 participant
Summary by CodeRabbit
New Features
--lease-idoption to the lease creation command, allowing users to specify a custom lease ID when creating leases.✏️ Tip: You can customize this high-level summary in your review settings.