s3: add LocationConstraint in PutBucket

s3: add LocationConstraint in PutBucket

The default value of and empty body is only valid for us-east-1, the 
so called classic ec2 region. Non US regions required a location 
constraint.

https://code.launchpad.net/~dave-cheney/goamz/s3-location-constraint/+merge/112478

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-07-03 08:28:33]

Please take a look.

[niemeyer, 2012-07-03 10:56:36]

Looks good, but the testing side needs some tweaks.

https://codereview.appspot.com/6344050/diff/4001/s3/s3_test.go
File s3/s3_test.go (right):

https://codereview.appspot.com/6344050/diff/4001/s3/s3_test.go#newcode23
s3/s3_test.go:23: s.s3 = s3.New(auth, aws.Region{Name: aws.USEast.Name,
S3Endpoint: testServer.URL})
This should be faux-region-1 as well, and below we should have a test that
verifies that the content is actually provided as mentioned (through req.Body).

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

https://codereview.appspot.com/6344050/diff/4001/s3/s3test/server.go#newcode52
s3/s3test/server.go:52: // false is LocationConstraint must not be specified.
If the real server requires it, we don't need this option. Let's keep the logic
below that requires it at all times.

[dave_cheney.net, 2012-07-05 06:15:54]

Please take a look.

https://codereview.appspot.com/6344050/diff/4001/s3/s3_test.go
File s3/s3_test.go (right):

https://codereview.appspot.com/6344050/diff/4001/s3/s3_test.go#newcode23
s3/s3_test.go:23: s.s3 = s3.New(auth, aws.Region{Name: aws.USEast.Name,
S3Endpoint: testServer.URL})
On 2012/07/03 10:56:36, niemeyer wrote:
> This should be faux-region-1 as well, and below we should have a test that
> verifies that the content is actually provided as mentioned (through
req.Body).

Done.

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

https://codereview.appspot.com/6344050/diff/4001/s3/s3test/server.go#newcode52
s3/s3test/server.go:52: // false is LocationConstraint must not be specified.
On 2012/07/03 10:56:36, niemeyer wrote:
> If the real server requires it, we don't need this option. Let's keep the
logic
> below that requires it at all times.

It's more complicated than that, you _must_ supply LocationConstraint if you are
using us-east-1. Conversely you _must_ supply the LocationConstraint if you are
not using us-east-1.

I've changed it so that LocationConstraint is always required when talking to
the s3test server. Thereby implying that this server can never claim to be
us-east-1

[dave_cheney.net, 2012-07-05 06:20:30]

Please take a look.

[niemeyer, 2012-07-05 21:14:14]

Thanks. LGTM with the following detail sorted. Please feel free to submit it
directly, unless you'd prefer to discuss further.

https://codereview.appspot.com/6344050/diff/2002/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/6344050/diff/2002/s3/s3.go#newcode74
s3/s3.go:74: if s3.Region.Name != aws.USEast.Name {
I didn't realize we'd need this when I suggested we used .Name instead. I still
think .Name is a good idea, but we shouldn't hardcode it here like that, or
people using other implementations will stumble on this and be unable to fix
without hacking.

Would you mind to introduce a bool:

    S3LocationConstraint: true/false

in the aws.Region values, and then change the test above to use it, instead of
hardcoding it?

https://codereview.appspot.com/6344050/diff/2002/s3/s3i_test.go
File s3/s3i_test.go (right):

https://codereview.appspot.com/6344050/diff/2002/s3/s3i_test.go#newcode83
s3/s3i_test.go:83: return s.s3.Bucket(fmt.Sprintf("%s-%s-%s", name,
s.s3.Region.Name, strings.ToLower(s.s3.Auth.AccessKey)))
Why is this necessary? AFAIK, it blows up with domain names only, but in those
cases the Bucket method itself lowercases it. Can you please file a bug
describing the problem with some debugging info, if possible, so we can fix it
later?

https://codereview.appspot.com/6344050/diff/2002/s3/s3i_test.go#newcode86
s3/s3i_test.go:86: const testBucket = "goamz"
Please preserve the original bucket name. It's important for people to be told
this is a test bucket when they see it in S3, and not think it's something
required by goamz.

[dave_cheney.net, 2012-07-05 23:46:56]

Please take a look.

https://codereview.appspot.com/6344050/diff/2002/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/6344050/diff/2002/s3/s3.go#newcode74
s3/s3.go:74: if s3.Region.Name != aws.USEast.Name {
On 2012/07/05 21:14:15, niemeyer wrote:
> I didn't realize we'd need this when I suggested we used .Name instead. I
still
> think .Name is a good idea, but we shouldn't hardcode it here like that, or
> people using other implementations will stumble on this and be unable to fix
> without hacking.
> 
> Would you mind to introduce a bool:
> 
>     S3LocationConstraint: true/false
> 
> in the aws.Region values, and then change the test above to use it, instead of
> hardcoding it?

The problem is this value would be true for all regions with the single
exception of us-east-1.

How about, 

   S3EmptyLocationConstraint: true   // only for USEast

The name may need some massaging, but I think the intent is clear.

https://codereview.appspot.com/6344050/diff/2002/s3/s3i_test.go
File s3/s3i_test.go (right):

https://codereview.appspot.com/6344050/diff/2002/s3/s3i_test.go#newcode83
s3/s3i_test.go:83: return s.s3.Bucket(fmt.Sprintf("%s-%s-%s", name,
s.s3.Region.Name, strings.ToLower(s.s3.Auth.AccessKey)))
On 2012/07/05 21:14:15, niemeyer wrote:
> Why is this necessary? AFAIK, it blows up with domain names only, but in those
> cases the Bucket method itself lowercases it. Can you please file a bug
> describing the problem with some debugging info, if possible, so we can fix it
> later?

You can't create a bucket name with upper case characters in EUWest :(

I'll file a bug.

https://codereview.appspot.com/6344050/diff/2002/s3/s3i_test.go#newcode86
s3/s3i_test.go:86: const testBucket = "goamz"
On 2012/07/05 21:14:15, niemeyer wrote:
> Please preserve the original bucket name. It's important for people to be told
> this is a test bucket when they see it in S3, and not think it's something
> required by goamz.

Done.

[dave_cheney.net, 2012-07-06 03:25:09]

*** Submitted:

s3: add LocationConstraint in PutBucket

The default value of and empty body is only valid for us-east-1, the 
so called classic ec2 region. Non US regions required a location 
constraint.

R=niemeyer
CC=
https://codereview.appspot.com/6344050

https://codereview.appspot.com/6344050/diff/2002/s3/s3.go
File s3/s3.go (right):

https://codereview.appspot.com/6344050/diff/2002/s3/s3.go#newcode74
s3/s3.go:74: if s3.Region.Name != aws.USEast.Name {
Done. I've implemented your suggestion.