local: added storage backend

local: added storage backend

Added a small web server acting as storage backend.

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

Requires: https://code.launchpad.net/~themue/juju-core/010-local-removed-old-approach/+merge/148719

(do not edit description out of merge proposal)

[TheMue, 2013-02-15 20:04:56]

Please take a look.

[TheMue, 2013-02-18 13:15:41]

Please take a look.

[TheMue, 2013-02-19 10:23:21]

Please take a look.

[TheMue, 2013-02-19 10:59:28]

Please take a look.

[TheMue, 2013-02-19 11:27:36]

Please take a look.

[fwereade, 2013-02-20 08:16:14]

Coming along nicely; a few thoughts:

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend.go
File environs/local/backend.go (right):

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend.go#ne...
environs/local/backend.go:98: http.Error(w, fmt.Sprintf("403 %v", err),
http.StatusForbidden)
These errors feel more like 500s to me..?

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend.go#ne...
environs/local/backend.go:102: defer out.Close()
this should come after the error check; out might be nil

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test....
environs/local/backend_test.go:65: content: "this is file 'yadda'",
Needs tests for paths below the top level, I think.

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test....
environs/local/backend_test.go:80: url :=
fmt.Sprintf("http://localhost:60006/%s/%s", environName, gt.name)
I don't think there's much point having the environName here... don't we have do
do all our differentiating by what port we're serving on anyway?

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test....
environs/local/backend_test.go:115: found:  []string{"inner/bar", "inner/baz",
"inner/foo"},
A file called "innkeeper" and a test for "inn", perhaps?

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test....
environs/local/backend_test.go:218: name: "dog",
Just to be explicit, perhaps a "../something" test here too?

[TheMue, 2013-02-20 11:17:52]

Please take a look.

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend.go
File environs/local/backend.go (right):

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend.go#ne...
environs/local/backend.go:98: http.Error(w, fmt.Sprintf("403 %v", err),
http.StatusForbidden)
On 2013/02/20 08:16:14, fwereade wrote:
> These errors feel more like 500s to me..?

Done.

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend.go#ne...
environs/local/backend.go:102: defer out.Close()
On 2013/02/20 08:16:14, fwereade wrote:
> this should come after the error check; out might be nil

Done.

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test....
environs/local/backend_test.go:65: content: "this is file 'yadda'",
On 2013/02/20 08:16:14, fwereade wrote:
> Needs tests for paths below the top level, I think.

Done.

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test....
environs/local/backend_test.go:80: url :=
fmt.Sprintf("http://localhost:60006/%s/%s", environName, gt.name)
On 2013/02/20 08:16:14, fwereade wrote:
> I don't think there's much point having the environName here... don't we have
do
> do all our differentiating by what port we're serving on anyway?

Aaargh, good catch. I misunderstood the idea of one backend supporting multiple
environments on one host. But that only covers the storage path in the file
system. The servers indeed only need different port.

https://codereview.appspot.com/7303097/diff/5002/environs/local/backend_test....
environs/local/backend_test.go:115: found:  []string{"inner/bar", "inner/baz",
"inner/foo"},
On 2013/02/20 08:16:14, fwereade wrote:
> A file called "innkeeper" and a test for "inn", perhaps?

Took "barin", "bazin", "fooin" as I need the prefix for the test below.

[TheMue, 2013-02-20 14:56:05]

Please take a look.

[fwereade, 2013-02-22 09:46:42]

Thanks for all this; LGTM. But please add a comment pointing out that the local
provider doesn't need this and should really use a much simpler Storage, and
that this code may be useful in storage-free environs, but requires
authentication work before that's viable.

[dimitern, 2013-02-22 10:25:03]

LGTM, just a few trivials.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend.go
File environs/local/backend.go (right):

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend.go#n...
environs/local/backend.go:50: // handleList returns the names in the storage to
the client.
s/names/file names/ ?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend.go#n...
environs/local/backend.go:107: if err != nil {
if _, err := io.Copy(...); err != nil {
..
} ?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend.go#n...
environs/local/backend.go:114: // handleDelete removes data from the storage.
s/data/a file/ ?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:44: s.listener.Close()
Are the files/dirs cleaned up after this? We don't want stale data from previous
test runs, do we?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:48: name    string
Do you really need 4 named structs with the same fields for each set of tests?
Why not define it once and then use it, or maybe define it inline var getTests
[]struct{...}{...}..

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:55: // Get existing file.
Why not put the comment into an about: or summary: string?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:96: // root is passed without relation to the
handler
see below

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:103: // root is passed without relation to the
handler
s/relation to/invoking/ ?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:110: // no file.
s/no file/not a file/ ?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:121: if gt.status != 0 {
look below

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:145: // List with a full filename.
as above - maybe make the comments part of the struct as a string field and
print it below?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:180: // root is passed without relation to the
handler
s/relation to/invoking/ ?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:193: if lt.status != 0 {
look below for the similar comment

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:225: // doesn't get aware of it.
s/doesn't get/isn't/

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:244: if pt.status != 0 {
this seems wrong - what if err != nil and resp is nil? check after the assert on
err?

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:305: if rt.status != 0 {
as above

[TheMue, 2013-02-22 14:01:38]

*** Submitted:

local: added storage backend

Added a small web server acting as storage backend.

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

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend.go
File environs/local/backend.go (right):

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend.go#n...
environs/local/backend.go:50: // handleList returns the names in the storage to
the client.
On 2013/02/22 10:25:03, dimitern wrote:
> s/names/file names/ ?

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend.go#n...
environs/local/backend.go:107: if err != nil {
On 2013/02/22 10:25:03, dimitern wrote:
> if _, err := io.Copy(...); err != nil {
> ..
> } ?

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend.go#n...
environs/local/backend.go:114: // handleDelete removes data from the storage.
On 2013/02/22 10:25:03, dimitern wrote:
> s/data/a file/ ?

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:44: s.listener.Close()
On 2013/02/22 10:25:03, dimitern wrote:
> Are the files/dirs cleaned up after this? We don't want stale data from
previous
> test runs, do we?

The data directory is a temporary one created and cleaned up by gocheck. So it's
clean afterwards.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:48: name    string
On 2013/02/22 10:25:03, dimitern wrote:
> Do you really need 4 named structs with the same fields for each set of tests?
> Why not define it once and then use it, or maybe define it inline var getTests
> []struct{...}{...}..

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:55: // Get existing file.
On 2013/02/22 10:25:03, dimitern wrote:
> Why not put the comment into an about: or summary: string?

Sure, would be possible and if I would log it it would helpful. But here they
are only intended to explain the sometimes interesting behavior of the Go HTTP
daemon regarding relative paths. So shorter comments are only to make it
complete. ;)

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:96: // root is passed without relation to the
handler
On 2013/02/22 10:25:03, dimitern wrote:
> see below

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:103: // root is passed without relation to the
handler
On 2013/02/22 10:25:03, dimitern wrote:
> s/relation to/invoking/ ?

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:110: // no file.
On 2013/02/22 10:25:03, dimitern wrote:
> s/no file/not a file/ ?

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:121: if gt.status != 0 {
On 2013/02/22 10:25:03, dimitern wrote:
> look below

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:145: // List with a full filename.
On 2013/02/22 10:25:03, dimitern wrote:
> as above - maybe make the comments part of the struct as a string field and
> print it below?

See above.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:180: // root is passed without relation to the
handler
On 2013/02/22 10:25:03, dimitern wrote:
> s/relation to/invoking/ ?

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:193: if lt.status != 0 {
On 2013/02/22 10:25:03, dimitern wrote:
> look below for the similar comment

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:225: // doesn't get aware of it.
On 2013/02/22 10:25:03, dimitern wrote:
> s/doesn't get/isn't/

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:244: if pt.status != 0 {
On 2013/02/22 10:25:03, dimitern wrote:
> this seems wrong - what if err != nil and resp is nil? check after the assert
on
> err?

Done.

https://codereview.appspot.com/7303097/diff/20001/environs/local/backend_test...
environs/local/backend_test.go:305: if rt.status != 0 {
On 2013/02/22 10:25:03, dimitern wrote:
> as above

Done.