Code review comment for lp:~axwalk/juju-core/localstorage-to-httpstorage

Revision history for this message
Tim Penhey (thumper) wrote :

A direct test for the glob change, and a helper function to create the
storage are needed.

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend.go
File environs/httpstorage/backend.go (right):

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend.go#newcode54
environs/httpstorage/backend.go:54: defer readcloser.Close()
Why defer? There is no harm in calling Close right now.

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend.go#newcode75
environs/httpstorage/backend.go:75: if req.ContentLength <= -1 {
isn't " < 0 " more readable and understandable?

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend_test.go
File environs/httpstorage/backend_test.go (right):

https://codereview.appspot.com/13736043/diff/4001/environs/httpstorage/backend_test.go#newcode109
environs/httpstorage/backend_test.go:109: // Get on a directory returns
a 500 as it is
I argue with this status code.

5xx means it is our fault. 4xx means it is your fault. This is
certainly a bad request.

https://codereview.appspot.com/13736043/diff/4001/environs/testing/storage.go
File environs/testing/storage.go (right):

https://codereview.appspot.com/13736043/diff/4001/environs/testing/storage.go#newcode33
environs/testing/storage.go:33: listener, err :=
httpstorage.Serve("localhost:0", underlying)
Given this is the second or third time I have seen these lines of code,
perhaps a testing helper function would be good.

https://codereview.appspot.com/13736043/

« Back to merge proposal