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 07:46:30 +0000 Message-ID: <1469F7D4-7DCF-4FCD-8EC5-2427B1A1B6B9@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1599676324464181911==" List-Id: --===============1599676324464181911== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable 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=E2=80=99= t want to start now. 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=3D48c9= a8f88fdde2b922cb6548e450c9a1407851fb;hb=3DHEAD#l124) which we use to log the = request, configure extra settings like an upstream proxy, etc. Just calling u= rlopen() is not sufficient. The TTL is set to 600. I cannot find anything about this in the upstream docu= mentation, but that seems to be an eternity compared to the other providers w= here the TTL is at maximum 60s. If there is a way to choose, then we should u= se 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/reco= rdDeleteTypeName Then the error handling is incorrect as ddns handles all mentioned error code= s 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 thi= s as it introduces some major problems. Please submit an improved patch. Best, -Michael > On 15 Mar 2023, at 12:19, Stefan Schantl wrot= e: >=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 --===============1599676324464181911==--