-
Notifications
You must be signed in to change notification settings - Fork 418
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Domainnameshop provider #1625
Domainnameshop provider #1625
Conversation
Hi there! Thanks for writing this up! The Linode provider has a similar situation with TTLs. You might take a look at how they handle that situation. Tom |
Thank you for the tip on the TTL issue. Weirdly now Not quite sure what the issue is.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far!
Before we merge this I'd like to request that you reorganize the filenames (and move functions if needed). I know this is a big change, but doing it now is better than rewriting it all later. I'd really appreciate it! Thanks! |
I will look into that, do you have any suggestions for how to fix the issue I mentioned with 10:gentxt_TXT:Create_TXT_10? Not quite sure how to fix it, I might be mistaken, but isn't those two the same thing? If so, how can I go about fixing the records given from
|
About gentxt_TXT:Create_TXT_10 : They are the same thing but the pkg/diff isn't smart enough to know that. If you call See how other providers use is:
HTH |
|
Suggestion: Skip that test. Nobody depends on that behavior and I'm considering eliminating that test. |
Would another option potentially be to have a loop over i.e for _, rc := range dc.Records {
if rc.HasFormatIdenticalToTXT() {
rc.SetTargetTXT(strings.Join(rc.TxtStrings, ""))
}
} |
I think that might work. Give it a try! |
That made it pass the tests! |
All checks passed and I think it is restructured like you intended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ready to merge?
Yup, should be ready to go |
Thanks for contributing the new provider! I'm excited to see this new addition! I'd like to have your email address in case Github has a problem. Can you send it to tlimoncelli at stack over flow dot com ? Thanks! |
Closes #1611
Most tests pass, except for the
GeneralACD
tests that change the TTLs as DomainNameShop only supports TTLs that are a multiple of 60 (i.e. TTL % 60 == 0). I am not quite sure what the best solution for handeling those instances. I am open for suggestions.Other than that there are a few features that can be added later. But I can create issues for them once this PR has been resolved.