-
Notifications
You must be signed in to change notification settings - Fork 13
feat: when cancelling OS upgrade, delete any plugin files that were d… #1823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ownloaded as part of the upgrade
WalkthroughA cleanup operation is added to the revertFiles function in UnraidUpdateCancel.php, which now removes the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1823 +/- ##
==========================================
+ Coverage 52.04% 52.06% +0.02%
==========================================
Files 874 874
Lines 50372 50396 +24
Branches 5017 5014 -3
==========================================
+ Hits 26214 26238 +24
Misses 24083 24083
Partials 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidUpdateCancel.php (1)
49-52: LGTM! Cleanup logic correctly implements the feature.The code appropriately checks for directory existence before deletion and uses a hardcoded path that eliminates injection risks. The implementation is consistent with existing file operation patterns in this function (e.g., line 38's
shell_exec("mv -f ...")).Optional enhancement: Consider capturing the result of
shell_execto verify the deletion succeeded, though the current approach is consistent with the existing codebase style:// Delete plugin files that were downloaded during the OS upgrade if (is_dir("/boot/config/plugins-nextboot")) { - shell_exec("rm -rf /boot/config/plugins-nextboot"); + $result = shell_exec("rm -rf /boot/config/plugins-nextboot 2>&1"); + if ($result !== null && $result !== "") { + error_log("Warning: Issue deleting plugins-nextboot: " . $result); + } }Based on learnings,
rm -rffor broader cleanup is intentional and accepted practice in this codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidUpdateCancel.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidUpdateCancel.php
🧠 Learnings (3)
📓 Common learnings
Learnt from: elibosley
Repo: unraid/api PR: 1657
File: web/scripts/deploy-dev.sh:37-41
Timestamp: 2025-09-04T15:26:34.416Z
Learning: In web/scripts/deploy-dev.sh, the command `rm -rf /usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/*` intentionally removes all contents of the unraid-components directory before deploying standalone components. This broader cleanup is desired behavior according to the maintainer elibosley.
📚 Learning: 2025-09-04T15:26:34.416Z
Learnt from: elibosley
Repo: unraid/api PR: 1657
File: web/scripts/deploy-dev.sh:37-41
Timestamp: 2025-09-04T15:26:34.416Z
Learning: In web/scripts/deploy-dev.sh, the command `rm -rf /usr/local/emhttp/plugins/dynamix.my.servers/unraid-components/*` intentionally removes all contents of the unraid-components directory before deploying standalone components. This broader cleanup is desired behavior according to the maintainer elibosley.
Applied to files:
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidUpdateCancel.php
📚 Learning: 2025-09-04T18:42:53.531Z
Learnt from: pujitm
Repo: unraid/api PR: 1658
File: plugin/plugins/dynamix.unraid.net.plg:73-79
Timestamp: 2025-09-04T18:42:53.531Z
Learning: In the dynamix.unraid.net plugin, versions 6.12.1-6.12.14 and 6.12.15 prereleases are intentionally allowed to install with warnings (rather than immediate cleanup) to provide users with a grace period and notice before functionality is completely removed. This is a deliberate UX decision to avoid immediately breaking existing setups while encouraging upgrades.
Applied to files:
plugin/source/dynamix.unraid.net/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidUpdateCancel.php
…ownloaded as part of the upgrade
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.