charm: add Bundle.Manifest

charm: add Bundle.Manifest

this is intended to be used by the uniter/charm package, to support git-free
charm updating.

This CL now includes content from https://codereview.appspot.com/69110043/
which was a prereq added after the fact; everything outside utils/zip is
unique to this CL and needs review.

https://code.launchpad.net/~fwereade/juju-core/charm-bundle-manifest/+merge/201811

(do not edit description out of merge proposal)

[fwereade, 2014-01-15 16:48:30]

Please take a look.

[rog, 2014-01-15 17:41:13]

LGTM with a suggestion and a query below.

https://codereview.appspot.com/49960047/diff/1/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/49960047/diff/1/charm/bundle.go#newcode150
charm/bundle.go:150: func (b *Bundle) withZipReader(f withZipReaderFunc) error {
This doesn't feel too idiomatic to me.

I think I'd prefer:

type zipReadCloser struct {
    io.Closer
    *zip.Reader
}

func (b *Bundle) bundleReader() (*zipReadCloser, error) {
      // If we don't have a Path, try to use the original
      // ReaderAt,
      if b.Path == "" {
          r, err := zip.NewReader(b.r, b.size)
          if err != nil {
               return nil, err
          }
          return &zipReadCloser{Closer: ioutil.NopCloser(nil), Reader: r}
      }
      f, err := os.Open(b.Path)
      etc
      return &zipReadCloser{Closer: f, Reader: r}
}

Then the control flow in Manifest and ExpandTo is more obvious
(we don't have to know when or how many times the
argument function is called back).

https://codereview.appspot.com/49960047/diff/1/charm/bundle.go#newcode203
charm/bundle.go:203: lasterr = err
Why show only the last error?

[fwereade, 2014-01-21 12:58:13]

Please take a look.

https://codereview.appspot.com/49960047/diff/1/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/49960047/diff/1/charm/bundle.go#newcode150
charm/bundle.go:150: func (b *Bundle) withZipReader(f withZipReaderFunc) error {
On 2014/01/15 17:41:14, rog wrote:
> This doesn't feel too idiomatic to me.
> 
> I think I'd prefer:
> 
> type zipReadCloser struct {
>     io.Closer
>     *zip.Reader
> }
> 
> func (b *Bundle) bundleReader() (*zipReadCloser, error) {
>       // If we don't have a Path, try to use the original
>       // ReaderAt,
>       if b.Path == "" {
>           r, err := zip.NewReader(b.r, b.size)
>           if err != nil {
>                return nil, err
>           }
>           return &zipReadCloser{Closer: ioutil.NopCloser(nil), Reader: r}
>       }
>       f, err := os.Open(b.Path)
>       etc
>       return &zipReadCloser{Closer: f, Reader: r}
> }
> 
> Then the control flow in Manifest and ExpandTo is more obvious
> (we don't have to know when or how many times the
> argument function is called back).

Hmm, so the choice is between doing an automatic close when required, and
forcing the client to close whether it's required or not. My heart still favours
the first, but the latter probably *would* be clearer when reading client code
in isolation. Cheers.

https://codereview.appspot.com/49960047/diff/1/charm/bundle.go#newcode203
charm/bundle.go:203: lasterr = err
On 2014/01/15 17:41:14, rog wrote:
> Why show only the last error?

Search me. WRT ExpandTo this is a pure refactor without changing behaviour.
Can't dodge it for long, I'll be dealing with ExpandTo in a followup.

[dimitern, 2014-02-17 13:52:32]

LGTM, but after having done the repackaging of (any) uploaded charms (including
zips from Windows with subdir, etc.), I'll be a bit cautious how to treat file
paths in the archive, which ultimately end up in the manifest.

https://codereview.appspot.com/49960047/diff/20001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/49960047/diff/20001/charm/bundle.go#newcode186
charm/bundle.go:186: manifest.Add(zfile.Name)
If we're doing just this, we might end up with a manifest containing entries
like:

./xyz/../metadata.yaml
xx\from\windows.zip
aa////b/c
subdir/metadata.yaml (or config.yaml, etc. - these need to be in the charm dir
root)

So we need some path cleanup on each entry, like we do in apiserver's
charmsHandler (see fixPath there and extractSingleFile) (i.e. replace any
slashes, possibly duplicated or more, with a single filepath.Separator, then
call filepath.Clean on the result).

Or alternatively, we could just scan for fishy paths and reject the charm as
invalid (if we got it through the usual channels - cs: or from an upload, it
can't be invalid because we repackage them as needed).

https://codereview.appspot.com/49960047/diff/20001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/49960047/diff/20001/charm/bundle_test.go#newco...
charm/bundle_test.go:98: c.Skip("cannot symlink")
Skip? Isn't this worth failing the test?

https://codereview.appspot.com/49960047/diff/20001/charm/bundle_test.go#newco...
charm/bundle_test.go:314: func extBundleDir(c *gc.C, dirpath string)
*charm.Bundle {
Nice to have these two in one place!

[fwereade, 2014-02-27 14:23:54]

Please take a look.

https://codereview.appspot.com/49960047/diff/20001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/49960047/diff/20001/charm/bundle_test.go#newco...
charm/bundle_test.go:98: c.Skip("cannot symlink")
On 2014/02/17 13:52:32, dimitern wrote:
> Skip? Isn't this worth failing the test?

IIRC the other places we do this are to make the tests pass on windows. I'll
determine canSymlink in SetUpSuite, I think, should make it clearer.

[fwereade, 2014-02-28 13:49:32]

Please take a look.

[fwereade, 2014-03-06 17:43:48]

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

https://codereview.appspot.com/49960047/diff/60001/state/apiserver/charms.go#...
state/apiserver/charms.go:122: // not identify a file, or a symlink that
resolves to a file, a 403 forbidden error
ehh fix docs.

[dimitern, 2014-03-06 17:54:51]

LGTM with some minor suggestions below

https://codereview.appspot.com/49960047/diff/60001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/49960047/diff/60001/charm/bundle_test.go#newco...
charm/bundle_test.go:77: c.Assert(manifest, gc.DeepEquals,
set.NewStrings(dummyManifest...))
I'd use jc.DeepEquals always just so in case it fails, the error message is more
friendly, but up to you.

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

https://codereview.appspot.com/49960047/diff/60001/state/apiserver/charms.go#...
state/apiserver/charms.go:126: // TODO(fwereade) 20140127 lp:1285685
Shouldn't this be
// TODO(fwereade) 2014-01-27 bug #1285685
// ...
?

https://codereview.appspot.com/49960047/diff/60001/state/apiserver/charms.go#...
state/apiserver/charms.go:341: return strings.Count(path,
string(filepath.Separator))
return strings.Count(path, "/") ? how about windows?

https://codereview.appspot.com/49960047/diff/60001/utils/zip/zip_test.go
File utils/zip/zip_test.go (right):

https://codereview.appspot.com/49960047/diff/60001/utils/zip/zip_test.go#newc...
utils/zip/zip_test.go:1: // Copyright 2011, 2012, 2013, 2014 Canonical Ltd.
2011-2014 or just 2014

[frankban, 2014-03-06 18:09:10]

Very nice! LGTM

https://codereview.appspot.com/49960047/diff/60001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/49960047/diff/60001/charm/bundle.go#newcode177
charm/bundle.go:177: // Manifest returns a sorted list of the charm's contents.
// Manifest returns a set of the charm's contents.

[frankban, 2014-03-06 18:09:11]

Very nice! LGTM

https://codereview.appspot.com/49960047/diff/60001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/49960047/diff/60001/charm/bundle.go#newcode177
charm/bundle.go:177: // Manifest returns a sorted list of the charm's contents.
// Manifest returns a set of the charm's contents.

[fwereade, 2014-03-06 18:12:09]

https://codereview.appspot.com/49960047/diff/60001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/49960047/diff/60001/charm/bundle.go#newcode177
charm/bundle.go:177: // Manifest returns a sorted list of the charm's contents.
On 2014/03/06 18:09:11, frankban wrote:
> // Manifest returns a set of the charm's contents.

Done.

https://codereview.appspot.com/49960047/diff/60001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/49960047/diff/60001/charm/bundle_test.go#newco...
charm/bundle_test.go:77: c.Assert(manifest, gc.DeepEquals,
set.NewStrings(dummyManifest...))
On 2014/03/06 17:54:51, dimitern wrote:
> I'd use jc.DeepEquals always just so in case it fails, the error message is
more
> friendly, but up to you.

Done.

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

https://codereview.appspot.com/49960047/diff/60001/state/apiserver/charms.go#...
state/apiserver/charms.go:122: // not identify a file, or a symlink that
resolves to a file, a 403 forbidden error
On 2014/03/06 17:43:48, fwereade wrote:
> ehh fix docs.

Done.

https://codereview.appspot.com/49960047/diff/60001/state/apiserver/charms.go#...
state/apiserver/charms.go:126: // TODO(fwereade) 20140127 lp:1285685
On 2014/03/06 17:54:51, dimitern wrote:
> Shouldn't this be
> // TODO(fwereade) 2014-01-27 bug #1285685
> // ...
> ?

Done.

https://codereview.appspot.com/49960047/diff/60001/state/apiserver/charms.go#...
state/apiserver/charms.go:341: return strings.Count(path,
string(filepath.Separator))
On 2014/03/06 17:54:51, dimitern wrote:
> return strings.Count(path, "/") ? how about windows?

dammit, yeah, that code used to run against the filesystem. Well spotted.

https://codereview.appspot.com/49960047/diff/60001/utils/zip/zip_test.go
File utils/zip/zip_test.go (right):

https://codereview.appspot.com/49960047/diff/60001/utils/zip/zip_test.go#newc...
utils/zip/zip_test.go:1: // Copyright 2011, 2012, 2013, 2014 Canonical Ltd.
On 2014/03/06 17:54:51, dimitern wrote:
> 2011-2014 or just 2014

Done.