luci-app-rustdesk-server: fixes firmware update causes config loss - #8946
smallprogram wants to merge 1 commit into
Conversation
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; commit message matches the diff. Three inline comments — the main one is that the new guard can never be true, because the package already ships the same section in /etc/config/rustdesk-server as a conffile.
Generated by Claude Code
402beb3 to
29abd53
Compare
|
request review and merge @jow- |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (force-push 402beb3 → 29abd53); the commit message still matches the diff.
The two substantive points from the previous review are addressed: the never-reachable guard is gone because the whole uci-defaults script is removed, and PKG_VERSION now moves 20260313 → 20260815 so the fix is actually reachable by apk/opkg upgrade. The shipped root/etc/config/rustdesk-server plus its conffiles registration in Makefile:13-15 still provide the defaults on fresh install and preserve user values across sysupgrade, so nothing is lost by the removal.
One documentation leftover flagged inline.
Generated by Claude Code
Fixes: openwrt#8935 This fixes the issue where configuration settings were lost during a firmware upgrade Signed-off-by: David Mandy <smallprogramzhusir@gmail.com>
29abd53 to
45bc5e0
Compare
|
This Claude Code is amazing. @efahl |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (force-push 29abd53 → 45bc5e0); no new issues found.
The only delta since the last review is the README architecture tree, which now drops the uci-defaults/50-luci-rustdesk-server line and promotes init.d/rustdesk-server to the last └── branch of the etc/ subtree — exactly the leftover flagged previously. No remaining references to the removed script anywhere in the package, and the commit message still matches the diff. CI on 45bc5e0 is green (build, eslint, FormalityCheck).
Generated by Claude Code
|
How can I get this PR merged? |
BKPepe
left a comment
BKPepe
left a comment
There was a problem hiding this comment.
Hi everyone,
I believe that pull request #8188 shouldn't have been merged in the first place. CC: @luminoso
If AI is going to be used, things should be seen through to the end or even better, implemented so they work out of the box on the first try, rather than half-baking everything.
The problem here is that we're constantly trying to patch leaks as they pop up. Ideally, the config and init script should be packaged alongside the binary, and that binary belongs in the OpenWrt packages repository. Instead, the current approach tells the user: "Install LuCI and it will just work" except it doesn't, because you still have to manually download something elsewhere. That is not user-friendly at all.
I might sound a bit strict and this might not be a popular take, but even this commit 1b47d9c shouldn't have been merged. It’s the exact kind of flood-damage quick fix I'm talking about; all that's really needed here is conffiles.
Look, this UI either needs a complete overhaul or it should be removed entirely, because I honestly wonder how many people actually use it in its current state.
I know a lot of what I wrote here isn't directly relevant to your change, but if we don't address it now, we never will and I couldn't help but notice once I opened it up and saw what else was tucked away in those files. I'm sorry. I know this isn't strictly on you and that you're just trying to help, but 🤷
|
Thanks for raising this. I still fully plan to package the binaries for rustdesk app so that installing it or the app includes all the needed binaries out of the box. It’s certainly long overdue, and I’m aiming to have this ready in the next month or so, since I have to study how to do packaging for openwrt. As the keep.d, I haven't worked extensively with keep.d mechanics yet, so if you or anyone else has recommendations on the cleanest implementation, please feel free to share your thoughts. Right now seems that packaging the binaries would solve having the seamless user experience of "install luci and it will just work" done it in one go. |
|
I believe this can be closed in favour of the split the review asked for: the server itself is now a package, openwrt/packages#30572, and the app change that depends on it is #9058. The uci-defaults and keep.d files are gone from the app. |
The app shipped the init script, the UCI config, a uci-defaults script, a sysupgrade keep list and two /usr/libexec wrappers, but not the hbbs and hbbr binaries, so every user had to fetch those by hand. Those files now live in the rustdesk-server package in the packages feed, and installing luci-app-rustdesk-server pulls it in. The uci-defaults script that rewrote enabled to 0 on every install is gone with them, which is the same removal PR openwrt#8946 proposed. The view talks to ubus directly now: service list for the instance status, rc list for the boot state, rc init for the service buttons and for regenerating the key, file read and remove for the key pair. The ucode rpcd plugin is dropped. The binary column is gone, since the package now ships hbbs and hbbr, but the view keeps the per component Enabled column, the Start, Stop and Restart buttons and the Start at Boot toggle. A new open_firewall option replaces the banner that asked for hand written firewall rules. Installs of the previous version own /etc/init.d/rustdesk-server and /etc/config/rustdesk-server. With opkg the old luci-app-rustdesk-server has to be removed before the new one is installed; apk resolves the move within one transaction. Fixes: openwrt#8935 Co-authored-by: David Mandy <smallprogramzhusir@gmail.com> Signed-off-by: Guilherme Cardoso <luminoso@gmail.com>
As you wish—it's up to you. |
The app shipped the init script, the UCI config, a uci-defaults script, a sysupgrade keep list and two /usr/libexec wrappers, but not the hbbs and hbbr binaries, so every user had to fetch those by hand. Those files now live in the rustdesk-server package in the packages feed, and installing luci-app-rustdesk-server pulls it in. The uci-defaults script that rewrote enabled to 0 on every install is gone with them, which is the same removal PR openwrt#8946 proposed. The view talks to ubus directly now: service list for the instance status, rc list for the boot state, file read and remove for the key pair, and the init script through file exec for the service buttons and for regenerating the key, with the ACL listing the five allowed actions. The ucode rpcd plugin is dropped. The binary column is gone, since the package now ships hbbs and hbbr, but the view keeps the per component Enabled column, the Start, Stop and Restart buttons and the Start at Boot toggle. A new open_firewall option replaces the banner that asked for hand written firewall rules. Installs of the previous version own /etc/init.d/rustdesk-server and /etc/config/rustdesk-server. With opkg the old luci-app-rustdesk-server has to be removed before the new one is installed; apk resolves the move within one transaction. Fixes: openwrt#8935 Co-authored-by: David Mandy <smallprogramzhusir@gmail.com> Signed-off-by: Guilherme Cardoso <luminoso@gmail.com>
5 participants
Fixes #8935
This fixes the issue where configuration settings were lost during a firmware upgrade.
Pull request details
Description
fixes firmware update causes configuration loss.
Tested on
OpenWrt version: OpenWrt 25.12.2
LuCI version: LuCI openwrt-25.12 branch
Web browser(s): Chrome
Checklist