add tracking to created resources in OpenDAL#657
add tracking to created resources in OpenDAL#657mangelajo merged 6 commits intojumpstarter-dev:mainfrom
Conversation
WalkthroughAdds created-resource tracking and a public retrieval API to the OpenDAL driver and client, exposes optional automatic removal of tracked resources on driver close, propagates the flag to multiple drivers (HTTP, TFTP, ISCSI, RidesX, Flashers), and updates tests and documentation (including port-aware HTTP tests). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Driver as Driver (Http/Tftp/ISCSI/Flashers)
participant Opendal as Opendal
participant FS as Backend FS
rect rgb(245,250,255)
note right of Opendal: Track created resources in _created_paths
User->>Driver: create_dir / open(write) / copy / rename / delete
Driver->>Opendal: perform operation(...)
Opendal->>FS: filesystem operation
FS-->>Opendal: result
Opendal-->>Opendal: update _created_paths
Opendal-->>Driver: success
end
rect rgb(245,255,240)
note over Driver,Opendal: Close phase (optional cleanup)
User->>Driver: close()
Driver->>Opendal: close()
alt remove_created_on_close == true
Opendal->>FS: remove tracked files/dirs
FS-->>Opendal: removed / errors handled
else remove_created_on_close == false
Opendal-->>Opendal: keep created resources
end
Opendal-->>Driver: closed
Driver-->>User: done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 0
🧹 Nitpick comments (7)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (2)
11-17: Avoid port TOCTOU races: use pytest’s unused_tcp_port fixture instead of get_free_port.Binding to port 0 and releasing it is racy. Prefer pytest’s built-in fixture for reliability.
Apply this diff to remove the helper:
-def get_free_port() -> int: - """Get a free port using socket binding""" - with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: - s.bind(('', 0)) - s.listen(1) - port = s.getsockname()[1] - return portThen, in the http fixture, use the fixture-provided port:
- port = get_free_port() + port = unused_tcp_portAnd update the fixture signature (outside the selected lines) to:
@pytest.fixture def http(tmp_path, unused_tcp_port): ...Based on learnings
27-28: Fixture lifecycle LGTM; minor tweak to use pytest’s port fixture.Start/stop/close flow looks correct. Replace get_free_port() with unused_tcp_port as noted above to reduce flakiness.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
397-425: New tracking APIs on the client look consistent and type-validated.Return types align with the driver endpoints; validate_call(validate_return=True) is appropriate.
Optionally expose CLI commands for visibility (non-blocking):
- opendal get-created-files
- opendal get-created-dirs
- opendal get-all-created
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (4)
31-35: _file_paths is currently unused — either wire it in or remove to avoid confusion.It’s set in open() but never read. If the intent is lifecycle accounting, consider cleaning all maps on file_close; otherwise drop the field.
As an example (outside the selected lines), update file_close to prevent leaks:
@export @validate_call(validate_return=True) async def file_close(self, /, fd: UUID) -> None: await self._fds[fd].close() self._fds.pop(fd, None) self._metadata.pop(fd, None) self._file_paths.pop(fd, None)Based on learnings
157-160: Normalize directory paths in tracking to a consistent form.Avoid mixing "dir" and "dir/". Store trailing-slashed paths in _created_dirs.
Apply this minimal diff:
- self._created_dirs.add(path) + self._created_dirs.add(path if path.endswith("/") else f"{path.rstrip('/')}/")
222-236: Expose tracking getters with validation for consistency.Consider adding validate_call(validate_return=True) like other endpoints for uniformity (optional).
@export +@validate_call(validate_return=True) async def get_created_files(self) -> list[str]: ...Repeat for get_created_directories and get_all_created_resources.
140-149: Track creations for copy() targets as well.copy() results in a newly created target; add it to _created_files for completeness with the stated policy.
Outside the selected lines, update copy():
@export @validate_call(validate_return=True) async def copy(self, /, source: str, target: str): await self._operator.copy(source, target) self._created_files.add(target)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#584
File: packages/jumpstarter/jumpstarter/client/grpc.py:38-54
Timestamp: 2025-08-18T07:13:37.619Z
Learning: User michalskrivanek prefers inline code over small helper function extractions when the abstraction doesn't significantly improve readability or reduce complexity. They value straightforward, readable code over reducing minor duplication.
🧬 Code graph analysis (3)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (3)
get_created_files(223-225)get_created_directories(228-230)get_all_created_resources(233-235)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (4)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (4)
HttpServer(23-136)client(49-51)start(54-71)stop(74-87)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (7)
client(37-38)client(256-257)client(292-293)client(313-314)get_created_files(223-225)get_created_directories(228-230)get_all_created_resources(233-235)packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py (3)
start(13-20)put_file(59-73)stop(22-30)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (3)
get_created_files(398-405)get_created_directories(408-415)get_all_created_resources(418-425)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (5)
open(224-232)get_created_files(398-405)get_created_directories(408-415)get_all_created_resources(418-425)close(138-142)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
close(32-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 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: e2e
🔇 Additional comments (5)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (2)
74-106: Good coverage for OpenDAL tracking on server close.Solid assertions that created files are tracked pre-close; logging is a nice-to-have but not necessary for correctness.
107-135: API exercise for get_created_ looks good.*Covers empty state, file creation, and combined getter. Nice.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (3)
48-64: Tracking on write/append modes aligns with the PR intent; confirm append semantics are desired.You add files for ("wb","w","ab","a") regardless of prior existence. If “append to existing file” should count as a “created” resource per your policy, this is correct. Otherwise, gate on pre-open stat metadata.
If you want to only track brand-new files, change to:
- if mode in ("wb", "w", "ab", "a"): - self._created_files.add(path) + if mode in ("wb", "w", "ab", "a") and metadata is None: + self._created_files.add(path)
219-221: LGTM: copy_exporter_file marks target as created.Matches the tracking policy and complements open() tracking.
238-252: Close() summary logging is helpful and non-intrusive.Debug-level summary with sorted entries is a good balance.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
looks great, just some comments on methods I think we don't really need.
| The driver provides methods to inspect what has been created: | ||
|
|
||
| ```python | ||
| # Get list of created files | ||
| created_files = await driver.get_created_files() | ||
|
|
||
| # Get list of created directories | ||
| created_dirs = await driver.get_created_directories() | ||
|
|
||
| # Get both as a tuple: (directories, files) | ||
| dirs, files = await driver.get_all_created_resources() | ||
| ``` |
There was a problem hiding this comment.
no, it's not necessary. just....originally i was wondering if it could be handy to retrieve stats of what has been created/written so far. it's a pretty low level driver with others deriving from it.
It's easy to remove if you prefer.
michalskrivanek
force-pushed
the
opendal-cleanup
branch
from
d09a48b to
1ece217
Compare
|
let me try to simplify by tracking only a single full path for both files and dirs, and just remove whatever the resulting type is. as we also need to do that for copy() and rename() and in the end we don't really care what are we tracking until the end when we remove it |
michalskrivanek
force-pushed
the
opendal-cleanup
branch
from
1ece217 to
4f80e43
Compare
michalskrivanek
force-pushed
the
opendal-cleanup
branch
from
4f80e43 to
f7dff84
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (1)
124-141: Fix HttpServer.close: async cleanup path is skipped in sync contexts.anyio.get_current_task() raises outside an async task, triggering the warning and skipping cleanup; sockets may remain open. Use asyncio loop detection and run/schedule cleanup accordingly.
Apply:
@@ - def close(self): - if self.runner: - try: - if anyio.get_current_task(): - anyio.from_thread.run(self._async_cleanup) - except Exception as e: - self.logger.warning(f"HTTP server cleanup failed synchronously: {e}") - self.runner = None - super().close() + def close(self): + if self.runner: + try: + import asyncio + try: + loop = asyncio.get_running_loop() + except RuntimeError: + # No running loop in this thread: run cleanup to completion. + asyncio.run(self._async_cleanup()) + else: + # In an event loop: schedule best-effort cleanup. + loop.create_task(self._async_cleanup()) + except Exception as e: + self.logger.warning(f"HTTP server cleanup failed: {e}") + self.runner = None + super().close()And ensure asyncio is imported if not already:
-import anyio +import anyio +import asyncio
🧹 Nitpick comments (3)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
34-39: Make cleanup policy configurable (avoid hardcoding True).Plumb a BaseFlasher-level flag and pass it through so callers can disable cleanup for persistent roots; or at minimum, rely on the child defaults (already True) to reduce duplication. Based on learnings.
Apply:
- self.children["tftp"] = Tftp(root_dir=self.tftp_dir, remove_created_on_close=True) + self.children["tftp"] = Tftp(root_dir=self.tftp_dir, remove_created_on_close=self.remove_created_on_close)- self.children["http"] = HttpServer(root_dir=self.http_dir, remove_created_on_close=True) + self.children["http"] = HttpServer(root_dir=self.http_dir, remove_created_on_close=self.remove_created_on_close)Add the field to BaseFlasher:
@dataclass(kw_only=True) class BaseFlasher(Driver): """driver for Jumpstarter""" + remove_created_on_close: bool = TruePlease confirm http_dir/tftp_dir point to ephemeral locations in your deployments to avoid accidental cleanup of shared content. If needed, I can add a follow-up PR to wire this into config.
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py (1)
28-32: Expose cleanup toggle on the driver (don’t hardcode True).Mirror other drivers and allow customizing via a RideSXDriver field; pass it through to Opendal.
@dataclass(kw_only=True) class RideSXDriver(Driver): """RideSX Driver""" storage_dir: str = field(default="/var/lib/jumpstarter/ridesx") + remove_created_on_close: bool = True @@ - self.children["storage"] = Opendal( + self.children["storage"] = Opendal( scheme="fs", kwargs={"root": self.storage_dir}, - remove_created_on_close=True # Clean up temporary firmware files on close + remove_created_on_close=self.remove_created_on_close # Clean up temporary firmware files on close )Given decompression writes into storage_dir, confirm it’s an isolated path so cleanup can safely remove created files at close.
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
64-68: Propagation looks correct; clarify expectations for cleanup scope.You forward remove_created_on_close to the Opendal child, but LUN files are created directly via os.open/truncate in this driver, so they won’t be tracked or cleaned by Opendal. If users set remove_created_on_close=True on ISCSI, they may expect image cleanup. Consider documenting this or (in a follow-up) routing file creation via the storage child so it’s tracked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/source/reference/package-apis/drivers/opendal.yaml(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(1 hunks)packages/jumpstarter-driver-http/README.md(1 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py(2 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py(3 hunks)packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(2 hunks)packages/jumpstarter-driver-opendal/README.md(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(5 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(1 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py(1 hunks)packages/jumpstarter-driver-tftp/README.md(1 hunks)packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/jumpstarter-driver-http/README.md
- packages/jumpstarter-driver-tftp/README.md
- packages/jumpstarter-driver-iscsi/README.md
- packages/jumpstarter-driver-opendal/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#584
File: packages/jumpstarter/jumpstarter/client/grpc.py:38-54
Timestamp: 2025-08-18T07:13:37.619Z
Learning: User michalskrivanek prefers inline code over small helper function extractions when the abstraction doesn't significantly improve readability or reduce complexity. They value straightforward, readable code over reducing minor duplication.
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Applied to files:
packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py
🧬 Code graph analysis (8)
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(23-295)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(23-295)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(23-295)
packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(23-295)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(23-295)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (2)
packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py (1)
Tftp(28-175)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (1)
HttpServer(23-141)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (15)
open(224-232)rename(269-280)rename(464-465)remove_all(283-292)remove_all(469-470)create_dir(295-308)create_dir(474-475)delete(311-324)delete(479-480)get_created_resources(398-405)copy(257-266)copy(458-459)close(138-142)exists(327-337)exists(484-486)packages/jumpstarter/jumpstarter/driver/base.py (1)
close(85-87)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (5)
packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (5)
HttpServer(23-141)client(54-56)start(59-76)stop(79-92)close(124-132)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (8)
client(36-37)client(300-301)client(336-337)client(357-358)get_created_resources(226-228)Opendal(23-295)exists(171-172)close(231-247)packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py (3)
start(13-20)put_file(59-73)stop(22-30)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (4)
get_created_resources(398-405)exists(327-337)exists(484-486)close(138-142)
🪛 GitHub Actions: Run Tests
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py
[error] 139-139: AttributeError: 'Opendal' object has no attribute '_created_files' during test_opendal_cleanup_on_close. The Opendal class does not initialize the '_created_files' tracking set; initialize in init or adjust the test.
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (9)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (1)
30-30: Propagating the cleanup policy to storage — LGTM.Defaulting to True for ephemeral web roots is reasonable; passing the flag into Opendal aligns behavior across drivers.
Confirm deployments with persistent web roots explicitly set remove_created_on_close=False to prevent unintended deletions (Opendal only cleans for scheme 'fs').
Also applies to: 40-44
packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py (1)
42-42: LGTM — cleanup policy propagated to storage.Default True fits ephemeral TFTP roots; passing through to Opendal is consistent with the rest of the PR.
As with HTTP, ensure root_dir is isolated so cleanup can safely remove created boot artifacts.
Also applies to: 55-59
packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py (1)
49-49: Good default for persistence.Defaulting remove_created_on_close to False is appropriate for iSCSI disk images.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (4)
16-17: Nice: bind to an unused port in fixture.Reduces flakiness from port collisions.
64-90: Tracking via client API looks good.Asserting get_created_resources before close verifies behavior without depending on logs.
95-115: Direct tracking API test is fine.Covers export surface without coupling to internals.
117-171: No changes needed for opendal driver tests
Search shows no_created_filesinpackages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py, so tests already use_created_pathsand won’t raise an AttributeError.Likely an incorrect or invalid review comment.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
225-229: Client export is useful for observability and tests.Keeping get_created_resources exported makes sense given the new tests and diagnostics.
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (1)
16-18: Port injection via unused_tcp_port is a good stabilization.Helps avoid flaky CI due to port conflicts.
michalskrivanek
force-pushed
the
opendal-cleanup
branch
2 times, most recently
from
bd24774 to
cf3e277
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
35-41: Normalize with posixpath to handle backslashes and dot segments (optional).Current join-based normalization drops redundant slashes but not Windows backslashes or ./../ segments. Consider a slightly stronger normalizer that still keeps code simple.
+import posixpath @@ - def _normalize_path(self, path: str) -> str: - """Normalize path by stripping leading/trailing slashes and collapsing redundant slashes.""" - if not path: - return path - # Strip leading/trailing slashes and collapse multiple slashes - normalized = '/'.join(part for part in path.split('/') if part) - return normalized + def _normalize_path(self, path: str) -> str: + """Normalize to POSIX form: collapse //, resolve . and .., drop leading slash.""" + if not path: + return path + p = posixpath.normpath(str(path).replace("\\", "/")) + return "" if p in (".", "/") else p.lstrip("/")
239-256: Close() behavior is sensible; minor follow-up.
- fs-only cleanup and summary logging are fine. Optionally clear _created_paths after successful cleanup to avoid double-removal on repeated close() calls.
@@ - if self.remove_created_on_close: + if self.remove_created_on_close: if self.scheme == "fs": - self._cleanup_created_resources() + self._cleanup_created_resources() + self._created_paths.clear()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/source/reference/package-apis/drivers/opendal.yaml(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(1 hunks)packages/jumpstarter-driver-http/README.md(1 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py(2 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py(3 hunks)packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(2 hunks)packages/jumpstarter-driver-opendal/README.md(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(5 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(1 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py(1 hunks)packages/jumpstarter-driver-tftp/README.md(1 hunks)packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py
- packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
- packages/jumpstarter-driver-http/README.md
- packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
- packages/jumpstarter-driver-opendal/README.md
- packages/jumpstarter-driver-tftp/README.md
- packages/jumpstarter-driver-iscsi/README.md
- docs/source/reference/package-apis/drivers/opendal.yaml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#584
File: packages/jumpstarter/jumpstarter/client/grpc.py:38-54
Timestamp: 2025-08-18T07:13:37.619Z
Learning: User michalskrivanek prefers inline code over small helper function extractions when the abstraction doesn't significantly improve readability or reduce complexity. They value straightforward, readable code over reducing minor duplication.
📚 Learning: 2025-09-29T11:17:06.730Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#657
File: packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py:257-306
Timestamp: 2025-09-29T11:17:06.730Z
Learning: User michalskrivanek prefers keeping recursive directory removal (shutil.rmtree) in cleanup operations when there's already a safety flag (remove_created_on_close) to control the feature. They believe everything under the configured directory should be safe to remove if cleanup is enabled.
Applied to files:
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
🧬 Code graph analysis (3)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (4)
packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (5)
HttpServer(23-141)client(54-56)start(59-76)stop(79-92)close(124-132)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (7)
client(44-45)client(311-312)client(347-348)client(368-369)get_created_resources(234-236)Opendal(23-306)close(239-255)packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py (3)
start(13-20)put_file(59-73)stop(22-30)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(23-306)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (15)
open(224-232)rename(269-280)rename(464-465)remove_all(283-292)remove_all(469-470)create_dir(295-308)create_dir(474-475)delete(311-324)delete(479-480)get_created_resources(398-405)copy(257-266)copy(458-459)close(138-142)exists(327-337)exists(484-486)
🪛 GitHub Actions: Run Tests
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py
[error] 139-139: AttributeError: 'Opendal' object has no attribute '_created_files'. Did you mean: '_created_paths'?
🪛 GitHub Check: ruff
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
[failure] 257-257: Ruff (C901)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py:257:9: C901 _cleanup_created_resources is too complex (11 > 10)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
[error] 257-257: Ruff: C901 _cleanup_created_resources is too complex (11 > 10).
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (6)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (4)
67-70: Good: track writes on open().Recording the path for any write mode matches the PR intent to track creations even if the file already existed.
148-158: Copy/rename tracking aligns with “track by final target; discard source”.This matches the simplified model discussed in the PR comments.
163-176: Consistent updates on remove_all/create_dir/delete.Adding/discarding normalized paths keeps the set coherent.
230-237: get_created_resources returns a defensive copy.API shape and copy() usage look good.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (1)
34-39: Good: propagate remove_created_on_close to Tftp/Http defaults.This aligns cleanup behavior across the stack.
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (1)
30-44: Default-on cleanup and propagation to OpenDAL look correct.Enabling cleanup by default and passing the flag into storage is consistent with the OpenDAL tracking changes.
| # Get root path from kwargs | ||
| root_path = self.kwargs.get("root", "/") | ||
|
|
||
| # Separate files and directories, then remove in proper order | ||
| files_to_remove = [] | ||
| dirs_to_remove = [] | ||
|
|
||
| for path in self._created_paths: | ||
| try: | ||
| full_path = os.path.join(root_path, path) | ||
| # Safety check: ensure resolved path is within root_path | ||
| if not os.path.abspath(full_path).startswith(os.path.abspath(root_path) + os.sep): | ||
| continue | ||
| if os.path.exists(full_path): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Harden cleanup joins and cut complexity to fix Ruff C901.
- Use abspath on all joins and consistently enforce “under root” checks on both classification and removal steps (TOCTOU mitigation).
- Introduce a tiny local helper to reduce complexity (C901 11→<=10) without changing behavior.
- Keep recursive rmtree as per preference (guarded by under-root checks). Based on learnings.
@@
- # Get root path from kwargs
- root_path = self.kwargs.get("root", "/")
+ # Get and normalize root path
+ root_path = os.path.abspath(self.kwargs.get("root", "/"))
@@
- for path in self._created_paths:
+ def _safe_join_under_root(p: str) -> str | None:
+ full = os.path.abspath(os.path.join(root_path, p.lstrip("/")))
+ # allow root itself to pass the check but we won't delete it
+ if full == root_path or full.startswith(root_path + os.sep):
+ return full
+ self.logger.error(f"Refusing to operate outside root: {full}")
+ return None
+
+ for path in self._created_paths:
try:
- full_path = os.path.join(root_path, path)
- # Safety check: ensure resolved path is within root_path
- if not os.path.abspath(full_path).startswith(os.path.abspath(root_path) + os.sep):
- continue
+ full_path = _safe_join_under_root(path)
+ if not full_path:
+ continue
if os.path.exists(full_path):
if os.path.isdir(full_path):
dirs_to_remove.append(path)
else:
files_to_remove.append(path)
except Exception as e:
self.logger.error(f"Failed to check path {path}: {e}")
@@
- for file_path in files_to_remove:
+ for file_path in files_to_remove:
try:
- full_path = os.path.join(root_path, file_path.lstrip("/"))
+ full_path = _safe_join_under_root(file_path)
+ if not full_path:
+ continue
os.remove(full_path)
self.logger.debug(f"Removed created file: {file_path}")
except Exception as e:
self.logger.error(f"Failed to remove file {file_path}: {e}")
@@
- for dir_path in sorted(dirs_to_remove, reverse=True):
+ for dir_path in sorted(dirs_to_remove, reverse=True):
try:
- full_path = os.path.join(root_path, dir_path.lstrip("/"))
+ full_path = _safe_join_under_root(dir_path)
+ if not full_path:
+ continue
# Only remove if empty
try:
os.rmdir(full_path)
self.logger.debug(f"Removed created directory: {dir_path}")
except OSError:
# Directory not empty, try to remove recursively if it was created by us
shutil.rmtree(full_path)
self.logger.debug(f"Removed created directory tree: {dir_path}")
except Exception as e:
self.logger.error(f"Failed to remove directory {dir_path}: {e}")Run ruff locally to confirm C901 is resolved:
Also applies to: 284-306
🏁 Script executed:
#!/bin/bash
ruff check packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.pyLength of output: 497
Extract and split _cleanup_created_resources into smaller methods
The _cleanup_created_resources method still exceeds the C901 complexity threshold (11 > 10). Extract the path-classification loop (building files_to_remove/dirs_to_remove) and the subsequent removal logic into separate private helpers so that each method’s complexity is ≤ 10.
michalskrivanek
force-pushed
the
opendal-cleanup
branch
from
cf3e277 to
15db44a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/jumpstarter-driver-iscsi/README.md (1)
50-58: Fix markdown emphasis style to satisfy markdownlint (MD049).Replace asterisks with underscores for emphasis in the table (e.g., "auto" → "auto").
-| `host` | IP address to bind the target to. Empty string will auto-detect | str | no | *auto* | +| `host` | IP address to bind the target to. Empty string will auto-detect | str | no | _auto_ |packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
71-73: Avoid tracking empty normalized paths ('').Guard all add/discard operations so we never store ''. This prevents noisy summaries and edge cases during cleanup. Keeps behavior identical for real paths.
@@ async def open(...): - if mode in ("wb", "w", "ab", "a"): - self._created_paths.add(self._normalize_path(path)) + if mode in ("wb", "w", "ab", "a"): + norm = self._normalize_path(path) + if norm: + self._created_paths.add(norm) @@ async def copy(...): - self._created_paths.add(self._normalize_path(target)) + norm = self._normalize_path(target) + if norm: + self._created_paths.add(norm) @@ async def rename(...): - self._created_paths.discard(self._normalize_path(source)) - self._created_paths.add(self._normalize_path(target)) + src = self._normalize_path(source) + dst = self._normalize_path(target) + if src: + self._created_paths.discard(src) + if dst: + self._created_paths.add(dst) @@ async def remove_all(...): - self._created_paths.discard(self._normalize_path(path)) + norm = self._normalize_path(path) + if norm: + self._created_paths.discard(norm) @@ async def create_dir(...): - self._created_paths.add(self._normalize_path(path)) + norm = self._normalize_path(path) + if norm: + self._created_paths.add(norm) @@ async def delete(...): - self._created_paths.discard(self._normalize_path(path)) + norm = self._normalize_path(path) + if norm: + self._created_paths.discard(norm) @@ async def copy_exporter_file(...): - self._created_paths.add(self._normalize_path(target)) + norm = self._normalize_path(target) + if norm: + self._created_paths.add(norm)Based on learnings.
Also applies to: 153-154, 159-161, 166-167, 172-173, 178-179, 233-235
266-275: Harden cleanup path handling and clear tracking after removal.Normalize root once, join safely (handle leading '/'), allow equality for root (but don’t delete it), and clear the set post‑cleanup to avoid repeated logs on subsequent closes. Behavior unchanged otherwise; recursive rmtree retained.
@@ def _cleanup_created_resources(self): - # Get root path from kwargs - root_path = self.kwargs.get("root", "/") + # Get and normalize root path once + root_path = os.path.abspath(self.kwargs.get("root", "/")) @@ - for path in sorted(self._created_paths, reverse=True): + for path in sorted(self._created_paths, reverse=True): try: - full_path = os.path.join(root_path, path) - # Safety check: ensure resolved path is within root_path - if not os.path.abspath(full_path).startswith(os.path.abspath(root_path) + os.sep): + # Safe-join: avoid leading '/' escaping root and re-abspath joined path + full_path = os.path.abspath(os.path.join(root_path, str(path).lstrip("/"))) + # Safety check: ensure resolved path is within root_path (or equals root) + if not (full_path == root_path or full_path.startswith(root_path + os.sep)): continue @@ except Exception as e: self.logger.error(f"Failed to remove path {path}: {e}") + + # Reset tracking after cleanup to avoid duplicate logs/removals on repeated close() + self._created_paths.clear()Based on learnings.
Also applies to: 277-287, 289-290
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (1)
260-265: Keep tests aligned with driver normalization.Use the driver’s
_normalize_pathinstead of manual.strip('/')to reflect real behavior (handles backslashes and dot segments).- driver._created_paths.add(dir_path.strip('/')) + driver._created_paths.add(driver._normalize_path(dir_path)) @@ - driver._created_paths.add("same_dir".strip('/')) - driver._created_paths.add("same_dir/".strip('/')) - driver._created_paths.add("/same_dir".strip('/')) - driver._created_paths.add("/same_dir/".strip('/')) + driver._created_paths.add(driver._normalize_path("same_dir")) + driver._created_paths.add(driver._normalize_path("same_dir/")) + driver._created_paths.add(driver._normalize_path("/same_dir")) + driver._created_paths.add(driver._normalize_path("/same_dir/"))Also applies to: 282-287
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (1)
34-35: Remove stray debug print from test.Avoid printing in tests to keep output clean.
- print(http.storage.stat(filename)) + # Debug output removed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/source/reference/package-apis/drivers/opendal.yaml(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(1 hunks)packages/jumpstarter-driver-http/README.md(1 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py(2 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py(3 hunks)packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(2 hunks)packages/jumpstarter-driver-opendal/README.md(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(5 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(1 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py(1 hunks)packages/jumpstarter-driver-tftp/README.md(1 hunks)packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/jumpstarter-driver-tftp/README.md
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter-driver-http/README.md
- packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py
- packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
- packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
- packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py
- packages/jumpstarter-driver-opendal/README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#584
File: packages/jumpstarter/jumpstarter/client/grpc.py:38-54
Timestamp: 2025-08-18T07:13:37.619Z
Learning: User michalskrivanek prefers inline code over small helper function extractions when the abstraction doesn't significantly improve readability or reduce complexity. They value straightforward, readable code over reducing minor duplication.
📚 Learning: 2025-09-29T11:17:06.730Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#657
File: packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py:257-306
Timestamp: 2025-09-29T11:17:06.730Z
Learning: User michalskrivanek prefers keeping recursive directory removal (shutil.rmtree) in cleanup operations when there's already a safety flag (remove_created_on_close) to control the feature. They believe everything under the configured directory should be safe to remove if cleanup is enabled.
Applied to files:
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
🧬 Code graph analysis (3)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (5)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (5)
HttpServer(23-141)client(54-56)start(59-76)stop(79-92)close(124-132)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (8)
client(47-48)client(294-295)client(330-331)client(351-352)get_created_resources(237-239)Opendal(23-289)exists(182-183)close(242-258)packages/jumpstarter/jumpstarter/driver/base.py (2)
client(95-98)close(85-87)packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py (3)
start(13-20)put_file(59-73)stop(22-30)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (4)
get_created_resources(398-405)exists(327-337)exists(484-486)close(138-142)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(23-289)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (15)
open(224-232)rename(269-280)rename(464-465)remove_all(283-292)remove_all(469-470)create_dir(295-308)create_dir(474-475)delete(311-324)delete(479-480)get_created_resources(398-405)copy(257-266)copy(458-459)close(138-142)exists(327-337)exists(484-486)packages/jumpstarter/jumpstarter/driver/base.py (1)
close(85-87)
🪛 markdownlint-cli2 (0.18.1)
packages/jumpstarter-driver-iscsi/README.md
55-55: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
55-55: Emphasis style
Expected: underscore; Actual: asterisk
(MD049, emphasis-style)
⏰ 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: 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 (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
🔇 Additional comments (1)
docs/source/reference/package-apis/drivers/opendal.yaml (1)
8-10: Clarify cleanup scope and behavior in the config docs.Document that automatic cleanup is currently supported only for scheme "fs", and that tracking may include paths rewritten this session even if they existed before.
- # Optional: automatically remove created files/directories when driver closes - # Note: all tracked paths are normalized by stripping leading/trailing slashes + # Optional: automatically remove created files/directories when driver closes + # Notes: + # - Currently supported only for scheme: "fs". + # - Paths are tracked when created/written this session (even if they previously existed). + # - All tracked paths are normalized by stripping leading/trailing slashes remove_created_on_close: false
michalskrivanek
force-pushed
the
opendal-cleanup
branch
from
15db44a to
7c37be9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (2)
240-291: Normalization test is solid; consider a few more edge casesAdd cases for ".", "/", and paths with backslashes (e.g., "dir\sub") to lock behavior across platforms; parametrize for brevity.
293-317: Stronger signal if you exercise copy/rename instead of mutating internalsThese asserts prove set semantics, but calling driver.copy()/driver.rename() would verify the tracking hooks. If convenient, mark the test anyio and invoke the async methods; otherwise keep as-is.
packages/jumpstarter-driver-opendal/README.md (1)
42-51: Show both driver (async) and client (sync) usageAdd a client-side snippet alongside the async example to avoid confusion:
created_resources = await driver.get_created_resources() # Returns set[str] # Example usage for path in created_resources: print(f"Created: {path}") + +# Client (sync) usage +# created_resources = client.get_created_resources()packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (1)
62-92: Tracking-on-close test reads wellMembership check against get_created_resources verifies the integration path. If you want, assert that paths are normalized (no leading slash) for extra certainty.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
164-179: Optional: prune descendants on remove_all to keep the set tidyCurrently only the root is discarded; descendants remain (harmless but stale). Consider removing all tracked entries under the removed subtree:
async def remove_all(self, /, path: str): await self._operator.remove_all(path) - self._created_paths.discard(self._normalize_path(path)) + root = self._normalize_path(path) + if root: + # Drop root and any tracked descendant + to_drop = {p for p in self._created_paths if p == root or p.startswith(root + "/")} + self._created_paths.difference_update(to_drop)
242-259: Consider clearing tracking after reporting/cleanupAfter close(), the driver is done. Clearing frees memory and avoids accidental reuse:
if self.remove_created_on_close: if self.scheme == "fs": self._cleanup_created_resources() else: self.logger.warning(f"Cleanup not supported for scheme '{self.scheme}' - only 'fs' is supported") - super().close() + # Reset tracking post-summary/cleanup + self._created_paths.clear() + super().close()Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/source/reference/package-apis/drivers/opendal.yaml(1 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py(3 hunks)packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-opendal/README.md(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(5 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/source/reference/package-apis/drivers/opendal.yaml
- packages/jumpstarter-driver-iscsi/README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#584
File: packages/jumpstarter/jumpstarter/client/grpc.py:38-54
Timestamp: 2025-08-18T07:13:37.619Z
Learning: User michalskrivanek prefers inline code over small helper function extractions when the abstraction doesn't significantly improve readability or reduce complexity. They value straightforward, readable code over reducing minor duplication.
📚 Learning: 2025-09-29T11:17:06.730Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#657
File: packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py:257-306
Timestamp: 2025-09-29T11:17:06.730Z
Learning: User michalskrivanek prefers keeping recursive directory removal (shutil.rmtree) in cleanup operations when there's already a safety flag (remove_created_on_close) to control the feature. They believe everything under the configured directory should be safe to remove if cleanup is enabled.
Applied to files:
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
🧬 Code graph analysis (4)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (5)
packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (5)
HttpServer(23-141)client(54-56)start(59-76)stop(79-92)close(124-132)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (8)
client(47-48)client(295-296)client(331-332)client(352-353)get_created_resources(237-239)Opendal(23-290)exists(182-183)close(242-258)packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py (3)
start(13-20)put_file(59-73)stop(22-30)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (7)
write_bytes(99-103)write_bytes(176-183)write_bytes(422-424)get_created_resources(398-405)exists(327-337)exists(484-486)close(138-142)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
get_created_resources(237-239)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
Opendal(23-290)_normalize_path(35-44)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (15)
open(224-232)rename(269-280)rename(464-465)remove_all(283-292)remove_all(469-470)create_dir(295-308)create_dir(474-475)delete(311-324)delete(479-480)get_created_resources(398-405)copy(257-266)copy(458-459)close(138-142)exists(327-337)exists(484-486)packages/jumpstarter/jumpstarter/driver/base.py (1)
close(85-87)
⏰ 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 (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (10)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (1)
397-405: Client exposure of get_created_resources looks goodPublic sync wrapper aligns with the driver method and tests. No further surface needed right now.
packages/jumpstarter-driver-opendal/README.md (1)
40-41: Scope note is clear“filesystem only” cleanup callout prevents over-expectations for non-fs schemes.
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (3)
16-23: Good call binding to an unused TCP portPrevents flakiness from port collisions in CI.
93-114: Export methods smoke test LGTMInitial empty set then single add via put_file is straightforward.
115-169: Cleanup-on-close scenario is correct (and fixes prior _created_files vs _created_paths issue)Manual seeding + verifying removal/preservation covers both modes well.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (5)
32-45: Centralized path normalization: +1Covers backslashes, dot segments, and redundant slashes; trimming to a canonical form will prevent duplicates.
70-73: Write-mode creation tracking is placed correctlyAdded after successful open; set semantics avoid duplicates.
151-161: copy/rename tracking logic is correctTarget add + source discard (normalized) matches the desired unified tracking.
236-240: Accessor makes sense; mirrors client APIReturning a copy prevents external mutation. Keeps API consistent with the client.
260-291: Cleanup routine: safe joins + recursive removal are appropriate here
- Abspath + under-root checks prevent traversal.
- rmdir → rmtree fallback ensures dirs are removed when requested.
- Reverse-sorted iteration helps with nesting.
No changes requested. Keep rmtree as you prefer when the opt-in flag is set.
michalskrivanek
force-pushed
the
opendal-cleanup
branch
2 times, most recently
from
689e28a to
b809328
Compare
|
Thank you for handling this @michalskrivanek !!! looks great |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (1)
137-137: Consider using public API instead of private attribute.The tests directly access
_created_paths(lines 137, 158), but a publicget_created_resources()method is available. Using the public API would make tests more maintainable and less brittle to internal implementation changes.Consider refactoring to use the public API:
- cleanup_driver._created_paths.add(cleanup_filename) + # Instead of manipulating private state, simulate creation through the driver + # Or if direct manipulation is needed for testing, document whyHowever, if direct manipulation is necessary for test setup (to avoid async complications), document the reason with a comment.
Also applies to: 158-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/source/reference/package-apis/drivers/opendal.yaml(1 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py(3 hunks)packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-opendal/README.md(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(5 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-opendal/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py
- docs/source/reference/package-apis/drivers/opendal.yaml
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
- packages/jumpstarter-driver-iscsi/README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T11:17:06.730Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#657
File: packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py:257-306
Timestamp: 2025-09-29T11:17:06.730Z
Learning: User michalskrivanek prefers keeping recursive directory removal (shutil.rmtree) in cleanup operations when there's already a safety flag (remove_created_on_close) to control the feature. They believe everything under the configured directory should be safe to remove if cleanup is enabled.
Applied to files:
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (3)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (15)
open(224-232)rename(269-280)rename(464-465)remove_all(283-292)remove_all(469-470)create_dir(295-308)create_dir(474-475)delete(311-324)delete(479-480)get_created_resources(398-405)copy(257-266)copy(458-459)close(138-142)exists(327-337)exists(484-486)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
close(32-35)packages/jumpstarter/jumpstarter/driver/base.py (1)
close(85-87)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (5)
packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (5)
HttpServer(23-141)client(54-56)start(59-76)stop(79-92)close(124-132)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (8)
client(47-48)client(295-296)client(331-332)client(352-353)get_created_resources(237-239)Opendal(23-290)exists(182-183)close(242-258)packages/jumpstarter/jumpstarter/driver/base.py (2)
client(95-98)close(85-87)packages/jumpstarter-driver-http/jumpstarter_driver_http/client.py (3)
start(13-20)put_file(59-73)stop(22-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- 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)
🔇 Additional comments (15)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (13)
70-72: LGTM: Write-mode tracking implementation is correct.The tracking logic correctly identifies write modes and normalizes the path before adding it to the tracking set. The comment appropriately explains the assumption about pre-existing files.
153-153: LGTM: Copy operation tracking is correct.Correctly tracks the target path after a successful copy operation.
159-160: LGTM: Rename operation tracking is correct.Correctly removes the source path and adds the target path to maintain accurate tracking across rename operations.
166-166: LGTM: remove_all tracking update is correct.Correctly discards the path from tracking after recursive removal.
172-172: LGTM: create_dir tracking is correct.Correctly tracks directory creation.
178-178: LGTM: delete tracking update is correct.Correctly discards the path from tracking after deletion.
233-234: LGTM: copy_exporter_file tracking is correct.Correctly tracks the target path after copying a file from the exporter, with appropriate comment explaining the assumption.
236-239: Consider the necessity of exposing get_created_resources to clients.Based on the PR objectives comment from michalskrivanek about whether it's necessary to export these to the client, and the fact that tests directly access
_created_paths, consider if this export is needed for the client API or if it's primarily for internal/testing purposes.Could you clarify the intended use case for
get_created_resources()on the client side? Based on previous comments, there was a question about whether this needs to be exposed to clients.
242-258: LGTM: close() implementation follows proper cleanup sequence.The close method correctly:
- Logs diagnostic summary of created paths
- Conditionally performs cleanup based on
remove_created_on_closeflag- Only supports cleanup for 'fs' scheme with appropriate warning for others
- Calls
super().close()to ensure parent cleanup
273-276: Path traversal safety check has correct logic.The safety check correctly:
- Resolves
root_pathandfull_pathto absolute paths- Allows root itself to pass (but won't delete it)
- Requires
full_pathto start withroot_path + os.septo be under root- Logs error and continues on violation
This properly prevents path traversal attacks. Based on learnings.
279-288: Recursive directory removal is appropriate with safety flag.The implementation correctly:
- Attempts
os.rmdir()first for empty directories- Falls back to
shutil.rmtree()for non-empty directories- Is guarded by the
remove_created_on_closeflag- Is protected by path traversal checks
This aligns with the user's preference to keep recursive removal when cleanup is explicitly enabled. Based on learnings.
260-290: C901 complexity threshold satisfied. No further action required.
35-44: Please manually run the following corrected Python snippet and share its output for the listed edge cases to confirm_normalize_pathbehavior:import posixpath def _normalize_path(path: str) -> str: if not path: return path path = path.replace('\\', '/') normalized = posixpath.normpath(path).strip('/') return normalized if normalized != '.' else '' test_cases = [ "", "/", ".", "./", "/.", "//", "a//b", "a/./b", "a/../b", "/a/b", "a/b/", "../a", "../../a", ] for case in test_cases: print(f"{repr(case):10s} -> {repr(_normalize_path(case))}")packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (2)
16-17: LGTM: Port isolation improves test reliability.The fixture correctly uses
unused_tcp_portto bind the HTTP server to an available port, preventing port conflicts in parallel test execution.
115-169: LGTM: Comprehensive cleanup test coverage.The test correctly validates both scenarios:
- Cleanup enabled: verifies file removal after close
- Cleanup disabled: verifies file persistence after close
The test structure is clear and assertions are appropriate.
every file or directory creating is tracked, even if the file already existed.
…resources on close defaults to false. attempts to remove all files and directories created using "fs" scheme (local files on exporter filesystem)
so that when the session ends there are no leftovers. ISCSI driver defaults to False assuming that the disk files are meant to be persistent.
and simplify tracking to a single resource "path".
Head branch was pushed to by a user without write access
michalskrivanek
force-pushed
the
opendal-cleanup
branch
from
ed3b823 to
29ee63c
Compare
|
rebased |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (1)
115-169: LGTM: Cleanup behavior validated correctly.The test properly validates both cleanup-enabled and cleanup-disabled scenarios. Direct manipulation of
_created_pathsis acceptable here since the test specifically validates the cleanup mechanism. The assertions are clear and the test structure is sound.Optionally, consider creating files through the driver's public API (e.g.,
write()) instead of manually adding to_created_paths, which would make the test more end-to-end and less brittle to internal refactoring.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
260-290: Consider extracting path resolution to reduce cyclomatic complexityThe
_cleanup_created_resourcesmethod has elevated complexity (C901 ≥ 11). While the logic is correct and the recursive removal is intentional per learnings, extracting the path resolution and safety check into a helper would improve maintainability.Consider this refactor:
def _cleanup_created_resources(self): """Remove all created files and directories.""" import os import shutil # Get root path from kwargs root_path = os.path.abspath(self.kwargs.get("root", "/")) + + def safe_join(path: str) -> str | None: + """Join path under root_path and verify it stays within bounds.""" + full_path = os.path.abspath(os.path.join(root_path, str(path).lstrip("/"))) + # Safety check: ensure resolved path is within root_path but not the root itself + if full_path == root_path or not full_path.startswith(root_path + os.sep): + return None + return full_path # Remove all paths in reverse order (handles nested directories properly) for path in sorted(self._created_paths, reverse=True): try: - # Safe-join: avoid leading '/' escaping root and re-abspath joined path - full_path = os.path.abspath(os.path.join(root_path, str(path).lstrip("/"))) - # Safety check: ensure resolved path is within root_path but not the root itself - if full_path == root_path or not full_path.startswith(root_path + os.sep): - continue + full_path = safe_join(path) + if not full_path: + continue if os.path.exists(full_path): if os.path.isdir(full_path): try: os.rmdir(full_path) self.logger.debug(f"Removed created directory: {path}") except OSError: shutil.rmtree(full_path) self.logger.debug(f"Removed created directory tree: {path}") else: os.remove(full_path) self.logger.debug(f"Removed created file: {path}") except Exception as e: self.logger.error(f"Failed to remove path {path}: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
docs/source/reference/package-apis/drivers/opendal.yaml(1 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(1 hunks)packages/jumpstarter-driver-http/README.md(1 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py(2 hunks)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py(3 hunks)packages/jumpstarter-driver-iscsi/README.md(1 hunks)packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py(2 hunks)packages/jumpstarter-driver-opendal/README.md(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(5 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py(1 hunks)packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py(1 hunks)packages/jumpstarter-driver-tftp/README.md(1 hunks)packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py
- packages/jumpstarter-driver-iscsi/jumpstarter_driver_iscsi/driver.py
- packages/jumpstarter-driver-http/README.md
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py
- packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py
- packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver_test.py
- packages/jumpstarter-driver-tftp/README.md
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py
- docs/source/reference/package-apis/drivers/opendal.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T11:17:06.730Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter#657
File: packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py:257-306
Timestamp: 2025-09-29T11:17:06.730Z
Learning: User michalskrivanek prefers keeping recursive directory removal (shutil.rmtree) in cleanup operations when there's already a safety flag (remove_created_on_close) to control the feature. They believe everything under the configured directory should be safe to remove if cleanup is enabled.
Applied to files:
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py
🧬 Code graph analysis (3)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (4)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (15)
open(224-232)rename(269-280)rename(464-465)remove_all(283-292)remove_all(469-470)create_dir(295-308)create_dir(474-475)delete(311-324)delete(479-480)get_created_resources(398-405)copy(257-266)copy(458-459)close(138-142)exists(327-337)exists(484-486)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (1)
close(124-132)packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py (1)
close(172-175)packages/jumpstarter/jumpstarter/driver/base.py (1)
close(85-87)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (3)
packages/jumpstarter/jumpstarter/common/utils.py (1)
serve(31-39)packages/jumpstarter-driver-http/jumpstarter_driver_http/driver.py (5)
HttpServer(23-141)client(54-56)start(59-76)stop(79-92)close(124-132)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (8)
client(47-48)client(295-296)client(331-332)client(352-353)get_created_resources(237-239)Opendal(23-290)exists(182-183)close(242-258)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py (1)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
Opendal(23-290)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
🔇 Additional comments (19)
packages/jumpstarter-driver-http/jumpstarter_driver_http/driver_test.py (4)
1-1: LGTM: Import necessary for caplog level configuration.The logging import is correctly used in
test_opendal_tracking_on_http_server_closeto set the capture level.
16-17: LGTM: Port binding prevents test conflicts.The fixture now accepts
unused_tcp_portand binds the server to a unique port, enabling safe concurrent test execution.
61-91: LGTM: Comprehensive tracking validation test.The test properly validates OpenDAL's resource tracking through the public API. The caplog setup and context manager usage are correct. The comment accurately reflects that tracking is verified via
get_created_resources()before close.
93-113: LGTM: Clean API validation test.The test effectively validates the
get_created_resources()method behavior, checking both the initial empty state and post-creation tracking.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (12)
26-26: LGTM - Field addition follows dataclass conventionsThe
remove_created_on_closefield is properly declared withbool = Falsedefault, maintaining backward compatibility.
32-34: LGTM - Tracking set initialized correctlyThe
_created_pathsset usesdefault_factory=setwhich is the correct pattern for mutable default values in dataclasses.
35-44: LGTM - Path normalization implementation is soundThe
_normalize_pathmethod correctly:
- Handles backslashes for cross-platform compatibility
- Uses
posixpath.normpathto collapse//,./, and../segments- Strips leading/trailing slashes for consistent storage
- Returns empty string for root paths
70-72: LGTM - Write mode tracking is appropriateTracking files opened in write modes (
wb,w,ab,a) correctly captures file creation. The comment about pre-existing files being remnants is a pragmatic approach.
153-153: LGTM - Copy target trackingThe
copy()method correctly tracks the target path after the operation succeeds.
159-160: LGTM - Rename tracking updates both source and targetThe
rename()method correctly:
- Removes the source from tracking (it no longer exists at that path)
- Adds the target to tracking (it now exists at the new path)
166-166: LGTM - Remove tracking maintainedThe
remove_all()method correctly discards the removed path from tracking.
172-172: LGTM - Directory creation trackingThe
create_dir()method correctly adds the created directory to tracking.
178-178: LGTM - Delete tracking maintainedThe
delete()method correctly discards the deleted path from tracking.
233-234: LGTM - Exporter file copy trackingThe
copy_exporter_file()method correctly tracks the target path after successful copy. The comment aligns with the tracking philosophy.
236-239: LGTM - Resource tracking API exportBased on the PR discussion, the maintainer confirmed this public API is useful for derived drivers to retrieve creation statistics. The method correctly returns a copy of the set to prevent external modification.
242-258: LGTM - Close method with appropriate safeguardsThe
close()method:
- Logs helpful debug information about created paths
- Only attempts cleanup when
remove_created_on_closeis enabled- Restricts cleanup to
fsscheme with a clear warning for other schemes- Properly calls
super().close()to maintain the driver hierarchypackages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py (1)
28-32: LGTM - Appropriate cleanup configuration for temporary firmwareSetting
remove_created_on_close=Truefor the RideSX storage is correct since firmware files uploaded during flashing are temporary and should be cleaned up after the session. The inline comment clearly documents this intent.packages/jumpstarter-driver-opendal/README.md (1)
21-67: LGTM - Comprehensive and clear documentationThe new documentation sections effectively cover:
- Configuration parameters with clear descriptions
- Detailed tracking behavior for all operations
- Automatic cleanup mechanics and limitations (filesystem-only)
- API usage examples
- Practical use cases for both temporary and persistent storage
The note about pre-existing files being treated as created aligns with the implementation philosophy.
packages/jumpstarter-driver-iscsi/README.md (1)
44-61: LGTM - Documentation correctly reflects persistent storage use caseThe documentation appropriately:
- Sets
remove_created_on_close: falseas the default for iSCSI disk images- Explains the rationale that disk images are typically reused across sessions
- Provides clear guidance on when to enable cleanup (temporary disk images)
This default contrasts correctly with the RidesX driver (which uses
truefor temporary firmware files).
|
Successfully created backport PR for |
2 participants
every file or directory creating is tracked, even if the file already existed.
Summary by CodeRabbit
New Features
Documentation
Tests