Vanity name servers + specs #67
Conversation
|
@jcaudle I just noticed that the feature was provided under a new Client service. We already have the NameServer service available, and I believe the vanity feature should be part of it as it will also reflects the current API documentation. Can I ask you to include the You may need to rename |
|
|
||
| # Enable vanity name servers for a domain. | ||
| # | ||
| # @see https://developer.dnsimple.com/nameservers/vanity-nameservers/#enable |
There was a problem hiding this comment.
The updated link is https://developer.dnsimple.com/v1/nameservers/vanity-nameservers/#enable
There is no |
Yes, I agree. I will give more context and will save us from future conflicts. |
| # | ||
| # @return [void] | ||
| # @raise [RequestError] When the request fails. | ||
| def enable_vanity_name_servers(domain, names) |
There was a problem hiding this comment.
I just noticed the method is missing the option = {} final parameter. See for example
https://github.com/aetrion/dnsimple-ruby/blob/master/lib/dnsimple/client/domains_privacy.rb#L13
The same applies to disable.
The reason it's there, is because it allows to pass custom headers and params to the client.
See eb452f9
There was a problem hiding this comment.
I'd imagine we need to add a validation of at least ns1 being present too, correct? I'm thinking of https://github.com/aetrion/dnsimple-ruby/blob/master/lib/dnsimple/client/domains_records.rb#L33 as an example of this since there is a payload that will be required for this to work properly.
I'm also wondering if it'd be better to have the one using this client provide the server_source value, but I don't particularly understand the purpose of dnsimple as a value here.
There was a problem hiding this comment.
Technically, as of today we allow blank NS and we skip the blanks on our side. I would not be too much worried about validation here.
Just make sure that the Hash you create inside the method has higher priority over any value provided in options.
There was a problem hiding this comment.
I've added the ability to pass options in with 7edb58e
|
It's good to me. Thanks! |
This pull request supersedes and closes #63 by adding specs to the vanity name server client provided by @iseem. Thanks @iseem!