Code review comment for lp:~frankban/juju-core/get-charm-api

Please take a look.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go
File state/apiserver/charms.go (right):

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode125
state/apiserver/charms.go:125: for _, file := range reader.File {
On 2014/02/25 17:40:03, dimitern wrote:
> gofmt again?

Done, and it seems already ok.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode138
state/apiserver/charms.go:138: } else {
On 2014/02/25 17:40:03, dimitern wrote:
> Instead of else, you could just return in the if block above.

We need contents in the scope, that's why I used else.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode511
state/apiserver/charms.go:511: // processGet handles a charm file
download GET request after authentication.
On 2014/02/25 17:40:03, dimitern wrote:
> s/download//

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode546
state/apiserver/charms.go:546: // save the corresponding zip archive to
the given charmArchivePath.
On 2014/02/25 17:40:03, dimitern wrote:
> s/save/saves/

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#newcode580
state/apiserver/charms.go:580: if err =
os.Rename(tempCharmArchive.Name(), charmArchivePath); err != nil {
On 2014/02/25 17:40:03, dimitern wrote:
> In this case I think you should defer
os.Remove(tempCharmArchive.Name()) here.

Good catch, fixed.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (right):

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode265
state/apiserver/charms_test.go:265: func (s *charmsSuite)
TestGetReturnsFileNotFound(c *gc.C) {
On 2014/02/25 17:40:03, dimitern wrote:
> s/TestGetReturnsFileNotFound/TestGetReturnsNotFoundWhenMissing/ ?

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode271
state/apiserver/charms_test.go:271: // Ensure a 404 is returned fo files
not included in the charm.
On 2014/02/25 17:40:03, dimitern wrote:
> s/fo/for/

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode283
state/apiserver/charms_test.go:283: func (s *charmsSuite)
TestGetReturnsDirectoryForbidden(c *gc.C) {
On 2014/02/25 17:40:03, dimitern wrote:
> Similarly,

s/TestGetReturnsDirectoryForbidden/TestGetReturnsForbiddenWithDirectory/
?

Done.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_test.go#newcode441
state/apiserver/charms_test.go:441: ctype :=
resp.Header.Get("content-type")
On 2014/02/25 17:40:03, dimitern wrote:
> s/content-type/Content-Type/

Done.

https://codereview.appspot.com/67750045/