search

leamas / ddupdate

Update DNS Data for Dynamic IP Addresses
MIT License
40 stars 28 forks source link

Feature Add dnshome.de DynDNS-Service #47

Closed m-jung closed 2 years ago

m-jung commented 3 years ago

I've added a service- and address- plugin for the, lesser known, dnshome.de DynDNS-Service. Also i changed the ddupdate-config a little, so that the user can now choose the address-plugin to resolve the IP addresses and check the result immediatly. After that he/she can configure if IPv4, IPv6 or both shall be updated too.

I have tested the plugin against their Web-APIs manually as well as live in action. For the test (and my own purpose) i've merged the changes into /debian and ran ddupdate-config successfully on my Raspberry Pi 4b.

Environment Details: Operating System: Raspbian GNU/Linux 10 (buster) Kernel: Linux 5.4.79-v7l+ Architecture: arm

If you feel anything is wrong, do not hasitate to give me feedback and/or take changes as you like.

leamas commented 2 years ago

Too much water under the bridges since this was submitted, mostly due to me. Sorry for that.

That said, if you could rewrite this so it was only about the new plugin I would happily merge it. However, I don't see the need to fuss with ddupdate-config, and changing it means tough to handle test requirements.

leamas commented 2 years ago

Also: Please split the combined AddressPlugin and ServicePlugin into two separate files. The logic cannot really cope with this living on a single file, it breaks all sorts of expectations.

leamas commented 2 years ago

I have actually merged the plugin into devel in f0de63a. This is done as two separate files to fit the overall organization. If you have time and motivation: please review the plugins!

I'm closing this, still open to smaller PRs. But when it comes to config.py, please file an issue with the problem you want to solve before making PRs so we can discuss thinsg before submitting code.

m-jung commented 2 years ago

Thank you for merging the plugin.

Regardings the review: I'm not sure about the removed code 200 check, since i don't know how urllib.request.urlopen() handles states other than 200. I've added the check because the endpoint could return 400 alongside a readable body which could lead to an unexpected behavior later on. Other than that, your changes make sense to me.

Regarding config.py: When using ddupdate i found that the configuration is not as fault tolerant as one might wish. So I've adjusted it a bit in that regard. However, I understand your point about smaller dedicated commits and am trying to accommodate that.

leamas commented 2 years ago

However, I understand your point about smaller dedicated commits and am trying to accommodate that.

Please feel welcome doing so. Note that there are quite big changes to ddupdate-config in current devel...