Re: Fwd: Add support for godaddy.com

Re: Fwd: Add support for godaddy.com

[Stefan Schantl]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

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 

Re: Add support for godaddy.com

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]

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=48c9a8f88fdde2b922cb6548e450c9a1407851fb;hb=HEAD#l124) 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(a)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(a)lists.ipfire.org
> https://lists.ipfire.org/mailman/listinfo/ddns

Re: Add support for godaddy.com

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 3807 bytes --]

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(a)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(a)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=48c9a8f88fdde2b922cb6548e450c9a1407851fb;hb=HEAD#l124) 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(a)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(a)lists.ipfire.org
> > https://lists.ipfire.org/mailman/listinfo/ddns
> 
> _______________________________________________
> ddns mailing list
> ddns(a)lists.ipfire.org
> https://lists.ipfire.org/mailman/listinfo/ddns

Re: Add support for godaddy.com

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 4911 bytes --]

Hello again,

> On 16 Mar 2023, at 10:14, Costas Tyfoxylos <costas.tyf(a)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(a)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(a)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=48c9a8f88fdde2b922cb6548e450c9a1407851fb;hb=HEAD#l124) 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(a)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(a)lists.ipfire.org
> > https://lists.ipfire.org/mailman/listinfo/ddns
> 
> _______________________________________________
> ddns mailing list
> ddns(a)lists.ipfire.org
> https://lists.ipfire.org/mailman/listinfo/ddns