Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNSMADEEASY: populate zone cache after creating zone #3333

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

das7pad
Copy link
Contributor

@das7pad das7pad commented Jan 8, 2025

Hi @vojtad!
While reviewing all the ZoneCreator implementations, I noticed that the DNSMADEEASY provider has an incomplete caching implementation for zones. The provider is populating the cache once on first access. Any zones that are created will not be readable in the same life-cycle of dnscontrol. This PR is populating the zone cache after creating a zone. Would you mind giving this a try and let me know how it goes? Thanks!

Note: I've reduced the contents of the zone cache to just the domain ID as that is what each called is using only; and what the API response from creating the zone provides too (the API response from creating the zone is similar to the response from listing, but not quite the same).
Bonus: I've also done some code cleanup from spurious nested pointers. These changes are in it's own commit to ease the review.

Part of #3007

@vojtad
Copy link
Contributor

vojtad commented Jan 12, 2025

Hi, thanks for the contribution!

All proposed changes looks good to me. I am not using DNSMADEEASY provider anymore and I am not able to run the tests right now. Did you run them and did they all pass?

Also, would you be interested in taking over maintaining the provider if you are using it actively?

@das7pad
Copy link
Contributor Author

das7pad commented Jan 12, 2025

Hi, thanks for the contribution!

All proposed changes looks good to me. I am not using DNSMADEEASY provider anymore and I am not able to run the tests right now. Did you run them and did they all pass?

Also, would you be interested in taking over maintaining the provider if you are using it actively?

Thanks for taking a look! No, I did not run the tests. I do not have an account with them for testing and at 225$ per year on their cheapest plan, it would be quite expensive to take on the maintainership.

@tlimoncelli WDYT about putting a call for maintainers into the README? (Side note: Is it worth asking any users of these driver without maintainer whether they are interested in taking on the maintenance? For example by printing a message at run-time.)

@vojtad
Copy link
Contributor

vojtad commented Jan 12, 2025 via email

@vojtad
Copy link
Contributor

vojtad commented Jan 21, 2025

I ran the tests and 3 of them failed. I am not sure they are related to your changes. They might be related to some changes on DNSMADEEASY side or something else. I will try to find time to look into this in following weeks. I will keep you posted.

Failing tests:

        --- FAIL: TestDNSProviders/dnscontroltest.com/22:complex_TXT:a_0-byte_TXT (1.41s)
        --- FAIL: TestDNSProviders/dnscontroltest.com/23:TXT_backslashes:TXT_with_backslashs (4.02s)
        --- FAIL: TestDNSProviders/dnscontroltest.com/49:ALIAS_to_nonfqdn:ALIAS_at_root (1.98s)

Also, production account is not needed to run tests. They all run against sandbox API endpoints at api.sandbox.dnsmadeeasy.com. I can provide my test account or account can be created at https://auth.sandbox.dnsmadeeasy.com/.

The only thing when running DNSMADEEASY tests is timeout needs to be increased to 2h because of hitting their API limits. However, they were willing to increase limits in the past during the provider development. Maybe they would be open to increasing them permanently for the testing account.

If there is no new maintainer I can keep the provider for now. I won't be able to actively work on new features but when pinged I can review PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants