Remove opendal dependency from QEMU driver by ambient-code[bot] · Pull Request #535 · jumpstarter-dev/jumpstarter

ambient-code · 2026-04-09T15:57:23Z

Summary

Move FlasherInterface ABC to core jumpstarter package (jumpstarter.driver.flasher) — it's a pure abstract class with no opendal imports
Create a simplified FlasherClient in core (jumpstarter.client.flasher) that handles local files via resource_async and HTTP URLs via PresignedRequestResource, without requiring the opendal library
Update QEMU driver to import FlasherInterface from jumpstarter.driver instead of jumpstarter_driver_opendal.driver
Remove jumpstarter-driver-opendal from QEMU's pyproject.toml dependencies
Update test to use direct HTTP URLs instead of opendal.Operator

Backward Compatibility

The opendal package's own FlasherInterface is unchanged — drivers that depend on opendal (esp32, pi-pico, dutlink, sdwire) continue to work without modification
The core FlasherInterface.client() points to jumpstarter.client.flasher.FlasherClient (simplified, no opendal)
The opendal FlasherInterface.client() continues pointing to jumpstarter_driver_opendal.client.FlasherClient (full opendal support)

Test plan

All 7 QEMU unit tests pass (resize, set_disk_size, set_memory_size)
Integration test (test_driver_qemu) fails only due to missing qemu-img binary in CI environment (pre-existing)
Linting passes on all changed files
Verify other drivers (esp32, pi-pico, dutlink, sdwire) still import correctly from opendal

Related #441

🤖 Generated with Claude Code

netlify · 2026-04-09T15:57:28Z

❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →

Name	Link
🔨 Latest commit	`bffbd65`
🔍 Latest deploy log	https://app.netlify.com/projects/jumpstarter-docs/deploys/69d917eb17b9810009854c5b

coderabbitai · 2026-04-09T15:57:31Z

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fb911b23-4fc5-4f15-8118-dd55cec4329e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch ambient-fix/441-remove-opendal-qemu

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

ambient-code · 2026-04-09T16:32:23Z

Fixed the CI failures by adding click>=8.1.7.2 to the jumpstarter package dependencies.

The new jumpstarter/client/flasher.py file imports click for its CLI functionality, but this dependency was missing from the package's pyproject.toml. This caused ModuleNotFoundError: No module named 'click' errors when tests tried to import modules that depend on the flasher client.

The version requirement >=8.1.7.2 matches the convention used in other jumpstarter driver packages.

ambient-code · 2026-04-10T12:33:18Z

Added unit tests for FlasherClient URL routing in 6bacdd62 to address @mangelajo's feedback about test coverage for the HTTP URL code path.

The new tests in flasher_test.py cover:

_parse_path correctly distinguishing local paths from HTTP/HTTPS URLs
_http_url_adapter producing correct PresignedRequestResource for read (GET) and write (PUT) modes
FlasherClient._flash_single routing HTTP URLs through the presigned request path (not the local file adapter)
FlasherClient.dump routing HTTP URLs correctly

This ensures the URL download logic is exercised in CI without needing to download a full Fedora image.

ambient-code · 2026-04-10T13:32:14Z

Fixed the lint-python CI failure in 6c8eee9 — removed two unused imports in flasher_test.py that ruff flagged as F401:

pathlib.Path (line 3)
jumpstarter.common.resources.PresignedRequestResource (line 56)

The netlify deploy preview failure is unrelated to this PR's changes.

mangelajo · 2026-04-10T14:37:16Z

Rebase the pr please.

ambient-code · 2026-04-10T15:31:55Z

Rebased onto main (d7a65e0). The branch now has a clean history on top of the latest main.

ambient-code · 2026-04-12T07:03:20Z

Rebase complete. The branch ambient-fix/441-remove-opendal-qemu is already up to date with main (merge base is the current tip of main at d7a65e0). No conflicts were encountered.

ambient-code · 2026-04-13T20:36:31Z

Addressed @raballew's review feedback in 0c2d55c:

Fixed in this commit:

Removed module-level docstrings from flasher.py, driver/flasher.py, and flasher_test.py to match project conventions
Fixed _AsyncIteratorStream.aclose() to propagate close to the wrapped async generator (ensures file handle cleanup on error paths)
Added test_dump_routes_local_path to cover the missing dump routing test for local file paths

Tracked for follow-up in #441:

