ride4: add support for fastboot erase#851
Conversation
📝 WalkthroughWalkthroughThe PR adds ChangesErase Partition Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant RideSXClient
participant RideSXDriver
participant fastboot
User->>RideSXClient: j storage erase <partition>
RideSXClient->>RideSXDriver: erase_partition(device_id, partition)
RideSXDriver->>fastboot: fastboot -s device_id erase partition
fastboot-->>RideSXDriver: status / stdout / stderr
RideSXDriver-->>RideSXClient: result
RideSXClient-->>User: echoed status
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
bennyz
added
the
build-pr-images/jumpstarter
Container ImagesThe following container images have been built for this PR:
Images expire after 7 days. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
497-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCLI
erasediscards the result and gives no user feedback or confirmation.
self.erase_partition(partition)'s return value is silently dropped, so a successfulj storage eraseprints nothing to confirm success. Additionally, erase is a destructive/irreversible operation with no confirmation prompt, unlike the risk typically associated with wiping a partition.♻️ Proposed fix
`@base.command`() `@click.argument`("partition") - def erase(partition): + `@click.option`("--yes", "-y", is_flag=True, help="Skip confirmation prompt") + def erase(partition, yes): """Erase a partition using fastboot. \b Examples: j storage erase recoveryinfo j storage erase userdata """ - self.erase_partition(partition) + if not yes: + click.confirm(f"This will erase partition '{partition}'. Continue?", abort=True) + result = self.erase_partition(partition) + click.echo(f"Erased partition '{partition}': {result.get('status')}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py` around lines 497 - 508, The CLI erase command in the erase function discards the result of self.erase_partition(partition) and gives no confirmation to the user. Update erase to capture the return value from erase_partition and print or echo a clear success message when the operation completes, and add a confirmation prompt before invoking the destructive erase action so accidental wipes are prevented.python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py (1)
346-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded 120s timeout is inconsistent with other operation timeouts.
decompression_timeout,flash_timeout, andcontinue_timeoutare all configurable dataclass fields (Lines 20-22), but the erase timeout is hardcoded inline. Consider adding anerase_timeoutfield for consistency and configurability.♻️ Proposed fix
decompression_timeout: int = field(default=15 * 60) # 15 minutes flash_timeout: int = field(default=30 * 60) # 30 minutes continue_timeout: int = field(default=20 * 60) # 20 minutes + erase_timeout: int = field(default=120) # 2 minutes- result = subprocess.run(cmd, capture_output=True, text=True, check=True, timeout=120) + result = subprocess.run(cmd, capture_output=True, text=True, check=True, timeout=self.erase_timeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py` at line 346, The erase operation in the subprocess.run call inside driver.py uses a hardcoded 120-second timeout, unlike the other configurable timeouts. Add an erase_timeout dataclass field alongside decompression_timeout, flash_timeout, and continue_timeout, then update the erase path to use that field instead of the inline constant in the method that runs the erase command.python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py (1)
433-433: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist repeated
DriverErrorimport to module top.
from jumpstarter.client.core import DriverErroris re-imported inline in three separate test functions. Move it to the top-level imports once instead of duplicating per-test.♻️ Suggested consolidation
+from jumpstarter.client.core import DriverError + def test_erase_partition_failure(ridesx_driver): with serve(ridesx_driver) as client: - from jumpstarter.client.core import DriverError - with patch("subprocess.run") as mock_subprocess:(repeat removal for
test_erase_partition_timeoutandtest_erase_partition_fastboot_not_found)Also applies to: 447-447, 458-458
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py` at line 433, The test module is re-importing DriverError inside multiple test functions instead of using a single top-level import. Move the import from jumpstarter.client.core to the module-level imports in driver_test and remove the repeated inline imports from test_erase_partition, test_erase_partition_timeout, and test_erase_partition_fastboot_not_found so the symbol is defined once and reused consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py`:
- Around line 36-51: The erase_partition flow leaves the device powered on,
unlike the flash paths that clean up afterward. Update erase_partition in
client.py to mirror the _execute_flash_operation behavior by calling
_power_off_if_available after the erase_partition call completes, while keeping
the existing fastboot detection and click.ClickException handling unchanged. Use
the erase_partition method and _power_off_if_available helper as the key places
to make the change.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py`:
- Around line 497-508: The CLI erase command in the erase function discards the
result of self.erase_partition(partition) and gives no confirmation to the user.
Update erase to capture the return value from erase_partition and print or echo
a clear success message when the operation completes, and add a confirmation
prompt before invoking the destructive erase action so accidental wipes are
prevented.
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py`:
- Line 433: The test module is re-importing DriverError inside multiple test
functions instead of using a single top-level import. Move the import from
jumpstarter.client.core to the module-level imports in driver_test and remove
the repeated inline imports from test_erase_partition,
test_erase_partition_timeout, and test_erase_partition_fastboot_not_found so the
symbol is defined once and reused consistently.
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py`:
- Line 346: The erase operation in the subprocess.run call inside driver.py uses
a hardcoded 120-second timeout, unlike the other configurable timeouts. Add an
erase_timeout dataclass field alongside decompression_timeout, flash_timeout,
and continue_timeout, then update the erase path to use that field instead of
the inline constant in the method that runs the erase command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5bad5f9-51d5-4840-9463-6f5557152171
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-ridesx/README.mdpython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py
bennyz
force-pushed
the
erase-recoveryinfo
branch
from
f0690ba to
ae7b728
Compare
|
|
||
| self.logger.info(f"Erasing partition '{partition}' on device {device_id}") | ||
|
|
||
| cmd = ["fastboot", "-s", device_id, "erase", "--", partition] |
There was a problem hiding this comment.
why the -- between erase and partition?
There was a problem hiding this comment.
that's weird, i missed this hallucination but for some reason the operation worked correctly in the lab
There was a problem hiding this comment.
yep, knowing you I suspected it was lab-tested but I was looking at the fastboot command and didn't seem to be needed.
There was a problem hiding this comment.
updated and retested, not sure why it worked though, an explanation could be that it tries to erase both "--" and "partition"
bennyz
force-pushed
the
erase-recoveryinfo
branch
from
ae7b728 to
6a8a957
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py (1)
330-363: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider guarding against argument/flag injection for
partitionanddevice_id.
subprocess.runhere uses a list of args with noshell=True, so the static analysis CWE-78 flag on Line 347 is a false positive — there's no shell interpolation risk. However,partitionanddevice_idare passed through unvalidated as fastboot CLI arguments; a value beginning with-(e.g. a leading-dash partition name) could be misinterpreted byfastbootas a flag instead of a positional argument. Giveneraseis destructive/irreversible, this is worth a stricter check than existing methods likeflash_with_fastbootapply topartition_name/device_id.🛡️ Optional stricter validation
if not partition or not partition.strip(): raise ValueError("Partition name cannot be empty") + if partition.startswith("-"): + raise ValueError(f"Invalid partition name: '{partition}'")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py` around lines 330 - 363, The erase_partition method in driver.py should reject fastboot arguments that could be interpreted as flags, not just empty values. Add stricter validation for both device_id and partition in erase_partition so they cannot start with a dash (matching the safer checks used in flash_with_fastboot for partition_name/device_id), and keep the subprocess.run call as a list-based invocation. Ensure any invalid input raises a clear ValueError before building the fastboot command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py`:
- Around line 330-363: The erase_partition method in driver.py should reject
fastboot arguments that could be interpreted as flags, not just empty values.
Add stricter validation for both device_id and partition in erase_partition so
they cannot start with a dash (matching the safer checks used in
flash_with_fastboot for partition_name/device_id), and keep the subprocess.run
call as a list-based invocation. Ensure any invalid input raises a clear
ValueError before building the fastboot command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8a27215-f074-4dc9-8499-fc36d52ea902
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-ridesx/README.mdpython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-driver-ridesx/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py
Support `fastboot erase` operation via the ride4 driver. This allows erasing partitions the following way: j storage erase recoveryinfo Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
erase-recoveryinfo
branch
from
6a8a957 to
ba4858e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py (1)
331-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn type hint style inconsistency.
erase_partitionuses the lowercase builtin genericdict[str, str], while other exported methods in this file (e.g.flash_with_fastbootat Line 178) useDict[str, str]imported fromtyping. Consider aligning for consistency within the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py` at line 331, The return type annotation on erase_partition is inconsistent with the rest of driver.py, since it uses the builtin generic dict[str, str] while exported methods like flash_with_fastboot use typing.Dict[str, str]. Update erase_partition to use the same Dict[str, str] style as the other public methods in this file so the type hints are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py`:
- Line 331: The return type annotation on erase_partition is inconsistent with
the rest of driver.py, since it uses the builtin generic dict[str, str] while
exported methods like flash_with_fastboot use typing.Dict[str, str]. Update
erase_partition to use the same Dict[str, str] style as the other public methods
in this file so the type hints are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11c0ea0f-b99b-42a8-8f2a-b2f49a7a1a89
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-ridesx/README.mdpython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter-driver-ridesx/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py
|
Successfully created backport PR for |
2 participants
Support
fastboot eraseoperation via the ride4 driver.This allows erasing partitions the following way: