s3: add support for dreamhost and bucket names in the domain

s3: add support for dreamhost and bucket names in the domain

S3 supports two ways to address bucket names: via a common
domain name plus a path, and via a domain named after the
user bucket. This adds support to the latter convention.

It also includes support for talking to DreamHost's S3-like
endpoint, and fixes a bug in the tests.

https://code.launchpad.net/~niemeyer/goamz/dreamhost-support/+merge/106497

(do not edit description out of merge proposal)

[niemeyer, 2012-05-19 17:44:14]

Please take a look.

[rog, 2012-05-22 13:55:43]

LGTM with a few minors and taste issues.

https://codereview.appspot.com/6211070/diff/1/aws/aws.go
File aws/aws.go (right):

https://codereview.appspot.com/6211070/diff/1/aws/aws.go#newcode23
aws/aws.go:23: S3BucketEndpoint string // Not needed by AWS S3. Use %s for
bucket name.
I'm not sure about using %s here. it seems a bit hackish and error-prone, and
obscure when the string is defined. how about using "$name" and using
strings.Replace to implement it rather than fmt.Sprintf?

https://codereview.appspot.com/6211070/diff/1/aws/aws.go#newcode31
aws/aws.go:31: "https://s3.amazonaws.com", "",
i think the "" should be on a line of its own, here and below, rather than
trying to pretend it's not really there :-)

alternatively use tagged struct initialisations.

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

https://codereview.appspot.com/6211070/diff/1/s3/s3.go#newcode288
s3/s3.go:288: endpointLocation = fmt.Sprintf(endpointLocation, bucket)
endpointLocation = strings.Replace(endpointLocation, "$name", bucket, -1) ?

https://codereview.appspot.com/6211070/diff/1/s3/s3test/server.go
File s3/s3test/server.go (right):

https://codereview.appspot.com/6211070/diff/1/s3/s3test/server.go#newcode380
s3/s3test/server.go:380: fatalf(400, "BucketNotEmpty", "The bucket you tried to
delete is not empty")
ahem. oops.

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

https://codereview.appspot.com/6211070/diff/1/s3/sign.go#newcode88
s3/sign.go:88: bucket := strings.Join(parts[:len(parts)-3], ".")
i'm a bit uncomfortable with the out-of-range panic if the host doesn't conform
to the expected form. could we give a nicer error here when we panic?

[niemeyer, 2012-05-22 14:16:55]

Please take a look.

https://codereview.appspot.com/6211070/diff/1/aws/aws.go
File aws/aws.go (right):

https://codereview.appspot.com/6211070/diff/1/aws/aws.go#newcode23
aws/aws.go:23: S3BucketEndpoint string // Not needed by AWS S3. Use %s for
bucket name.
On 2012/05/22 13:55:43, rog wrote:
> I'm not sure about using %s here. it seems a bit hackish and error-prone, and
> obscure when the string is defined. how about using "$name" and using
> strings.Replace to implement it rather than fmt.Sprintf?

Done with ${bucket}, as discussed.

https://codereview.appspot.com/6211070/diff/1/aws/aws.go#newcode31
aws/aws.go:31: "https://s3.amazonaws.com", "",
On 2012/05/22 13:55:43, rog wrote:
> i think the "" should be on a line of its own, here and below, rather than
> trying to pretend it's not really there :-)

Done.

> alternatively use tagged struct initialisations.

Having them fully defined prevents misses when new fields are added.

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

https://codereview.appspot.com/6211070/diff/1/s3/s3.go#newcode288
s3/s3.go:288: endpointLocation = fmt.Sprintf(endpointLocation, bucket)
On 2012/05/22 13:55:43, rog wrote:
> endpointLocation = strings.Replace(endpointLocation, "$name", bucket, -1) ?

Done.

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

https://codereview.appspot.com/6211070/diff/1/s3/sign.go#newcode88
s3/sign.go:88: bucket := strings.Join(parts[:len(parts)-3], ".")
On 2012/05/22 13:55:43, rog wrote:
> i'm a bit uncomfortable with the out-of-range panic if the host doesn't
conform
> to the expected form. could we give a nicer error here when we panic?

As we talked, a panic can't happen here.

[rog, 2012-05-22 14:22:47]

LGTM

[niemeyer, 2012-05-22 14:26:30]

*** Submitted:

s3: add support for dreamhost and bucket names in the domain

S3 supports two ways to address bucket names: via a common
domain name plus a path, and via a domain named after the
user bucket. This adds support to the latter convention.

It also includes support for talking to DreamHost's S3-like
endpoint, and fixes a bug in the tests.

R=rog
CC=
https://codereview.appspot.com/6211070