Having opendal FlasherInterface inherit from the core one (or removing it once remaining drivers migrate)
Adding focused streaming integration test for _local_file_adapter
Adding tests for dict-based multi-target flash (happy path + ArgumentError case)

ambient-code · 2026-04-15T11:37:16Z

Addressed @raballew's latest review feedback in da11134:

Fixed in this commit:

Added direct unit tests for _AsyncIteratorStream (receive, send, aclose propagation, extra_attributes, empty iterator) and _FileWriteObjectStream (write+readback, receive, aclose lifecycle)
Added TestFlasherClientMultiTarget covering dict-based multi-target flash (happy path + ArgumentError guard)
compression parameter now emits a warnings.warn() when used with HTTP URLs instead of being silently ignored, with corresponding tests

Deferred to #441 (follow-up):

Having opendal FlasherInterface inherit from the core one -- this touches a different package and risks breaking other drivers (esp32, pi-pico, dutlink, sdwire). Should be a dedicated PR with cross-driver test verification.
Parameter naming alignment (dump(target, partition) vs dump(path, target)) -- the core ABC intentionally mirrors the existing opendal ABC verbatim. Renaming should happen holistically when the ABCs are unified.

raballew

Lgtm now, if #441 is agreed upon as follow up.

mangelajo · 2026-04-16T13:55:12Z

Lgtm now, if #441 is agreed upon as follow up.

Thanks, I will bring the topic up on today's meeting.

ambient-code · 2026-05-06T18:34:47Z

Review Comment Summary & Fix Plan

Reviewed all outstanding comments from org members. Here is the plan:

Changes to make in this PR:

Fix _AsyncIteratorStream.aclose() - Propagate close to the wrapped async generator to ensure proper file handle cleanup on error paths (per @raballew)
Add warning for compression with HTTP URLs - Log a warning when compression is specified with HTTP URL paths since it's silently ignored (per @raballew)
Add tests for _local_file_adapter streaming - Direct unit test with a real temp file to exercise the streaming and chunking behavior (per @raballew)
Add tests for dict-based multi-target flash - Cover both the happy path and the ArgumentError case (per @raballew)
Add test for dump with local path routing - Complete the routing test coverage (per @raballew)

Deferred to #441:

Parallel FlasherInterface ABCs consolidation (opendal inheriting from core) - confirmed intentional by @mangelajo
Parameter naming alignment (target vs partition) between driver and client

No change needed:

Module-level docstrings - confirmed intentional by @mangelajo

Additional issue:

PR currently has merge conflicts that need to be resolved via rebase

Generated with Claude Code

ambient-code · 2026-05-06T18:34:52Z

Review Comment Summary & Fix Plan

Reviewed all outstanding comments from org members. Here is the plan:

Changes to make in this PR:

Fix _AsyncIteratorStream.aclose() - Propagate close to the wrapped async generator to ensure proper file handle cleanup on error paths (per @raballew)
Add warning for compression with HTTP URLs - Log a warning when compression is specified with HTTP URL paths since it is silently ignored (per @raballew)
Add tests for _local_file_adapter streaming - Direct unit test with a real temp file to exercise the streaming and chunking behavior (per @raballew)
Add tests for dict-based multi-target flash - Cover both the happy path and the ArgumentError case (per @raballew)
Add test for dump with local path routing - Complete the routing test coverage (per @raballew)

Deferred to #441:

Parallel FlasherInterface ABCs consolidation (opendal inheriting from core) - confirmed intentional by @mangelajo
Parameter naming alignment (target vs partition) between driver and client

No change needed:

Module-level docstrings - confirmed intentional by @mangelajo

Additional issue:

PR currently has merge conflicts that need to be resolved via rebase

Generated with Claude Code

ambient-code · 2026-05-06T18:35:55Z

Rebased onto latest main to resolve merge conflicts. The conflict was in driver.py imports -- the main branch added from jumpstarter.common.fls import get_fls_binary (for OCI flashing support), which needed to coexist with this PR's FlasherInterface import. Both imports are now present.

All previously implemented review feedback (aclose propagation, compression warnings, stream tests, multi-target tests, dump local path test) is preserved in the rebased commits.

ambient-code · 2026-05-06T19:34:54Z

Fixed a CI failure in type-check-python caused by an incomplete migration in 662dcaa:

Root cause: client.py was not updated when jumpstarter-driver-opendal was removed from pyproject.toml dependencies. It still imported FlasherClient from jumpstarter_driver_opendal.client, causing the type checker to fail with unresolved-import.

