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

Use cloudflare-go #1267

Merged
merged 5 commits into from
Sep 30, 2021
Merged

Use cloudflare-go #1267

merged 5 commits into from
Sep 30, 2021

Conversation

tresni
Copy link
Contributor

@tresni tresni commented Sep 29, 2021

Previously this was using a homemade implementation of the Cloudflare API. I really wanted to implement registrar support for Cloudflare, but I didn't want to have to write all the necessary structs by hand. This also means we should automatically gain benefits of any improvements in the cloudflare-go library.

Integration tests look good, but go-staticcheck says that pageRuleConstraint is unused which is a dirty lie. Soon I should have another PR adding support for Cloudflare as a registrar.

@tresni tresni changed the title Use cloudflare-go WIP: Use cloudflare-go Sep 29, 2021
@tresni
Copy link
Contributor Author

tresni commented Sep 29, 2021

Turns out that activating the proxy is not covered by the test suite. When I actually try to run this with a domain using proxied records I get:

#2: ACTIVATE PROXY for new record @ CNAME 1 $TARGET.
FAILURE! cannot modify record if domain or record id are empty

Looking into this now

Forgot to set the ID when we created a new records.  This didn't fail in the integrations tests so I missed it.
@tresni tresni changed the title WIP: Use cloudflare-go Use cloudflare-go Sep 29, 2021
To prevent something like what I did from happening in the future.
@tresni tresni closed this Sep 29, 2021
@tresni tresni reopened this Sep 29, 2021
@tresni
Copy link
Contributor Author

tresni commented Sep 29, 2021

@tlimoncelli I don't know why this isn't doing integration tests for Cloudflare as I have the necessary secrets on my side. tresni#6 should show the results here soon as I couldn't think of a better way to prove that it worked.

@tlimoncelli
Copy link
Contributor

Holy cow! This is quite a PR!

“One of my most productive days was throwing away 1,000 lines of code.”
— Ken Thompson, co-inventor of Unix and C

Ever since the official cloudflare-go module was released I'd been thinking about incorporating it in dnscontrol. Thanks for working through all the changes. This is going to make cloudflare a lot more reliable.

creds: As far as why aren't the integration tests working: Github takes the secrets from the owner's repo. I see that's what it is doing in tresni#6. I also ran the tests from my system and everything is fine.

go-staticcheck: I wouldn't worry about it. Maybe move the struct's definition to be above its first use?

Tom

CC @captncraig because his excellent, clean, code made this rewrite so easy. Look at how often our home-grown structs and funcs were basically the same as what the official module had.

@tresni
Copy link
Contributor Author

tresni commented Sep 30, 2021

@tlimoncelli let me know if there’s anything else you’d like to see.

@tlimoncelli tlimoncelli merged commit d8941a0 into StackExchange:master Sep 30, 2021
@tlimoncelli tlimoncelli mentioned this pull request Nov 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants