Hello Costas,
I would like to drop a couple of comments about this patch:
It uses f-strings which we do not use at all in ddns and so I wouldn’t want to start now.
The newly added provider class does not make use of the send_request method (https://git.ipfire.org/?p=ddns.git;a=blob;f=src/ddns/system.py;h=48c9a8f88fd...) which we use to log the request, configure extra settings like an upstream proxy, etc. Just calling urlopen() is not sufficient.
The TTL is set to 600. I cannot find anything about this in the upstream documentation, but that seems to be an eternity compared to the other providers where the TTL is at maximum 60s. If there is a way to choose, then we should use a lower value here.
This does not support IPv6 at all. The A record is hardcoded although GoDaddy supports AAAA records, too.
Then it is not being implemented that a record can be deleted even though the API supports it: https://developer.godaddy.com/doc/endpoint/domains#/v1/recordDeleteTypeName
Then the error handling is incorrect as ddns handles all mentioned error codes automatically. It is not necessary to perform manual stuff here. So this is just dead code that would never be reached when using send_request().
I am very grateful for your patch submission but it cannot be merged like this as it introduces some major problems. Please submit an improved patch.
Best, -Michael
On 15 Mar 2023, at 12:19, Stefan Schantl stefan.schantl@ipfire.org wrote:
Hello Costas,
thanks for sending your patch here and once again for sending the same patch as a pull request on github.
I really don't know, why your first mail did not appear in my inbox, nor got displayed on our patchwork infrastructure.
After a quick code review and some basic testing with invalid data, everything seems to work and I finally merged your patchset.
In addition to this I tagged a new version of ddns, which will be part of one of the next upcomming Core updates.
Best regards,
-Stefan
ddns mailing list ddns@lists.ipfire.org https://lists.ipfire.org/mailman/listinfo/ddns