Enhance flash CLI for partition-based and multiple image flashing#159
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCLI and client now support multi-image flashing: partition:FILE mappings and optional block device are parsed into per-operation tuples; each operation resolves a flash target (partition label → /dev/disk/by-partlabel/... or block device), fetches image/checksum, and invokes the flasher per target with updated logging. 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
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 -->
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).
|
I will take this for a test ride ASAP since we don't have automated testing for it yet. |
eballetbo
force-pushed
the
flash-to-partitions
branch
from
edc5c8a to
2f5d556
Compare
|
A linter job is complaining about too much complexity in _perform_flash_operation I don't think I add much more complexity to that function, probably this function needs some refactor, but could be done as a follow up patches? |
I think by simply extracting this part to a separate method we can please the linter: # Check if partition is a valid target device name in the manifest
# If not, treat it as a partition label and use the specified block_device or default target
partition_label = None
if partition and partition in manifest.spec.targets:
target = partition
else:
# partition is a partition label, not a target device
partition_label = partition
# Use specified block_device if provided, otherwise use default target
if block_device:
target = block_device
else:
target = self.call("get_default_target") or manifest.spec.default_targetI do agree it doesn't really add much complexity, but the linter just counts branches |
eballetbo
force-pushed
the
flash-to-partitions
branch
from
2f5d556 to
3cd10f3
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 304-356: The _resolve_and_prepare_target function treats a
provided partition string as a manifest target if it matches
manifest.spec.targets, causing collisions with partition labels and silently
discarding block_device; update the logic to detect this ambiguity and handle it
explicitly: if partition is provided and also exists in manifest.spec.targets,
log a warning (or raise/choose priority per policy) mentioning the collision
between partition_label and manifest target name, include which branch was
chosen, and preserve or surface block_device instead of discarding it; adjust
callers or documentation if you choose deterministic priority, and reference
_resolve_and_prepare_target, manifest.spec.targets, partition_label,
block_device, _get_target_device, and call("get_default_target") when
implementing the change.
🧹 Nitpick comments (3)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (3)
1400-1423: Multi-partition flashing triggers a full device reboot cycle per image.Each call to
self.flash()in this loop performs the entire lifecycle: bundle setup → busybox boot → network config → image transfer → flash → reboot. Forflash -t rootfs:rootfs.img -t boot:boot.img, this means two complete boot cycles, which could be very slow on real hardware.This is fine as a first iteration, but consider adding a TODO or tracking issue for a future optimization that batches multiple partitions within a single busybox session.
1401-1405: Minor: double space in log message for single-operation case.When
len(flash_operations) == 1,op_numis"", so the f-string produces"Flashing default target with 'image.img'"(note double space). The trailing.strip()doesn't fix inner whitespace.💅 Suggested 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()) + op_desc = f"partition '{target_partition}'" if target_partition else "default target" + 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}'")
1288-1300: The--targetoption name collides with the-tshort form.
--targethas no explicit short form, while the partitions option uses-tas its flag. Click won't confuse them (one is--target, the other is-t), but for users this is easy to mix up — especially since-ttypically looks like the short form of--target. Consider renaming the block device option to--block-deviceor giving it an explicit short form like-bto reduce confusion.
eballetbo
force-pushed
the
flash-to-partitions
branch
from
3cd10f3 to
2282aab
Compare
|
@mangelajo I think we have some potential for a little UX confusion, in the base flasher -t is short for --target (and that's how ridesx uses it for example), but here these are different flags |
but in ridesx4 in the end those "targets" are partitions on disk. Isn't that equivalent to this? |
yes, i just mean in the base flash -t is short for --target, but here they mean different things, -t is for partitions and --target is for the block device it's not necessarily bad, just something to consider, maybe switching back to -p/--partition would make sense |
|
For me was also confusing but I intentionally used -t to follow what is done in ridex4 flasher. But looking again, |
no no, in my opinion it's fine, just something to be aware of, but i think as usage of OCI will increase these flags will not be used as much, so it's not a big deal |
|
opps, I was about to merge but conflict, let's see if I can resolve it. |
|
Ok, I was trying to resolve by my brain is fried at this point :) may be you guys can give it a try tomorrow, it's just clashing with the oci credentials PR |
|
@mangelajo i don't mind reverting my PR, it needs an additional fix anyway |
|
I was trying to rebase my patches on top of HEAD this morning, but found some errors which I'm not sure are introduced by my rebase, looks like I need to normalize to str(...) for some string/URL operations, otherwise I fail to flash with my workflow. Is that the fix are you talking about? If that's the case I'll leave to you ;-) |
|
@eballetbo can you share what you test, so i can make sure it works after rebasing? |
|
I only do basic tests: But after rebasing and fix some conflicts, I got this running the second command: Which makes sense, because PosixPath has no the 'startswith' attribute, so we should probably normalize these kinds of paths to str(...) |
…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>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
flash-to-partitions
branch
from
2282aab to
cea7d23
Compare
|
@eballetbo updated the PR, it seems to work on my end, please let me know if it's acceptable |
|
@bennyz LGTM |
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