Implement the get charm file API.

Implement the get charm file API.

Add to the HTTPS API server the ability
to serve local charm files.

As discussed with Rick and Dimiter if the query includes the file,
then the file contents are served. If file points to a directory
an error is returned. If the query does not include a file, then
a JSON response is sent including a list of charm files.

QA:
- Download the Ghost charm from https://github.com/hatched/ghost-charm
  (the zip archive can be downloaded from the sidebar on the right).
- Bootstrap a local environment using this branch.
- Upload the ghost local charm, e.g.:
  `curl -ikL --data-binary @/path/to/ghost-charm-master.zip -H "Content-Type: application/zip" https://user-admin:MYPASSWD@10.0.3.1:17070/charms?series=precise`. 
  The response should be something like: {"CharmURL":"local:precise/ghost-4"}.
- Download ghost charm files, e.g.:
  `curl -ikLG -d "url=local:precise/ghost-4&file=icon.svg" https://user-admin:MYPASSWD@10.0.3.1:17070/charms`
  or
  `curl -ikLG -d "url=local:precise/ghost-4&file=hooks/install" https://user-admin:MYPASSWD@10.0.3.1:17070/charms`:
  you should see the file contents.
- Ensure directories are not allowed, e.g.:
  `curl -ikLG -d "url=local:precise/ghost-4&file=hooks" https://user-admin:MYPASSWD@10.0.3.1:17070/charms`: you should see a 403 forbidden.
- Retrieve a list of the charm files:
  `curl -ikLG -d "url=local:precise/ghost-4" https://user-admin:MYPASSWD@10.0.3.1:17070/charms`.
- Done, destroy the environment, thank you!

https://code.launchpad.net/~frankban/juju-core/get-charm-api/+merge/207994

(do not edit description out of merge proposal)

[frankban, 2014-02-24 18:39:03]

Please take a look.

[dimitern, 2014-02-25 10:07:09]

LGTM, thanks for doing this!

https://codereview.appspot.com/67750045/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/67750045/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:509: // CharmsResponse is the server response to
charm upload or get requests.
s/get/GET/

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

https://codereview.appspot.com/67750045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:540: if !strings.HasPrefix(filePath, bundlePath+"/") {
Please, use filepath.Dir(filePath) != bundlePath here instead.

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

https://codereview.appspot.com/67750045/diff/1/state/apiserver/charms_test.go...
state/apiserver/charms_test.go:249: "expected url=CharmURL query argument")
I'd prefer if you formatted this and other places where assertErrorResponse is
too long, like this:

s.assertErrorResponse(
    c, resp, http.StatusBadRequest,
    expected url=CharmURL query argument",
)

[dimitern, 2014-02-25 12:04:02]

On second though and after discussion with the team on today's standup, some
changes are needed before this can land. Please ping me to take a look again
when ready.

Instead of downloading AND extracting the charm, we got a general agreement you
should cache the charm archive, and then use something like charm.zipOpen to
read a single file from it, giving a path. You can also simplify file path
related operations, because the zip reader allows you to range over all files,
including their paths and do simple matching urlFileArg == zipFile.Name(). Look
into findArchiveRootDir and fixPath inside the charms handler in apiserver.

[frankban, 2014-02-25 17:03:52]

Please take a look.

https://codereview.appspot.com/67750045/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/67750045/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:509: // CharmsResponse is the server response to
charm upload or get requests.
On 2014/02/25 10:07:09, dimitern wrote:
> s/get/GET/

Done.

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

https://codereview.appspot.com/67750045/diff/1/state/apiserver/charms_test.go...
state/apiserver/charms_test.go:249: "expected url=CharmURL query argument")
On 2014/02/25 10:07:09, dimitern wrote:
> I'd prefer if you formatted this and other places where assertErrorResponse is
> too long, like this:
> 
> s.assertErrorResponse(
>     c, resp, http.StatusBadRequest,
>     expected url=CharmURL query argument",
> )

Done.

[dimitern, 2014-02-25 17:40:02]

Much better, thanks! LGTM with just a few formatting suggestions below. Make
sure you've run gofmt on the file, because indentation looks inconsistent in
places.

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#...
state/apiserver/charms.go:125: for _, file := range reader.File {
gofmt again?

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#...
state/apiserver/charms.go:138: } else {
Instead of else, you could just return in the if block above.

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#...
state/apiserver/charms.go:511: // processGet handles a charm file download GET
request after authentication.
s/download//

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#...
state/apiserver/charms.go:546: // save the corresponding zip archive to the
given charmArchivePath.
s/save/saves/

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms.go#...
state/apiserver/charms.go:580: if err = os.Rename(tempCharmArchive.Name(),
charmArchivePath); err != nil {
In this case I think you should defer os.Remove(tempCharmArchive.Name()) here.

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

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:265: func (s *charmsSuite)
TestGetReturnsFileNotFound(c *gc.C) {
s/TestGetReturnsFileNotFound/TestGetReturnsNotFoundWhenMissing/ ?

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:271: // Ensure a 404 is returned fo files not
included in the charm.
s/fo/for/

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:271: // Ensure a 404 is returned fo files not
included in the charm.
Please, put a blank line before this one, for readability (I'll use "b" in all
similar cases below).

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:283: func (s *charmsSuite)
TestGetReturnsDirectoryForbidden(c *gc.C) {
Similarly,
s/TestGetReturnsDirectoryForbidden/TestGetReturnsForbiddenWithDirectory/ ?

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:289: // Ensure a 403 is returned if the requested
file is a directory.
b

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:302: // Ensure the file contents are properly
returned.
b

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:335: // Ensure charm files are properly listed.
b

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:353: // Create and save the zip archive.
b

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:367: // Ensure the cached contents are properly
retrieved.
b

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
state/apiserver/charms_test.go:441: ctype := resp.Header.Get("content-type")
s/content-type/Content-Type/

[frankban, 2014-02-26 09:34:28]

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#...
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#...
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#...
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#...
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#...
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_tes...
File state/apiserver/charms_test.go (right):

https://codereview.appspot.com/67750045/diff/20001/state/apiserver/charms_tes...
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_tes...
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_tes...
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_tes...
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.

[frankban, 2014-02-26 09:35:30]

Thank you!

[dimitern, 2014-02-26 10:01:02]

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#...
state/apiserver/charms.go:138: } else {
On 2014/02/26 09:34:28, frankban wrote:
> 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.

Then you can just move the assignment out of the if.