fix(power): activate power-button files in full releases - #2711
fix(power): activate power-button files in full releases#2711Eli Bosley (elibosley) wants to merge 1 commit into
Conversation
Full Unraid releases extract the webgui etc tree under /usr/local, while PR test plugins overlay etc directly under /. Resolve both layouts into the runtime /etc paths before acpid and elogind start. Refs: https://linear.app/lime-technology/issue/DST-139/configurable-power-button-behavior-acpi-prevent-accidental-shutdowns
WalkthroughThe change centralizes ACPI and elogind power-button runtime setup, updates startup scripts to use it, adds runtime coverage for supported layouts, and runs that coverage in the pull-request workflow. ChangesPower-button runtime configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 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 |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![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 (1)
tests/power-button-runtime.sh (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the elogind drop-in mode.
The test verifies only the ACPI handler executable bit. Assert mode
0644for20-unraid-powerbutton.confin both layouts. This detects a regression that can prevent elogind from reading the drop-in.Proposed test update
[[ -x "$POWERBUTTON_RUNTIME_ROOT/etc/acpi/acpi_handler.sh" ]] || exit 1 + [[ "$(stat -c '%a' "$POWERBUTTON_RUNTIME_ROOT/etc/elogind/logind.conf.d/20-unraid-powerbutton.conf")" == 644 ]] || exit 1 }🤖 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 `@tests/power-button-runtime.sh` around lines 34 - 36, Extend the power-button runtime assertions after the existing ACPI executable check to validate that 20-unraid-powerbutton.conf has mode 0644 in both the ACPI and elogind layouts. Use the existing POWERBUTTON_RUNTIME_ROOT paths and fail the test when either mode does not match.
🤖 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 `@etc/rc.d/rc.acpid`:
- Line 14: Update the source logic in etc/rc.d/rc.acpid at line 14 and
etc/rc.d/rc.elogind at line 29 to resolve /etc/rc.d/rc.powerbutton first, then
fall back to /usr/local/etc/rc.d/rc.powerbutton. Ensure both scripts load the
shared powerbutton_configure_acpid and powerbutton_configure_elogind functions
before starting their daemons.
---
Nitpick comments:
In `@tests/power-button-runtime.sh`:
- Around line 34-36: Extend the power-button runtime assertions after the
existing ACPI executable check to validate that 20-unraid-powerbutton.conf has
mode 0644 in both the ACPI and elogind layouts. Use the existing
POWERBUTTON_RUNTIME_ROOT paths and fail the test when either mode does not
match.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03137fc1-5674-4b38-9f66-89143e40bd95
📒 Files selected for processing (5)
.github/workflows/pr-plugin-build.ymletc/rc.d/rc.acpidetc/rc.d/rc.elogindetc/rc.d/rc.powerbuttontests/power-button-runtime.sh
|
|
||
| # run & log functions | ||
| . /etc/rc.d/rc.runlog | ||
| . /etc/rc.d/rc.powerbutton |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Load rc.powerbutton from the full-release path.
The package mapping installs etc/rc.d/rc.powerbutton under /usr/local/etc/rc.d, but both scripts source only /etc/rc.d/rc.powerbutton. In a full release, powerbutton_configure_acpid and powerbutton_configure_elogind are undefined. The power-button files are not configured before either daemon starts.
etc/rc.d/rc.acpid#L14-L14: resolve/etc/rc.d/rc.powerbuttonfirst, then fall back to/usr/local/etc/rc.d/rc.powerbutton.etc/rc.d/rc.elogind#L29-L29: use the same resolver and fallback order.
Proposed update
-. /etc/rc.d/rc.powerbutton
+if [[ -r /etc/rc.d/rc.powerbutton ]]; then
+ . /etc/rc.d/rc.powerbutton
+else
+ . /usr/local/etc/rc.d/rc.powerbutton
+fi📍 Affects 2 files
etc/rc.d/rc.acpid#L14-L14(this comment)etc/rc.d/rc.elogind#L29-L29
🤖 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 `@etc/rc.d/rc.acpid` at line 14, Update the source logic in etc/rc.d/rc.acpid
at line 14 and etc/rc.d/rc.elogind at line 29 to resolve
/etc/rc.d/rc.powerbutton first, then fall back to
/usr/local/etc/rc.d/rc.powerbutton. Ensure both scripts load the shared
powerbutton_configure_acpid and powerbutton_configure_elogind functions before
starting their daemons.
1 participant
Summary
The released OS included PR 2671's power-button files under
/usr/local/etc, but boot services consume runtime files under/etc. This PR resolves the release layout before acpid and elogind start, while preserving the existing preview-plugin layout.Why This Exists
The latest full-release QA on DST-139 / OS-463 failed: Larry tested Unraid 7.4.0-beta.0.2 with PR 2671 included and a clean boot, but Do nothing still shut the server down. The UI passed; the stock ACPI handler remained active.
Resolution
A shared boot helper detects the direct
/etcpreview-plugin layout and the/usr/local/etcfull-release layout. It materializes the handler and elogind drop-in into the runtime/etcpaths before the services start.Reviewer Considerations
/etclayout remains authoritative when present, so preview-plugin overlays continue to work./etc; no existing runtime override is overwritten.Behavior Changes
On a clean full-release boot, the configurable ACPI handler is installed and elogind loads the power-key ignore drop-in. The Power Options setting now controls a physical power-button event without requiring a plugin or manual daemon restarts.
Implementation Summary
rc.powerbuttonruntime path configuration.rc.acpidandrc.elogindto configure their runtime files before starting.Verification
bash tests/power-button-runtime.shbash -non the changed shell scripts and PR-plugin generatorshellcheck --severity=warningon the new helper and testgit diff --checkRisk
Low. The change only adds a fallback copy from the known release packaging location and leaves existing runtime files untouched. A clean full-release QAVM retest is still required.
Summary by CodeRabbit
New Features
Bug Fixes
Tests