Fix:

Updated client.py to import FlasherClient from jumpstarter.client (the new core module) instead of jumpstarter_driver_opendal.client
Removed the operator parameter from QemuFlasherClient.flash() since the core FlasherClient does not use opendal operators

Other CI failures are pre-existing (not caused by this PR):

check-warnings and pytest-matrix failures are caused by a 502 Bad Gateway from quay.io when fetching controller versions -- an external infrastructure issue

Generated with Claude Code

ambient-code · 2026-05-06T20:32:41Z

Fixed the lint-python CI failure in c1d9f44 -- added a blank line to separate the local-folder jumpstarter import from the third-party jumpstarter_driver_* imports in client.py, per ruff I001 (import block un-sorted or un-formatted).

Generated with Claude Code

raballew · 2026-05-27T20:13:10Z

@ambient-code rebase onto main

Move FlasherInterface to core jumpstarter package (jumpstarter.driver.flasher) and create a simplified FlasherClient (jumpstarter.client.flasher) that streams local files via resource_async and passes HTTP URLs as PresignedRequestResource for exporter-side download, eliminating the need for the opendal library. Update QEMU driver to import from the new core location and remove jumpstarter-driver-opendal from its dependencies. Update tests to use direct HTTP URLs instead of opendal Operator. Closes #441 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The new FlasherClient in jumpstarter/client/flasher.py imports click but it was not listed in the dependencies, causing import errors in tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Ensures the HTTP URL code path in FlasherClient is covered by tests, since the integration test only exercises this path when no cached image is available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Remove unused `pathlib.Path` and `PresignedRequestResource` imports that were flagged by ruff (F401). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…l test - Remove module-level docstrings from flasher.py, driver/flasher.py, and flasher_test.py to match project conventions (per raballew review) - Fix _AsyncIteratorStream.aclose() to propagate close to wrapped async generator, ensuring file handles are cleaned up on error paths - Add test_dump_routes_local_path to cover dump routing for local file paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ssion warning - Add unit tests for _AsyncIteratorStream and _FileWriteObjectStream (receive/send/aclose lifecycle, error propagation, empty iterator) - Add tests for dict-based multi-target flash (happy path + ArgumentError) - Warn when compression parameter is used with HTTP URLs instead of silently ignoring it - Add tests for the compression warning behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The client.py file was missed during the opendal removal: it still imported FlasherClient from jumpstarter_driver_opendal.client, which breaks type-check-python CI since jumpstarter-driver-opendal is no longer a dependency. Updated to import from jumpstarter.client and removed the unused operator parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Separate the local-folder jumpstarter import from third-party jumpstarter_driver_* imports with a blank line, per ruff I001. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ambient-code · 2026-05-27T20:34:17Z

Rebased onto latest main (33ad352). The branch now has a clean history with all 8 PR commits on top of the current main tip. No conflicts were encountered.

Generated with Claude Code

…QemuFlasherClient Address review feedback: add return type annotations and parameter types to FlasherInterface ABC methods, narrow _parse_path return type to a discriminated union, use Literal["rb", "wb"] for mode parameters in adapter functions, and add full type annotations to QemuFlasherClient.flash. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ambient-code · 2026-05-28T11:36:09Z

Review feedback response

Addressed the latest review comments from @raballew (7 comments posted 2026-05-28):

Accepted and implemented (pushed in `a7d0975`):

