Merge lp:~nick-craig-wood/goamz/list-buckets into lp:~gophers/goamz/trunk

Proposed by Nick Craig-Wood
Status: Needs review
Proposed branch: lp:~nick-craig-wood/goamz/list-buckets
Merge into: lp:~gophers/goamz/trunk
Diff against target: 429 lines (+173/-34)
12 files modified
aws/aws.go (+7/-1)
aws/aws_test.go (+9/-0)
ec2/ec2_test.go (+4/-0)
exp/mturk/mturk_test.go (+4/-0)
exp/sdb/sdb_test.go (+4/-0)
exp/sns/sns_test.go (+4/-0)
iam/iam_test.go (+6/-2)
s3/multi.go (+3/-2)
s3/responses_test.go (+19/-0)
s3/s3.go (+69/-27)
s3/s3_test.go (+30/-0)
testutil/http.go (+14/-2)
To merge this branch: bzr merge lp:~nick-craig-wood/goamz/list-buckets
Reviewer Review Type Date Requested Status
The Go Language Gophers Pending
Review via email: mp+146925@code.launchpad.net

Description of the change

s3: Implement ListBuckets method

https://codereview.appspot.com/7305051/

To post a comment you must log in.
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Looking good, thanks for this. Just a few trivial details to sort out.

