Skip to content

Vultr: New Provider!#219

Merged
tlimoncelli merged 7 commits intoStackExchange:masterfrom
pgaskin:add-vultr-provider
Oct 12, 2017
Merged

Vultr: New Provider!#219
tlimoncelli merged 7 commits intoStackExchange:masterfrom
pgaskin:add-vultr-provider

Conversation

@pgaskin
Copy link
Copy Markdown
Collaborator

@pgaskin pgaskin commented Oct 10, 2017

I have added the Vultr provider. This PR includes full support for all supported features from Vultr (SRV, CAA, CreateDomains).

I have tested this with both the integration tests, and my own domain.

  • DNS Provider
  • Documentation
  • Tests

See issue #217

@pgaskin
Copy link
Copy Markdown
Collaborator Author

pgaskin commented Oct 10, 2017

cc @captncraig

@pgaskin pgaskin mentioned this pull request Oct 10, 2017
@pgaskin
Copy link
Copy Markdown
Collaborator Author

pgaskin commented Oct 10, 2017

I am finished this PR.

@captncraig captncraig self-assigned this Oct 11, 2017
@captncraig
Copy link
Copy Markdown
Contributor

Wow! this looks great. I'll take a good look over it tomorrow.

@pgaskin
Copy link
Copy Markdown
Collaborator Author

pgaskin commented Oct 11, 2017

@captncraig Thanks! Btw, I have written code in go before 😃.

Also, I will submit some more PRs for improving the docs, and some of the other providers.

@captncraig
Copy link
Copy Markdown
Contributor

Sure, some of these bits have grown organically, so I'd love any feedback you have to offer. Some of the pain points I know about, but its always good to have a new set of eyes for honest feedback.

@pgaskin
Copy link
Copy Markdown
Collaborator Author

pgaskin commented Oct 11, 2017

@captncraig ok.

@pgaskin
Copy link
Copy Markdown
Collaborator Author

pgaskin commented Oct 11, 2017

Review please

}

var vd *vultr.DNSDomain
for _, d := range domains {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does their api do any kind of paging we need to worry about?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Passes integration tests and I tried myself with 300 entries.

rc.Target = dnsutil.AddOrigin(target, dc.Name)
}

// #rtype_variations
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this #comment everywhere here. Just at the top so this function shows up in grep.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the docs on adding rtypes were not all that clear, I guess I will fix that next.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

// #rtype_variations
if r.Type == "SRV" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there are some helper functions in the models package for splitting out srv/caa records from a single field representation. Is vultr doing something odd so that those don't work?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to look into that, but Vultr does quite a few funny things, so it may be better to manually handle it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The helper functions do not use the right order for Vultr.

@pgaskin
Copy link
Copy Markdown
Collaborator Author

pgaskin commented Oct 11, 2017

@captncraig Ready for another review.

@tlimoncelli
Copy link
Copy Markdown
Contributor

Looks great! Thanks!

@tlimoncelli tlimoncelli merged commit 2342751 into StackExchange:master Oct 12, 2017
@pgaskin
Copy link
Copy Markdown
Collaborator Author

pgaskin commented Oct 12, 2017

@tlimoncelli No problem!

@captncraig captncraig changed the title Added Vultr provider (#217) Vultr: New Provider! Oct 19, 2017
koesie10 pushed a commit to koesie10/dnscontrol that referenced this pull request Nov 14, 2017
* Added Vultr provider
* Fixed tests
* Fixed CI build validation
* Add unsupported features
* Added #rtype_variations tags according to stackexchange.github.io/dnscontrol/adding-new-rtypes
* Add title (for compatibility with StackExchange#223)
* Removed extra rtype_variations
rblenkinsopp pushed a commit to rblenkinsopp/dnscontrol that referenced this pull request Aug 21, 2020
* Added Vultr provider
* Fixed tests
* Fixed CI build validation
* Add unsupported features
* Added #rtype_variations tags according to stackexchange.github.io/dnscontrol/adding-new-rtypes
* Add title (for compatibility with StackExchange#223)
* Removed extra rtype_variations
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.

4 participants