do power off by default#313
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplaces single Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
1521-1528:⚠️ Potential issue | 🔴 CriticalBug:
--no-power-offflag becomes non-functional with this change.With
flag_value=Falseanddefault=False, both providing and omitting the flag results inpower_off=False:
--no-power-offprovided →power_off=False- Flag omitted →
power_off=FalseUsers have no CLI mechanism to power off the device after flashing.
To properly implement "do not power off by default", invert the flag semantics:
🐛 Proposed fix using `--power-off` flag
`@click.option`( - "--no-power-off", + "--power-off/--no-power-off", "power_off", - is_flag=True, - flag_value=False, default=False, - help="Leave device powered on after flashing", + help="Power off device after flashing (default: leave powered on)", )This creates a flag pair where:
- No flag →
power_off=False(stays on)--power-off→power_off=True(powers off)--no-power-off→power_off=False(stays on, explicit)🤖 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 CLI flag for powering off is inverted so providing or omitting "--no-power-off" both yield power_off=False; fix by changing the click option for the "power_off" parameter to use a paired boolean flag (e.g. "--power-off/--no-power-off") with default=False so that no flag keeps the device powered on and "--power-off" sets power_off=True; update the click.option decorator that declares "--no-power-off" for the power_off argument accordingly and adjust any docs/tests that expect the old behavior.python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (1)
436-443:⚠️ Potential issue | 🔴 CriticalSame issue:
--no-power-offflag becomes non-functional.Consistent with the flashers client, this flag now always results in
power_off=Falseregardless of whether it's provided.Apply the same fix for consistency across drivers:
🐛 Proposed fix using `--power-off` flag
`@click.option`( - "--no-power-off", + "--power-off/--no-power-off", "power_off", - is_flag=True, - flag_value=False, default=False, - help="Leave device powered on after flashing", + help="Power off device after flashing (default: leave 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 436 - 443, The CLI option currently defines only "--no-power-off" which makes power_off always False; change the click.option to expose a paired flag so Click produces a real boolean. Replace the single "--no-power-off" decorator with a combined option like "--power-off/--no-power-off" (targeting the same parameter name power_off) or switch to a single "--power-off" flag with default True and an explicit "--no-power-off" inverse, so that the function receiving power_off (in client.py where the click.option is declared) gets the correct True/False value depending on the user's flag.
🤖 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 1521-1528: The CLI flag for powering off is inverted so providing
or omitting "--no-power-off" both yield power_off=False; fix by changing the
click option for the "power_off" parameter to use a paired boolean flag (e.g.
"--power-off/--no-power-off") with default=False so that no flag keeps the
device powered on and "--power-off" sets power_off=True; update the click.option
decorator that declares "--no-power-off" for the power_off argument accordingly
and adjust any docs/tests that expect the old behavior.
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py`:
- Around line 436-443: The CLI option currently defines only "--no-power-off"
which makes power_off always False; change the click.option to expose a paired
flag so Click produces a real boolean. Replace the single "--no-power-off"
decorator with a combined option like "--power-off/--no-power-off" (targeting
the same parameter name power_off) or switch to a single "--power-off" flag with
default True and an explicit "--no-power-off" inverse, so that the function
receiving power_off (in client.py where the click.option is declared) gets the
correct True/False value depending on the user's flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b3c8b95-b03d-4769-a3e4-76441dbaf834
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
default-no-poweroff
branch
from
641523e to
e9b790d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-ridesx/README.md`:
- Around line 106-124: The doctest setup creates ridesx_client and
ridesx_power_client but the subsequent `{testcode}` calls to
ridesx_client.flash() fail because image paths like "/path/to/boot.img" don't
exist; either (A) in the `{testsetup}` create temporary sample image files
(e.g., use tempfile.NamedTemporaryFile or write small dummy files) and pass
those real paths so _upload_file_if_needed() → OpendalFile.write_from_path() →
OpendalAdapter/operator.stat succeeds, or (B) change the offending `{testcode}`
blocks to non-executable code-blocks so they are not run as doctests; locate
usages by searching for ridesx_client.flash(), _upload_file_if_needed(),
OpendalFile.write_from_path(), and OpendalAdapter in the README to apply the
chosen fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe952ca5-691b-41ea-96e8-c45351b5f5bb
📒 Files selected for processing (1)
python/packages/jumpstarter-driver-ridesx/README.md
bennyz
force-pushed
the
default-no-poweroff
branch
from
f804178 to
5fa206b
Compare
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
default-no-poweroff
branch
from
5fa206b to
4f0ef59
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-ridesx/README.md (1)
136-144: Consider clarifying thepower_clientvariable.The examples use
power_clientbut don't show where it comes from. For better clarity, consider adding a brief setup comment showing how to obtain the power client instance, similar to how the configuration section shows separatestorageandpowerdrivers.Example:
# Get power client from the power driver instance # power_client = ... (from your RideSXPowerDriver connection) # Turn device power on power_client.on()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-ridesx/README.md` around lines 136 - 144, The examples reference power_client without showing how to obtain it; add a brief setup line before the usage examples that shows obtaining the client from the RideSX power driver (e.g., indicate instantiation or retrieval from RideSXPowerDriver or the power driver variable used in the config), referencing symbols like power_client and RideSXPowerDriver so readers know where power_client comes from and can follow the snippet.
🤖 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/README.md`:
- Around line 136-144: The examples reference power_client without showing how
to obtain it; add a brief setup line before the usage examples that shows
obtaining the client from the RideSX power driver (e.g., indicate instantiation
or retrieval from RideSXPowerDriver or the power driver variable used in the
config), referencing symbols like power_client and RideSXPowerDriver so readers
know where power_client comes from and can follow the snippet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 259c79a4-e994-419d-a868-10120846158e
📒 Files selected for processing (1)
python/packages/jumpstarter-driver-ridesx/README.md
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.8
git worktree add -d .worktree/backport-313-to-release-0.8 origin/release-0.8
cd .worktree/backport-313-to-release-0.8
git switch --create backport-313-to-release-0.8
git cherry-pick -x e9b790dc78d5ccda27b033de9c3b870a9b996f26 4f0ef59ba48ff25be44cc130eb03ca3df7d3ec49 |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.8
git worktree add -d .worktree/backport-313-to-release-0.8 origin/release-0.8
cd .worktree/backport-313-to-release-0.8
git switch --create backport-313-to-release-0.8
git cherry-pick -x e9b790dc78d5ccda27b033de9c3b870a9b996f26 4f0ef59ba48ff25be44cc130eb03ca3df7d3ec49 |
2 participants
--no-power-off should be used explicitly to keep device on after flash
Summary by CodeRabbit