https://codereview.appspot.com/7305051/diff/1/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode43
s3/s3.go:43: // The following fields are only set on Buckets returned
from the ListBuckets call
s/Buckets/buckets/

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode45
s3/s3.go:45: CreationDate string // Date the bucket was created, e.g.,
2009-02-03T16:45:09.000Z
Seems like this should be a native time.Time.

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode74
s3/s3.go:74: type BucketInfo struct {
s/BucketInfo/bucketInfo/

This is used only internally, so doesn't have to be exposed in the
public API.

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode80
s3/s3.go:80: type Buckets struct {
s/Buckets/bucketInfoResp/, for the same reason.

The doc also needs fixing.

https://codereview.appspot.com/7305051/diff/1/s3/s3.go#newcode444
s3/s3.go:444: if req.baseurl == "" {
That logic is changed in trunk, both due to retries and to fix a bug.
Trunk needs to be merged back so we can see what the actual change
needed is here.

https://codereview.appspot.com/7305051/

Revision history for this message
Nick Craig-Wood (nick-craig-wood) wrote :

Arg, I've messed something up - probably this branch!

bzr diff -c38 | diffstat
 responses_test.go | 19 +++++++++++++++
 s3.go | 68 ++++++++++++++++++++++++++++++++++++++++++------------
 s3_test.go | 29 +++++++++++++++++++++++
 3 files changed, 102 insertions(+), 14 deletions(-)

Will try a new branch

Unmerged revisions

38. By Nick Craig-Wood

Implement S3.List to list all Buckets

37. By Ian Booth

Support EC2_ env variables

Allow the user to specify their auth credentials using EC2_ style
environment variables. I ran go fmt and it also made some changes.

The tests failed to run due to a test http server not being closed when
each test suite finished. I added a fix for this also, so now all tests
pass when go test ./... is run from the project's root directory.

R=jameinel, Danilo
CC=
https://codereview.appspot.com/9545045

36. By Gustavo Niemeyer

s3: close http response Body properly

The interface of the run method was error prone.

R=dfc, anacrolix
CC=
https://codereview.appspot.com/9611047

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'aws/aws.go'
--- aws/aws.go 2012-11-13 23:59:04 +0000
+++ aws/aws.go 2013-06-27 11:56:25 +0000
@@ -162,10 +162,16 @@
162162
163// EnvAuth creates an Auth based on environment information.163// EnvAuth creates an Auth based on environment information.
164// The AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment164// The AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment
165// variables are used.165// variables are used as the first preference, but EC2_ACCESS_KEY
166// and EC2_SECRET_KEY environment variables are also supported.
166func EnvAuth() (auth Auth, err error) {167func EnvAuth() (auth Auth, err error) {
167 auth.AccessKey = os.Getenv("AWS_ACCESS_KEY_ID")168 auth.AccessKey = os.Getenv("AWS_ACCESS_KEY_ID")
168 auth.SecretKey = os.Getenv("AWS_SECRET_ACCESS_KEY")169 auth.SecretKey = os.Getenv("AWS_SECRET_ACCESS_KEY")
170 // We fallback to EC2_ env variables if the AWS_ variants are not used.
171 if auth.AccessKey == "" && auth.SecretKey == "" {
172 auth.AccessKey = os.Getenv("EC2_ACCESS_KEY")
173 auth.SecretKey = os.Getenv("EC2_SECRET_KEY")
174 }
169 if auth.AccessKey == "" {175 if auth.AccessKey == "" {
170 err = errors.New("AWS_ACCESS_KEY_ID not found in environment")176 err = errors.New("AWS_ACCESS_KEY_ID not found in environment")
171 }177 }
172178
=== modified file 'aws/aws_test.go'
--- aws/aws_test.go 2013-01-31 14:52:05 +0000
+++ aws/aws_test.go 2013-06-27 11:56:25 +0000
@@ -52,6 +52,15 @@
52 c.Assert(auth, Equals, aws.Auth{SecretKey: "secret", AccessKey: "access"})52 c.Assert(auth, Equals, aws.Auth{SecretKey: "secret", AccessKey: "access"})
53}53}
5454
55func (s *S) TestEnvAuthLegacy(c *C) {
56 os.Clearenv()
57 os.Setenv("EC2_SECRET_KEY", "secret")
58 os.Setenv("EC2_ACCESS_KEY", "access")
59 auth, err := aws.EnvAuth()
60 c.Assert(err, IsNil)
61 c.Assert(auth, Equals, aws.Auth{SecretKey: "secret", AccessKey: "access"})
62}
63
55func (s *S) TestEncode(c *C) {64func (s *S) TestEncode(c *C) {
56 c.Assert(aws.Encode("foo"), Equals, "foo")65 c.Assert(aws.Encode("foo"), Equals, "foo")
57 c.Assert(aws.Encode("/"), Equals, "%2F")66 c.Assert(aws.Encode("/"), Equals, "%2F")
5867
=== modified file 'ec2/ec2_test.go'
--- ec2/ec2_test.go 2013-01-31 14:52:05 +0000
+++ ec2/ec2_test.go 2013-06-27 11:56:25 +0000
@@ -26,6 +26,10 @@
26 s.ec2 = ec2.New(auth, aws.Region{EC2Endpoint: testServer.URL})26 s.ec2 = ec2.New(auth, aws.Region{EC2Endpoint: testServer.URL})
27}27}
2828
29func (s *S) TearDownSuite(c *C) {
30 testServer.Stop()
31}
32
29func (s *S) TearDownTest(c *C) {33func (s *S) TearDownTest(c *C) {
30 testServer.Flush()34 testServer.Flush()
31}35}
3236
=== modified file 'exp/mturk/mturk_test.go'
--- exp/mturk/mturk_test.go 2013-01-31 14:52:05 +0000
+++ exp/mturk/mturk_test.go 2013-06-27 11:56:25 +0000
@@ -35,6 +35,10 @@
35 }35 }
36}36}
3737
38func (s *S) TearDownSuite(c *C) {
39 testServer.Stop()
40}
41
38func (s *S) TearDownTest(c *C) {42func (s *S) TearDownTest(c *C) {
39 testServer.Flush()43 testServer.Flush()
40}44}
4145
=== modified file 'exp/sdb/sdb_test.go'
--- exp/sdb/sdb_test.go 2013-01-31 14:52:05 +0000
+++ exp/sdb/sdb_test.go 2013-06-27 11:56:25 +0000
@@ -26,6 +26,10 @@
26 s.sdb = sdb.New(auth, aws.Region{SDBEndpoint: testServer.URL})26 s.sdb = sdb.New(auth, aws.Region{SDBEndpoint: testServer.URL})
27}27}
2828
29func (s *S) TearDownSuite(c *C) {
30 testServer.Stop()
31}
32
29func (s *S) TearDownTest(c *C) {33func (s *S) TearDownTest(c *C) {
30 testServer.Flush()34 testServer.Flush()
31}35}
3236
=== modified file 'exp/sns/sns_test.go'
--- exp/sns/sns_test.go 2013-01-31 14:52:05 +0000
+++ exp/sns/sns_test.go 2013-06-27 11:56:25 +0000
@@ -26,6 +26,10 @@
26 s.sns = sns.New(auth, aws.Region{SNSEndpoint: testServer.URL})26 s.sns = sns.New(auth, aws.Region{SNSEndpoint: testServer.URL})
27}27}
2828
29func (s *S) TearDownSuite(c *C) {
30 testServer.Stop()
31}
32
29func (s *S) TearDownTest(c *C) {33func (s *S) TearDownTest(c *C) {
30 testServer.Flush()34 testServer.Flush()
31}35}
3236
=== modified file 'iam/iam_test.go'
--- iam/iam_test.go 2013-03-02 02:11:38 +0000
+++ iam/iam_test.go 2013-06-27 11:56:25 +0000
@@ -27,6 +27,10 @@
27 s.iam = iam.New(auth, aws.Region{IAMEndpoint: testServer.URL})27 s.iam = iam.New(auth, aws.Region{IAMEndpoint: testServer.URL})
28}28}
2929
30func (s *S) TearDownSuite(c *C) {
31 testServer.Stop()
32}
33
30func (s *S) TearDownTest(c *C) {34func (s *S) TearDownTest(c *C) {
31 testServer.Flush()35 testServer.Flush()
32}36}
@@ -146,8 +150,8 @@
146 {150 {
147 Path: "/division_abc/subdivision_xyz/product_1234/",151 Path: "/division_abc/subdivision_xyz/product_1234/",
148 Name: "Managers",152 Name: "Managers",
149 Id: "AGPIODR4TAW7CSEXAMPLE",153 Id: "AGPIODR4TAW7CSEXAMPLE",
150 Arn: "arn:aws:iam::123456789012:group/division_abc/subdivision_xyz/product_1234/Managers",154 Arn: "arn:aws:iam::123456789012:group/division_abc/subdivision_xyz/product_1234/Managers",
151 },155 },
152 }156 }
153 c.Assert(resp.Groups, DeepEquals, expected)157 c.Assert(resp.Groups, DeepEquals, expected)
154158
=== modified file 's3/multi.go'
--- s3/multi.go 2013-02-13 12:20:38 +0000
+++ s3/multi.go 2013-06-27 11:56:25 +0000
@@ -175,14 +175,15 @@
175 if err != nil {175 if err != nil {
176 return Part{}, err176 return Part{}, err
177 }177 }
178 resp, err := m.Bucket.S3.run(req, nil)178 hresp, err := m.Bucket.S3.run(req)
179 if shouldRetry(err) && attempt.HasNext() {179 if shouldRetry(err) && attempt.HasNext() {
180 continue180 continue
181 }181 }
182 if err != nil {182 if err != nil {
183 return Part{}, err183 return Part{}, err
184 }184 }
185 etag := resp.Header.Get("ETag")185 hresp.Body.Close()
186 etag := hresp.Header.Get("ETag")
186 if etag == "" {187 if etag == "" {
187 return Part{}, errors.New("part upload succeeded with no ETag")188 return Part{}, errors.New("part upload succeeded with no ETag")
188 }189 }
189190
=== modified file 's3/responses_test.go'
--- s3/responses_test.go 2013-02-12 04:50:11 +0000
+++ s3/responses_test.go 2013-06-27 11:56:25 +0000
@@ -196,3 +196,22 @@
196 <HostId>kjhwqk</HostId>196 <HostId>kjhwqk</HostId>
197</Error>197</Error>
198`198`
199
200var GetBucketsListResultDump = `
201<ListAllMyBucketsResult xmlns="http://doc.s3.amazonaws.com/2006-03-01">
202 <Owner>
203 <ID>bcaf1ffd86f461ca5fb16fd081034f</ID>
204 <DisplayName>webfile</DisplayName>
205 </Owner>
206 <Buckets>
207 <Bucket>
208 <Name>quotes</Name>
209 <CreationDate>2006-02-03T16:45:09.000Z</CreationDate>
210 </Bucket>
211 <Bucket>
212 <Name>samples</Name>
213 <CreationDate>2006-02-03T16:41:58.000Z</CreationDate>
214 </Bucket>
215 </Buckets>
216</ListAllMyBucketsResult>
217`
199218
=== modified file 's3/s3.go'
--- s3/s3.go 2013-02-11 17:15:59 +0000
+++ s3/s3.go 2013-06-27 11:56:25 +0000
@@ -40,6 +40,9 @@
40type Bucket struct {40type Bucket struct {
41 *S341 *S3
42 Name string42 Name string
43 // The following fields are only set on buckets returned from the ListBuckets call
44 Owner Owner // Owner of the bucket
45 CreationDate time.Time // When the bucket was created
43}46}
4447
45// The Owner type represents the owner of the object in an S3 bucket.48// The Owner type represents the owner of the object in an S3 bucket.
@@ -64,7 +67,44 @@
64 if s3.Region.S3BucketEndpoint != "" || s3.Region.S3LowercaseBucket {67 if s3.Region.S3BucketEndpoint != "" || s3.Region.S3LowercaseBucket {
65 name = strings.ToLower(name)68 name = strings.ToLower(name)
66 }69 }
67 return &Bucket{s3, name}70 return &Bucket{S3: s3, Name: name}
71}
72
73// bucketsResp is returned by S3.Buckets
74type bucketsResp struct {
75 Owner Owner
76 Buckets []struct {
77 Name string
78 CreationDate string // Date the bucket was created, e.g., 2009-02-03T16:45:09.000Z
79 } `xml:"Buckets>Bucket"`
80}
81
82// List returns a list of all buckets owned by the sender
83//
84// See http://docs.amazonwebservices.com/AmazonS3/latest/API/RESTServiceGET.html
85func (s3 *S3) ListBuckets() (buckets []*Bucket, err error) {
86 req := &request{
87 method: "GET",
88 bucket: "",
89 path: "",
90 }
91 result := &bucketsResp{}
92 err = s3.query(req, result)
93 if err != nil {
94 return nil, err
95 }
96 buckets = make([]*Bucket, len(result.Buckets))
97 for i := range buckets {
98 info := result.Buckets[i]
99 b := s3.Bucket(info.Name)
100 b.CreationDate, err = time.Parse(time.RFC3339Nano, info.CreationDate)
101 if err != nil {
102 return nil, err
103 }
104 b.Owner = result.Owner
105 buckets[i] = b
106 }
107 return buckets, nil
68}108}
69109
70var createBucketConfiguration = `<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/"> 110var createBucketConfiguration = `<CreateBucketConfiguration xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
@@ -156,14 +196,14 @@
156 return nil, err196 return nil, err
157 }197 }
158 for attempt := attempts.Start(); attempt.Next(); {198 for attempt := attempts.Start(); attempt.Next(); {
159 resp, err := b.S3.run(req, nil)199 hresp, err := b.S3.run(req)
160 if shouldRetry(err) && attempt.HasNext() {200 if shouldRetry(err) && attempt.HasNext() {
161 continue201 continue
162 }202 }
163 if err != nil {203 if err != nil {
164 return nil, err204 return nil, err
165 }205 }
166 return resp.Body, nil206 return hresp.Body, nil
167 }207 }
168 panic("unreachable")208 panic("unreachable")
169}209}
@@ -382,10 +422,18 @@
382// body will be unmarshalled on it.422// body will be unmarshalled on it.
383func (s3 *S3) query(req *request, resp interface{}) error {423func (s3 *S3) query(req *request, resp interface{}) error {
384 err := s3.prepare(req)424 err := s3.prepare(req)
385 if err == nil {425 if err != nil {
386 _, err = s3.run(req, resp)426 return err
387 }427 }
388 return err428 hresp, err := s3.run(req)
429 if err != nil {
430 return err
431 }
432 if resp != nil {
433 err = xml.NewDecoder(hresp.Body).Decode(resp)
434 }
435 hresp.Body.Close()
436 return nil
389}437}
390438
391// prepare sets up req to be delivered to S3.439// prepare sets up req to be delivered to S3.
@@ -410,21 +458,21 @@
410 req.path = "/" + req.path458 req.path = "/" + req.path
411 }459 }
412 req.signpath = req.path460 req.signpath = req.path
413 if req.bucket != "" {461 req.baseurl = s3.Region.S3BucketEndpoint
414 req.baseurl = s3.Region.S3BucketEndpoint462 if req.baseurl == "" {
415 if req.baseurl == "" {463 // Use the path method to address the bucket.
416 // Use the path method to address the bucket.464 req.baseurl = s3.Region.S3Endpoint
417 req.baseurl = s3.Region.S3Endpoint465 if req.bucket != "" {
418 req.path = "/" + req.bucket + req.path466 req.path = "/" + req.bucket + req.path
419 } else {467 }
420 // Just in case, prevent injection.468 } else {
421 if strings.IndexAny(req.bucket, "/:@") >= 0 {469 // Just in case, prevent injection.
422 return fmt.Errorf("bad S3 bucket: %q", req.bucket)470 if strings.IndexAny(req.bucket, "/:@") >= 0 {
423 }471 return fmt.Errorf("bad S3 bucket: %q", req.bucket)
424 req.baseurl = strings.Replace(req.baseurl, "${bucket}", req.bucket, -1)472 }
425 }473 req.baseurl = strings.Replace(req.baseurl, "${bucket}", req.bucket, -1)
426 req.signpath = "/" + req.bucket + req.signpath
427 }474 }
475 req.signpath = "/" + req.bucket + req.signpath
428 }476 }
429477
430 // Always sign again as it's not clear how far the478 // Always sign again as it's not clear how far the
@@ -440,9 +488,7 @@
440}488}
441489
442// run sends req and returns the http response from the server.490// run sends req and returns the http response from the server.
443// If resp is not nil, the XML data contained in the response491func (s3 *S3) run(req *request) (*http.Response, error) {
444// body will be unmarshalled on it.
445func (s3 *S3) run(req *request, resp interface{}) (*http.Response, error) {
446 if debug {492 if debug {
447 log.Printf("Running S3 request: %#v", req)493 log.Printf("Running S3 request: %#v", req)
448 }494 }
@@ -480,10 +526,6 @@
480 if hresp.StatusCode != 200 && hresp.StatusCode != 204 {526 if hresp.StatusCode != 200 && hresp.StatusCode != 204 {
481 return nil, buildError(hresp)527 return nil, buildError(hresp)
482 }528 }
483 if resp != nil {
484 err = xml.NewDecoder(hresp.Body).Decode(resp)
485 hresp.Body.Close()
486 }
487 return hresp, err529 return hresp, err
488}530}
489531
490532
=== modified file 's3/s3_test.go'
--- s3/s3_test.go 2013-02-12 04:50:11 +0000
+++ s3/s3_test.go 2013-06-27 11:56:25 +0000
@@ -4,6 +4,7 @@
4 "bytes"4 "bytes"
5 "io/ioutil"5 "io/ioutil"
6 "net/http"6 "net/http"
7 "net/url"
7 "testing"8 "testing"
89
9 "launchpad.net/goamz/aws"10 "launchpad.net/goamz/aws"
@@ -33,6 +34,7 @@
3334
34func (s *S) TearDownSuite(c *C) {35func (s *S) TearDownSuite(c *C) {
35 s3.SetAttemptStrategy(nil)36 s3.SetAttemptStrategy(nil)
37 testServer.Stop()
36}38}
3739
38func (s *S) SetUpTest(c *C) {40func (s *S) SetUpTest(c *C) {
@@ -275,3 +277,31 @@
275 c.Assert(len(data.Contents), Equals, 0)277 c.Assert(len(data.Contents), Equals, 0)
276 c.Assert(data.CommonPrefixes, DeepEquals, []string{"photos/2006/feb/", "photos/2006/jan/"})278 c.Assert(data.CommonPrefixes, DeepEquals, []string{"photos/2006/feb/", "photos/2006/jan/"})
277}279}
280
281// List buckets docs: See http://docs.amazonwebservices.com/AmazonS3/latest/API/RESTServiceGET.html
282
283func (s *S) TestListBuckets(c *C) {
284 testServer.Response(200, nil, GetBucketsListResultDump)
285
286 buckets, err := s.s3.ListBuckets()
287 c.Assert(err, IsNil)
288
289 req := testServer.WaitRequest()
290 c.Assert(req.Method, Equals, "GET")
291 c.Assert(req.URL.Path, Equals, "/")
292 c.Assert(req.Header["Date"], Not(Equals), "")
293 c.Assert(req.Form, DeepEquals, url.Values{})
294
295 c.Assert(len(buckets), Equals, 2)
296
297 c.Assert(buckets[0].Name, Equals, "quotes")
298 c.Assert(buckets[0].CreationDate, Equals, time.Date(2006, 2, 3, 16, 45, 9, 0, time.UTC))
299 c.Assert(buckets[0].Owner.ID, Equals, "bcaf1ffd86f461ca5fb16fd081034f")
300 c.Assert(buckets[0].Owner.DisplayName, Equals, "webfile")
301
302 c.Assert(buckets[1].Name, Equals, "samples")
303 c.Assert(buckets[1].CreationDate, Equals, time.Date(2006, 2, 3, 16, 41, 58, 0, time.UTC))
304 c.Assert(buckets[1].Owner.ID, Equals, "bcaf1ffd86f461ca5fb16fd081034f")
305 c.Assert(buckets[1].Owner.DisplayName, Equals, "webfile")
306
307}
278308
=== modified file 'testutil/http.go'
--- testutil/http.go 2013-02-01 05:55:19 +0000
+++ testutil/http.go 2013-06-27 11:56:25 +0000
@@ -15,6 +15,7 @@
15 URL string15 URL string
16 Timeout time.Duration16 Timeout time.Duration
17 started bool17 started bool
18 listener net.Listener
18 request chan *http.Request19 request chan *http.Request
19 response chan ResponseFunc20 response chan ResponseFunc
20}21}
@@ -42,11 +43,11 @@
42 if err != nil {43 if err != nil {
43 panic(err)44 panic(err)
44 }45 }
45 l, err := net.Listen("tcp", u.Host)46 s.listener, err = net.Listen("tcp", u.Host)
46 if err != nil {47 if err != nil {
47 panic(err)48 panic(err)
48 }49 }
49 go http.Serve(l, s)50 go http.Serve(s.listener, s)
5051
51 s.Response(203, nil, "")52 s.Response(203, nil, "")
52 for {53 for {
@@ -60,6 +61,17 @@
60 s.WaitRequest() // Consume dummy request.61 s.WaitRequest() // Consume dummy request.
61}62}
6263
64func (s *HTTPServer) Stop() {
65 if s.listener == nil {
66 return
67 }
68 err := s.listener.Close()
69 if err != nil {
70 panic(err)
71 }
72 s.listener = nil
73}
74
63// Flush discards all pending requests and responses.75// Flush discards all pending requests and responses.
64func (s *HTTPServer) Flush() {76func (s *HTTPServer) Flush() {
65 for {77 for {

Subscribers

People subscribed via source and target branches