feat: show expires at and remaining columns in lease listing#343
Open
raballew wants to merge 3 commits intojumpstarter-dev:mainfrom
Open
feat: show expires at and remaining columns in lease listing#343raballew wants to merge 3 commits intojumpstarter-dev:mainfrom
raballew wants to merge 3 commits intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughReplaces lease listing columns "BEGIN TIME" and "DURATION" with "EXPIRES AT" and "REMAINING", adds Lease helpers to compute expiration and remaining time, and adds unit and end-to-end tests validating the new display and computation logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
e2e/tests.bats (1)
345-357: Make this E2E test self-contained to avoid order dependence.This test depends on suite-prepared state (
test-client-oidc, active exporters) and usesjmp delete leases --all, which broadens side effects beyond the lease created here. Please make setup/cleanup local to this test so it can run deterministically in isolation.Based on learnings: "E2E tests in bats should be runnable in isolation. Do not rely on prior suite setup or shared state across tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests.bats` around lines 345 - 357, This test is order-dependent because it relies on pre-existing client and exporter state and uses a global cleanup; modify the test to be self-contained by creating and using a unique test client (instead of relying on "jmp config client use test-client-oidc"), ensure any required exporter is started or validated with wait_for_exporter, create the lease with a unique selector/label so it’s identifiable, capture the created lease identifier from the "jmp create lease" output, assert on "jmp get leases" as before, and then delete only that specific lease (and the test client) rather than calling "jmp delete leases --all" so setup and teardown are local to the test.python/packages/jumpstarter/jumpstarter/client/grpc_test.py (1)
443-449: Add a deterministic non-expiredREMAININGassertion.Current tests validate
expiredandNone, but not the positive formatting output (Xd Yh Zm). Adding one fixed-clock test would better protect the new display behavior from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/grpc_test.py` around lines 443 - 449, Add a deterministic positive-case test for Lease._format_remaining that asserts a future datetime produces the expected "Xd Yh Zm" string: create a fixed "now" (e.g., datetime(2020,1,1,0,0,0)), compute a future time (e.g., now + timedelta(days=1, hours=2, minutes=3)), call Lease._format_remaining(future_time) and assert it equals "1d 2h 3m"; add this as test_format_remaining_future in grpc_test.py and ensure the test uses the fixed clock (or computes expected output from that fixed reference) so it is deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests.bats`:
- Around line 345-357: This test is order-dependent because it relies on
pre-existing client and exporter state and uses a global cleanup; modify the
test to be self-contained by creating and using a unique test client (instead of
relying on "jmp config client use test-client-oidc"), ensure any required
exporter is started or validated with wait_for_exporter, create the lease with a
unique selector/label so it’s identifiable, capture the created lease identifier
from the "jmp create lease" output, assert on "jmp get leases" as before, and
then delete only that specific lease (and the test client) rather than calling
"jmp delete leases --all" so setup and teardown are local to the test.
In `@python/packages/jumpstarter/jumpstarter/client/grpc_test.py`:
- Around line 443-449: Add a deterministic positive-case test for
Lease._format_remaining that asserts a future datetime produces the expected "Xd
Yh Zm" string: create a fixed "now" (e.g., datetime(2020,1,1,0,0,0)), compute a
future time (e.g., now + timedelta(days=1, hours=2, minutes=3)), call
Lease._format_remaining(future_time) and assert it equals "1d 2h 3m"; add this
as test_format_remaining_future in grpc_test.py and ensure the test uses the
fixed clock (or computes expected output from that fixed reference) so it is
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13b28fb4-15ef-4f17-9102-6798826273fe
📒 Files selected for processing (3)
e2e/tests.batspython/packages/jumpstarter/jumpstarter/client/grpc.pypython/packages/jumpstarter/jumpstarter/client/grpc_test.py
…rter-dev#32) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
force-pushed
the
022-lease-expiration-display
branch
from
f958df9 to
6c0aafe
Compare
…eases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
raballew
force-pushed
the
022-lease-expiration-display
branch
2 times, most recently
from
212a314 to
583f539
Compare
Rich wraps the "EXPIRES AT" column header across two lines when the terminal is narrower than the table, causing the substring assertion to fail in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BEGIN TIMEandDURATIONcolumns withEXPIRES ATandREMAININGinjmp get leasesoutputEXPIRES ATshows the calculated expiration timestampREMAININGshows human-readable time left (e.g.2h 15m) orexpiredFixes #32
Test plan
test_rich_add_columns_has_expires_at_and_remaining)test_compute_expires_at_*)test_format_remaining_*)test_rich_add_rows_*)jmp get leasesoutput contains EXPIRES AT and REMAINING columns🤖 Generated with Claude Code