local: add storage

local: add storage

Added environ.Storage implementation using the local
storage backend. The tests so far are a copy from
environs/jujutest to check if the behavior is adequate.

https://code.launchpad.net/~themue/juju-core/012-local-storage/+merge/149246

Requires: https://code.launchpad.net/~themue/juju-core/011-local-storage-backend/+merge/148809

(do not edit description out of merge proposal)

[TheMue, 2013-02-19 11:34:58]

Please take a look.

[dimitern, 2013-02-19 12:06:09]

Overall LGTM, modulo some trivial suggestions.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go
File environs/local/storage.go (right):

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:11: 
d

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:14: 
var _ environ.Storage = (*storage)(nil) ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:30: // exist, it should return a *NotFoundError.
s/*NotFoundError/NotFoundError - or even use roger's NotFoundf() ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:68: if string(body) == "" {
len(body) == 0 ?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:76: // URL returns a URL that can be used to access
the given storage file.
s/a URL/an URL

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:82: // The length must give the total length of the
file.
s/must give/must be set to/

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:98: return errors.New(resp.Status)
Include the StatusCode in the error text?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:120: return errors.New(resp.Status)
as above

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go
File environs/local/storage_test.go (right):

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#...
environs/local/storage_test.go:70: c.Assert(err, IsNil)
if it's check*, use c.Check, rather than c.Assert - like you're doing above.
Otherwise, call it assert*

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#...
environs/local/storage_test.go:83: for a := attempt.Start(); a.Next(); {
If you're not retrying (attempt strategy is zeroed out), why do you do that
here?

https://codereview.appspot.com/7346052/diff/1/environs/local/storage_test.go#...
environs/local/storage_test.go:121: c.Check(data, DeepEquals, contents)
Mixing Assert and Check seems a bit random here, care to explain or unify these?

[TheMue, 2013-02-19 13:36:42]

Please take a look.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go
File environs/local/storage.go (right):

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:11: 
On 2013/02/19 12:06:10, dimitern wrote:
> d

Done.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:14: 
On 2013/02/19 12:06:10, dimitern wrote:
> var _ environ.Storage = (*storage)(nil) ?

Done.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:14: 
On 2013/02/19 12:06:10, dimitern wrote:
> var _ environ.Storage = (*storage)(nil) ?

Done.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:30: // exist, it should return a *NotFoundError.
On 2013/02/19 12:06:10, dimitern wrote:
> s/*NotFoundError/NotFoundError - or even use roger's NotFoundf() ?

It's defined that way in environs and used the same way multiple times.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:68: if string(body) == "" {
On 2013/02/19 12:06:10, dimitern wrote:
> len(body) == 0 ?

Done.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:76: // URL returns a URL that can be used to access
the given storage file.
On 2013/02/19 12:06:10, dimitern wrote:
> s/a URL/an URL

Done.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:82: // The length must give the total length of the
file.
On 2013/02/19 12:06:10, dimitern wrote:
> s/must give/must be set to/

Done.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:98: return errors.New(resp.Status)
On 2013/02/19 12:06:10, dimitern wrote:
> Include the StatusCode in the error text?

Done.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:98: return errors.New(resp.Status)
On 2013/02/19 12:06:10, dimitern wrote:
> Include the StatusCode in the error text?

Done.

https://codereview.appspot.com/7346052/diff/1/environs/local/storage.go#newco...
environs/local/storage.go:120: return errors.New(resp.Status)
On 2013/02/19 12:06:10, dimitern wrote:
> as above

Done.

[dimitern, 2013-02-19 13:42:21]

LGTM

[TheMue, 2013-02-20 12:04:29]

Please take a look.

[TheMue, 2013-02-20 14:58:50]

Please take a look.

[fwereade, 2013-02-22 10:09:04]

LGTM with the AttemptStrategy stuff dropped.

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test.go
File environs/local/storage_test.go (right):

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test...
environs/local/storage_test.go:18: var noRetry = trivial.AttemptStrategy{}
There's no need for an AttemptStrategy here.

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test...
environs/local/storage_test.go:22: // be removed again.
These tests should be written properly, and should not be earmarked for
deletion. Do we really write these sorts of tests against an *Environ* in the
other cases? If so, what have we been smoking?

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test...
environs/local/storage_test.go:85: func checkFileDoesNotExist(c *C, storage
environs.StorageReader, name string, attempt trivial.AttemptStrategy) {
AttemptStrategy not needed.

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test...
environs/local/storage_test.go:99: func checkFileHasContents(c *C, storage
environs.StorageReader, name string, contents []byte, attempt
trivial.AttemptStrategy) {
AttemptStrategy not needed.

[TheMue, 2013-02-22 14:31:34]

*** Submitted:

local: add storage

Added environ.Storage implementation using the local
storage backend. The tests so far are a copy from
environs/jujutest to check if the behavior is adequate.

R=dimitern, fwereade
CC=
https://codereview.appspot.com/7346052

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test.go
File environs/local/storage_test.go (right):

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test...
environs/local/storage_test.go:18: var noRetry = trivial.AttemptStrategy{}
On 2013/02/22 10:09:04, fwereade wrote:
> There's no need for an AttemptStrategy here.

Done.

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test...
environs/local/storage_test.go:85: func checkFileDoesNotExist(c *C, storage
environs.StorageReader, name string, attempt trivial.AttemptStrategy) {
On 2013/02/22 10:09:04, fwereade wrote:
> AttemptStrategy not needed.

Done.

https://codereview.appspot.com/7346052/diff/12001/environs/local/storage_test...
environs/local/storage_test.go:99: func checkFileHasContents(c *C, storage
environs.StorageReader, name string, contents []byte, attempt
trivial.AttemptStrategy) {
On 2013/02/22 10:09:04, fwereade wrote:
> AttemptStrategy not needed.

Done.