|
|
|
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)
Patch Set 1 #
Total comments: 4
Patch Set 2 : charm: add Bundle.Manifest #
Total comments: 4
Patch Set 3 : charm: add Bundle.Manifest #Patch Set 4 : charm: add Bundle.Manifest #
Total comments: 12
Total messages: 11
|
fwereade
Please take a look.
|
11 years, 11 months ago (2014年01月15日 16:48:30 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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?
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.
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!
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.
Please take a look.
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.
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
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.
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.
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.