Hello again,
On 16 Mar 2023, at 10:14, Costas Tyfoxylos costas.tyf@gmail.com wrote:
Hi Michael,
Just briefly had a look at the code to refamiliarise myself with it and noticed that the code base is on the very old % interpolation so converting to fstrings either makes a lot of sense or none at all depending on how you look at it. :) I have also had a look at the urlopen comment you made and I cannot use the send_request method as you suggest, at least not without patching that as the headers set by it define a "Basic" authorization header with the username and password but godaddy expects an "sso-key" header instead. So I either need to bypass send_request and use url_open or patch send_request to be able to amend the authorization header with a custom entry. What do you think?
You *will* have to use send_request, because it implements so much. For example using an upstream proxy. If you omit that, your provider won’t function for those users and that would just be weird and hard to debug.
Then, send_request does not require you to use HTTP Basic authentication. It will do it for you if you pass the appropriate parameters, but you don’t have to do that.
If you need to pass any additional headers, there is nothing wrong with extending the function and having it take a dictionary with any additional headers. That should be a single commit, please.
-Michael
kind regards Costas
On Thu, Mar 16, 2023 at 9:53 AM 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?
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
-
Michael Tremer