Fixed an RPM issue that also broke OS-tree installs (#29) - #30
Fixed an RPM issue that also broke OS-tree installs (#29)#30PizzaLovingNerd wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRPM packaging now uses dedicated lifecycle hooks for SELinux policy management. The hooks support legacy policy migration, final removal, service reloads, and immutable-host compatibility tests. Generic DEB hooks no longer manage SELinux policy. ChangesRPM packaging lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RPM transaction
participant postinstall-rpm.sh
participant selinux-install-rpm.sh
participant semodule
participant NetworkManager
RPM transaction->>postinstall-rpm.sh: Run RPM post-install hook
postinstall-rpm.sh->>selinux-install-rpm.sh: Install and migrate SELinux policy
selinux-install-rpm.sh->>semodule: Install nm_netbird at priority 200
postinstall-rpm.sh->>NetworkManager: Reload D-Bus and NetworkManager
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/test-immutable-packaging.sh (1)
244-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a scenario for a failing
semodule -lfull -m.The scenarios cover a failed priority-200 install, a checksum mismatch, an absent legacy module, and a failed legacy removal. They do not cover the branch at
packaging/scripts/postinstall-rpm.shLines 82-85, where the checksum inspection itself fails. That branch must preserve both the legacy module and the marker. Olderpolicycoreutilsversions withoutsemodule -mreach exactly this path, so it is the most likely real-world branch on old hosts.Add a
FAIL_MODULE_LIST=1case to the fakesemoduleand assert that neither-X 400 -r nm_netbirdruns nor the marker is removed.🤖 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 `@scripts/test-immutable-packaging.sh` around lines 244 - 253, Extend the immutable packaging test around the fake semodule setup to support FAIL_MODULE_LIST=1, then add a scenario covering checksum inspection failure from semodule -lfull -m. Assert that legacy removal with -X 400 -r nm_netbird is not attempted and the legacy marker remains after the postinstall script exits.packaging/rpm/network-manager-netbird.spec.tmpl (1)
83-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDeduplicate the migration logic shared with
postinstall-rpm.sh.This
%postblock now reimplements the priority-200 install, marker validation, checksum comparison, and legacy priority-400 cleanup thatpackaging/scripts/postinstall-rpm.shalready implements. The two copies differ in style: the spec uses|| var=plus-ztests, while the script uses|| { ...; return 0; }and amodule_checksumhelper. Both run as root and must stay behaviorally identical.
scripts/test-immutable-packaging.shexecutes only the nFPM scripts. For the spec it asserts two literal substrings (Lines 67-69). A future change to one copy can therefore diverge without a test failure.Consider shipping one helper script and calling it from both packaging paths, for example install a
nm-netbird-selinux-lifecyclehelper under%{_datadir}/%{name}/and invoke it from%postand%postun.🤖 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 `@packaging/rpm/network-manager-netbird.spec.tmpl` around lines 83 - 136, Extract the shared SELinux lifecycle logic from the spec’s %post block and packaging/scripts/postinstall-rpm.sh into one installed nm-netbird-selinux-lifecycle helper, including priority-200 installation, marker validation, checksum comparison, and legacy priority-400 cleanup. Update both packaging paths to invoke this helper from their install and uninstall flows, preserving identical behavior and adapting scripts/test-immutable-packaging.sh so the shared implementation remains covered without relying on duplicated literal blocks.packaging/scripts/postinstall-rpm.sh (1)
49-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun
-mon the oldest supported RHEL/Fedora target.
semodule -lfull -mcurrently prints priority, name, type, status, thensha256:HASH, so$4can readdisabledfor a disabled module. Run the command on the oldest supported target before relying on the checksum field offset.🤖 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 `@packaging/scripts/postinstall-rpm.sh` around lines 49 - 61, Validate `semodule -lfull -m` output on the oldest supported RHEL/Fedora target before finalizing `module_checksum`. Update the awk parsing to extract the checksum from the actual `sha256:HASH` field rather than assuming `$4`, including disabled-module output, while preserving priority and `nm_netbird` filtering.
🤖 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 `@scripts/test-immutable-packaging.sh`:
- Around line 131-138: Update rewrite_for_harness to replace the bare
/var/lib/network-manager-netbird directory with the harness-specific path,
keeping the existing longer legacy marker replacement before the directory
replacement. Add a fake rmdir command and include it in the fake-command symlink
setup so cleanup attempts are intercepted and asserted instead of touching the
host filesystem.
---
Nitpick comments:
In `@packaging/rpm/network-manager-netbird.spec.tmpl`:
- Around line 83-136: Extract the shared SELinux lifecycle logic from the spec’s
%post block and packaging/scripts/postinstall-rpm.sh into one installed
nm-netbird-selinux-lifecycle helper, including priority-200 installation, marker
validation, checksum comparison, and legacy priority-400 cleanup. Update both
packaging paths to invoke this helper from their install and uninstall flows,
preserving identical behavior and adapting scripts/test-immutable-packaging.sh
so the shared implementation remains covered without relying on duplicated
literal blocks.
In `@packaging/scripts/postinstall-rpm.sh`:
- Around line 49-61: Validate `semodule -lfull -m` output on the oldest
supported RHEL/Fedora target before finalizing `module_checksum`. Update the awk
parsing to extract the checksum from the actual `sha256:HASH` field rather than
assuming `$4`, including disabled-module output, while preserving priority and
`nm_netbird` filtering.
In `@scripts/test-immutable-packaging.sh`:
- Around line 244-253: Extend the immutable packaging test around the fake
semodule setup to support FAIL_MODULE_LIST=1, then add a scenario covering
checksum inspection failure from semodule -lfull -m. Assert that legacy removal
with -X 400 -r nm_netbird is not attempted and the legacy marker remains after
the postinstall script exits.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 919f5581-9a67-4f48-b785-35e6ec08e047
📒 Files selected for processing (10)
.github/workflows/release.yml.goreleaser.ymlTaskfile.ymldocs/reference.mdpackaging/rpm/network-manager-netbird.spec.tmplpackaging/scripts/postinstall-rpm.shpackaging/scripts/postinstall.shpackaging/scripts/postremove-rpm.shpackaging/scripts/postremove.shscripts/test-immutable-packaging.sh
💤 Files with no reviewable changes (2)
- packaging/scripts/postremove.sh
- packaging/scripts/postinstall.sh
1 participant
Description
Fixes #29
Validation
Tested on Fedora Silverblue
task quality:fullfor risky changesSummary by CodeRabbit
Bug Fixes
Tests