Add snapshots endpoints. #108

phillbaker · 2016-11-14T16:25:28Z

https://developers.digitalocean.com/documentation/changelog/api-v2/add-volume-snapshots/

cc @andrewsomething @mikejholly @mjsteger

andrewsomething · 2016-11-14T16:44:20Z

snapshots.go

+
+// Snapshot represents a DigitalOcean Snapshot
+type Snapshot struct {
+	ID            string   `json:"id,omitempty"`


The snapshots API mixes the type here. It returns a string for volumes and an int for Droplets. A simple test programs that lists all snapshots errors for me with:

Something bad happened: json: cannot unmarshal number into Go value of type string

This should be fixed in the API.

A little closer, I'm still seeing the same error. ResourceType is in the same situation, a string for volumes and an int for Droplets.

Here's the test program I'm using: https://gist.github.com/andrewsomething/d43ccaf333531001612647405188d4ac

Er, ResourceID maybe?

opps, that's what I meant.

aybabtme · 2016-11-16T07:00:18Z

snapshots_test.go

+package godo
+
+import (
+	// "encoding/json"


remove commented out code

aybabtme · 2016-11-16T07:00:44Z

snapshots.go

+
+// ListVolume lists all the volume snapshots.
+func (s *SnapshotsServiceOp) ListVolume(opt *ListOptions) ([]Snapshot, *Response, error) {
+	listOpt := listSnapshotOptions{ResourceType: "volume"}


There's existing code for volume snapshots, maybe it can be used. If not, it should be cleaned up if this is the implementation moving forward:

https://github.com/digitalocean/godo/blob/master/storage.go#L24-L38

and

https://github.com/digitalocean/godo/blob/master/storage.go#L153-L252

Yup, that code is all still relevant!

That code refers to api.digitalocean.com/v2/volumes/$VOLUME_ID/snapshots and api.digitalocean.com/v2/volumes/$VOLUME_ID/snapshots/$SNAPSHOT_ID, while this code refers to api.digitalocean.com/v2/snapshots/$SNAPSHOT_ID. Both sets of endpoints now exist.

However, I'll do a followup PR to remove all Beta for the code.

Oh, hm, https://github.com/digitalocean/godo/blob/master/storage.go#L153-L172 maybe should be removed as dupe.

aybabtme · 2016-11-16T07:05:22Z

snapshots.go

+type Snapshot struct {
+	ID            string   `json:"id,omitempty"`
+	Name          string   `json:"name,omitempty"`
+	ResourceID    string   `json:"type,omitempty"`


duplicate JSON tag here

aybabtme · 2016-11-16T07:07:15Z

snapshots.go

+	ResourceType  string   `json:"type,omitempty"`
+	Regions       []string `json:"regions,omitempty"`
+	MinDiskSize   int      `json:"min_disk_size,omitempty"`
+	SizeGigabytes int      `json:"size_gigabytes,omitempty"`


the rest of the code uses SizeGigaBytes (capitalized B):

https://github.com/digitalocean/godo/blob/master/storage.go#L79

https://github.com/digitalocean/godo/blob/master/storage.go#L53

aybabtme · 2016-11-16T07:07:57Z

snapshots.go

+	ResourceID    string   `json:"type,omitempty"`
+	ResourceType  string   `json:"type,omitempty"`
+	Regions       []string `json:"regions,omitempty"`
+	MinDiskSize   int      `json:"min_disk_size,omitempty"`


This field lacks a unit and order of magnitude qualifier: _bytes or _gigabytes.

Yup - we messed up when introducing in the API. We've called it MinDiskSize in other spots in godo as well. It's GB though, I can open an issue to clarify the naming in godo across the board.

phillbaker · 2016-11-18T16:27:40Z

Cleaned up! Anyone want to take another look?

aybabtme · 2016-11-27T06:30:18Z

storage_test.go

 			SizeGigaBytes: 100,
-			CreatedAt:     time.Date(2002, 10, 02, 15, 00, 00, 50000000, time.UTC),
+			Created:       "2002-10-02T15:00:00.05Z",


any reason to use string instead of time.Time? Usually typed things > string things.

Hm, looks like we're mixed between strings and time.Time and Created vs. CreatedAt:

godo/storage.go

Line 56 in 2ff8a02

CreatedAt time.Time `json:"created_at"`

godo/images.go

Line 39 in 5c91372

Created string `json:"created_at,omitempty"`

godo/droplets.go

Line 55 in c3a2aad

Created string `json:"created_at,omitempty"`

I can do another PR to standardize the others, what do you think of settling on Created as a time.Time?

yup, defly should all be time.Time

aybabtme · 2016-11-29T01:45:57Z

LGTM

andrewsomething reviewed Nov 14, 2016

View reviewed changes

aybabtme suggested changes Nov 16, 2016

View reviewed changes

phillbaker force-pushed the feature/snapshots branch from ea74695 to 2d57369 Compare November 17, 2016 20:47

Add snapshots endpoints, unify code with storage snapshots.

e715166

phillbaker force-pushed the feature/snapshots branch from 2d57369 to e715166 Compare November 18, 2016 16:27

aybabtme reviewed Nov 27, 2016

View reviewed changes

aybabtme approved these changes Nov 27, 2016

View reviewed changes

phillbaker mentioned this pull request Nov 29, 2016

What is the status of Snapshot API in godo? #112

Closed

phillbaker merged commit bec8984 into master Nov 30, 2016

phillbaker deleted the feature/snapshots branch November 30, 2016 03:58

phillbaker mentioned this pull request Nov 30, 2016

Remove betastorageservice, in production. #114

Merged

xmudrii mentioned this pull request Dec 1, 2016

Implementing new unified Snapshots API digitalocean/doctl#169

Merged

phillbaker mentioned this pull request Dec 2, 2016

Align all timestamps to use *Timestamp types. #116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snapshots endpoints. #108

Add snapshots endpoints. #108

phillbaker commented Nov 14, 2016

andrewsomething Nov 14, 2016

phillbaker Nov 17, 2016

andrewsomething Nov 18, 2016

phillbaker Nov 18, 2016

andrewsomething Nov 18, 2016

aybabtme Nov 16, 2016

aybabtme Nov 16, 2016 •

edited

Loading

phillbaker Nov 17, 2016 •

edited

Loading

phillbaker Nov 17, 2016

aybabtme Nov 16, 2016

aybabtme Nov 16, 2016

aybabtme Nov 16, 2016

phillbaker Nov 17, 2016

phillbaker commented Nov 18, 2016

aybabtme Nov 27, 2016

phillbaker Nov 29, 2016

aybabtme Nov 29, 2016

aybabtme commented Nov 29, 2016

Add snapshots endpoints. #108

Add snapshots endpoints. #108

Conversation

phillbaker commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aybabtme Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

phillbaker Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillbaker commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aybabtme commented Nov 29, 2016

aybabtme Nov 16, 2016 •

edited

Loading

phillbaker Nov 17, 2016 •

edited

Loading