add --no-power-off to flashers#296
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Client
participant Flasher as FlashOperation
participant Power as PowerController
User->>CLI: run flash command (--no-power-off?)
CLI->>Client: flash(..., power_off=bool)
Client->>Flasher: perform flash operation
Flasher-->>Client: flash result (success/failure)
alt power_off == true
Client->>Power: power_off device
Power-->>Client: powered off
else power_off == false
Client-->>User: log "left powered on (--no-power-off)"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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)
1586-1608:⚠️ Potential issue | 🟠 MajorApply
--no-power-offonly to the last flash in a batch.Line 1608 forwards the user flag into every per-image
self.flash()call. That is unsafe for multi--tflows, because_perform_flash_operation()always issues arebootbefore returning; withpower_off=False, the next iteration can start while the DUT is still rebooting. Keep intermediate operations on the existing power-off path and use the user’s choice only for the final image.🛠️ Suggested adjustment
# Execute each flash operation for idx, (image_file, target_partition, block_device) in enumerate(flash_operations): 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()) + should_power_off = True if idx < len(flash_operations) - 1 else power_off + # Perform the flash operation self.flash( image_file, partition=target_partition, block_device=block_device, os_image_checksum=checksum, force_exporter_http=force_exporter_http, force_flash_bundle=force_flash_bundle, cacert_file=cacert, insecure_tls=insecure_tls, headers=headers_dict, bearer_token=bearer, retries=retries, method=method, fls_version=fls_version, fls_binary_url=fls_binary_url, - power_off=power_off, + power_off=should_power_off, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py` around lines 1586 - 1608, The loop that calls self.flash(...) in the flash batch forwards the user-specified power_off flag to every image, which can cause the next iteration to start while the device is rebooting; change the call in the for-loop (inside _perform_flash_operation / the batch-flashing logic) to compute an is_last value (idx == len(flash_operations) - 1) and pass power_off only for the final flash (power_off if is_last else True) so intermediate flashes use the existing power-off behavior while the user's choice applies to the last image.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 1586-1608: The loop that calls self.flash(...) in the flash batch
forwards the user-specified power_off flag to every image, which can cause the
next iteration to start while the device is rebooting; change the call in the
for-loop (inside _perform_flash_operation / the batch-flashing logic) to compute
an is_last value (idx == len(flash_operations) - 1) and pass power_off only for
the final flash (power_off if is_last else True) so intermediate flashes use the
existing power-off behavior while the user's choice applies to the last image.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04966f53-bc58-407f-b95f-6e69843c4068
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
bennyz
force-pushed
the
optional-power-off
branch
from
a35102c to
b7b6a7d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
115-119: Use neutral log wording in the shared helper.
_power_off_if_available()is now used by the programmatic API as well, so mentioning--no-power-offin the log is CLI-specific and a bit misleading outside that path.Suggested tweak
- self.logger.info("leaving device powered on (--no-power-off)") + self.logger.info("leaving device powered on")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py` around lines 115 - 119, The helper _power_off_if_available uses a CLI-specific message; update the logger call inside that function (the self.logger.info(...) in _power_off_if_available) to use neutral wording that doesn't reference --no-power-off (e.g., "leaving device powered on (power_off disabled)" or similar) so the message is accurate for both CLI and programmatic callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py`:
- Around line 115-119: The helper _power_off_if_available uses a CLI-specific
message; update the logger call inside that function (the self.logger.info(...)
in _power_off_if_available) to use neutral wording that doesn't reference
--no-power-off (e.g., "leaving device powered on (power_off disabled)" or
similar) so the message is accurate for both CLI and programmatic callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbab643c-6b6b-485c-8c73-ac316c4f8d2b
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Sometimes we would not want to power off the device after flashing Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
optional-power-off
branch
from
b7b6a7d to
9344a3b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
516-517: Keep shared logs CLI-agnostic.
BaseFlasherClient.flash()is public API as well as CLI plumbing, so mentioning--no-power-offin the log leaks CLI details into programmatic use.🧹 Small log cleanup
- self.logger.info("Leaving target powered on (--no-power-off)") + self.logger.info("Leaving target powered on")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py` around lines 516 - 517, The log in BaseFlasherClient.flash() currently mentions a CLI flag and leaks CLI details; change the logger.info call that now says "Leaving target powered on (--no-power-off)" to a CLI-agnostic message such as "Leaving target powered on" or "Leaving target powered on per request" so the public API doesn't expose CLI-specific text; update the log in the flash method where this message is emitted.python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (2)
126-160: Add regression tests for the opt-out cleanup path.The new behavior is centralized here, but the existing
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.py:45-73only checks thecall()sequence for a success case. It would not catchpower_off=Falsestill calling_power_off_if_available, and it does not cover the failure path at all. Please add at least one success test and one error-path test that assert the cleanup is skipped when the flag is false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py` around lines 126 - 160, Add regression tests for _execute_flash_operation to cover the opt-out cleanup path: create one success test and one error-path test that call client._execute_flash_operation(operation_func, power_off=False) and assert client._power_off_if_available is NOT invoked. In each test, patch/mocking should stub out client.boot_to_fastboot and provide a fake operation_func (a callable that returns normally for the success test and raises an exception for the error test), and verify that on success the operation's result/flow is correct and on error the original exception is propagated (or wrapped behavior preserved) while _power_off_if_available remains uncalled; name the new tests to reflect success and error scenarios and place them alongside the existing tests in client_test.py.
167-175: Addpower_offto theFlasherClientInterfaceabstract signature.
RideSXClient.flash(..., power_off=...)is one of multiple implementations that include this parameter—BaseFlasherClient(injumpstarter-driver-flashers) already has it too. The abstract interface inpython/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py:527-550should includepower_offto reflect this shared behavior across implementations and ensure type consistency.Minimal follow-up
# python/packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py def flash( self, path: PathBuf | dict[str, PathBuf], *, target: str | None = None, operator: Operator | dict[str, Operator] | None = None, compression: Compression | None = None, + power_off: bool = True, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py` around lines 167 - 175, The FlasherClientInterface abstract method signature is missing the power_off parameter, causing mismatches with implementations like RideSXClient.flash and BaseFlasherClient; update the abstract method (FlasherClientInterface.flash) to include the power_off: bool = True parameter (or the same typing as implementations) and propagate that parameter into the abstract method's docstring/type hints so all implementers match the interface and type check correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 1521-1528: The --no-power-off click.option currently maps to the
variable power_off but the flashing loop applies power_off=False only to the
final item in a multi-target run; either change the flashing loop so each
iteration uses the passed power_off value (i.e., don't override power_off
per-iteration) or update the option help text to explicitly state it only
affects the final flash in a batch. Locate the click.option("--no-power-off") /
power_off variable and the loop that iterates over targets in the flashing
routine (where power_off is currently applied only for the last item) and
implement one of the two fixes: propagate power_off to every iteration, or
change the help string to "Leave device powered on after flashing (applies only
to the final target in a batch)".
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 516-517: The log in BaseFlasherClient.flash() currently mentions a
CLI flag and leaks CLI details; change the logger.info call that now says
"Leaving target powered on (--no-power-off)" to a CLI-agnostic message such as
"Leaving target powered on" or "Leaving target powered on per request" so the
public API doesn't expose CLI-specific text; update the log in the flash method
where this message is emitted.
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py`:
- Around line 126-160: Add regression tests for _execute_flash_operation to
cover the opt-out cleanup path: create one success test and one error-path test
that call client._execute_flash_operation(operation_func, power_off=False) and
assert client._power_off_if_available is NOT invoked. In each test,
patch/mocking should stub out client.boot_to_fastboot and provide a fake
operation_func (a callable that returns normally for the success test and raises
an exception for the error test), and verify that on success the operation's
result/flow is correct and on error the original exception is propagated (or
wrapped behavior preserved) while _power_off_if_available remains uncalled; name
the new tests to reflect success and error scenarios and place them alongside
the existing tests in client_test.py.
- Around line 167-175: The FlasherClientInterface abstract method signature is
missing the power_off parameter, causing mismatches with implementations like
RideSXClient.flash and BaseFlasherClient; update the abstract method
(FlasherClientInterface.flash) to include the power_off: bool = True parameter
(or the same typing as implementations) and propagate that parameter into the
abstract method's docstring/type hints so all implementers match the interface
and type check correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 804809f6-3718-463d-9c3c-b588a8f88a7d
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Comment on lines
+1521
to
+1528
| @click.option( | ||
| "--no-power-off", | ||
| "power_off", | ||
| is_flag=True, | ||
| flag_value=False, | ||
| default=True, | ||
| help="Leave device powered on after flashing", | ||
| ) |
Contributor
There was a problem hiding this comment.
Clarify batch semantics of --no-power-off.
Line 1527 says the device is left on “after flashing”, but Line 1610 applies power_off=False only to the final item in a multi--t run. That help text is misleading for batch flashes. Either honor the flag for every operation or document that it only affects the last flash in the batch.
✏️ Suggested help-text tweak
- help="Leave device powered on after flashing",
+ help="Leave device powered on after flashing; in batch mode this only applies to the final operation",Also applies to: 1592-1610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`
around lines 1521 - 1528, The --no-power-off click.option currently maps to the
variable power_off but the flashing loop applies power_off=False only to the
final item in a multi-target run; either change the flashing loop so each
iteration uses the passed power_off value (i.e., don't override power_off
per-iteration) or update the option help text to explicitly state it only
affects the final flash in a batch. Locate the click.option("--no-power-off") /
power_off variable and the loop that iterates over targets in the flashing
routine (where power_off is currently applied only for the last item) and
implement one of the two fixes: propagate power_off to every iteration, or
change the help string to "Leave device powered on after flashing (applies only
to the final target in a batch)".
mangelajo
approved these changes
Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sometimes we would not want to power off the device after flashing
Summary by CodeRabbit
New Features
UX