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

Added new actions for Volume and Volume-Action #173

Merged
merged 5 commits into from
Dec 23, 2016

Conversation

xmudrii
Copy link
Contributor

@xmudrii xmudrii commented Dec 20, 2016

I gave it a try, I hope it's OK 👍 =)

cc @aybabtme

Copy link
Contributor

@aybabtme aybabtme left a comment

Choose a reason for hiding this comment

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

@nanzhong will be looking but from my point of view this LGTM, thanks!

@@ -7,12 +7,13 @@ type Volume struct {
*godo.Volume
}

// VolumesService is an interface for interacting with DigitalOcean's account api.
// VolumesService is an interface for interacting with DigitalOcean's volume api.
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch

@nanzhong
Copy link
Contributor

lgtm but will need a rebase and small change now that #171 is merged right?

@xmudrii
Copy link
Contributor Author

xmudrii commented Dec 23, 2016

@nanzhong is it good now? :)

@@ -134,4 +134,6 @@ const (

// ArgResourceType is the resource type for snapshot.
ArgResourceType = "resource"
// ArgSnapshotDesc is the description for volume snapshot.
ArgSnapshotDesc = "snapshot-desc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry tiny nitpick I just noticed, could we move this up to under ArgSnapshotName to make it a little easier to find? https://github.com/digitalocean/doctl/pull/173/files#diff-07ee57a49d1c0b692bd79676cc31dd6fR54

I'll merge this PR after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a problem, thanks for noting. Moved both up. 👍

Copy link
Contributor

@nanzhong nanzhong left a comment

Choose a reason for hiding this comment

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

lgtm, will merge once ci is 🍏

@nanzhong nanzhong merged commit b4a5dd5 into digitalocean:master Dec 23, 2016
@xmudrii xmudrii deleted the volume-extended branch December 23, 2016 18:27
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.

3 participants