Lease progress details while users wait#688
Conversation
WalkthroughAdds LeaseAcquisitionSpinner and integrates it into the lease acquisition flow to provide terminal-aware spinner/status updates, per-tick progress, and state-driven messages; includes a new comprehensive test suite for the spinner behavior and async interactions. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client/_acquire
participant LAS as LeaseAcquisitionSpinner
participant LS as Lease Service
Note over C,LAS: Start acquisition context
C->>LAS: enter() (creates Console if TTY)
LAS-->>C: ready to update
loop every 1s (up to N ticks)
C->>LS: get_lease_status()
LS-->>C: status
C->>LAS: update_status(status)
alt status == Ready
LAS-->>C: display success message (stop spinner)
C-->>C: break loop
else status == Unsatisfiable/Invalid/Released
C-->>C: raise appropriate error / handle
else
LAS-->>LAS: set message (Pending/... with progress)
C->>LAS: tick() (spinner.tick or log elapsed)
end
end
C->>LAS: exit() (stop spinner/cleanup)
Note over C,LAS: Acquisition finished
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter/jumpstarter/client/lease.py(4 hunks)packages/jumpstarter/jumpstarter/client/lease_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/common/condition.py (4)
condition_true(26-27)condition_message(16-23)condition_false(30-31)condition_present_and_equal(6-13)
packages/jumpstarter/jumpstarter/client/lease_test.py (1)
packages/jumpstarter/jumpstarter/client/lease.py (4)
LeaseAcquisitionSpinner(300-354)_is_terminal_available(311-318)update_status(335-346)tick(348-354)
⏰ 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: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- 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)
- GitHub Check: pytest-matrix (macos-15, 3.11)
|
so for colors, there is https://no-color.org/ that rich obeys, but for spinners, etc, asking chatgpt: Disable interactivity Disable progress bars Force plain output I'd propose we use NONINTERACTIVE, WDYT? |
mangelajo
force-pushed
the
lease-waiting
branch
from
535ea50 to
537a587
Compare
mangelajo
force-pushed
the
lease-waiting
branch
from
537a587 to
8316398
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/client/lease.py (1)
316-323: Consider using Rich Console's built-in terminal detection.The manual TTY check reimplements what Rich's Console already provides. Rich Console has built-in methods and respects its own environment variables (e.g.,
TTY_INTERACTIVE,TTY_COMPATIBLEfrom Rich 14.1.0).Based on learnings about Rich 14.1.0, consider using Rich's built-in detection:
def _is_terminal_available(self) -> bool: - """Check if we're running in a terminal/TTY.""" - return ( - hasattr(sys.stdout, 'isatty') and - sys.stdout.isatty() and - hasattr(sys.stderr, 'isatty') and - sys.stderr.isatty() - ) + """Check if we're running in a terminal/TTY using Rich's detection.""" + return self.console.is_terminalThis leverages Rich's sophisticated terminal detection which handles edge cases and respects Rich's own environment variables. Note: You'll need to move this check to after Console initialization.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter/jumpstarter/client/lease.py(4 hunks)packages/jumpstarter/jumpstarter/client/lease_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter/jumpstarter/client/lease_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter/jumpstarter/client/lease.py (2)
packages/jumpstarter/jumpstarter/common/condition.py (4)
condition_true(26-27)condition_message(16-23)condition_false(30-31)condition_present_and_equal(6-13)packages/jumpstarter/jumpstarter/client/exceptions.py (1)
LeaseError(4-5)
⏰ 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). (4)
- GitHub Check: e2e
- 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)
🔇 Additional comments (5)
packages/jumpstarter/jumpstarter/client/lease.py (5)
2-3: LGTM!The new imports (
os,sys,rich.console.Console) are necessary for the spinner functionality and terminal detection.Also applies to: 25-25
118-128: LGTM!The status update logic correctly handles different lease conditions and provides appropriate user feedback. The use of
condition_trueandcondition_messagehelpers ensures consistency with the codebase's condition-checking patterns.Note: Line 127 has a typo ("server to" with extra space) that was already flagged in a previous review.
312-314: Document environment variable handling and consider Rich's TTY_INTERACTIVE.The
NONINTERACTIVEenvironment variable is a sensible choice for controlling interactive features. However, Rich 14.1.0 introducedTTY_INTERACTIVEfor this exact purpose. You should either:
- Document that
NONINTERACTIVEis the preferred way to disable interactivity in this codebase- Consider also checking
TTY_INTERACTIVE=0for consistency with Rich's conventions- Note that Rich's Console will independently respect
NO_COLOR,FORCE_COLOR, etc.From PR comments, you're investigating standard environment variables that Rich follows. The learnings confirm Rich 14.1.0 supports
TTY_INTERACTIVEto force interactive mode on/off. Consider checking both:def _is_non_interactive(self) -> bool: - """Check if the user desires a NONINTERACTIVE environment.""" - return os.environ.get("NONINTERACTIVE", "false").lower() in ["true", "1"] + """Check if the user desires a NONINTERACTIVE environment. + + Respects both NONINTERACTIVE (jumpstarter convention) and TTY_INTERACTIVE (Rich convention). + """ + noninteractive = os.environ.get("NONINTERACTIVE", "false").lower() in ["true", "1"] + tty_interactive = os.environ.get("TTY_INTERACTIVE", "1") == "0" + return noninteractive or tty_interactiveBased on learnings
325-359: LGTM!The context manager implementation is clean with proper resource management. The
update_statusmethod provides good fallback behavior by logging tologger.infowhen no terminal is available, and thetickmethod maintains a consistent UX by updating the elapsed time without changing the message.
136-178: Add fallback logging for non-interactive environments
update_statusonly updates the spinner when_should_show_spinneris true, so on non-TTY runs the success message (with or without emoji) is never emitted. Introduce a fallback—e.g. calllogger.info(success_msg)(stripped of emoji if preferred) when_should_show_spinneris false—to ensure the “Lease … acquired” message appears in CI or non-Unicode logs.Likely an incorrect or invalid review comment.
mangelajo
force-pushed
the
lease-waiting
branch
from
8316398 to
0e2be15
Compare
|
oops, now it's really updated, I had pushed branch to the wrong repo. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/client/lease.py (2)
312-314: Consider additional environment variables for broader compatibility.The current implementation checks only
NONINTERACTIVE, which aligns with the PR proposal. However, the PR comments mention several other widely-used environment variables that indicate non-interactive environments:
CI=1(standard CI environment indicator)TERM=dumb(non-interactive terminal)NO_PROGRESS=1(suppress progress bars)NO_SPINNER=1(suppress spinners)Consider expanding the check to support these additional variables for better compatibility with various CI/CD systems and tools.
Example implementation:
def _is_non_interactive(self) -> bool: - """Check if the user desires a NONINTERACTIVE environment.""" - return os.environ.get("NONINTERACTIVE", "false").lower() in ["true", "1"] + """Check if the environment indicates non-interactive mode.""" + # Check various environment variables that indicate non-interactive mode + if os.environ.get("NONINTERACTIVE", "false").lower() in ["true", "1"]: + return True + if os.environ.get("CI", "false").lower() in ["true", "1"]: + return True + if os.environ.get("TERM", "") == "dumb": + return True + if os.environ.get("NO_PROGRESS", "false").lower() in ["true", "1"]: + return True + if os.environ.get("NO_SPINNER", "false").lower() in ["true", "1"]: + return True + return False
316-323: Consider leveraging Rich's built-in TTY detection.Rich 14.1.0 introduced the
TTY_INTERACTIVEenvironment variable specifically for controlling interactive mode. Instead of implementing custom TTY detection, you could use Rich'sConsole.is_interactiveproperty which respectsTTY_INTERACTIVEand other Rich-specific environment variables.Based on learnings
Example implementation:
-def _is_terminal_available(self) -> bool: - """Check if we're running in a terminal/TTY.""" - return ( - hasattr(sys.stdout, 'isatty') and - sys.stdout.isatty() and - hasattr(sys.stderr, 'isatty') and - sys.stderr.isatty() - ) +def _is_terminal_available(self) -> bool: + """Check if we're running in a terminal/TTY.""" + # Rich's is_interactive respects TTY_INTERACTIVE and other env vars + return self.console.is_interactiveThen simplify line 309:
-self._should_show_spinner = self._is_terminal_available() and not self._is_non_interactive() +self._should_show_spinner = self._is_terminal_available() and not self._is_non_interactive()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter/jumpstarter/client/lease.py(4 hunks)packages/jumpstarter/jumpstarter/client/lease_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter/jumpstarter/client/lease.py (1)
packages/jumpstarter/jumpstarter/common/condition.py (4)
condition_true(26-27)condition_message(16-23)condition_false(30-31)condition_present_and_equal(6-13)
packages/jumpstarter/jumpstarter/client/lease_test.py (1)
packages/jumpstarter/jumpstarter/client/lease.py (4)
LeaseAcquisitionSpinner(301-359)_is_terminal_available(316-323)update_status(340-351)tick(353-359)
⏰ 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 (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 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)
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter/jumpstarter/client/lease.py (4)
2-3: LGTM!The new imports (
os,sys,Console) are necessary for environment variable checking, TTY detection, and spinner display functionality.Also applies to: 25-25
118-127: LGTM!The status update logic correctly interprets lease conditions and provides appropriate user feedback. The use of existing condition helper functions ensures consistency with the rest of the codebase.
134-179: LGTM!The refactored acquisition flow correctly integrates the spinner, handles all lease states appropriately, and provides tick-based progress updates for better UX. The previous timeout concern has been addressed by breaking immediately after success message.
325-359: LGTM!The context manager and update methods are well-implemented:
__enter__and__exit__properly manage spinner lifecycleupdate_statusprovides appropriate fallback to logging when no consoletickcorrectly preserves the message while updating elapsed time- The hardcoded color styling (
[blue],[dim]) is fine—Rich automatically respectsNO_COLORand related environment variables for color output controlpackages/jumpstarter/jumpstarter/client/lease_test.py (1)
1-230: Excellent test coverage!The test suite is comprehensive and well-structured:
- Covers initialization with and without lease name
- Validates TTY detection across various scenarios (all-tty, no-tty, partial-tty)
- Tests context manager behavior with and without console
- Verifies status updates and tick behavior in different modes
- Tests elapsed time formatting and message preservation
- Includes async integration test
- Uses appropriate mocking techniques
All test cases are relevant and correctly validate the
LeaseAcquisitionSpinnerbehavior.
(cherry picked from commit 5f3a046)
|
Successfully created backport PR for |
3 participants
when no tty:
Summary by CodeRabbit
New Features
Tests