Type annotations on FlasherInterface ABC (#r3316704536): Added source: Any, target: Any, partition: str | None, and -> None return types
_parse_path return type (#r3316704558): Narrowed from tuple[Path | None, str | None] to tuple[Path, None] | tuple[None, str]
Literal type for mode (#r3316704566): Changed mode: str to mode: Literal["rb", "wb"] on _local_file_adapter and _http_url_adapter
QemuFlasherClient.flash annotations (#r3316704575): Added full type annotations matching the parent class

Declined:

ABC unit test (#r3316704543): Testing Python's ABCMeta enforcement and a single-line client() classmethod does not add meaningful value -- the interface is exercised through existing driver/client tests
FlasherInterface inheritance (#r3316704549): Previously discussed and confirmed intentional by @mangelajo -- tracked in Remove opendal dependency, preserve exporter-side presigned downloads #441
CLI code deduplication (#r3316704554): Intentional for phase 1 -- consolidation requires modifying the opendal package to import from core, tracked in Remove opendal dependency, preserve exporter-side presigned downloads #441

ambient-code · 2026-05-28T21:32:31Z

Addressing latest review feedback

Thank you @raballew for the detailed review. Here is my plan to address the 7 comments from your latest review:

Will fix in this PR:

Type annotations on FlasherInterface ABC (comment 3316704536) - Add type annotations for flash/dump methods
_parse_path return type (comment 3316704558) - Narrow to discriminated union: tuple[Path, None] | tuple[None, str]
Literal type for mode (comment 3316704566) - Change to Literal["rb", "wb"]
QemuFlasherClient.flash annotations (comment 3316704575) - Add full type annotations

Will not change (with rationale):

ABC unit test (comment 3316704543) - Testing Python's ABCMeta enforcement and a single-line classmethod does not add meaningful value; the ABC is already exercised through existing driver/client tests
FlasherInterface inheritance (comment 3316704549) - Previously discussed and confirmed intentional by @mangelajo - tracked in Remove opendal dependency, preserve exporter-side presigned downloads #441
CLI code deduplication (comment 3316704554) - Intentional for phase 1; consolidation requires modifying the opendal package to import from core - tracked in Remove opendal dependency, preserve exporter-side presigned downloads #441

I'll push the fixes shortly.

Generated with Claude Code

raballew · 2026-05-28T14:33:15Z

+from __future__ import annotations
+
+from abc import ABCMeta, abstractmethod
+from typing import Any
+
+
+class FlasherInterface(metaclass=ABCMeta):
+    @classmethod
+    def client(cls) -> str:
+        return "jumpstarter.client.flasher.FlasherClient"
+
+    @abstractmethod
+    def flash(self, source: Any, target: str | None = None) -> None: ...
+
+    @abstractmethod
+    def dump(self, target: Any, partition: str | None = None) -> None: ...


[MEDIUM] The FlasherInterface ABC is a new 16-line file with zero test coverage. There is no test verifying that client() returns the expected string "jumpstarter.client.flasher.FlasherClient", and no test confirming that instantiating a subclass without implementing the abstract methods raises TypeError.

Adding a small test for these two behaviors would close the coverage gap.

AI-generated, human reviewed

This was discussed in the previous review round (comment r3316704543) where you acknowledged my pushback with "ack". The position remains the same: testing Python's ABCMeta enforcement and a single-line client() classmethod returning a string literal would be testing language guarantees rather than our code. The FlasherInterface ABC is exercised through the existing driver and client tests that subclass it.

…type - Add return type annotations to FlasherClientInterface.flash(), FlasherClientInterface.dump(), FlasherClientInterface.cli(), FlasherClient.flash(), FlasherClient.dump(), and FlasherClient._flash_single() - Narrow _AsyncIteratorStream.iterator from Any to AsyncGenerator[bytes, None] for better static analysis Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ambient-code · 2026-05-29T05:35:33Z

Review feedback response

Addressed the latest review comments from @raballew (6 comments in review 4381893748, posted 2026-05-29):

Accepted and implemented (pushed in `357f37e`):

Return type annotations (#r3318530516): Added -> Any to FlasherClientInterface.flash(), .dump(), FlasherClient.flash(), .dump(), ._flash_single(), and -> click.Group to FlasherClientInterface.cli()
_AsyncIteratorStream.iterator type (#r3318530547): Narrowed from Any to AsyncGenerator[bytes, None] using collections.abc

Declined (with rationale):

cli() test coverage (#r3318530502): The CLI is a thin click wrapper delegating to already-tested flash()/dump() methods; testing it would primarily exercise click's argument parsing machinery
FlasherInterface ABC test (#r3318530508): Previously discussed and acknowledged in prior review round -- testing Python's ABCMeta enforcement does not add meaningful value

Acknowledged for follow-up (#441):

Any for primary data parameters (#r3318530531): Not a regression, will improve when ABCs are unified
Naming inconsistency (#r3318530538): Not a regression, will align when ABCs are unified

Generated with Claude Code

ambient-code · 2026-05-30T01:32:32Z

@raballew The PR has been rebased onto main. The latest commit (357f37e) from 2026-05-29 is now based on the current main branch (33ad352 from 2026-05-27).

ambient-code Bot mentioned this pull request Apr 9, 2026

Remove opendal dependency, preserve exporter-side presigned downloads #441

Open

ambient-code Bot mentioned this pull request Apr 9, 2026

E2E test failure: Dial rejected with Available status despite server-side retry fix #538

Closed

mangelajo reviewed Apr 10, 2026

View reviewed changes

Comment thread python/packages/jumpstarter/jumpstarter/client/flasher.py Outdated

mangelajo enabled auto-merge (squash) April 10, 2026 09:35

mangelajo approved these changes Apr 10, 2026

View reviewed changes

mangelajo disabled auto-merge April 10, 2026 09:39

mangelajo reviewed Apr 10, 2026

View reviewed changes

Comment thread python/packages/jumpstarter/jumpstarter/client/flasher.py

ambient-code Bot force-pushed the ambient-fix/441-remove-opendal-qemu branch from 6c8eee9 to bffbd65 Compare April 10, 2026 15:31

raballew self-requested a review April 13, 2026 17:10

raballew requested changes Apr 13, 2026

View reviewed changes

ambient-code Bot force-pushed the ambient-fix/441-remove-opendal-qemu branch from 0c2d55c to 69cbb0e Compare April 15, 2026 07:47

raballew requested changes Apr 15, 2026

View reviewed changes

mangelajo requested a review from raballew April 15, 2026 15:29

raballew approved these changes Apr 15, 2026

View reviewed changes

ambient-code Bot force-pushed the ambient-fix/441-remove-opendal-qemu branch from da11134 to 177a915 Compare May 6, 2026 18:35

Ambient Code Bot and others added 8 commits May 27, 2026 20:33

Add click dependency to jumpstarter package

603deba

The new FlasherClient in jumpstarter/client/flasher.py imports click but it was not listed in the dependencies, causing import errors in tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add unit tests for FlasherClient URL routing and path parsing

04de902

Ensures the HTTP URL code path in FlasherClient is covered by tests, since the integration test only exercises this path when no cached image is available. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fix lint: remove unused imports in flasher_test.py

caeba92

Remove unused `pathlib.Path` and `PresignedRequestResource` imports that were flagged by ruff (F401). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fix import sorting in client.py to pass ruff lint

c359a91

Separate the local-folder jumpstarter import from third-party jumpstarter_driver_* imports with a blank line, per ruff I001. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ambient-code Bot force-pushed the ambient-fix/441-remove-opendal-qemu branch from c1d9f44 to c359a91 Compare May 27, 2026 20:34

raballew requested changes May 28, 2026

View reviewed changes

raballew requested changes May 29, 2026

View reviewed changes

raballew approved these changes May 29, 2026

View reviewed changes

Conversation

ambient-code Bot commented Apr 9, 2026 • edited by mangelajo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Backward Compatibility

Test plan

Uh oh!

netlify Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →

Uh oh!

coderabbitai Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

ambient-code Bot commented Apr 9, 2026

Uh oh!

Uh oh!

Uh oh!

ambient-code Bot commented Apr 10, 2026

Uh oh!

ambient-code Bot commented Apr 10, 2026

Uh oh!

mangelajo commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ambient-code Bot commented Apr 10, 2026

Uh oh!

ambient-code Bot commented Apr 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambient-code Bot commented Apr 13, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambient-code Bot commented Apr 15, 2026

Uh oh!

raballew left a comment

Choose a reason for hiding this comment

Uh oh!

mangelajo commented Apr 16, 2026

Uh oh!

ambient-code Bot commented May 6, 2026

Review Comment Summary & Fix Plan

Changes to make in this PR:

Deferred to #441:

No change needed:

Additional issue:

Uh oh!

ambient-code Bot commented May 6, 2026

Review Comment Summary & Fix Plan

Changes to make in this PR:

Deferred to #441:

No change needed:

Additional issue:

Uh oh!

ambient-code Bot commented May 6, 2026

Uh oh!

ambient-code Bot commented May 6, 2026

Uh oh!

ambient-code Bot commented May 6, 2026

Uh oh!

raballew commented May 27, 2026

Uh oh!

ambient-code Bot commented May 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambient-code Bot commented Apr 9, 2026 •

edited by mangelajo

Loading

netlify Bot commented Apr 9, 2026 •

edited

Loading

coderabbitai Bot commented Apr 9, 2026 •

edited

Loading

mangelajo commented Apr 10, 2026 •

edited

Loading

Accepted and implemented (pushed in `a7d0975`):

Accepted and implemented (pushed in `357f37e`):