load ssh identity lazily#715
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe SSH driver's identity file read was moved from eager initialization to lazy, on-first-use loading: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant SSHWrapper
participant FS as FileSystem
Caller->>SSHWrapper: call get_ssh_identity()
alt ssh_identity cached
SSHWrapper-->>Caller: return cached ssh_identity
else ssh_identity missing & ssh_identity_file set
SSHWrapper->>FS: read `ssh_identity_file`
alt file read success
FS-->>SSHWrapper: file contents
SSHWrapper-->>Caller: return and cache ssh_identity
else read error
FS-->>SSHWrapper: read error
SSHWrapper--x Caller: raise ConfigurationError
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
46-49: Prefer specific exception types over broadExceptioncatch.Catching all exceptions can mask unexpected errors. File operations typically raise
OSErroror its subclasses.- except Exception as e: - raise ConfigurationError(f"Failed to read ssh_identity_file '{self.ssh_identity_file}': {e}") from None + except OSError as e: + raise ConfigurationError(f"Failed to read ssh_identity_file '{self.ssh_identity_file}': {e}") from eNote: Also removed
from Noneto preserve the exception chain for better debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py(1 hunks)
⏰ 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). (5)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
|
@michalskrivanek there is a test that needs fixing: FAILED jumpstarter_driver_ssh/driver_test.py::test_ssh_identity_file_read_error - Failed: DID NOT RAISE <class 'jumpstarter.common.exceptions.ConfigurationError'> |
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
FAILED jumpstarter_driver_ssh/driver_test.py::test_ssh_identity_file_read_error - Failed: DID NOT RAISE <class 'jumpstarter.common.exceptions.ConfigurationError'>
This one needs fixing
on first use, to prevent a failure on startup if the file doesn't exist yet
Head branch was pushed to by a user without write access
michalskrivanek
force-pushed
the
lazy-ssh
branch
from
2f775e3 to
af48116
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
44-49: Lazy load looks good; consider path expansion + explicit UTF-8 read.This preserves the new behavior. Two small hardening tweaks:
- Expand env vars and ~ in ssh_identity_file.
- Read with explicit UTF-8 to avoid platform-default surprises.
Apply within this block:
- if self.ssh_identity is None and self.ssh_identity_file: - try: - self.ssh_identity = Path(self.ssh_identity_file).read_text() - except Exception as e: - raise ConfigurationError(f"Failed to read ssh_identity_file '{self.ssh_identity_file}': {e}") from None + if self.ssh_identity is None and self.ssh_identity_file: + try: + path = Path(os.path.expandvars(self.ssh_identity_file)).expanduser() + self.ssh_identity = path.read_text(encoding="utf-8") + except Exception as e: + raise ConfigurationError(f"Failed to read ssh_identity_file '{self.ssh_identity_file}': {e}") from NoneAdd import at top:
-from dataclasses import dataclass -from pathlib import Path +from dataclasses import dataclass +import os +from pathlib import Path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
get_ssh_identity(42-50)SSHWrapper(9-50)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)
⏰ 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 (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
394-405: LGTM: validates lazy load and caching semantics.Asserts None before first use, then value cached after first read. Clear and aligned with new behavior.
424-431: LGTM: fixes the previously failing expectation (error on first use).Instance creation succeeds; ConfigurationError is raised on get_ssh_identity(). Matches the lazy-load design and resolves the earlier test failure.
Also applies to: 434-434
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.7
git worktree add -d .worktree/backport-715-to-release-0.7 origin/release-0.7
cd .worktree/backport-715-to-release-0.7
git switch --create backport-715-to-release-0.7
git cherry-pick -x af481163563459b69089b230a27a59de93847df0 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.7
git worktree add -d .worktree/backport-715-to-release-0.7 origin/release-0.7
cd .worktree/backport-715-to-release-0.7
git switch --create backport-715-to-release-0.7
git cherry-pick -x af481163563459b69089b230a27a59de93847df0 |
2 participants
on first use, to prevent a failure on startup if the file doesn't exist yet
Summary by CodeRabbit