lpac: don't overwrite env variables #25258
Conversation
Don't override env variables which are already set when execuring the wrapper script. This makes it possible to override the options set by UCI in the calling programm. Signed-off-by: David Bauer <mail@david-bauer.net>
| HTTP_BACKEND="$(uci_get lpac global http_backend curl)" | ||
| HTTP_DEBUG="$(uci_get lpac global http_debug 0)" |
There was a problem hiding this comment.
- Extra variables
- Default values are unneeded and shouldn't be used in the script here (because it's a code)
There was a problem hiding this comment.
Why are default values not needed?
I don't go with "extra variables, they do not propagate outside the script.
There was a problem hiding this comment.
Separating code and data is a good practice.
Additionally, the default values are just not needed here.
Please pay attention to the alternative implementation which doesn't use the defaults in code.
There was a problem hiding this comment.
I don't go with "extra variables, they do not propagate outside the script.
Having a mix of the real Lpac's env vars and your extra (unneeded) ones makes the script harder to read.
| function export_if_not_in_env { | ||
| eval "value=\${$1}" | ||
| if [ -z "$value" ]; then | ||
| export "$1=$2" | ||
| fi | ||
| } |
There was a problem hiding this comment.
IMO, the function is overkill, see my approach which goes without:
https://github.com/openwrt/packages/pull/25046/files
There was a problem hiding this comment.
It provides a clean way of achieving the common task of only overwriting set values from the outside script.
There was a problem hiding this comment.
I wouldn't name it "a clean way", sorry.
| export_if_not_in_env LPAC_HTTP "$HTTP_BACKEND" | ||
| if [ "$HTTP_DEBUG" -eq 1 ]; then | ||
| export LIBEUICC_DEBUG_HTTP="1" | ||
| export_if_not_in_env LIBEUICC_DEBUG_HTTP "1" |
There was a problem hiding this comment.
The problem with your handling of the debug options, as I see it, is that you have to interpret their values, which our scripts shouldn't do anyway.
Not addressed.
See the alternative approach above, again.
There was a problem hiding this comment.
So you mean to not set the debug env variables if they are not enabled? This would suffice if we remove UCI handling from them, which i am fine with.
There was a problem hiding this comment.
Right now, the UCI handling is in place, so we could (and should) rely on that instead of trying to be smart and interfere with lpac's envs interpretation in this script.
Maintainer: me
Compile tested: (put here arch, model, OpenWrt version)
Run tested: (put here arch, model, OpenWrt version, tests done)
Description:
Don't override env variables which are already set when execuring the wrapper script. This makes it possible to override the options set by UCI in the calling programm.