luci-app-attendedsysupgrade: add uci-defaults script support#8703
luci-app-attendedsysupgrade: add uci-defaults script support#8703firlin123 wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the single new commit. The change is well-scoped: the new 40_luci-attendedsysupgrade uci-defaults script matches the established cache-clear/rpcd-reload convention used by other luci apps, the rpcd ucode get_defaults method (fs.stat/fs.readfile, size cap) is correct API usage, the ACL entry is wired to the new method, and the defaults field is correctly omitted from the build request when unset. Two minor inline notes below (trailing whitespace; silent rejection of oversized/invalid files). The commit message line-length warning is already reported by the formality bot.
Generated by Claude Code
1888178 to
d5eb355
Compare
Allow users to specify a custom first-boot script (uci-defaults) to be included in the firmware build request to the ASU server. Signed-off-by: Vladislav Pereyaslov <firlin123@gmail.com>
This comment has been minimized.
This comment has been minimized.
Introduce dedicated error codes for uci-defaults script handling to display explicit frontend warnings if the file is missing, empty, or exceeds the 40 KiB ASU server limit. Signed-off-by: Vladislav Pereyaslov <firlin123@gmail.com>
d5eb355 to
d4e5e16
Compare
| }; | ||
| } | ||
| // Enforce the 40 KiB official ASU server limit | ||
| if (st.size > 40960) { |
There was a problem hiding this comment.
You can't hard code this number as it varies between servers and can change arbitrarily even on the "official" upstream. You need to fetch it from the server limits: curl https://sysupgrade.openwrt.org/json/v1/overview.json | jsonfilter -e '$.server.max_defaults_length'
There was a problem hiding this comment.
Removed the hard-coded number and now using server's max_defaults_length in the latest commit.
| const ERR_FILE_TOO_BIG = 3; | ||
|
|
||
| function get_defaults() { | ||
| const path = uci.cursor().get("attendedsysupgrade", "client", "defaults_path"); |
There was a problem hiding this comment.
This value is already in the config in attendedsysupgrade.owut.init_script. See https://openwrt.org/docs/guide-user/installation/sysupgrade.owut#using_a_uci-defaults_script
There was a problem hiding this comment.
I was not aware that a config for this already existed. Switched to using attendedsysupgrade.owut.init_script in the latest commit.
| profile, | ||
| version: candidates[0][0], | ||
| packages: Object.keys(packages).sort(), | ||
| defaults_checked: '1' |
There was a problem hiding this comment.
I tried making it bool but the form.Flag only accepts "1" and 1 (which it type-coerces to "1" anyway).
| form.Value, | ||
| 'defaults_path', | ||
| _('Defaults file path'), | ||
| _('Specify the path to your first-boot script (e.g., <code>/rom/etc/uci-defaults/99-asu-defaults</code> inserted by the Firmware Selector.)') |
There was a problem hiding this comment.
If no config value exists, but the /rom/.../99-asu-defaults file exists, this field should auto-populate with the existing file path.
There was a problem hiding this comment.
Because I switched to attendedsysupgrade.owut.init_script as you suggested, I don't control the defaults of this config anymore, owut does.
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (d4e5e16 "improve uci-defaults validation"); no new issues found.
The new commit cleanly addresses both notes from my prior review: oversized/invalid/empty files now surface an explicit alert-message warning instead of silently dropping the option, and the trailing whitespace is gone. The ucode error-code handling (fs.stat type/size checks, length(trim(contents)) emptiness check) is correct, the expect/resolveDefault defaults were updated consistently, and the commit message matches the diff. Wrapping data.defaults_path in E('code', {}, ...).outerHTML also correctly escapes the path before it reaches innerHTML.
The substantive open points (hardcoded 40 KiB limit vs. the server's max_defaults_length, reuse of attendedsysupgrade.owut.init_script, the bool question, and path auto-population) are already covered in @efahl's inline comments on this head, so I won't duplicate them.
Generated by Claude Code
This comment has been minimized.
This comment has been minimized.
33b9e93 to
1327ff7
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (1327ff7 "switch to owut config and server-driven limits"). The switch to attendedsysupgrade.owut.init_script and the server-supplied max_defaults_length correctly addresses @efahl's earlier notes, and the new ERR_EMPTY_FILE/ERR_SERVER_FAILED/ERR_INIT_DISALLOWED error paths are wired consistently between the rpcd ucode and the frontend. One inline note on a TypeError reachable when the overview request rejects for a SNAPSHOT device.
Generated by Claude Code
1327ff7 to
c5482d5
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (c5482d5, replacing the previously-reviewed 1327ff7); no new issues found.
The only delta since my last review is the overview.js refactor that hoists init_script_path out of the branches and guards results[0] with optional chaining (results[0]?.ok). This correctly addresses the TypeError I flagged previously: when the overview request rejects (e.g. for a SNAPSHOT device), results[0] is the resolveDefault fallback rather than a response object, and ?.ok now yields null → server_info = null → ERR_SERVER_FAILED. The added typeof server_info.max_defaults_length !== 'number' check also avoids passing NaN into callGetInitScript(Math.floor(...)) when the server advertises allow_defaults without a numeric limit. The commit subject (75 chars) and message match the diff, and Test Formalities now passes on this head.
Generated by Claude Code
c5482d5 to
4311997
Compare
|
Fixed eslint |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the force-pushed head (4311997e, replacing the previously-reviewed c5482d5); no new issues found.
The only delta since my last review is wrapping the default: case body in overview.js (handleCheck's switch (init_script_error)) in a block { }. This correctly resolves the eslint no-case-declarations error caused by the lexical declarations (let msg, let alertBox, const kib) in that case, with no behavioral change. The reworded third commit subject is now 75 chars and matches its diff.
Generated by Claude Code
| @@ -61,6 +66,22 @@ return view.extend({ | |||
| o.default = '0'; | |||
| o.rmempty = false; | |||
|
|
|||
| // For now, only display if the owut section exists in uci (owut installed or section is manually added). | |||
| // We should probably move init script path from attendedsysupgrade.owut.init_script to some common place | |||
There was a problem hiding this comment.
Yeah, too late now. That would require a significant rewrite of how owut gather's uci default values, and corresponding upgrade scripting, so not going to happen. LuCI already grabs another owut value (rootfs_size), and will always be a second class citizen compared to the far-more-capable owut.
There was a problem hiding this comment.
So should we not display that config at all and only make it configurable via cli (like the owut.rootfs_size)? Or just remove the "for now" phrasing from the comment?
| #!/bin/sh | ||
| rm -rf /var/luci-modulecache/; rm -f /var/luci-indexcache; | ||
| [ -x /etc/init.d/rpcd ] && /etc/init.d/rpcd reload | ||
| exit 0 |
There was a problem hiding this comment.
Why is this whole file here? The tmp directories are deleted on reboot, and the rpcd reloading isn't anything that attended sysupgrade should care about.
There was a problem hiding this comment.
I honestly don't know. I just looked at other luci apps with ucode rpcd service and they all seem to have this file so I copied it here too. Is it not needed in this case?
| return { | ||
| contents: "", | ||
| path: path, | ||
| error: ERR_EMPTY_FILE |
There was a problem hiding this comment.
If people want to include an empty placeholder script, I don't see any reason to disallow that.
There was a problem hiding this comment.
Allowed empty files in latest revision.
4311997 to
83ae949
Compare
…limits Reuse exiting 'owut.init_script' instead of separate 'client.defaults_path'. Use ASU server's 'max_defaults_length' instead of hardcoded 40 KiB limit. Signed-off-by: Vladislav Pereyaslov <firlin123@gmail.com>
83ae949 to
ae849bc
Compare
Pull request details
Description
Added the ability to specify uci-defaults script (
defaultsfield in request to ASU) to be sent to ASU server during firmware generation.Note: This is my first PR ever and I'm an ESL speaker. Let me know if changes are needed!
Screenshot or video of changes (if applicable)
Configuration tab screenshot:

Upgrade modal screenshot:

Warning in upgrade modal screenshot:

Tested on
Board: Raspberry Pi Compute Module 4 (bcm27xx/bcm2711)
OpenWrt version: OpenWrt 25.12.2 (r32802-f505120278) and 25.12.4 (r32933-4ccb782af7)
LuCI version: LuCI openwrt-25.12 branch (26.158.67103
e99e132) and LuCI openwrt-25.12 branch (26.163.740245476212)Web browser(s): Brave 1.90.121 (Official Build) (64-bit), Chromium: 148.0.7778.96
Checklist
Signed-off-by: <my@email.address>row (viagit commit --signoff).<package name>: titlefirst line subject for packages.PKG_VERSIONin the Makefile.