client lease improvements#704
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds optional begin_time and effective timing fields across gRPC client, config, CLI, and protos; updates create/update/list signatures and rendering to show BEGIN TIME and RELEASE TIME; monitor_async uses effective timing for end-time with a 30s check cadence; tests and proto generation/CLI parsing updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as CLI (create/update/get)
participant CFG as ClientConfig
participant RPC as gRPC Client
participant S as Server
U->>CLI: lease create/update --begin-time=... [--duration=...]
CLI->>CFG: create_lease(selector,duration,begin_time) / update_lease(name,duration?,begin_time?)
CFG->>RPC: CreateLease/UpdateLease RPC (Lease proto includes begin_time if provided)
RPC->>S: RPC (Lease proto + update_mask if update)
S-->>RPC: Lease proto (may include effective_begin_time/effective_duration/effective_end_time)
RPC->>RPC: expected_release = (effective_begin_time or begin_time) + (effective_duration or duration) or effective_end_time
RPC-->>CFG: Lease model (with effective_* and begin_time)
CFG-->>CLI: Lease
CLI-->>U: Render BEGIN TIME and RELEASE TIME (replaces START TIME)
sequenceDiagram
autonumber
participant L as Lease.monitor_async
note over L: Uses effective_begin_time & effective_duration when present
loop periodic checks (sleep = min(remain, 30s))
L->>L: end_time = effective_end_time or (effective_begin_time + effective_duration)
L->>L: remain = end_time - now (tz-aware)
alt remain <= 0
L-->>L: stop monitoring
else remain within threshold
note right of L: emit single threshold-entry log
end
L->>L: sleep(min(remain, 30s))
end
opt Missing begin_time or duration
L->>L: sleep(1s) and retry
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
157-168: Avoid treating unset Timestamp as epoch 1970 (proto presence check).In proto3, message fields exist with default values;
if data.effective_begin_timeis truthy even when unset, producing 1970-01-01. Use HasField (and optionally non-zero seconds/nanos) to decide presence for both begin and end.Apply this diff:
- effective_begin_time = None - if data.effective_begin_time: - effective_begin_time = data.effective_begin_time.ToDatetime( - tzinfo=datetime.now().astimezone().tzinfo, - ) + effective_begin_time = None + if hasattr(data, "HasField") and data.HasField("effective_begin_time"): + ts = data.effective_begin_time + if ts.seconds or ts.nanos: + effective_begin_time = ts.ToDatetime( + tzinfo=datetime.now().astimezone().tzinfo, + ) @@ - effective_end_time = None - if data.effective_end_time: - effective_end_time = data.effective_end_time.ToDatetime( - tzinfo=datetime.now().astimezone().tzinfo, - ) + effective_end_time = None + if hasattr(data, "HasField") and data.HasField("effective_end_time"): + ts = data.effective_end_time + if ts.seconds or ts.nanos: + effective_end_time = ts.ToDatetime( + tzinfo=datetime.now().astimezone().tzinfo, + )packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
426-436: Update default lease_info shape to 4‑tuple for consistency.This test still uses a 3‑tuple; elsewhere the default is now (“”, “Available”, “”, “”). Align it to avoid confusion and keep shapes consistent.
Apply this diff:
- lease_info = ("", "Available", "") - assert lease_info == ("", "Available", "") + lease_info = ("", "Available", "", "") + assert lease_info == ("", "Available", "", "")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/jumpstarter-cli/jumpstarter_cli/get.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/get_test.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/login.py(1 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(9 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(9 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (13)
WithOptions(20-24)add_display_columns(27-40)add_exporter_row(43-65)Exporter(91-124)rich_add_columns(104-105)rich_add_columns(182-187)rich_add_columns(233-240)rich_add_columns(298-299)rich_add_rows(107-121)rich_add_rows(189-196)rich_add_rows(242-250)rich_add_rows(301-303)get_status(201-215)
packages/jumpstarter-cli/jumpstarter_cli/get.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-156)packages/jumpstarter/jumpstarter/config/client.py (1)
list_exporters(156-199)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
ListExporters(328-344)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)
packages/jumpstarter-cli/jumpstarter_cli/get_test.py (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/opt.py (1)
parse_comma_separated(163-203)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (4)
rich_add_columns(81-91)rich_add_columns(139-140)rich_add_rows(110-125)rich_add_rows(142-144)
⏰ 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). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (14)
packages/jumpstarter/jumpstarter/config/client.py (1)
163-165: Display flags plumbed correctly; looks good.include_start_time/include_end_time are added and propagated on the response alongside include_online. This keeps proto requests unchanged and confines concerns to rendering. LGTM.
Also applies to: 169-173
packages/jumpstarter/jumpstarter/client/grpc.py (5)
23-25: New WithOptions flags are clear and scoped.Adding show_start_time/show_end_time here is the right place; defaults False preserve behavior.
37-41: Conditional time columns are correct.Columns render only when show_leases is on, then per-start/end flags. Matches tests.
113-121: Lease time rendering logic looks correct.Start/end times are formatted only when flags are enabled and values present. Good separation of concerns.
223-225: ExporterList options wiring is consistent.include_start_time/include_end_time added and propagated to WithOptions for columns/rows. Good encapsulation.
Also applies to: 234-239, 243-248
43-64: add_exporter_row lease_info handling safe as-is
All existing call sites use either None (default) or a 4-tuple; no 3-tuple callers found.packages/jumpstarter/jumpstarter/client/lease.py (1)
251-269: Improved monitor cadence and end-time calc LGTM.Using effective_end_time with fallback and a 30s check interval reduces drift and logs once upon entering threshold. Solid improvement.
packages/jumpstarter/jumpstarter/client/grpc_test.py (2)
50-76: Columns with leases/time flags: assertions match rendering.The expected headers for leases plus optional START/END TIME look correct and align with display code.
138-143: Lease info 4‑tuple usage in rows looks correct.Tests updated to pass a 4‑tuple and verify column counts where time columns are disabled. Good coverage.
Also applies to: 150-155, 162-167
packages/jumpstarter-cli/jumpstarter_cli/get.py (2)
24-25: LGTM! New options properly integrated.The addition of
start_timeandend_timeto the allowed values and help text is correct and consistent with the broader changes in this PR.
33-44: Excellent implementation of implicit leases enabling.The logic correctly enables leases implicitly when either
start_timeorend_timeis requested, since these are properties of lease objects. The implementation is clean, well-commented, and the function call properly passes all the new flags to the backend.packages/jumpstarter-cli/jumpstarter_cli/get_test.py (3)
17-17: LGTM! Test allowed values updated consistently.The allowed values set now includes
start_timeandend_time, matching the implementation inget.py.
43-56: LGTM! Error messages correctly updated.The expected error messages now include all four allowed values in sorted order:
end_time, leases, online, start_time. This matches the actual behavior when invalid options are provided.
189-232: Excellent test coverage for implicit leases behavior.The new test methods comprehensively validate the implicit leases enabling logic:
test_with_options_start_time_implies_leases: Verifiesstart_timealone enables leasestest_with_options_end_time_implies_leases: Verifiesend_timealone enables leasestest_with_options_both_times_implies_leases: Verifies both together enable leasesAll tests correctly assert that
include_onlineremains independent of the time options, ensuring proper flag isolation.
| tokens = await oidc.authorization_code_grant() | ||
| print(tokens) | ||
| config.token = tokens["access_token"] | ||
| ClientConfigV1Alpha1.save(config) # ty: ignore[invalid-argument-type] | ||
| except Exception as e: |
There was a problem hiding this comment.
Remove plaintext token print (secrets leak).
Printing the OIDC tokens exposes sensitive credentials in stdout/logs. Remove this line; never log tokens. If needed, log non-sensitive metadata (issuer, client_id).
Apply this diff:
- tokens = await oidc.authorization_code_grant()
- print(tokens)
+ tokens = await oidc.authorization_code_grant()Based on learnings
📝 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.
| tokens = await oidc.authorization_code_grant() | |
| print(tokens) | |
| config.token = tokens["access_token"] | |
| ClientConfigV1Alpha1.save(config) # ty: ignore[invalid-argument-type] | |
| except Exception as e: | |
| tokens = await oidc.authorization_code_grant() | |
| config.token = tokens["access_token"] | |
| ClientConfigV1Alpha1.save(config) # ty: ignore[invalid-argument-type] | |
| except Exception as e: |
🤖 Prompt for AI Agents
In packages/jumpstarter-cli/jumpstarter_cli/login.py around lines 151 to 155,
remove the plaintext token print to avoid leaking sensitive credentials; delete
the print(tokens) line and ensure only the access token is assigned to
config.token and saved, and if logging is required emit only non-sensitive
metadata (e.g., issuer or client_id) rather than token contents.
michalskrivanek
force-pushed
the
lease-monitor
branch
from
6c4d2df to
122d53d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/client/lease.py (2)
262-265: Simplify minute rounding logic.The logging logic correctly prevents multiple log messages by using a threshold window. However, the minute calculation uses non-standard rounding.
Apply this diff to use standard rounding:
- logger.info("Lease {} ending in {} minutes at {}".format( - self.name, int((remain.total_seconds() + 30) // 60), end_time)) + logger.info("Lease {} ending in {} minutes at {}".format( + self.name, round(remain.total_seconds() / 60), end_time))
256-256: Simplify timezone handling.The timezone expression
datetime.now(tz=datetime.now().astimezone().tzinfo)is unnecessarily complex.Apply this diff to simplify:
- remain = end_time - datetime.now(tz=datetime.now().astimezone().tzinfo) + remain = end_time - datetime.now(tz=end_time.tzinfo)Or more simply:
- remain = end_time - datetime.now(tz=datetime.now().astimezone().tzinfo) + remain = end_time - datetime.now().astimezone()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter/jumpstarter/client/grpc.py(8 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/config/exporter.py (2)
rich_add_columns(224-226)rich_add_rows(228-233)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/leases.py (4)
rich_add_columns(81-91)rich_add_columns(139-140)rich_add_rows(110-125)rich_add_rows(142-144)
⏰ 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: build
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: e2e
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (9)
packages/jumpstarter/jumpstarter/client/lease.py (2)
251-251: LGTM!The
check_intervalconstant improves readability and makes the periodic check interval explicit. The 30-second interval is appropriate for monitoring lease status without excessive polling.
254-260: LGTM! Enhanced lease monitoring with end-time tracking.The updated end-time calculation correctly uses the new
effective_begin_timeandeffective_durationfields from the Lease model. The logic properly handles missing fields by falling back to the previous behavior (lines 266-267), ensuring backward compatibility.packages/jumpstarter/jumpstarter/client/grpc.py (7)
23-24: LGTM! Display control flags added.The new
show_start_timeandshow_end_timeflags inWithOptionsprovide clear control over time column visibility.
37-40: LGTM! Conditional column rendering.The columns are correctly added only when the corresponding display flags are enabled.
43-63: LGTM! Function signature and logic updated for 4-tuple lease_info.The signature change from 3-tuple to 4-tuple is correctly implemented throughout. The unpacking, conditional row construction, and default values all handle the new start_time and end_time fields properly.
132-137: LGTM! Lease model augmented with effective timing fields.The new
effective_durationandeffective_end_timefields support the enhanced lease monitoring and display features introduced in this PR.
158-173: LGTM! Protobuf deserialization correctly extracts timing fields.The
from_protobufmethod properly extractseffective_durationandeffective_end_timefrom the protobuf message, converting them to Python types with appropriate timezone handling. The None checks ensure safe handling of missing fields.Also applies to: 179-183
113-120: LGTM! Exporter row rendering includes formatted start/end times.The formatting logic correctly:
- Conditionally formats start_time and end_time based on display flags
- Uses consistent datetime format string
- Handles None values with empty strings
- Constructs the 4-tuple lease_info with all required elements
229-230: LGTM! ExporterList display flags wired end-to-end.The
include_start_timeandinclude_end_timefields are correctly added to ExporterList and propagated throughWithOptionsin bothrich_add_columnsandrich_add_rowsmethods, ensuring consistent display behavior.Also applies to: 240-254
michalskrivanek
force-pushed
the
lease-monitor
branch
2 times, most recently
from
e68900a to
502f9d8
Compare
michalskrivanek
changed the title
michalskrivanek
force-pushed
the
lease-monitor
branch
from
502f9d8 to
4705ee3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter/jumpstarter/client/grpc_test.py (2)
122-128: Consider testing the begin_time fallback logic.The test only sets
effective_begin_timebut notbegin_time. The implementation ingrpc.py(lines 183-195) has fallback logic to displaybegin_timewheneffective_begin_timeis not available. Consider adding a test case that verifies this fallback behavior.
308-335: Consider testing behavior rather than implementation details.Lines 329-335 manually replicate the expected_release calculation logic from
grpc.py. This approach makes the test brittle—if the calculation changes, both the implementation and this test need updates.Consider refactoring to test the behavior through the public API instead:
- # Test the logic that builds lease_info tuple in rich_add_rows - options = WithOptions(show_leases=True) - if options.show_leases and exporter.lease: - lease_client = exporter.lease.client - lease_status = exporter.lease.get_status() - expected_release = "" - if exporter.lease.effective_begin_time and exporter.lease.effective_duration: - release_time = exporter.lease.effective_begin_time + exporter.lease.effective_duration - expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S") - lease_info = (lease_client, lease_status, expected_release) - - assert lease_info == ("my-client", "Expired", "2023-01-01 11:00:00") + # Test the behavior by rendering the table and checking the output + table = Table() + options = WithOptions(show_leases=True) + Exporter.rich_add_columns(table, options) + exporter.rich_add_rows(table, options) + + console = Console(file=StringIO(), width=120) + console.print(table) + output = console.file.getvalue() + + assert "my-client" in output + assert "Expired" in output + assert "2023-01-01 11:00:00" in outputpackages/jumpstarter/jumpstarter/client/grpc.py (1)
182-204: Clarify the comment for begin_time display logic.The comment on line 183 says "Show effective_begin_time if active, otherwise show scheduled begin_time", but the code doesn't check if the lease is active—it only checks if
effective_begin_timeexists. This could be misleading sinceeffective_begin_timemight exist even for inactive leases.Consider updating the comment to reflect the actual logic:
- # Show effective_begin_time if active, otherwise show scheduled begin_time + # Show effective_begin_time if available, otherwise show scheduled begin_time begin_time = "" if self.effective_begin_time: begin_time = self.effective_begin_time.strftime("%Y-%m-%d %H:%M:%S") elif self.begin_time: begin_time = self.begin_time.strftime("%Y-%m-%d %H:%M:%S")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter/jumpstarter/client/grpc.py(10 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter/jumpstarter/client/grpc_test.py (2)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
WithOptions(20-22)add_display_columns(25-35)packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-296)
⏰ 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). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.11)
- 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 (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
🔇 Additional comments (8)
packages/jumpstarter/jumpstarter/client/grpc_test.py (4)
51-51: LGTM!The test assertions correctly verify the new "EXPECTED RELEASE" column header in place of "START TIME".
Also applies to: 59-59
84-94: LGTM!The column count assertions correctly reflect the updated table structure with the new "EXPECTED RELEASE" column.
Also applies to: 108-118
163-191: LGTM!The test correctly verifies that the expected release time is calculated as begin_time + duration (10:00:00 + 1 hour = 11:00:00).
260-306: LGTM!The test comprehensively verifies all display features together, including the expected release calculation.
packages/jumpstarter/jumpstarter/client/grpc.py (4)
35-35: LGTM!The column header change from "START TIME" to "EXPECTED RELEASE" correctly reflects the new semantics where the displayed time represents when the lease is expected to be released (begin_time + duration).
100-104: Verify the expected_release calculation handles partial data.The calculation only computes
expected_releasewhen botheffective_begin_timeandeffective_durationexist. If only one is available,expected_releaseremains empty. Consider whether fallback logic is needed to usebegin_timeordurationwhen the effective values aren't set.For example, should this be updated to handle partial data?
expected_release = "" begin_time_value = self.lease.effective_begin_time or self.lease.begin_time duration_value = self.lease.effective_duration or self.lease.duration if begin_time_value and duration_value: release_time = begin_time_value + duration_value expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S")
113-171: LGTM!The new
effective_durationandbegin_timefields are correctly added to theLeasemodel with appropriate optional types, and the protobuf parsing logic properly handles both fields when present.
398-424: UpdateLease callers and error path coverage verified
- Only internal usage at
packages/jumpstarter/jumpstarter/config/client.py:247passes bothdurationandbegin_time.- No existing tests for the
ValueErrorwhen neither parameter is provided; add coverage for this path.
michalskrivanek
force-pushed
the
lease-monitor
branch
2 times, most recently
from
7652940 to
2980b60
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
144-159: Use HasField for message presence to avoid false negatives on zero values.Truthiness checks on message fields can misinterpret set-but-zero values (e.g., duration 0s). Use HasField for effective_duration and effective_begin_time.
Apply this diff:
- effective_duration = None - if data.effective_duration: - effective_duration = data.effective_duration.ToTimedelta() + effective_duration = None + if data.HasField("effective_duration"): + effective_duration = data.effective_duration.ToTimedelta() @@ - effective_begin_time = None - if data.effective_begin_time: + effective_begin_time = None + if data.HasField("effective_begin_time"): effective_begin_time = data.effective_begin_time.ToDatetime( tzinfo=datetime.now().astimezone().tzinfo, )
🧹 Nitpick comments (5)
packages/jumpstarter/jumpstarter/client/grpc.py (4)
100-105: Also show EXPECTED RELEASE for scheduled (not-yet-effective) leases.If only begin_time/duration are set (no effective_* yet), expected_release stays blank. Compute a fallback to match the PR intent.
Apply this diff:
- expected_release = "" - if self.lease.effective_begin_time and self.lease.effective_duration: - release_time = self.lease.effective_begin_time + self.lease.effective_duration - expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S") - lease_info = (lease_client, lease_status, expected_release) + expected_release = "" + release_time = None + if self.lease.effective_begin_time and self.lease.effective_duration: + release_time = self.lease.effective_begin_time + self.lease.effective_duration + elif self.lease.begin_time and self.lease.duration: + release_time = self.lease.begin_time + self.lease.duration + if release_time: + expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S") + lease_info = (lease_client, lease_status, expected_release)
426-428: Build FieldMask via paths for clarity and to avoid JSON quirks.Simplify FieldMask creation by setting paths directly.
Apply this diff:
- update_mask = field_mask_pb2.FieldMask() - update_mask.FromJsonString(",".join(update_fields)) + update_mask = field_mask_pb2.FieldMask(paths=update_fields)
183-204: Optional: include timezone in displayed timestamps.BEGIN TIME and EXPECTED RELEASE are formatted without timezone, which can confuse multi‑TZ users. Prefer including the offset (e.g., %z) or use isoformat(timespec="seconds").
384-389: Optional: normalize begin_time before serialization.If begin_time is naive, Timestamp.FromDatetime treats it as UTC. Consider normalizing to an explicit tz (e.g., UTC) to avoid ambiguity.
packages/jumpstarter/jumpstarter/client/lease.py (1)
251-266: Monitor scheduled leases too (fallback to begin_time+duration).When effective fields are missing but begin_time/duration are set, the monitor does nothing useful. Compute end_time from begin_time+duration in that case.
Apply this diff:
- if lease.effective_begin_time and lease.effective_duration: - end_time = lease.effective_begin_time + lease.effective_duration + if lease.effective_begin_time and lease.effective_duration: + end_time = lease.effective_begin_time + lease.effective_duration + elif lease.begin_time and lease.duration: + end_time = lease.begin_time + lease.duration + else: + await sleep(1) + continue - remain = end_time - datetime.now().astimezone() + remain = end_time - datetime.now().astimezone()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/jumpstarter-cli/jumpstarter_cli/common.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(10 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter/jumpstarter/client/grpc_test.py
- packages/jumpstarter-cli/jumpstarter_cli/common.py
🧰 Additional context used
🧬 Code graph analysis (4)
packages/jumpstarter-cli/jumpstarter_cli/update.py (2)
packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)packages/jumpstarter/jumpstarter/config/client.py (2)
update_lease(240-247)lease(135-143)
packages/jumpstarter-cli/jumpstarter_cli/create.py (5)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1)
opt_config(99-100)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
handle_exceptions_with_reauthentication(89-106)packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (1)
model_print(9-75)packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)packages/jumpstarter/jumpstarter/config/client.py (2)
create_lease(199-210)lease(135-143)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
ClientService(307-444)CreateLease(369-396)UpdateLease(398-436)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-296)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(71-89)
⏰ 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.11)
- 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
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (6)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
35-35: Header change to EXPECTED RELEASE looks good.packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
23-40: CLI update flow looks correct.Optional duration/begin_time, guard for empty updates, and pass-through to config align with the new API.
packages/jumpstarter/jumpstarter/config/client.py (2)
198-210: Begin time passthrough in create_lease looks good.
240-248: Update_lease API alignment looks good.packages/jumpstarter-cli/jumpstarter_cli/create.py (2)
24-28: Begin time support in create command looks good.
53-54: Passthrough to config is correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
27-27: Make options keyword‑only to prevent accidental positional misuseClick passes options as keywords; adding a keyword‑only barrier avoids breakage for any direct calls after the signature change.
-def update_lease(config, name: str, duration: timedelta | None, begin_time: datetime | None, output: OutputType): +def update_lease(config, name: str, *, duration: timedelta | None, begin_time: datetime | None, output: OutputType):packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
186-191: Align lease status vocabulary across code/testsTests assert "Active", while get_status() in grpc.py returns "In‑Use" for Ready=True. Consider standardizing the label to avoid confusion in UI and tests.
Also applies to: 304-306
packages/jumpstarter/jumpstarter/client/lease.py (1)
251-266: Harden monitor loop: avoid negative sleep and ensure single threshold logTwo tweaks improve robustness:
- Guard against tiny negative sleeps due to timing jitter.
- Ensure the “entering threshold” log fires only once (even if threshold < check_interval).
async def _monitor(): - check_interval = 30 # seconds - check periodically for external lease changes + check_interval = 30 # seconds - check periodically for external lease changes + threshold_notified = False while True: lease = await self.get() if lease.effective_begin_time and lease.effective_duration: end_time = lease.effective_begin_time + lease.effective_duration remain = end_time - datetime.now().astimezone() if remain < timedelta(0): # lease already expired, stopping monitor logger.info("Lease {} ended at {}".format(self.name, end_time)) break - # Log once when entering the threshold window - if threshold - timedelta(seconds=check_interval) <= remain < threshold: + # Log once when entering the threshold window + if (not threshold_notified) and (threshold - timedelta(seconds=check_interval) <= remain < threshold): logger.info("Lease {} ending in {} minutes at {}".format( self.name, int((remain.total_seconds() + 30) // 60), end_time)) - await sleep(min(remain.total_seconds(), check_interval)) + threshold_notified = True + await sleep(max(0.0, min(remain.total_seconds(), check_interval))) else: await sleep(1)packages/jumpstarter/jumpstarter/client/grpc.py (2)
100-105: Consider fallback to scheduled release time for non‑active leasesOnly computing EXPECTED RELEASE from effective_* hides useful info for pending/scheduled leases. Optionally fall back to begin_time+duration.
- expected_release = "" - if self.lease.effective_begin_time and self.lease.effective_duration: - release_time = self.lease.effective_begin_time + self.lease.effective_duration - expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S") + expected_release = "" + release_start = self.lease.effective_begin_time or self.lease.begin_time + release_dur = self.lease.effective_duration or self.lease.duration + if release_start and release_dur: + release_time = release_start + release_dur + expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S")
209-224: Status label namingget_status() returns "In‑Use" for Ready=True. If UI/tests prefer "Active", consider aligning string to reduce cognitive load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/jumpstarter-cli/jumpstarter_cli/common.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(10 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-cli/jumpstarter_cli/common.py
- packages/jumpstarter-cli/jumpstarter_cli/create.py
🧰 Additional context used
🧬 Code graph analysis (5)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
ClientService(307-444)CreateLease(369-396)UpdateLease(398-436)
packages/jumpstarter/jumpstarter/client/grpc_test.py (2)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
WithOptions(20-22)add_display_columns(25-35)packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)
packages/jumpstarter-cli/jumpstarter_cli/update.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
handle_exceptions_with_reauthentication(89-106)packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (1)
model_print(9-75)packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)packages/jumpstarter/jumpstarter/config/client.py (2)
update_lease(240-247)lease(135-143)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-296)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(71-89)
⏰ 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). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (4)
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
36-40: Validation is correct and user‑friendlyGuarding that at least one of --duration or --begin-time is provided is spot on and aligns with backend constraints.
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
51-51: Tests correctly reflect EXPECTED RELEASE column and countsHeader names and column counts match the updated rendering. Looks good.
Also applies to: 59-59, 94-95, 118-119, 179-180, 206-208, 306-307
packages/jumpstarter/jumpstarter/config/client.py (2)
199-211: Begin time propagation in create_lease is correctForwarding begin_time to ClientService.CreateLease with proper serialization upstream. LGTM.
240-248: Optional duration/begin_time in update_lease correctly forwardedMatches the gRPC client's UpdateLease contract and avoids unnecessary network calls when local validation fails. LGTM.
michalskrivanek
force-pushed
the
lease-monitor
branch
from
2980b60 to
d9a76af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
369-396: Normalize naive begin_time to tz-aware before building Timestamp
Timestamp.FromDatetimerequires tz-aware datetimes. If a caller passes a naive dt, this will raise. Normalize to a timezone (e.g., local or UTC) before conversion.-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone @@ async def CreateLease( @@ - if begin_time: - timestamp_pb = timestamp_pb2.Timestamp() - timestamp_pb.FromDatetime(begin_time) + if begin_time: + # Ensure timezone-aware; treat naive as local time for consistency with UI + if begin_time.tzinfo is None: + begin_time = begin_time.replace(tzinfo=datetime.now().astimezone().tzinfo) + timestamp_pb = timestamp_pb2.Timestamp() + timestamp_pb.FromDatetime(begin_time) @@ async def UpdateLease( @@ - if begin_time is not None: - timestamp_pb = timestamp_pb2.Timestamp() - timestamp_pb.FromDatetime(begin_time) + if begin_time is not None: + if begin_time.tzinfo is None: + begin_time = begin_time.replace(tzinfo=datetime.now().astimezone().tzinfo) + timestamp_pb = timestamp_pb2.Timestamp() + timestamp_pb.FromDatetime(begin_time)Also applies to: 404-437, 6-6
♻️ Duplicate comments (1)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
144-159: Proto presence checks: use HasField() for submessages to avoid TypeErrorTruthiness on Duration/Timestamp can raise; guard with
HasField().- effective_duration = None - if data.effective_duration: - effective_duration = data.effective_duration.ToTimedelta() + effective_duration = None + if data.HasField("effective_duration"): + effective_duration = data.effective_duration.ToTimedelta() @@ - effective_begin_time = None - if data.effective_begin_time: - effective_begin_time = data.effective_begin_time.ToDatetime( + effective_begin_time = None + if data.HasField("effective_begin_time"): + effective_begin_time = data.effective_begin_time.ToDatetime( tzinfo=datetime.now().astimezone().tzinfo, )
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/config/client.py (1)
240-248: Minor: type annotatenamefor clarityAdd
name: strto the signature.- async def update_lease( - self, - name, + async def update_lease( + self, + name: str, duration: timedelta | None = None, begin_time: datetime | None = None, ):packages/jumpstarter/jumpstarter/client/grpc.py (1)
38-52: Show expected release for scheduled leases tooIf a lease is scheduled (begin_time present) but not yet effective, compute expected release from begin_time + (effective_duration or duration) so users see a value.
- if lease_info: - lease_client, lease_status, expected_release = lease_info + if lease_info: + lease_client, lease_status, expected_release = lease_info else: lease_client, lease_status, expected_release = "", "Available", "" row_data.extend([lease_client, lease_status, expected_release]) @@ - expected_release = "" - if self.lease.effective_begin_time and self.lease.effective_duration: - release_time = self.lease.effective_begin_time + self.lease.effective_duration - expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S") + expected_release = "" + if self.lease.effective_begin_time and self.lease.effective_duration: + release_time = self.lease.effective_begin_time + self.lease.effective_duration + expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S") + elif self.lease.begin_time: + dur = self.lease.effective_duration or self.lease.duration + release_time = self.lease.begin_time + dur + expected_release = release_time.strftime("%Y-%m-%d %H:%M:%S") lease_info = (lease_client, lease_status, expected_release)Also applies to: 96-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/jumpstarter-cli/jumpstarter_cli/common.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(10 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter-cli/jumpstarter_cli/create.py
- packages/jumpstarter/jumpstarter/client/grpc_test.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-296)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(71-89)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
ClientService(307-444)CreateLease(369-396)UpdateLease(398-436)
packages/jumpstarter-cli/jumpstarter_cli/update.py (5)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1)
opt_config(99-100)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
handle_exceptions_with_reauthentication(89-106)packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (1)
model_print(9-75)packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)packages/jumpstarter/jumpstarter/config/client.py (2)
update_lease(240-247)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.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
🔇 Additional comments (3)
packages/jumpstarter/jumpstarter/client/lease.py (1)
249-266: Monitor logic switch to effective_ and periodic checks looks good*Correct end-time calculation and bounded sleep reduce polling load and improve accuracy.
packages/jumpstarter/jumpstarter/config/client.py (1)
199-211: Begin time forwarded in create_lease — LGTMAPI surface aligns with grpc service; no issues found.
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
23-41: Good UX: optional duration, new begin-time, and guard checkValidation ensures intent; forwarding matches config API.
michalskrivanek
force-pushed
the
lease-monitor
branch
from
d9a76af to
18184a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/jumpstarter-cli/jumpstarter_cli/common.py (1)
49-74: Catch ValidationError and normalize naive datetimes.TypeAdapter.validate_python raises ValidationError (not ValueError), so line 58's except clause won't catch parsing failures. Additionally, naive datetimes (without timezone) will cause Timestamp.FromDatetime to fail downstream. Normalize to local timezone after parsing.
Based on past review comments.
packages/jumpstarter/jumpstarter/client/grpc.py (1)
148-159: Use HasField() for proto presence checks.Checking submessage truthiness for Duration/Timestamp fields is unsafe and can raise TypeError at runtime. Use HasField() for explicit presence checks.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/jumpstarter-cli/jumpstarter_cli/common.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(10 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (13)
WithOptions(20-22)add_display_columns(25-35)Lease(117-227)get_status(213-227)Exporter(79-114)rich_add_columns(92-93)rich_add_columns(178-184)rich_add_columns(243-245)rich_add_columns(298-299)rich_add_rows(95-111)rich_add_rows(186-208)rich_add_rows(247-250)rich_add_rows(301-303)
packages/jumpstarter-cli/jumpstarter_cli/create.py (2)
packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)packages/jumpstarter/jumpstarter/config/client.py (2)
create_lease(199-210)lease(135-143)
packages/jumpstarter/jumpstarter/config/client.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
ClientService(311-448)CreateLease(373-400)UpdateLease(402-440)
packages/jumpstarter-cli/jumpstarter_cli/update.py (5)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1)
opt_config(99-100)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
handle_exceptions_with_reauthentication(89-106)packages/jumpstarter-cli-common/jumpstarter_cli_common/print.py (1)
model_print(9-75)packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)packages/jumpstarter/jumpstarter/config/client.py (2)
update_lease(240-247)lease(135-143)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-296)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(71-89)
🪛 GitHub Actions: Lint
packages/jumpstarter/jumpstarter/client/grpc_test.py
[error] 122-122: Ruff: W291 Trailing whitespace detected.
🪛 GitHub Check: ruff
packages/jumpstarter/jumpstarter/client/grpc_test.py
[failure] 123-123: Ruff (W291)
packages/jumpstarter/jumpstarter/client/grpc_test.py:123:79: W291 Trailing whitespace
[failure] 122-122: Ruff (W291)
packages/jumpstarter/jumpstarter/client/grpc_test.py:122:71: W291 Trailing whitespace
⏰ 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). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: Pages changed - jumpstarter-docs
- 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 (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (7)
packages/jumpstarter/jumpstarter/client/grpc.py (1)
409-428: LGTM! Proper validation for update fields.The validation at line 427-428 correctly ensures at least one of
durationorbegin_timeis provided before constructing the update mask. The field mask is built dynamically based on which parameters are non-None, which aligns with the expected update semantics.packages/jumpstarter/jumpstarter/config/client.py (2)
199-210: LGTM! Clean passthrough of begin_time parameter.The begin_time parameter is correctly added to the signature and forwarded to the underlying CreateLease service call.
239-247: LGTM! Update signature correctly supports optional fields.Both
durationandbegin_timeare appropriately optional with None defaults, and both are forwarded as keyword arguments to the underlying UpdateLease service call. This aligns with the validation logic in the ClientService layer.packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
20-54: LGTM! Clean integration of begin_time parameter.The opt_begin_time decorator is correctly applied, the parameter is added to the signature with appropriate typing, and the value is forwarded to config.create_lease. The docstring example could mention the new parameter, but the implementation is sound.
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
363-401: LGTM! Comprehensive test coverage for scheduled leases.The new test correctly validates that scheduled leases (those not yet active) display the expected release time computed from begin_time and duration. The test setup, assertions, and rendered output verification are all appropriate.
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
20-41: LGTM! Proper validation and parameter forwarding.The command correctly:
- Makes both duration and begin_time optional at the CLI level
- Validates that at least one is provided (line 36-37)
- Forwards both parameters to config.update_lease with clear keyword arguments
- Documents the constraint in the docstring
This aligns well with the underlying service validation in ClientService.UpdateLease.
packages/jumpstarter/jumpstarter/client/lease.py (1)
251-267: Threshold logging can trigger multiple times, not once as intended.The condition at line 262 allows logging on every iteration when
remainfalls within the threshold window. For example, with a 5-minute threshold and 30-second check_interval, the window spans from 4:30 to 5:00 remaining. Ifremainis 4:45, the code sleeps for 30 seconds, wakes at 4:15 remaining, and logs again—contradicting the "Log once" comment.To log only once when entering the threshold window, track whether you've already logged:
async def _monitor(): check_interval = 30 # seconds - check periodically for external lease changes + threshold_logged = False while True: lease = await self.get() if lease.effective_begin_time and lease.effective_duration: end_time = lease.effective_begin_time + lease.effective_duration remain = end_time - datetime.now().astimezone() if remain < timedelta(0): # lease already expired, stopping monitor logger.info("Lease {} ended at {}".format(self.name, end_time)) break # Log once when entering the threshold window - if threshold - timedelta(seconds=check_interval) <= remain < threshold: + if remain < threshold and not threshold_logged: logger.info("Lease {} ending in {} minutes at {}".format( self.name, int((remain.total_seconds() + 30) // 60), end_time)) + threshold_logged = True await sleep(min(remain.total_seconds(), check_interval)) else: await sleep(1)Likely an incorrect or invalid review comment.
michalskrivanek
force-pushed
the
lease-monitor
branch
2 times, most recently
from
b87fd87 to
f260968
Compare
michalskrivanek
force-pushed
the
lease-monitor
branch
from
2eeefd7 to
5b476bd
Compare
|
@mangelajo fixed the display of expected release for currently active leases, but i can't really test others. jumpstarter-dev/jumpstarter-controller#172 would help to test other cases:) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/jumpstarter-cli/jumpstarter_cli/common_test.py (1)
9-58: LGTM! Comprehensive test coverage for DateTimeParamType.The test class effectively covers:
- ISO 8601 parsing with timezone preservation
- Naive datetime normalization to local timezone
- Pass-through behavior for datetime objects
- Error handling for invalid inputs
Each test is well-documented with clear docstrings and specific assertions.
Consider adding tests for additional timezone formats beyond "Z" for more comprehensive coverage:
def test_parse_iso8601_with_offset_timezone(self): """Test parsing ISO 8601 datetime with offset timezone.""" dt = DATETIME.convert("2024-01-01T12:00:00+05:30", None, None) assert dt.year == 2024 assert dt.hour == 12 assert dt.tzinfo is not None # Verify offset is +05:30packages/jumpstarter/jumpstarter/client/lease.py (1)
262-264: Clarify "log once" comment or adjust implementation.The comment states "Log once when entering the threshold window," but the condition
threshold - timedelta(seconds=check_interval) <= remain < thresholdwill be true for multiple loop iterations (every 30 seconds) whileremainstays in that window, causing repeated log messages rather than a single one.If the intent is to log only on the first entry into the threshold window, consider adding a flag to track whether the threshold log has been emitted. Otherwise, update the comment to reflect that logging occurs periodically within the threshold window.
Example implementation for single log:
async def _monitor(): check_interval = 30 # seconds - check periodically for external lease changes + threshold_logged = False while True: lease = await self.get() if lease.effective_begin_time and lease.effective_duration: end_time = lease.effective_begin_time + lease.effective_duration remain = end_time - datetime.now().astimezone() if remain < timedelta(0): # lease already expired, stopping monitor logger.info("Lease {} ended at {}".format(self.name, end_time)) break - # Log once when entering the threshold window - if threshold - timedelta(seconds=check_interval) <= remain < threshold: + # Log once when entering the threshold window + if not threshold_logged and remain < threshold: logger.info("Lease {} ending in {} minutes at {}".format( self.name, int((remain.total_seconds() + 30) // 60), end_time)) + threshold_logged = True await sleep(min(remain.total_seconds(), check_interval)) else: await sleep(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-cli/jumpstarter_cli/common.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/common_test.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(10 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter-cli/jumpstarter_cli/common.py
🧰 Additional context used
🧬 Code graph analysis (5)
packages/jumpstarter-cli/jumpstarter_cli/common_test.py (1)
packages/jumpstarter-cli/jumpstarter_cli/common.py (4)
DateTimeParamType(49-65)DurationParamType(15-25)convert(18-25)convert(52-65)
packages/jumpstarter-cli/jumpstarter_cli/create.py (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1)
opt_config(99-100)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-210)lease(135-143)
packages/jumpstarter-cli/jumpstarter_cli/update.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)
update_lease(240-247)lease(135-143)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (13)
WithOptions(20-22)add_display_columns(25-35)Lease(116-226)get_status(212-226)Exporter(79-113)rich_add_columns(92-93)rich_add_columns(177-183)rich_add_columns(242-244)rich_add_columns(297-298)rich_add_rows(95-110)rich_add_rows(185-207)rich_add_rows(246-249)rich_add_rows(300-302)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-296)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(71-89)
⏰ 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.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- 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.11)
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
1-41: LGTM! Well-structured implementation of optional lease parameters.The changes correctly implement the new optional
begin_timeparameter alongside the existingdurationparameter:
- Import additions are appropriate for the new functionality
- Making
durationoptional (line 23) and adding@opt_begin_timedecorator (line 24) properly support the new flexibility- Validation logic (lines 36-38) correctly ensures at least one parameter is provided with a clear error message
- Function call (line 39) uses keyword arguments, which is good practice and aligns with the backend API signature in
client.py- Updated docstring (lines 30-34) clearly documents the new behavior and validation rules
The implementation is consistent with the backend API changes and follows CLI best practices.
packages/jumpstarter-cli/jumpstarter_cli/common_test.py (3)
1-6: LGTM! Clean and well-organized imports.The imports are properly organized with standard library, third-party, and local imports clearly separated.
61-89: LGTM! Thorough test coverage for DurationParamType.The test class comprehensively covers:
- ISO 8601 duration format (PT1H30M)
- HH:MM:SS time format
- Days and time format (e.g., "2 days, 01:30:00")
- Pass-through behavior for timedelta objects
- Error handling for invalid inputs
The tests are well-structured with clear docstrings and precise assertions.
1-90: Excellent test file with comprehensive coverage!This test module provides thorough validation for the new DateTimeParamType and DurationParamType classes. Key strengths:
- Clear structure: Well-organized into logical test classes
- Good documentation: Every test has a descriptive docstring
- Comprehensive coverage: Tests valid inputs, pass-through behavior, normalization, and error handling
- Proper testing patterns: Correct use of pytest.raises and click.BadParameter
- Alignment with implementation: Tests accurately reflect the behavior defined in common.py
The tests successfully validate the datetime and duration parameter parsing functionality introduced in this PR.
packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
1-1: LGTM! Clean integration of begin_time support.The changes properly thread
begin_timefrom CLI decorator through to the config layer, maintaining consistency with the related imports and parameter forwarding.Also applies to: 9-9, 24-24, 27-27, 53-53
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
51-51: LGTM! Comprehensive test coverage for display changes.The test updates thoroughly cover the transition from START TIME to EXPECTED RELEASE, including:
- Column header assertions across multiple scenarios
- Expected release time calculations
- New test case for scheduled leases with
begin_timefallbackThe test at lines 363-401 properly validates the rendering path when
effective_begin_timeis absent butbegin_timeis present.Also applies to: 59-59, 94-94, 118-118, 184-184, 196-196, 212-212, 294-294, 311-311, 334-340, 363-401
packages/jumpstarter/jumpstarter/client/grpc.py (1)
147-155: LGTM! Proper protobuf handling and API updates.The changes correctly:
- Use
HasField()for optional protobuf field presence checks (as addressed from previous review)- Add
effective_durationandbegin_timeto the Lease model and protobuf parsing- Update
CreateLeaseto accept and serialize optionalbegin_time- Update
UpdateLeaseto support partial updates with proper validation- Build appropriate
update_maskfrom provided fieldsAlso applies to: 168-169, 377-390, 405-427
michalskrivanek
force-pushed
the
lease-monitor
branch
from
5b476bd to
943426c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
122-126: Remove trailing whitespace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter/jumpstarter/client/grpc.py(9 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.767Z
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`.
📚 Learning: 2025-10-14T17:43:07.767Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#704
File: packages/jumpstarter/jumpstarter/client/grpc.py:100-107
Timestamp: 2025-10-14T17:43:07.767Z
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/grpc_test.pypackages/jumpstarter/jumpstarter/client/grpc.pypackages/jumpstarter/jumpstarter/client/lease.py
🧬 Code graph analysis (3)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (13)
WithOptions(20-22)add_display_columns(25-35)Lease(121-243)get_status(225-243)Exporter(79-118)rich_add_columns(92-93)rich_add_columns(190-196)rich_add_columns(259-261)rich_add_columns(314-315)rich_add_rows(95-115)rich_add_rows(198-220)rich_add_rows(263-266)rich_add_rows(317-319)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-299)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(71-89)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/config/client.py (1)
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). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- 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: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (8)
packages/jumpstarter/jumpstarter/client/lease.py (3)
265-267: LGTM! Clean threshold logging implementation.The single threshold-window log is a good improvement over multiple logging branches, and the condition correctly detects when remaining time enters the threshold window.
268-268: LGTM! Proper sleep interval calculation.Using
min(remain.total_seconds(), check_interval)ensures the monitor doesn't sleep past the lease expiration while maintaining periodic checks for external lease changes.
254-254: Document rationale for gating on effective_duration
The monitor loop waits for botheffective_begin_timeandeffective_durationbefore starting; if the server sets these fields at different times, monitoring may not start. Add a comment at lease.py:254 explaining this assumption or adjust the condition.packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
365-403: LGTM! Good test coverage for scheduled leases.The new test correctly validates that scheduled leases (with
begin_timebut noeffective_begin_time) display the expected release time asbegin_time + duration. This ensures the rendering logic handles all lease states properly.packages/jumpstarter/jumpstarter/client/grpc.py (4)
100-112: LGTM! Correct expected release time calculation.The logic correctly calculates expected release time for all lease states:
- Ended leases: Use
effective_end_time(actual end)- Active leases: Use
effective_begin_time + duration(expected end)- Scheduled leases: Use
begin_time + duration(expected end)This aligns with the learning that
effective_durationrepresents elapsed time, not total duration.Based on learnings.
153-174: LGTM! Safe protobuf field extraction.The use of
HasField()for checking presence of optional protobuf fields (effective_duration, begin_time, effective_begin_time, effective_end_time) is correct and prevents potential TypeError exceptions.
418-445: LGTM! Well-structured UpdateLease implementation.Good practices:
- Clear separation of update field building logic
- Proper validation that at least one field is provided (line 443-444)
- Correct use of FieldMask for partial updates
- Consistent error handling with translate_grpc_exceptions
394-407: LGTM! Clean CreateLease signature extension.The addition of the optional
begin_timeparameter with proper protobuf serialization is well-implemented. The conditional assignment (lines 404-407) only sets the field when provided, maintaining backward compatibility.
michalskrivanek
force-pushed
the
lease-monitor
branch
2 times, most recently
from
35db5bb to
f1c4f6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/client/lease.py (1)
254-270: Consider adding monitoring support for scheduled leases.Currently, scheduled leases (those with
begin_timebut not yet started) fall back to 1-second sleep intervals. Consider adding a branch to monitor scheduled leases more efficiently:if lease.effective_begin_time and lease.effective_duration: # ... existing active lease logic ... elif lease.begin_time: # Monitor scheduled lease expected_start = lease.begin_time time_until_start = expected_start - datetime.now().astimezone() if time_until_start > timedelta(0): await sleep(min(time_until_start.total_seconds(), check_interval)) else: await sleep(check_interval) # Started, waiting for effective times else: await sleep(1)This would provide better monitoring for leases scheduled to start in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Makefile(2 hunks)buf.gen.yaml(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/get_test.py(2 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(11 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- buf.gen.yaml
- packages/jumpstarter-cli/jumpstarter_cli/get.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#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/grpc.pypackages/jumpstarter/jumpstarter/client/lease.pypackages/jumpstarter/jumpstarter/config/client.pypackages/jumpstarter/jumpstarter/client/grpc_test.py
🧬 Code graph analysis (4)
packages/jumpstarter/jumpstarter/client/grpc.py (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-299)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(71-89)
packages/jumpstarter/jumpstarter/config/client.py (2)
packages/jumpstarter/jumpstarter/client/grpc.py (4)
ClientService(323-462)CreateLease(387-414)ListLeases(367-385)UpdateLease(416-454)packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
update_lease(27-41)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (13)
WithOptions(20-22)add_display_columns(25-35)Lease(121-239)get_status(221-239)Exporter(79-118)rich_add_columns(92-93)rich_add_columns(190-196)rich_add_columns(255-257)rich_add_columns(310-311)rich_add_rows(95-115)rich_add_rows(198-216)rich_add_rows(259-262)rich_add_rows(313-315)
packages/jumpstarter-cli/jumpstarter_cli/get_test.py (2)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
Lease(121-239)LeaseList(298-319)get_status(221-239)packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
create_test_lease(122-135)
⏰ 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). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (13)
packages/jumpstarter-cli/jumpstarter_cli/get_test.py (1)
245-348: LGTM! Comprehensive test coverage for lease filtering.The new
TestGetLeasesLogictest suite provides thorough coverage of server-side lease filtering behavior. The tests properly validate:
- Active-only filtering excluding expired leases
- Show-all mode including expired leases
- Multiple active leases handling
- All-expired scenarios
- Empty lease list handling
The helper function
create_test_leaseis well-designed with sensible defaults and clear parameter names.packages/jumpstarter/jumpstarter/client/lease.py (1)
251-270: Monitor logic correctly uses requested duration for release time.The monitoring logic correctly computes the expected release time using
effective_begin_time + duration(the configured/requested duration) rather thaneffective_duration(elapsed time so far), which aligns with the established learning.The 30-second check interval is appropriate for detecting external lease changes without excessive polling.
Based on learnings.
packages/jumpstarter/jumpstarter/config/client.py (3)
203-210: LGTM! API signature correctly extended with begin_time.The
create_leasemethod now properly accepts and forwards the optionalbegin_timeparameter to the underlying gRPC service, enabling scheduled lease creation.
230-238: Good default choice for only_active parameter.Setting
only_active=Trueas the default is a sensible choice that filters out expired leases by default, which is the most common use case.
244-249: LGTM! UpdateLease signature properly extended.The
update_leasemethod now correctly accepts optionaldurationandbegin_timeparameters and forwards them to the underlying service. The validation that at least one parameter must be provided is handled at the service layer.packages/jumpstarter/jumpstarter/client/grpc_test.py (3)
122-135: LGTM! Test helper properly reflects extended lease model.The
create_test_leasehelper method now includes all the new timing-related fields (effective_duration,begin_time,duration,effective_end_time), providing comprehensive test coverage for the expanded lease model.
340-354: Correct release time calculation in test.The test now correctly computes
expected_releaseusing the three-tier logic:
- Use
effective_end_timeif already ended- Use
effective_begin_time + durationfor active leases- Use
begin_time + durationfor scheduled leasesThis matches the production code and correctly uses
duration(configured/requested) rather thaneffective_duration(elapsed time so far).Based on learnings.
377-414: Excellent test coverage for scheduled leases.The new
test_exporter_scheduled_lease_expected_releasetest validates that scheduled leases (not yet started, withbegin_timebut noeffective_begin_time) correctly display their expected release time in the UI.packages/jumpstarter/jumpstarter/client/grpc.py (5)
100-112: Correct release time calculation across all lease states.The release time calculation correctly handles three scenarios:
- Ended leases: Uses
effective_end_time(actual end)- Active leases: Uses
effective_begin_time + duration(expected end based on requested duration)- Scheduled leases: Uses
begin_time + duration(expected end based on scheduled start)This correctly uses
duration(the configured/requested duration) rather thaneffective_duration(elapsed time so far) to show the expected release time.Based on learnings.
153-174: LGTM! Safe protobuf field parsing with HasField().The code correctly uses
HasField()to check for the presence of optional protobuf fields (effective_duration,begin_time,effective_end_time) before calling conversion methods. This prevents potentialTypeErrorexceptions when checking submessage truthiness.
198-207: Well-designed BEGIN TIME and DURATION display logic.The display logic correctly:
- Shows
effective_begin_timefor active leases,begin_timefor scheduled leases- Shows
effective_durationfor ended leases,durationfor active/scheduled leasesThis provides clear visibility into lease timing across all states.
223-225: Good status check for ended leases.Adding the check for
effective_end_timeto return "Ended" status ensures that completed leases are properly distinguished from active or expired ones.
416-442: Well-implemented UpdateLease with proper validation.The
UpdateLeasemethod correctly:
- Accepts optional
durationandbegin_timeparameters- Builds an update mask from only the provided fields
- Validates that at least one field is provided
- Properly serializes the fields to protobuf
The validation ensures that the method is called with meaningful updates.
we monitor lease status too frequently (every 5s) which doesn't correspond to the default grpc keepalive which we just made longer. Let's lighten the load a bit and poll less frequent, once in 30s. Changed the info message about lease expiring soon to more friendly "ending in 5 minutes". Also, use effective_begin_time + effective_duration for end time calculation. Once scheduled leases are implemented these may differ from begin_time and duration.
this allows to create scheduled leases that will start at the requested time. At begin_time the lease will start acquisition that may be delayed or fail if the selected exporter is not available, following the lease acquisition timeout setting.
"jmp get exporters --with leases" shows lease status and when is the lease expiring. That's more useful that current start time which doesn't tell you for how long it may be held. "jmp get leases" now shows lease begin time (the time it was acquired) in addition to duration. future/scheduled leases show their expected begin time and duration, currently active leases show actual begin time and actual duration so far.
this accounts for ended leases as well. If present it has the actual end time of the lease. change column to RELEASE TIME to make sense for ended leases as well
this indicates to the server that we want to see ended leases as well. Default behavior is to only get non-ended leases (pending or currently active)
Use "make protobuf-gen" to be consistent with controller. In python the imports need to be post-processed, unfortunately.
michalskrivanek
force-pushed
the
lease-monitor
branch
from
f1c4f6a to
b2ce590
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (1)
1-3: Remove outdated “NO CHECKED-IN PROTOBUF GENCODE” header comment. It conflicts with the project’s practice of tracking generated*_pb2.pyfiles—update or eliminate it to match current policy.
♻️ Duplicate comments (2)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py (1)
39-40: Generated code looks consistent.Same protobuf runtime pinning guidance as noted in kubernetes_pb2.py applies here.
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.py (1)
33-34: LGTM; descriptor options wiring aligns with other modules.Runtime version pinning advice is the same as previously noted.
🧹 Nitpick comments (1)
buf.gen.yaml (1)
11-11: Pin proto input to an immutable ref; avoidbranch: main. Also pin the gRPC Python plugin.Using a moving branch makes codegen non-reproducible and brittle. Pin to a tag or commit SHA, and version the
buf.build/grpc/pythonplugin for deterministic outputs.Apply for the proto input:
- branch: main + # Prefer a tag or commit SHA for reproducibility + # tag: vX.Y.Z + ref: <pinned-commit-sha>Also pin the gRPC Python plugin (choose a concrete version available on buf registry):
- - remote: buf.build/grpc/python + - remote: buf.build/grpc/python:vX.Y.Z out: ./packages/jumpstarter-protocol/jumpstarter_protocolIf you’d like, I can suggest a specific plugin version after confirming what you currently use in CI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Makefile(2 hunks)buf.gen.yaml(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/common.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/common_test.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/create.py(3 hunks)packages/jumpstarter-cli/jumpstarter_cli/get.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/get_test.py(2 hunks)packages/jumpstarter-cli/jumpstarter_cli/update.py(2 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py(3 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/common_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/jumpstarter_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py(1 hunks)packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py(1 hunks)packages/jumpstarter/jumpstarter/client/grpc.py(11 hunks)packages/jumpstarter/jumpstarter/client/grpc_test.py(11 hunks)packages/jumpstarter/jumpstarter/client/lease.py(1 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/jumpstarter-cli/jumpstarter_cli/common.py
- packages/jumpstarter/jumpstarter/client/lease.py
- Makefile
- packages/jumpstarter-cli/jumpstarter_cli/get.py
- packages/jumpstarter-cli/jumpstarter_cli/update.py
- packages/jumpstarter-cli/jumpstarter_cli/common_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T17:43:07.788Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#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 (5)
packages/jumpstarter-cli/jumpstarter_cli/get_test.py (2)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
Lease(121-239)LeaseList(298-319)get_status(221-239)packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
create_test_lease(122-135)
packages/jumpstarter-cli/jumpstarter_cli/create.py (3)
packages/jumpstarter-cli-common/jumpstarter_cli_common/config.py (1)
opt_config(99-100)packages/jumpstarter-cli/jumpstarter_cli/login.py (1)
relogin_client(141-155)packages/jumpstarter/jumpstarter/config/client.py (2)
create_lease(199-210)lease(135-143)
packages/jumpstarter/jumpstarter/config/client.py (2)
packages/jumpstarter/jumpstarter/client/grpc.py (4)
ClientService(323-462)CreateLease(387-414)ListLeases(367-385)UpdateLease(416-454)packages/jumpstarter-cli/jumpstarter_cli/update.py (1)
update_lease(27-41)
packages/jumpstarter/jumpstarter/client/grpc.py (3)
packages/jumpstarter/jumpstarter/config/client.py (1)
lease(135-143)packages/jumpstarter/jumpstarter/client/lease.py (1)
Lease(41-299)packages/jumpstarter/jumpstarter/common/grpc.py (1)
translate_grpc_exceptions(71-89)
packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
packages/jumpstarter/jumpstarter/client/grpc.py (13)
WithOptions(20-22)add_display_columns(25-35)Lease(121-239)get_status(221-239)Exporter(79-118)rich_add_columns(92-93)rich_add_columns(190-196)rich_add_columns(255-257)rich_add_columns(310-311)rich_add_rows(95-115)rich_add_rows(198-216)rich_add_rows(259-262)rich_add_rows(313-315)
⏰ 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: build
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- 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: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (13)
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/client/v1/client_pb2.py (1)
37-146: Generated code structure is correct.The protobuf-generated code is well-formed and consistent:
- The serialized descriptor blob (line 37) contains the updated schema definition
- Field options are properly configured for the new
only_activefield inListLeasesRequest(lines 93-94)- Message boundary positions are correctly updated to reflect the schema changes (lines 126-146)
The generated code correctly reflects the underlying protobuf schema modifications mentioned in the AI summary:
- Addition of
only_activefield toListLeasesRequest- Updates to
Leasemessage structure (effective timing fields)- Adjusted descriptor region boundaries
However, ensure the source
.protofile changes are thoroughly reviewed, as this file is automatically generated from that source.packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/router_pb2.py (1)
33-34: LGTM for generated descriptor wiring.Matches current protoc Python generation patterns; no action needed.
packages/jumpstarter-cli/jumpstarter_cli/create.py (1)
1-53: LGTM! Well-integrated begin_time support.The addition of
begin_timeparameter is cleanly integrated:
- Proper imports added
- Decorator applied correctly
- Function signature updated
- Parameter forwarded to the underlying service
packages/jumpstarter-cli/jumpstarter_cli/get_test.py (1)
245-348: LGTM! Comprehensive test coverage for lease filtering scenarios.The new
TestGetLeasesLogicclass provides good coverage of different lease states and filtering scenarios. The helper method and test cases properly simulate server-side filtering behavior through data structure construction.packages/jumpstarter/jumpstarter/config/client.py (3)
199-210: LGTM! Correct begin_time parameter addition.The
create_leasemethod properly accepts and forwards the optionalbegin_timeparameter to the underlying service.
225-238: LGTM! Well-designed only_active parameter.The addition of
only_activeparameter with a default ofTruemaintains backward compatibility while enabling control over lease filtering.
242-249: LGTM! Flexible update_lease signature.The updated method signature correctly supports updating either
duration,begin_time, or both, providing flexibility for lease modifications.packages/jumpstarter/jumpstarter/client/grpc_test.py (2)
122-354: LGTM! Test updates correctly reflect new display semantics.The test updates properly validate the new BEGIN TIME and RELEASE TIME display logic. The helper method and assertions correctly verify release time calculations (e.g., 10:00:00 + 1h = 11:00:00).
377-414: LGTM! Excellent test coverage for scheduled leases.The new test properly validates that scheduled leases (not yet started) display the expected release time calculated from
begin_time + duration.packages/jumpstarter/jumpstarter/client/grpc.py (4)
100-112: LGTM! Correct release time calculation logic.The three-branch logic correctly calculates release time for all lease states:
- Ended leases: use actual end time
- Active leases:
effective_begin_time + duration(configured duration)- Scheduled leases:
begin_time + durationThe use of
duration(noteffective_duration) is correct, aseffective_durationrepresents elapsed time so far.Based on learnings.
126-187: LGTM! Proper Lease model extension.The new fields (
effective_duration,begin_time,effective_end_time) are correctly:
- Declared in the model with appropriate types
- Parsed from protobuf using
HasField()for presence checks- Converted with proper timezone handling
- Passed to the Lease constructor
193-216: LGTM! Clear and comprehensive rendering logic.The BEGIN TIME and DURATION display logic effectively handles all lease states:
- BEGIN TIME: Shows actual start time when active, scheduled time otherwise
- DURATION: Shows actual duration for ended leases, requested duration otherwise
This provides users with the most relevant information for each lease state.
387-454: LGTM! Well-designed API updates with proper validation.Both
CreateLeaseandUpdateLeaseare correctly updated:
CreateLeaseconditionally includesbegin_timein the protobuf messageUpdateLeasesupports updatingduration,begin_time, or both- Validation ensures at least one field is provided in updates
FieldMaskis properly constructed from the updated fields
| _globals['DESCRIPTOR']._loaded_options = None | ||
| _globals['DESCRIPTOR']._serialized_options = b'\n\022com.jumpstarter.v1B\017KubernetesProtoP\001\242\002\003JXX\252\002\016Jumpstarter.V1\312\002\016Jumpstarter\\V1\342\002\032Jumpstarter\\V1\\GPBMetadata\352\002\017Jumpstarter::V1' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Ensure protobuf runtime is pinned to match the generated code (6.30.1).
Generated files enforce ValidateProtobufRuntimeVersion(6.30.1). Pin Python package protobuf to a compatible range (e.g., >=6.30.1,<7) in your packaging to avoid runtime ImportError at import time.
Run to verify dependency declarations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for protobuf runtime pins..."
# Common places: pyproject, setup.cfg, setup.py, requirements*, constraints*, Pipfile, poetry files
fd -a -E node_modules -E dist -E build '^(pyproject\.toml|setup\.cfg|setup\.py|requirements.*\.txt|constraints.*\.txt|Pipfile|poetry\.lock)$' || true
echo
rg -nC2 -i '\bprotobuf\b' || trueLength of output: 39960
Pin protobuf dependency to <7.0.0>
In packages/jumpstarter-protocol/pyproject.toml update the protobuf specifier on line 12 from
"protobuf>=6.30.1",
to
"protobuf>=6.30.1,<7.0.0",
so it matches ValidateProtobufRuntimeVersion(6.30.1) and avoids import‐time errors.
🤖 Prompt for AI Agents
In
packages/jumpstarter-protocol/jumpstarter_protocol/jumpstarter/v1/kubernetes_pb2.py
around lines 33 to 34, the generated file indicates runtime require protobuf
6.30.1 but the project pyproject currently allows protobuf >=6.30.1 (potentially
>=7); update packages/jumpstarter-protocol/pyproject.toml line 12 to pin the
protobuf dependency to "protobuf>=6.30.1,<7.0.0" so the installed runtime
matches ValidateProtobufRuntimeVersion(6.30.1) and prevents import-time
incompatibilities.
mangelajo
changed the title
|
Successfully created backport PR for |
2 participants
Summary by CodeRabbit
New Features
Improvements
CLI
Tests
Chores