From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: ddns@lists.ipfire.org Subject: Re: Add support for godaddy.com Date: Thu, 16 Mar 2023 11:12:59 +0000 Message-ID: <5827AB1C-7BFE-40CC-AFA8-8DCE1059D28A@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6025108899935775585==" List-Id: --===============6025108899935775585== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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 wrote: >=20 > Hi Michael, >=20 > Thanks for all the points, they all make sense, I made this patch looking a= t 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 o= ne 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 men= tion would you like me to go over all the code base and update it to using th= em since they are the standard for quite some time now? I completely understa= nd 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 implement= ing 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.pyt= hon.org/issue46200 Therefore, I am not indenting to use them in any (sub-)project in an around I= PFire. Please use the %-string format to keep it consistent. -Michael >=20 > kind regards > Costas >=20 > On Thu, Mar 16, 2023 at 8:46=E2=80=AFAM Michael Tremer wrote: > Hello Costas, >=20 > I would like to drop a couple of comments about this patch: >=20 > It uses f-strings which we do not use at all in ddns and so I wouldn=E2=80= =99t want to start now. >=20 > The newly added provider class does not make use of the send_request method= (https://git.ipfire.org/?p=3Dddns.git;a=3Dblob;f=3Dsrc/ddns/system.py;h=3D48= c9a8f88fdde2b922cb6548e450c9a1407851fb;hb=3DHEAD#l124) which we use to log th= e request, configure extra settings like an upstream proxy, etc. Just calling= urlopen() is not sufficient. >=20 > The TTL is set to 600. I cannot find anything about this in the upstream do= cumentation, 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. >=20 > This does not support IPv6 at all. The A record is hardcoded although GoDad= dy supports AAAA records, too. >=20 > Then it is not being implemented that a record can be deleted even though t= he API supports it: https://developer.godaddy.com/doc/endpoint/domains#/v1/re= cordDeleteTypeName >=20 > Then the error handling is incorrect as ddns handles all mentioned error co= des 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(). >=20 > I am very grateful for your patch submission but it cannot be merged like t= his as it introduces some major problems. Please submit an improved patch. >=20 > Best, > -Michael >=20 > > On 15 Mar 2023, at 12:19, Stefan Schantl wr= ote: > >=20 > > Hello Costas, > >=20 > > thanks for sending your patch here and once again for sending the same > > patch as a pull request on github. > >=20 > > I really don't know, why your first mail did not appear in my inbox, > > nor got displayed on our patchwork infrastructure. > >=20 > > After a quick code review and some basic testing with invalid data, > > everything seems to work and I finally merged your patchset. > >=20 > > In addition to this I tagged a new version of ddns, which will be part > > of one of the next upcomming Core updates. > >=20 > > Best regards, > >=20 > > -Stefan=20 > >=20 > > _______________________________________________ > > ddns mailing list > > ddns(a)lists.ipfire.org > > https://lists.ipfire.org/mailman/listinfo/ddns >=20 > _______________________________________________ > ddns mailing list > ddns(a)lists.ipfire.org > https://lists.ipfire.org/mailman/listinfo/ddns --===============6025108899935775585==--