Closed tlimoncelli closed 1 year ago
tlimoncelli
commented
1 year ago Note to @vatsalyagoel @tresni @Deraen @riyadhalnur @KaiSchwarz-cnic @jpbede: I've checked your providers off because your provider passed the automated tests. However I would appreciate you testing preview in your production environment to be sure there aren't any problems that aren't covered by the integration tests.
riku22
commented
1 year ago Hello.
When I run git checkout v3.30.0test after git pull, I get the error error: pathspec 'v3.30.0test' did not match any file(s) known to git.
I don't think there is such a tag.
Am i doing something wrong?
tlimoncelli
commented
1 year ago @riku22 Ah, I typed the wrong git command. Please try again. Thanks!
systemcrash
commented
1 year ago @tlimoncelli Favour: could you ask testers also to verify whether the new LOC record works for them (update the .Can() and submit Pr if so) 😄
riku22
commented
1 year ago LuaDNS integration tests passed.
systemcrash
commented
1 year ago ✅ Loopia - cd integrationtests; go test -v -verbose -provider LOOPIA -timeout 3600s
One test 🟡
✅ Loopia - cd integrationtests; go test -v -verbose -provider LOOPIA -diff2 -timeout 3600s
One test 🟡
Pretty sure this case was weird before your work.
✅ Loopia - preview+push
$ cd (to where I keep my dnsconfig.js)
$ dnscontrol preview
$ dnscontrol --diff2 preview
$ dnscontrol --diff2 push@tlimoncelli Favour: could you ask testers also to verify whether the new LOC record works for them (update the .Can() and submit Pr if so) 😄
Hey folks! This might be a good chance to see if the new LOC record is supported!
xddxdd
commented
1 year ago Passes all tests for Gcore provider. Tested with diff2 on/off, and with integration test and my own config.
kallsyms
commented
1 year ago Integration tests pass for oracle (both with and without -diff2). Looks like LOC is broken server-side though, so can't enable it :(
tlimoncelli
commented
1 year ago ✅ Loopia -
cd integrationtests; go test -v -verbose -provider LOOPIA -timeout 3600sOne test 🟡
Both of those tests look like the same issue. It is trying to change "foo" to "foo.example.com.". My guess is somewhere there should be a loop that turns all the targets for MX, CNAME, NS (and possibly others) from shortnames to FQDNs.
Let me know how I can help!
rblenkinsopp
commented
1 year ago Integration tests for HEDNS all pass (diff & diff2) except Single_LOC_record which is resolved by https://github.com/StackExchange/dnscontrol/pull/2216.
jpbede
commented
1 year ago However I would appreciate you testing
previewin your production environment to be sure there aren't any problems that aren't covered by the integration tests.
PowerDNS works in production.
blackshadev
commented
1 year ago Works in production. No new failures between master and the new branch.
I did find the integration tests failing which seems like a change on transIP there behalve and unrelated to your changes.
Do note: the new tag v3.30.0test is not available. I used the branch of the PR.
svernick
commented
1 year ago % git checkout v3.30.0test
error: pathspec 'v3.30.0test' did not match any file(s) known to git
Should I checkout tlim_corrector ?
From: Vincent Hagen @.> Reply-To: StackExchange/dnscontrol @.> Date: Friday, March 24, 2023 at 5:20 PM To: StackExchange/dnscontrol @.> Cc: "Vernick, Steve" @.>, Mention @.***> Subject: Re: [StackExchange/dnscontrol] Request for testing: GetZoneRecords() refactor (Issue #2214)
Works in production. No new failures between master and the new branch. Do note: the new tag v3.30.0test is not available. I used the branch of the PR.
I did find the integration tests failing which seems like a change on transIP there behalve and unrelated to your changes.
— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/StackExchange/dnscontrol/issues/2214*issuecomment-1483417573__;Iw!!GjvTz_vk!VNPcT0HfCwQWv9LL4V7nLAahWw751_a-MR_nnRdufjr15SelmXllrFn545jCvRxQpSKpBL2gspymqKpBFywt-T8$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AZ7IINFLM47AAM4NEFGLK4DW5YFW3ANCNFSM6AAAAAAWENODGA__;!!GjvTz_vk!VNPcT0HfCwQWv9LL4V7nLAahWw751_a-MR_nnRdufjr15SelmXllrFn545jCvRxQpSKpBL2gspymqKpBjxs3SvY$. You are receiving this because you were mentioned.Message ID: @.***>
willpower232
commented
1 year ago Alas it is not looking good for namecheap
go test -v -verbose -provider NAMECHEAP
go test -v -verbose -provider NAMECHEAP --diff2
I couldn't see a way of adding a LOC record in the namecheap UI so I presume that is not (yet) possible and did not try enabling it in the provider.
I haven't got enough time today to debug but I did run the tests against master and confirm they pass with the previously noted exception of 02:MX:Change_MX_p
Is it fairly obvious to see what the problem is when I do have some time?
tlimoncelli
commented
1 year ago Works in production. No new failures between master and the new branch.
Thanks for letting me know!
Do note: the new tag v3.30.0test is not available. I used the branch of the PR.
Good. I've updated the doc to recommend doing that instead. For some reason some people are seeing the tag and others aren't.
tlimoncelli
commented
1 year ago svernick Yes, please us the branch. (I've updated the OP since the tag isn't working for most people)
tlimoncelli
commented
1 year ago willpower232 I think 5fec2905d9c53cdc9c6890fda618ca60ec6d26b4 should fix it. Try the tlim_corrector branch and let me know.
It that doesn't fix it...
Look at line 140 to see some code that I didn't include because it seemed redundant. Maybe it was needed after all? My guess is that record.PopulateFromString() isn't working right for certain DNS record types. Put it in a case statement and call PopulateFromString() as the default. See models/t_parse.go for an example.
imlonghao
commented
1 year ago PORKBUN integration tests passed.
Deraen
commented
1 year ago I tested the Digitalocean provider with a production-like config, and it worked. I'm no longer using Digitalocean, but I could run my Route53 config after commenting out a few R53-specific records.
willpower232
commented
1 year ago Good news that the changes seem to have corrected most of the situation, there are now consistent failures for both with and without diff2
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/04:MX:Create_MX
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/07:manyTypesAtOnce:CreateManyTypesAtLabel
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/14:MX:Record_pointing_to_@
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/17:Case_Sensitivity:Create_CAPS
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/19:testByRecordSet:initialthe code you mentioned seems to be missing a dc and actual so if you think that is what is needed, I'll take a closer look when time allows
masterzen
commented
1 year ago LOC 🚫 doesn't work as is for OVH (in diff2 and regular modes, I'll have a look in the week) my own code works fine in diff2 mode.
tlimoncelli
commented
1 year ago the code you mentioned seems to be missing a
dcandactualso if you think that is what is needed, I'll take a closer look when time allows
I fixed that a few minutes later (oops!). Take a look at the latest tlim_corrector branch and let me know if that worked.
willpower232
commented
1 year ago I was referring to these lines missing dc and actual
the latest commit of the branch doesn't seem to change anything I'm afraid
tlimoncelli
commented
1 year ago @willpower232
I was referring to these lines missing
dcandactual
Does the current tlim_corrector branch compile and successfully run the integration tests? If so, then that commented out code can be deleted.
the latest commit of the branch doesn't seem to change anything I'm afraid
Make sure the branch you are using has record.SetLabel(dnsHost.Name, origin) at line 300. That is the line I added which, I hope, will fix the integration tests.
willpower232
commented
1 year ago I'm currently at a58fb7721419bb13cd7c952bb35903fff6feb19a as of yesterday, the setLabel seems to have fixed most of the tests but these ones still fail
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/04:MX:Create_MX
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/07:manyTypesAtOnce:CreateManyTypesAtLabel
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/14:MX:Record_pointing_to_@
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/17:Case_Sensitivity:Create_CAPS
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/19:testByRecordSet:initialgithub is having a bad time right now though so if you have pushed anything since then, it might not have actually pushed :sweat_smile:
tlimoncelli
commented
1 year ago I'm currently at a58fb77 as of yesterday, the setLabel seems to have fixed most of the tests but these ones still fail
This is the 2nd provider to have this issue. I think it might be on the dnscontrol end. Let me poke around.
tlimoncelli
commented
1 year ago I'm currently at a58fb77 as of yesterday, the setLabel seems to have fixed most of the tests but these ones still fail
This is the 2nd provider to have this issue. I think it might be on the dnscontrol end. Let me poke around.
I spoke too soon. I don't think that's the issue.
Could you try these tests on master and with/without diff2?
willpower232
commented
1 year ago alas there is only one fail on the master branch in both with and without diff2, the 04:Change_MX_p which has been failing since before I started running the tests.
tlimoncelli
commented
1 year ago @willpower232 Let's move this discussion to a separate issue. I've created https://github.com/StackExchange/dnscontrol/issues/2238
hnrgrgr
commented
1 year ago Some fixes are needed for AXFRDDNS to work. See https://github.com/StackExchange/dnscontrol/pull/2239
The MR also includes a fix for #1913 and a fix for a unrelated error discovered when testing the experimental branch.
tlimoncelli
commented
1 year ago I did some major surgery to the code of these 3 providers. Please run the tests asap as I don't want to break anything.
This provider had major changes but since I have a Cloudflare account, I was able to test them myself. However I encourage you to do extra testing yourself:
I would appreciate it if people could complete these tests by April 14 or sooner. Thanks!
D3luxee
commented
1 year ago I started to run the first tests but as usual it takes me a lot of time because of the hourly and daily rate limits of desec. Last time i had to spread the tests over three days.. and if you forgot on one day to run the tests.. it takes even longer. A bit annoying :D
tlimoncelli
commented
1 year ago I started to run the first tests but as usual it takes me a lot of time because of the hourly and daily rate limits of desec. Last time i had to spread the tests over three days.. and if you forgot on one day to run the tests.. it takes even longer. A bit annoying :D
Wow! That sounds annoying! I have a similar situation with the CSCGLOBAL provider (12 hours).
Here are some things I've done to work around this:
pager101, pager601 , and pager1201 integration tests. They're only needed for providers that send or receive data in batches. Most don't.-start 0 -end 19 the next is -start 20 -end 39, and so on. not(). You can always re-activate those tests if you suspect a regression.Hope those help!
Tom
juliusrickert
commented
1 year ago imlonghao
commented
1 year ago ClouDNS LGTM.
one test failed but also failed in master branch, tracked in #2244
Output: legacy.txt diff2.txt
(I'm not ClouDNS maintainer, the maintainer seems busy so I think I can test for him)
tlimoncelli
commented
1 year ago @imlonghao Thanks for the assist!
onlyhavecans
commented
1 year ago All tests for DNSimple work, including doing production previews and push with the -diff2 flag
costasd
commented
1 year ago Hi folks, integration & production (preview/push) tests for NS1 were successful in both legacy and diff2 modes.
KaiSchwarz-cnic
commented
1 year ago @tlimoncelli thanks for testing us in OT&E. We addressed a little refactoring PR independent of this request for testing. We tested on our end in OT&E as well -> working. Running the integration tests on production isn't useful imho - a domain name is required for doing that (no problem), but the DNS Functionality is not coming with differences for the production environment compared to the OT&E environment. I don't see a benefit in that regard.
tlimoncelli
commented
1 year ago @tlimoncelli thanks for testing us in OT&E. We addressed a little refactoring PR independent of this request for testing. We tested on our end in OT&E as well -> working. Running the integration tests on production isn't useful imho - a domain name is required for doing that (no problem), but the DNS Functionality is not coming with differences for the production environment compared to the OT&E environment. I don't see a benefit in that regard.
Sure, I've always assumed that testing in OT&E is sufficient. My request to test in production was a request to test any dnsconfig.js configurations you might be using.
Tom
das7pad
commented
1 year ago The integration tests passed for HETZNER with both diff modes. I'll send you a PR with some cleanup later this weekend.
D3luxee
commented
1 year ago The integration tests for deSec passed, good to go from my side.
tlimoncelli
commented
1 year ago Hey folks! Friendly remind to those that haven't spoken up yet:
I know that Github often doesn't deliver email reliably. I hope these messages are getting through.
The changes made to dnsmadeeasy are particularly big, so if anyone knows @vojtad or has an account there and can run the tests, I'd appreciate it.
vojtad
commented
1 year ago Hi! We've migrated our DNS from dnsmadeeasy to cloudflare but I will try to do my best to keep the provider up to date.
All tests are passing for dnsmadeeasy. However, I am not able to test it in production environment anymore.
svernick
commented
1 year ago akamaiedgedns tests passed.
go test -v -verbose -timeout 15m -provider AKAMAIEDGEDNS PASS ok github.com/StackExchange/dnscontrol/v3/integrationTest 290.844s
go test -v -verbose -timeout 15m -provider AKAMAIEDGEDNS --diff2 PASS ok github.com/StackExchange/dnscontrol/v3/integrationTest 277.566s
svernick
commented
1 year ago verify whether the new LOC record works for them (update the .Can() and submit Pr if so)
Should I make the change to the 'tlim_corrector' branch? That is, submit a PR to pull my change into 'tlim_corrector'?
tlimoncelli
commented
1 year ago Should I make the change to the 'tlim_corrector' branch? That is, submit a PR to pull my change into 'tlim_corrector'?
Base the PR against master. I'll merge it up to tlim_corrector afterwords. Thanks for checking!
tlimoncelli
commented
1 year ago This is going to be merged tomorrow or Saturday (May 14 or 15th). Last call for testing!
Hi maintainers!
I need your help! Please test this pre-release. I moved a lot of code around. It shouldn't have broken anything but I'd like your testing to be sure. I would appreciate it if your testing could be done by April 13, 2023.
This is a big PR (https://github.com/StackExchange/dnscontrol/pull/2120) and needs extra testing by the provider maintainers and anyone else interested.
Speaking of consistent names, in the future anything that DNSControl corrects will require two functions: If it manages FOO, the functions will be GetFOO() and GetFOOCorrections(). In the future I'll do some global renames related to this.
If you'd like to see the exact change, the PR is https://github.com/StackExchange/dnscontrol/pull/2120
Please test this release:
NOTE: Previously this suggested using the "v3.30.0test" tag. I changed it to the "tlim_corrector" branch because the tag seems to be not updating properly.
Run the integration test (replace NAMEDOTCOM with your provider)
Test it on your production configuration:
If all of that works, please consider making real changes using
--diff2 pushProject status
I would appreciate it if people could complete these tests by April 14 (3+ weeks). Thanks!