Hello Costas,
Thank you for your response, but please make sure to keep the list copied.
On 16 Mar 2023, at 08:53, Costas Tyfoxylos costas.tyf@gmail.com wrote:
Hi Michael,
Thanks for all the points, they all make sense, I made this patch looking at other implementations of providers so I guess some of the topics you raise are there for more of them. I will try to make some time to get an improved one in, but that will probably not be on a very short timeframe. As far as the f-strings are concerned, if I get around to fix all the other issues you mention would you like me to go over all the code base and update it to using them since they are the standard for quite some time now? I completely understand how you would not want to have an inconsistent approach to this but I can update everything if that helps more. Also if I have any issue with implementing any of the points is it ok if I reach out?
I am not keen on f-strings as they might not be safe to use: https://bugs.python.org/issue46200
Therefore, I am not indenting to use them in any (sub-)project in an around IPFire.
Please use the %-string format to keep it consistent.
-Michael
kind regards Costas
On Thu, Mar 16, 2023 at 8:46 AM Michael Tremer michael.tremer@ipfire.org wrote: 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
ddns mailing list ddns@lists.ipfire.org https://lists.ipfire.org/mailman/listinfo/ddns