Migrate to libdns 1.0 API#2
Conversation
|
Not a cloudns user, but a quick scan of the libdns-related code looks good! Nice work. And thank you! 🙏 |
@worr I am and appreciate you picking this up! |
worr
force-pushed
the
feature/libdns-1.0
branch
2 times, most recently
from
c994582 to
f404c1d
Compare
|
I've rebased on the new changes. I have some concerns with the new changes however. I already opened up #3 about the logging. One change that I think is worth discussing in context of this PR is the propagation checks. Now, the tests do not run well for me unless I pump up the timeout to many minutes, since at least ClouDNS changes don't hit my region as quickly. I'm not sure that under libdns 1.0, this is an acceptable amount of time to wait for these calls to complete, since the docs say the following:
This is in the context of retrying and recovery, but having calls to |
|
FWIW, I don't think libdns expects you to block until propagation completes. (You can do whatever you need to do in your tests, but the blocking isn't necessary in functions like SetRecords, etc.) Presumably, Caddy is the main consumer of libdns packages, and it actually does the waiting on propagation for you when it is necessary. |
|
Cool, in that case I've removed the propagation logic as well since it'd be redundant. |
This migrates to the new libdns 1.0 API, which has some major changes, such as excluding provider-specific details from the information passed between providers and consumers. This diff also addresses some bugs present as part of the old implementation that were necessary to meet the new 1.0 API featureset, including the following: * `SetRecords` would not properly handle RRSets, only modifying the exact record specified * Many record types, like `CAA`, `MX`, `NS` and `SRV` were not supported properly, since ClouDNS requires more parameters for these fields. * If authenticating with a `SubAuthID`, you cannot provide an `AuthID` in the API call
Propagation logic can explode the runtime of these functions, and the main consumer of libdns, caddy, already handles this out of the library. No need for it in an individual provider.
anxuanzi
left a comment
anxuanzi
left a comment
There was a problem hiding this comment.
Thank you guys so much for working on this! Sorry for the late response. I was working on the update but got distracted by other projects. Thanks again for the PR!
|
Wonderful! Thank you @anxuanzi |
4 participants
This migrates to the new libdns 1.0 API, which has some major changes, such as
excluding provider-specific details from the information passed between
providers and consumers.
This diff also addresses some bugs present as part of the old implementation
that were necessary to meet the new 1.0 API featureset, including the following:
SetRecordswould not properly handle RRSets, only modifying the exact recordspecified
CAA,MX,NSandSRVwere not supportedproperly, since ClouDNS requires more parameters for these fields.
SubAuthID, you cannot provide anAuthIDin theAPI call
I'm making this a draft PR for now since I want to add some more tests and also rebase in the new retry work that was added. An early review would definitely be appreciated :)