s3: add new s3test test server

https://code.launchpad.net/~rogpeppe/goamz/s3-s3test/+merge/89746

(do not edit description out of merge proposal)

[rog, 2012-01-24 15:11:53]

Please take a look.

[rog, 2012-01-25 09:36:33]

Please take a look.

[rog, 2012-01-25 09:37:41]

Please take a look.

[niemeyer, 2012-01-25 17:18:46]

Very nice design.

https://codereview.appspot.com/5573045/diff/3006/s3.go
File s3.go (right):

https://codereview.appspot.com/5573045/diff/3006/s3.go#newcode292
s3.go:292: return nil, fmt.Errorf("error executing S3 http request: %v", err)
Let's not wrap the error here. That's killing all the useful information from
http error value for no benefit.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go
File s3test/server.go (right):

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode42
s3test/server.go:42: // Server represents an s3 server.
// Server is a fake S3 server for testing purposes.
// All of the data for the server is kept in memory.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode44
s3test/server.go:44: addr     string
s/addr/url/

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode66
s3test/server.go:66: // A resource encapsulates the subject of a REST request.
s/REST/HTTP/, let's not fall onto the hype. These are standard HTTP methods.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode92
s3test/server.go:92: func (srv *Server) Address() string {
s/Address/URL/

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode93
s3test/server.go:93: return srv.addr
s/addr/url/

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode106
s3test/server.go:106: req.ParseForm()
Please handle the error.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode260
s3test/server.go:260: fatalf(400, "MethodNotAllowed", "The specified method is
not allowed against this resource")
func notAllowed() interface{} {
    fatalf(400, "MethodNotAllowed", "The specified method is not allowed against
this resource")
    return nil
}

func (nullResource) put(a *action) interface{} { return notAllowed() }
func (nullResource) get(a *action) interface{} { return notAllowed() }
func (nullResource) post(a *action) interface{} { return notAllowed() }
func (nullResource) delete(a *action) interface{} { return notAllowed() }

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode272
s3test/server.go:272: fatalf(400, "NotImplemented", "get not yet implemented on
buckets")
s/get/GET/

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode336
s3test/server.go:336: r >= 'a' && r <= 'z') {
This would be fine in a single line.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode415
s3test/server.go:415: func atoi64(s string) int64 {
Doesn't seem used.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode438
s3test/server.go:438: // TODO is this correct, or should we erase all previous
metadata?
Good question. Worth checking since it may lead to bugs in applications (juju)
trusting it one way or the other.

[rog, 2012-01-25 18:48:46]

Please take a look.

[rog, 2012-01-30 14:11:17]

PTAL

https://codereview.appspot.com/5573045/diff/3006/s3.go
File s3.go (right):

https://codereview.appspot.com/5573045/diff/3006/s3.go#newcode292
s3.go:292: return nil, fmt.Errorf("error executing S3 http request: %v", err)
On 2012/01/25 17:18:46, niemeyer wrote:
> Let's not wrap the error here. That's killing all the useful information from
> http error value for no benefit.

agreed; although perhaps we should be returning an Error with the right status
code.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go
File s3test/server.go (right):

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode42
s3test/server.go:42: // Server represents an s3 server.
On 2012/01/25 17:18:46, niemeyer wrote:
> // Server is a fake S3 server for testing purposes.
> // All of the data for the server is kept in memory.

Done.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode66
s3test/server.go:66: // A resource encapsulates the subject of a REST request.
On 2012/01/25 17:18:46, niemeyer wrote:
> s/REST/HTTP/, let's not fall onto the hype. These are standard HTTP methods.

Done.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode92
s3test/server.go:92: func (srv *Server) Address() string {
On 2012/01/25 17:18:46, niemeyer wrote:
> s/Address/URL/

agreed. but currently this is consistent with ec2test.Server. i'll change them
both (and the caller) in a later set of CLs if that's ok.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode106
s3test/server.go:106: req.ParseForm()
On 2012/01/25 17:18:46, niemeyer wrote:
> Please handle the error.

comment added

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode260
s3test/server.go:260: fatalf(400, "MethodNotAllowed", "The specified method is
not allowed against this resource")
On 2012/01/25 17:18:46, niemeyer wrote:
> func notAllowed() interface{} {
>     fatalf(400, "MethodNotAllowed", "The specified method is not allowed
against
> this resource")
>     return nil
> }
> 
> func (nullResource) put(a *action) interface{} { return notAllowed() }
> func (nullResource) get(a *action) interface{} { return notAllowed() }
> func (nullResource) post(a *action) interface{} { return notAllowed() }
> func (nullResource) delete(a *action) interface{} { return notAllowed() }
> 

Done.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode272
s3test/server.go:272: fatalf(400, "NotImplemented", "get not yet implemented on
buckets")
On 2012/01/25 17:18:46, niemeyer wrote:
> s/get/GET/

Done.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode336
s3test/server.go:336: r >= 'a' && r <= 'z') {
On 2012/01/25 17:18:46, niemeyer wrote:
> This would be fine in a single line.

Done.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode415
s3test/server.go:415: func atoi64(s string) int64 {
On 2012/01/25 17:18:46, niemeyer wrote:
> Doesn't seem used.

good catch. it was used by the range parsing only.

https://codereview.appspot.com/5573045/diff/3006/s3test/server.go#newcode438
s3test/server.go:438: // TODO is this correct, or should we erase all previous
metadata?
On 2012/01/25 17:18:46, niemeyer wrote:
> Good question. Worth checking since it may lead to bugs in applications (juju)
> trusting it one way or the other.

will leave it as as TODO for the moment, as we're not using metadata yet

[rog, 2012-01-30 14:17:13]

Please take a look.

[niemeyer, 2012-01-30 14:20:05]

LGTM, thanks.

[rog, 2012-01-30 14:23:29]

*** Submitted:

s3: add new s3test test server

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