Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add myaddr dynamic dns provider #885

Merged
merged 10 commits into from
Dec 24, 2024
Merged

Add myaddr dynamic dns provider #885

merged 10 commits into from
Dec 24, 2024

Conversation

brianshea2
Copy link
Contributor

@brianshea2 brianshea2 commented Dec 22, 2024

Adds new dynamic dns provider: https://myaddr.tools
Partially fixes #728 (additional PR for public IP provider to come)
Cheers!

Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 🎖️ 💯 !

A few comments of things to change here and there, but before jumping in this: would you mind creating another separate pull request for the public ip feature, since it should be a separate commit/PR really. Thank you 🙏 👍

cmd/ddns-updater/main.go Outdated Show resolved Hide resolved
internal/params/json.go Outdated Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/myaddr/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/myaddr/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/myaddr/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/myaddr/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/myaddr/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/myaddr/provider.go Outdated Show resolved Hide resolved
@brianshea2
Copy link
Contributor Author

Thank you for the quick review :) I believe I've addressed all your comments and made the code more consistent with the project.

I've reverted the public IP provider addition for now and will open a separate PR for that.

I understand your want for providing the "domain" to be consistent with other providers, and have removed the initialization code which fetched the domain corresponding to the provided "key". My concern now is there's nothing explicitly linking the provided "key" and "domain". I believe I've described the "domain" adequately in docs/myaddr.md to this point. But to reiterate, the provided "key" is the only identifier sent to the provider api, and is used to update all [*.]name-corresponding-to-key.myaddr.{tools,dev,io} in one fell swoop, which may or may not agree with the separately-user-provided "domain", now used only for checking if an update is required. What are your thoughts here? It's possible to perform an additional api request during each update to fetch the domain corresponding to "key" and compare to the provided "domain" if wanted.

@brianshea2 brianshea2 changed the title Add myaddr dynamic dns provider and addrtools public ip provider Add myaddr dynamic dns provider Dec 23, 2024
@brianshea2 brianshea2 requested a review from qdm12 December 23, 2024 03:12
@qdm12
Copy link
Owner

qdm12 commented Dec 24, 2024

I believe I've addressed all your comments and made the code more consistent with the project.

Awesome 💯

I've reverted the public IP provider addition for now and will open a separate PR for that.

Perfect 🥇

It's possible to perform an additional api request during each update to fetch the domain corresponding to "key" and compare to the provided "domain" if wanted.

Eh it's a bit on the user if he/she sets a key not matching the domain. We can do foolproof checks as much as possible, but this seems like a bit too much at the cost of over-complicating it 😄 The API will probably just say the key is wrong anyway.

I did however add a little check to prevent multiple domains to be defined in a myaddr settings block since one key is for one domain (and its subdomains), so there would be no point handling it as different entries/updates in parallel, except getting banned for sending too many update requests 😄

Merging this !!

@qdm12 qdm12 merged commit 03154c3 into qdm12:master Dec 24, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

myaddr.tools
2 participants