luci-app-squid: convert to JavaScript #7320
Conversation
|
This looks fine as-is. If you're feeling up to it, could you also add the extra variables found in the squid package please ( as at 23.05 ): validate_squid_section() {
uci_load_validate squid squid "$1" "$2" \
'config_file:string' \
'http_port:port:3128' \
'http_port_options:string' \
'ssldb:string' \
'ssldb_options:string' \
'coredump_dir:string' \
'visible_hostname:string:OpenWrt' \
'pinger_enable:string:off' \
'mime_table:string:/etc/squid/mime.conf'
}
|
Sure thing, should be fairly straightforward. |
|
I've basically got everything working with the new options, but during this I discovered that the pinger_enable option requires Squid to be started with the --enable-icmp parameter, which the principal package doesn't do. This can also be seen in the logs: Should we still expose this option even though it won't do anything? |
|
LuCI should match the principal package. |
|
Is it not what the start script makes possible?
|
A prerequisite of being able to use Also, another interesting thing is that the default compile option then becomes |
|
Ah. Okay. Previously you wrote started. I started to suspect it was a CTO. Technically it's possible to parse the help output of squid to scrape whether it's compiled so, but that's another problem for another day. Skip ping 👌 |
Sounds good. Yeah I see the misunderstanding, I had actually missed it was a compile-time option instead of a parameter, my bad. |
|
If it is a compile-time option, one alternative might be to add a note to the option explanation in LuCI, that it a compile option is required for the functionality to work. |
|
You can take a look at how we did it in (luci-app-)lldpd. |
|
FYI: I'm gonna remove the custom |
|
How's it coming with |
I basically have the entire PR sorted out on my end, just haven't had the time to finalize it. Since the principal package sets up a procd trigger on the uci squid section, there's no easy way for us anyway to handle the reload manually on the LuCI app with restart helpers, so I'm just gonna leave it the way it was before (as in the default |
|
PR should be good to go now. Changes since putting in draft:
|
applications/luci-app-squid/htdocs/luci-static/resources/view/squid.js
Outdated
Show resolved
Hide resolved
|
Once those are done, just squash all the commits together too, please. |
The conversion adds the missing parameters in the UI which is configurable in the principal package. Assumption has been made that the config file is stored in /etc/squid since the principal package limits the sysconfdir to this directory. If that assumption is changed in the future we need to adjust the ACL. Signed-off-by: Daniel Nilsson <dannil+github@protonmail.com>
|
Great. Thanks @dannil |
The conversion adds the missing parameters in the UI which is configurable in the principal package.
Assumption has been made that the config file is stored in /etc/squid since the principal package limits the sysconfdir to this directory. If that assumption is changed in the future we need to adjust the ACL.
Values are updated appropriately which the squid logs also indicates.
Updating the files via advanced settings also works.
As discussed in #7320 (comment), if squid is compiled with --enable-icmp, the option will be enabled, otherwise it'll be disabled with a tool tip explaining why.
Signed-off-by: <my@email.address>row (viagit commit --signoff)<package name>: titlefirst line subject for packagesPKG_VERSIONin the Makefile N/A