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

Add CertificatesService for managing certificates with the DigitalOcean API. #126

Merged
merged 2 commits into from
Feb 17, 2017

Conversation

viola
Copy link
Contributor

@viola viola commented Feb 15, 2017

This change set adds support for managing certificates with the DigitalOcean API @ https://developers.digitalocean.com/documentation/v2/#certificates

Copy link

@mpapi mpapi left a comment

Choose a reason for hiding this comment

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

LGTM.

Just one small comment for your consideration.

certificates.go Outdated

// Get an existing certificate by its identifier.
func (c *CertificatesServiceOp) Get(cID string) (*Certificate, *Response, error) {
path := fmt.Sprintf("%s/%s", certificatesBasePath, cID)
Copy link

Choose a reason for hiding this comment

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

You could use path.Join here as well, from the stdlib (though you'd have to rename the path var so it doesn't clash with the path package).

And for a couple of instances in later methods, too.

Not strictly necessary, but I feel like it's a little more idiomatic and also saves you from accidentally having double slashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpapi ty, good idea, will patch that, many thx for your review.

Copy link

Choose a reason for hiding this comment

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

👍

@viola viola merged commit 19ceffc into master Feb 17, 2017
@viola viola deleted the add-certificates branch February 17, 2017 18:18
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.

2 participants