Code review comment for lp:~axwalk/juju-core/lp1235102-httpstorage-head

On 2013/10/04 12:08:36, rog wrote:
> LGTM with some minor grumblings.

Thanks, I'll address these in another CL since I've already landed this.

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

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode65
> environs/httpstorage/backend.go:65: func (s *storageBackend)
authorised(req
> *http.Request) bool {
> s/authorised/authorized/

> We standardise on US spelling in this code base,
> to match Go's conventions.
> (I just spent a while grepping for "authorized")

Thanks, will update.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode76
> environs/httpstorage/backend.go:76: hostonly := req.Host
> Can't you use net.SplitHostPort here?

Yes. I thought there was a function, just didn't find it in my
admittedly short search.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode86
> environs/httpstorage/backend.go:86: url :=
fmt.Sprintf("https://%s:%d%s",
> hostonly, s.httpsPort, req.URL.Path)
> tbh this feel like abuse of the HEAD method.

Indeed :)
I considered doing redirects, but not all client methods follow them,
and it'd cause two requests for each Put/Remove.

> It should at least check that req.URL.Path is /.
> But if it's conventional, I *guess* it's ok.

So, the only way Path might be empty is if the request line had URI with
no path. Right?
Never going to happen for this, so I'd rather not complicate it.

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

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend_test.go#newcode152
> environs/httpstorage/backend_test.go:152: c.Assert(location.String(),
> gc.Matches, "https://localhost:[0-9]{5}/arbitrary")
> We should continue with the test and check that we can
> actually connect with the given URL.

Okay.

https://codereview.appspot.com/14388043/