Skip to content

Conversation

@LogExE
Copy link

@LogExE LogExE commented Dec 17, 2025

📦 Package Details

Maintainer: @feckert (?)

Description:

This PR adds support for changing DNS records through beget.com API.


✅ Formalities

  • I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

@BKPepe BKPepe requested a review from Copilot December 17, 2025 14:24
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@LogExE LogExE force-pushed the feature_beget_ddns branch from 0660404 to 6911fb5 Compare December 17, 2025 15:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +7 to +10
local __CHANGE_ENDPOINT_API="https://api.beget.com/api/dns/changeRecords"

local __RRTYPE
local __STATUS
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable __CHANGE_ENDPOINT_API is declared as local at the script's top level, which is outside any function. In shell scripts, local is only meaningful inside functions. This should be declared without the local keyword, similar to how other DDNS update scripts handle their variables at the top level.

Suggested change
local __CHANGE_ENDPOINT_API="https://api.beget.com/api/dns/changeRecords"
local __RRTYPE
local __STATUS
__CHANGE_ENDPOINT_API="https://api.beget.com/api/dns/changeRecords"
__RRTYPE
__STATUS

Copilot uses AI. Check for mistakes.
Comment on lines +7 to +10
local __CHANGE_ENDPOINT_API="https://api.beget.com/api/dns/changeRecords"

local __RRTYPE
local __STATUS
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variables __RRTYPE and __STATUS are declared as local at the script's top level, which is outside any function. In shell scripts, local is only meaningful inside functions. These should be declared without the local keyword, similar to how other DDNS update scripts handle their variables at the top level.

Suggested change
local __CHANGE_ENDPOINT_API="https://api.beget.com/api/dns/changeRecords"
local __RRTYPE
local __STATUS
__CHANGE_ENDPOINT_API="https://api.beget.com/api/dns/changeRecords"
__RRTYPE=
__STATUS=

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +42
-w "%{http_code}\n" \
-o $DATFILE 2>$ERRFILE)

write_log 7 "Beget API curl response: $(cat $DATFILE)"

Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script does not check the curl exit code or HTTP status code returned from the API. Unlike other DDNS update scripts (e.g., update_gandi_net.sh, update_digitalocean_com_v2.sh), there's no validation that the API call succeeded. The script should check if curl failed and validate that the HTTP status code indicates success before returning 0. Without this validation, DNS update failures may go undetected.

Suggested change
-w "%{http_code}\n" \
-o $DATFILE 2>$ERRFILE)
write_log 7 "Beget API curl response: $(cat $DATFILE)"
-w "%{http_code}" \
-o "$DATFILE" 2>"$ERRFILE")
__CURL_RC=$?
write_log 7 "Beget API curl response: $(cat $DATFILE)"
if [ "$__CURL_RC" -ne 0 ]; then
write_log 3 "Beget API curl request failed with exit code $__CURL_RC"
return 1
fi
case "$__STATUS" in
2??)
# HTTP 2xx, consider this a success
;;
*)
write_log 3 "Beget API returned HTTP error code $__STATUS"
return 1
;;
esac

Copilot uses AI. Check for mistakes.
rm $(1)/usr/share/ddns/default/route53-v1.json
rm $(1)/usr/share/ddns/default/cnkuai.cn.json
rm $(1)/usr/share/ddns/default/gandi.net.json
rm $(1)/usr/share/ddns/default/beget.com.json
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line contains inconsistent indentation - it uses spaces instead of tabs. The rest of the Makefile uses tabs for indentation. This line should use a tab character at the beginning to match the surrounding code style.

Suggested change
rm $(1)/usr/share/ddns/default/beget.com.json
rm $(1)/usr/share/ddns/default/beget.com.json

Copilot uses AI. Check for mistakes.
--data-urlencode "input_data=$json_payload" \
-w "%{http_code}\n" \
-o $DATFILE 2>$ERRFILE)

Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is trailing whitespace at the end of this line. This should be removed to maintain code cleanliness and consistency with the rest of the codebase.

Suggested change

Copilot uses AI. Check for mistakes.
json_add_object "records"
json_add_array "$__RRTYPE"
json_add_object ""
json_add_int "priority" 10
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The priority field is hardcoded to 10. According to DNS standards, priority is typically only relevant for MX (mail exchange) and SRV (service) records, not for A or AAAA records which are being set here. This field may be unnecessary or could cause API errors. Verify if the Beget API requires or accepts a priority field for A/AAAA records, and if not, this field should be removed.

Suggested change
json_add_int "priority" 10

Copilot uses AI. Check for mistakes.
@GeorgeSapkin
Copy link
Member

On top of all the other suggestions, you need to add a commit message describing the changes and use your real name both as author and sign-off.

@GeorgeSapkin GeorgeSapkin changed the title Feature: Beget DDNS API provider ddns-scripts: add beget.com api support Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants