store: serve charm bundle data at /charm/<curl path>

https://code.launchpad.net/~niemeyer/juju/go-store-server-charms/+merge/96287

(do not edit description out of merge proposal)

[niemeyer, 2012-03-07 03:19:20]

Please take a look.

[fwereade, 2012-03-07 14:29:54]

LGTM

https://codereview.appspot.com/5758061/diff/1/store/server.go
File store/server.go (right):

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode88
store/server.go:88: curl, err := charm.ParseURL("cs:" +
r.URL.Path[len("/charm/"):])
3 uses of "/charm/" might justify a package var, but I'm not really bothered
either way.

[rog, 2012-03-07 14:33:59]

https://codereview.appspot.com/5758061/diff/1/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/5758061/diff/1/.bzrignore#newcode1
.bzrignore:1: store*.6
this looks like an oversight.
BTW since the go tool we probably don't need most of these any more as it's so
clean.

https://codereview.appspot.com/5758061/diff/1/store/server.go
File store/server.go (right):

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode73
store/server.go:73: w.Header()["Content-Type"] = []string{"application/json"}
w.Header().Set("Content-Type", "application/json")

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode78
store/server.go:78: w.WriteHeader(http.StatusInternalServerError)
as per comment on the io.Copy below, i think this would be better on the
json.Marshal error only, and the write error ignored.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode84
store/server.go:84: if !strings.HasPrefix(r.URL.Path, "/charm/") {
no need for this - the ServeMux won't get here unless it's true.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode104
store/server.go:104: w.Header()["Content-Type"] =
[]string{"application/octet-stream"}
w.Header().Set("Content-Type", "application/octet-stream")

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode107
store/server.go:107: log.Printf("failed to stream charm %q: %v", curl, err)
i'm not sure that we really care at all about this error. the client can go away
on a whim.

[niemeyer, 2012-03-07 15:01:03]

https://codereview.appspot.com/5758061/diff/1/.bzrignore
File .bzrignore (right):

https://codereview.appspot.com/5758061/diff/1/.bzrignore#newcode1
.bzrignore:1: store*.6
On 2012/03/07 14:33:59, rog wrote:
> this looks like an oversight.
> BTW since the go tool we probably don't need most of these any more as it's so
> clean.

Good point, will fix.

https://codereview.appspot.com/5758061/diff/1/store/server.go
File store/server.go (right):

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode73
store/server.go:73: w.Header()["Content-Type"] = []string{"application/json"}
On 2012/03/07 14:33:59, rog wrote:
> w.Header().Set("Content-Type", "application/json")

Sounds good.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode78
store/server.go:78: w.WriteHeader(http.StatusInternalServerError)
On 2012/03/07 14:33:59, rog wrote:
> as per comment on the io.Copy below, i think this would be better on the
> json.Marshal error only, and the write error ignored.

A write error is still an error, and I want to know about them. I've fixed bugs
in the http package before.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode84
store/server.go:84: if !strings.HasPrefix(r.URL.Path, "/charm/") {
On 2012/03/07 14:33:59, rog wrote:
> no need for this - the ServeMux won't get here unless it's true.

I had that in mind, but I'd rather know about problems here, as the logic below
trusts on it to be true. I'll change it to a panic, though.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode88
store/server.go:88: curl, err := charm.ParseURL("cs:" +
r.URL.Path[len("/charm/"):])
On 2012/03/07 14:29:54, fwereade wrote:
> 3 uses of "/charm/" might justify a package var, but I'm not really bothered
> either way.

Given that they're a bit contained, and there's even some cross-checking between
the values, I'd prefer to keep them like that if that's ok with you. It feels
slightly easier to read the logic seeing the value than with a const.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode104
store/server.go:104: w.Header()["Content-Type"] =
[]string{"application/octet-stream"}
On 2012/03/07 14:33:59, rog wrote:
> w.Header().Set("Content-Type", "application/octet-stream")

Sounds good.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode107
store/server.go:107: log.Printf("failed to stream charm %q: %v", curl, err)
On 2012/03/07 14:33:59, rog wrote:
> i'm not sure that we really care at all about this error. the client can go
away
> on a whim.

That's what Russ said, but besides the point above, this is definitely not ok to
ignore. There's both a read and a write side.

[niemeyer, 2012-03-07 15:34:57]

Please take a look.

[rog, 2012-03-07 15:44:29]

LGTM but you might want to consider using http.StripPrefix.

https://codereview.appspot.com/5758061/diff/1/store/server.go
File store/server.go (right):

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode78
store/server.go:78: w.WriteHeader(http.StatusInternalServerError)
On 2012/03/07 15:01:03, niemeyer wrote:
> On 2012/03/07 14:33:59, rog wrote:
> > as per comment on the io.Copy below, i think this would be better on the
> > json.Marshal error only, and the write error ignored.
> 
> A write error is still an error, and I want to know about them. I've fixed
bugs
> in the http package before.

not entirely convinced, but fair enough.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode84
store/server.go:84: if !strings.HasPrefix(r.URL.Path, "/charm/") {
On 2012/03/07 15:01:03, niemeyer wrote:
> On 2012/03/07 14:33:59, rog wrote:
> > no need for this - the ServeMux won't get here unless it's true.
> 
> I had that in mind, but I'd rather know about problems here, as the logic
below
> trusts on it to be true. I'll change it to a panic, though.

ok. you could use http.StripPrefix too, which might look cleaner.

https://codereview.appspot.com/5758061/diff/1/store/server.go#newcode107
store/server.go:107: log.Printf("failed to stream charm %q: %v", curl, err)
On 2012/03/07 15:01:03, niemeyer wrote:
> On 2012/03/07 14:33:59, rog wrote:
> > i'm not sure that we really care at all about this error. the client can go
> away
> > on a whim.
> 
> That's what Russ said, but besides the point above, this is definitely not ok
to
> ignore. There's both a read and a write side.

ok, that's fine then.

[niemeyer, 2012-03-07 16:00:17]

*** Submitted:

store: serve charm bundle data at /charm/<curl path>

R=fwereade, rog
CC=
https://codereview.appspot.com/5758061