apiserver: Charm upload takes archives w/ subdirs

apiserver: Charm upload takes archives w/ subdirs

Fixed bug #1273464: HTTPS based API to upload local
charms fails for compressed charms with nested dirs
inside, instead of having metadata.yaml, etc. in the
zip root dir.

Changes only how uploads are handled: charms with
nested dirs are detected after uploading and then
repackaged to remove the nesting, before uploading
them to the provider storage.

Some code for handling zipped archives for charms
is copied from the charm package, but as fwereade
requested, the changes do not affect the charm
package itself.

https://code.launchpad.net/~dimitern/juju-core/280-lp-1273464-putcharm-archives-with-root-dir/+merge/204043

Requires: https://code.launchpad.net/~dimitern/juju-core/270-lp-1257649-ssh-timeout-bootstrap/+merge/203832

(do not edit description out of merge proposal)

[dimitern, 2014-01-30 17:01:13]

Please take a look.

[natefinch, 2014-01-30 18:56:35]

Pretty good, but can you look at the use of strings.suffix instead of
filepath.base?  I'm just thinking about edge cases... same with checking for
paths ending in slashes.

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

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:214: // findArchiveRootDir takes a *zip.Reader and
returns the rootDir of
You don't really need to say it takes the zip.Reader :)

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:223: if strings.HasSuffix(fh.Name, fname) {
Can you use filepath.Base for this?  probably safer that string processing. 
Same for the other filename operations in this file

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:243: func (h *charmsHandler) extractArchiveTo(zipr
*zip.Reader, stripPrefix, path string) error {
This looks like it needs to be split into multiple functions... it too long to
easily follow as-is.

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:258: defer f.Close()
Right now this will end up opening every file in the zip file before closing
them all at the end of the function... If you make the interior of the loop a
separate function, we'll only have one open at a time.

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:262: if strings.HasSuffix(fh.Name, "/") ||
mode&os.ModeDir != 0 {
Do we really have to check for the name ending in a slash?  Will the mode not
catch this?   Wondering if this will fail on Windows.

[dimitern, 2014-01-31 11:52:41]

Please take a look.

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

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:214: // findArchiveRootDir takes a *zip.Reader and
returns the rootDir of
On 2014/01/30 18:56:35, nate.finch wrote:
> You don't really need to say it takes the zip.Reader :)

Done.

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:223: if strings.HasSuffix(fh.Name, fname) {
On 2014/01/30 18:56:35, nate.finch wrote:
> Can you use filepath.Base for this?  probably safer that string processing. 
> Same for the other filename operations in this file

Done. Changed almost all path operations to use filepath, except for
strings.Contains(path, ".."), which filepath does not provide.

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:243: func (h *charmsHandler) extractArchiveTo(zipr
*zip.Reader, stripPrefix, path string) error {
On 2014/01/30 18:56:35, nate.finch wrote:
> This looks like it needs to be split into multiple functions... it too long to
> easily follow as-is.

Because it's almost carbon copied from the charm package. I wanted to keep all
of it the same and in one place. If you insist, I can still split it up into
multiple helpers, which will be called only once here.

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:258: defer f.Close()
On 2014/01/30 18:56:35, nate.finch wrote:
> Right now this will end up opening every file in the zip file before closing
> them all at the end of the function... If you make the interior of the loop a
> separate function, we'll only have one open at a time.

I extracted the loop body into a separate func.

https://codereview.appspot.com/54680045/diff/1/state/apiserver/charms.go#newc...
state/apiserver/charms.go:262: if strings.HasSuffix(fh.Name, "/") ||
mode&os.ModeDir != 0 {
On 2014/01/30 18:56:35, nate.finch wrote:
> Do we really have to check for the name ending in a slash?  Will the mode not
> catch this?   Wondering if this will fail on Windows.

This is now changed. It's like this in the original charm package code.

[natefinch, 2014-01-31 13:58:35]

LGTM, though I don't like the copy & paste of the charm code, it sounds like
that's the way it has to be for now.

[dimitern, 2014-01-31 15:17:14]

Please take a look.