ddns-scripts: fix netcup.com for multiple runs#29098
ddns-scripts: fix netcup.com for multiple runs#29098flubshi wants to merge 1 commit intoopenwrt:masterfrom
Conversation
The netcup ddns update script fails on a second run, because a readonly variable is assigned again. This is fixed by removing the readonly. Multiple hard-failures were replaced by soft-failure, to allow ddns to retry after specified retry timeout. Hard failure should only happen on essential failures like missing dependencies or credentials. Loading JSON from file is improved by using the function intended for this purpose instead of using cat to pass file content. Signed-off-by: Tim Flubshi <flubshi@gmail.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes repeatability and retry behavior of the netcup.com DDNS updater by removing a readonly reassignment error, improving JSON loading, and converting several hard-fail exits into retryable failures.
Changes:
- Make
__NETCUP_ENDPOINTre-source-safe by removingreadonlyassignment. - Use
json_load_filefor parsing the provider response stored in$DATFILE. - Convert several hard failures into
return 1paths to allow ddns framework retries.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| net/ddns-scripts/files/usr/lib/ddns/update_netcup_com.sh | Makes the netcup updater safe to source multiple times; improves JSON loading and makes several failures retryable. |
| net/ddns-scripts/Makefile | Bumps package release to publish the behavior fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # --------------------------------------------------------------------------- | ||
|
|
||
| readonly __NETCUP_ENDPOINT="https://ccp.netcup.net/run/webservice/servers/endpoint.php?JSON" | ||
| __NETCUP_ENDPOINT="https://ccp.netcup.net/run/webservice/servers/endpoint.php?JSON" |
There was a problem hiding this comment.
Dropping readonly fixes re-sourcing, but it also makes this constant mutable for the rest of the shell lifetime. Consider keeping it effectively constant without triggering reassignment errors by assigning only if unset (or once) and then using readonly __NETCUP_ENDPOINT without an = assignment. This preserves the original intent (constant endpoint) while remaining safe across multiple sourcing.
| __NETCUP_ENDPOINT="https://ccp.netcup.net/run/webservice/servers/endpoint.php?JSON" | |
| : "${__NETCUP_ENDPOINT:=https://ccp.netcup.net/run/webservice/servers/endpoint.php?JSON}" | |
| readonly __NETCUP_ENDPOINT |
| netcup_post || write_log 14 "netcup DDNS: HTTP request failed during login" | ||
| netcup_check_response "login" | ||
| if ! netcup_post; then | ||
| write_log 3 "netcup DDNS: HTTP request failed during login" |
There was a problem hiding this comment.
On early-return paths after building a JSON request, the json.sh internal state is left as-is. Add json_cleanup before return 1 in these netcup_post failure branches (login/infoDnsRecords/updateDnsRecords) to avoid leaking JSON state into subsequent retries within the same process.
| write_log 3 "netcup DDNS: HTTP request failed during login" | |
| write_log 3 "netcup DDNS: HTTP request failed during login" | |
| json_cleanup |
| if ! netcup_post; then | ||
| write_log 3 "netcup DDNS: HTTP request failed during login" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
When netcup_post fails, the log message does not include the underlying transport/tool error even though stderr is redirected to $ERRFILE in netcup_post. Consider appending the captured error output (or a short excerpt) to the log message to make retry failures diagnosable without increasing log level globally.
📦 Package Details
Maintainer: @feckert
(You can find this by checking the history of the package
Makefile.)Description:
This PR fixes an issue where the netcup DDNS update script fails on subsequent runs due to reassigning a
readonlyvariable. The variable has been changed to a regular assignment to ensure the script can be sourced multiple times without errors.Additionally, error handling has been improved:
write_log 14) have been replaced with soft failures (return 1) to allow the ddns framework to retry updates according to the configured retry policy.cat-based loading with json_load_file, which is the intended method for handling JSON input in this context.🧪 Run Testing Details
✅ Formalities
If your PR contains a patch:
git am(e.g., subject line, commit description, etc.)
We must try to upstream patches to reduce maintenance burden.