Enhance flash CLI for partition-based and multiple image flashing#159
Enhance flash CLI for partition-based and multiple image flashing#159eballetbo wants to merge 1 commit intojumpstarter-dev:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds multi-image and partition-target flashing: CLI accepts partition:FILE mappings and optional block device, Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (user)"
participant Client as "FlasherClient"
participant Fetch as "ImageFetcher/HTTP"
participant OS as "OS (/dev resolution)"
participant Flasher as "Flasher Operator"
CLI->>Client: parse FILE, -t partitions, block_device
Client->>Client: _resolve_flash_parameters() -> list of ops
loop per operation
Client->>Fetch: fetch/load image (URL or file)
Fetch-->>Client: image + checksum
Client->>OS: resolve partition label -> /dev/disk/by-partlabel/X (if label)
OS-->>Client: resolved flash_target
Client->>Flasher: invoke flash operation (flash_target, image, options)
Flasher-->>Client: success/failure
end
Client-->>CLI: aggregate results / exit status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
eballetbo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
106-112: Misplaced docstring - will not be recognized by documentation tools.The docstring on line 112 (
"""Flash image to DUT""") appears after executable code. Python docstrings must immediately follow the function definition to be properly recognized byhelp(), IDEs, and documentation generators.🔧 Proposed fix
def flash( # noqa: C901 self, path: PathBuf, *, partition: str | None = None, operator: Operator | None = None, os_image_checksum: str | None = None, force_exporter_http: bool = False, force_flash_bundle: str | None = None, cacert_file: str | None = None, insecure_tls: bool = False, headers: dict[str, str] | None = None, bearer_token: str | None = None, retries: int = 3, method: str = "fls", fls_version: str = "", fls_binary_url: str | None = None, ): + """Flash image to DUT""" if bearer_token: bearer_token = self._validate_bearer_token(bearer_token) if headers: headers = self._validate_header_dict(headers) - """Flash image to DUT""" should_download_to_httpd = True
🤖 Fix all issues with AI agents
In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 1344-1371: The code reads a checksum into the local variable
"checksum" (from os_image_checksum_file) but never passes it into the flash
routine; update the self.flash(...) call inside the for-loop to include the
checksum parameter (pass the read value, e.g. os_image_checksum=checksum or
checksum=checksum depending on the flash() signature) so that the flash method
(self.flash) receives and can verify the image checksum; locate the call to
self.flash in the loop that also references force_exporter_http,
force_flash_bundle, cacert_file, etc., and add the appropriate named argument.
🧹 Nitpick comments (3)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (3)
1180-1239: Well-implemented parameter resolution with clear mode separation.The method cleanly handles all four documented modes and provides helpful error messages. Using
split(':', 1)correctly handles edge cases where filenames might contain colons.Minor suggestion: The error message on line 1236 hardcodes
'jumpstarter-cli flash', which may not match the actual command invocation path. Consider using a generic reference or removing the specific command name.♻️ Suggested improvement
else: raise click.UsageError( "Must provide either FILE argument or -t options. " - "Use 'jumpstarter-cli flash --help' for usage examples" + "Use '--help' for usage examples" )
1247-1255: Potential user confusion between--targetand-toptions.Both options involve "target" conceptually but mean different things:
--targetspecifies the block device (e.g., 'emmc', 'usd')-tspecifies partition:filename pairsThis could lead to user confusion since
-tis typically a short form of--target. Consider using a different short option for partition specs, such as-p(for partition) or--partition-file.♻️ Suggested improvement
`@click.option`("--target", type=str, help="Block device to flash to (e.g., 'usd', 'emmc'). If not provided, uses default target.") `@click.option`( - "-t", + "-p", "partitions", multiple=True, - help="Flash file to partition: 'partition:filename'. Can be repeated for multiple partitions.", + help="Flash file to partition: 'partition:filename'. Can be repeated for multiple partitions. Example: -p rootfs:rootfs.img", )
1350-1371: Consider documenting multi-partition flash behavior.When flashing multiple partitions (e.g.,
-t rootfs:rootfs.img -t boot:boot.img), the current implementation performs a full flash cycle (boot to busybox, flash, reboot) for each partition. This means N partitions = N device reboots.While this is safe and leverages existing retry logic, it may result in longer flash times for multi-partition scenarios. Consider:
- Adding a note in the CLI help about this behavior
- Future optimization: batch partition writes within a single boot cycle
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Show resolved
Hide resolved
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Outdated
Show resolved
Hide resolved
eballetbo
force-pushed
the
flash-to-partitions
branch
from
2a7e905 to
eb05aa6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 1356-1377: The loop unpacks (image_file, target_partition,
block_device) from _resolve_flash_parameters but never forwards block_device to
self.flash(), so --target is ignored; fix by passing the unpacked block_device
into the flash call (e.g. add block_device=block_device or the appropriate named
parameter expected by flash) while keeping partition=target_partition so
single-file invocations respect --target; update the call site inside the loop
that invokes self.flash(...) to include the block_device argument and run tests
for both single-file and -t combined cases to confirm behavior.
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
1357-1360: Minor: Double space in log message for single operations.When
len(flash_operations) == 1,op_numis empty string, resulting in"Flashing partition..."(double space). The.strip()call only removes leading/trailing whitespace.♻️ Proposed fix
- op_num = f"{idx + 1}/{len(flash_operations)}" if len(flash_operations) > 1 else "" op_desc = f"partition '{target_partition}'" if target_partition else "default target" - - self.logger.info(f"Flashing {op_num} {op_desc} with '{image_file}'".strip()) + if len(flash_operations) > 1: + self.logger.info(f"Flashing {idx + 1}/{len(flash_operations)} {op_desc} with '{image_file}'") + else: + self.logger.info(f"Flashing {op_desc} with '{image_file}'")python/packages/jumpstarter-driver-flashers/README.md (1)
267-270: Consider clarifying terminology in API example.The example
flasherclient.flash("/path/to/image.raw.xz", partition="emmc")usespartition="emmc"whereemmcis actually a target/block device name, not a partition label. The comment "Flash into a specific partition" is misleading - it's flashing to a specific target block device.This reflects the dual-purpose nature of the
partitionparameter in the code, but could confuse API users.📝 Suggested documentation clarification
-Flash into a specific partition +Flash to a specific target block device ```python flasherclient.flash("/path/to/image.raw.xz", partition="emmc")
+Flash to a specific partition by label (on the default target)
+python +flasherclient.flash("/path/to/image.raw.xz", partition="rootfs") +</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Show resolved
Hide resolved
eballetbo
force-pushed
the
flash-to-partitions
branch
from
eb05aa6 to
edc5c8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 1180-1244: The function _resolve_flash_parameters returns 3-tuples
(image_file, target_partition, block_device) but some callers/loops unpack only
two values causing a ValueError; update any unpacking to expect three elements
(e.g., change "for a, b in flash_ops" to "for image_file, target_partition,
block_device in flash_ops") and ensure downstream uses reference the new
block_device variable (same fix applies to the similar unpacking around lines
1363-1374).
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
Show resolved
Hide resolved
|
I will take this for a test ride ASAP since we don't have automated testing for it yet. |
…ge flashing The `flash` CLI command now supports more flexible image flashing options. Users can specify images to flash to specific partitions using the new `-t` option (e.g., `-t rootfs:rootfs.img`). The CLI now also supports flashing multiple images in a single command when using the `-t` option. The command now handles: - Flashing a single file to a default or specified block device (`flash image.img`) - Flashing multiple file-partition pairs to a default or specified block device (`flash -t rootfs:rootfs.img -t boot:boot.img --target emmc`) This is implemented with a new internal `_resolve_flash_parameters` helper to validate and parse the various CLI arguments before executing each flash operation. The core `flash` method is updated to correctly interpret the `partition` argument as a partition label when provided, constructing the appropriate `/dev/disk/by-partlabel/` path. Signed-off-by: Enric Balletbo i Serra <eballetb@redhat.com>
eballetbo
force-pushed
the
flash-to-partitions
branch
from
edc5c8a to
2f5d556
Compare
3 participants
The
flashCLI command now supports more flexible image flashing options. Users can specify images to flash to specific partitions using the new-toption (e.g.,-t rootfs:rootfs.img). The CLI now also supports flashing multiple images in a single command when using the-toption.The command now handles:
flash image.img)flash -t rootfs:rootfs.img -t boot:boot.img --target emmc)This is implemented with a new internal
_resolve_flash_parametershelper to validate and parse the various CLI arguments before executing each flash operation. The coreflashmethod is updated to correctly interpret thepartitionargument as a partition label when provided, constructing the appropriate/dev/disk/by-partlabel/path.Summary by CodeRabbit
New Features
Documentation