charm: Fix bug #864164 - non-exec hooks

charm: Fix bug #864164 - non-exec hooks

Changed ExpandTo() and BundleTo() to set
executable permissions while processing
charm hook files. In the case of BundleTo(),
as mentioned in the bug, a warning is issued
to the log about this, while in ExpandTo()
it happens silently.
Permissions of hooks are set executable by owner.
Added tests and a testing charm with all hooks
defined.

https://code.launchpad.net/~dimitern/juju-core/bug-864164-executable-hooks/+merge/148698

(do not edit description out of merge proposal)

[dimitern, 2013-02-15 12:38:36]

Please take a look.

[fwereade, 2013-02-15 12:57:20]

On top of what I mention below, I'd really like some tests that we (1) don't
change non-hooks in the hooks dir and (2) don't pay any attention to missing
hooks. Otherwise, looking good, thanks.

https://codereview.appspot.com/7305096/diff/1/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/7305096/diff/1/charm/bundle_test.go#newcode89
charm/bundle_test.go:89: c.Assert(dir.Path, Equals, path)
I kinda feel that this block obfuscates what we're really testing here.

https://codereview.appspot.com/7305096/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7305096/diff/1/charm/dir.go#newcode175
charm/dir.go:175: charmName := filepath.Base(zp.root)
This isn't necessarily the charm name -- you need to get that from the Meta.
(But: do we definitely need to log that here anyway? I'd be inclined to log what
charm we were bundling somewhere before this point and allow the reader to infer
context.)

https://codereview.appspot.com/7305096/diff/1/charm/dir.go#newcode195
charm/dir.go:195: if setExec {
Could we just figure out setExec at this point?

https://codereview.appspot.com/7305096/diff/1/charm/dir_test.go
File charm/dir_test.go (right):

https://codereview.appspot.com/7305096/diff/1/charm/dir_test.go#newcode136
charm/dir_test.go:136: c.Assert(tlog, Matches, `(?m)`+expectHooks)
These tests seem a little bit weak -- they're testing the logging in quite a
casual way, which is ok, but don't seem to verify that the bits have actually
been set. I think there's some code already in here that expands a bundle
without passing through ExpandTo -- if we could use that to check the real
perms, I think that would be best.

https://codereview.appspot.com/7305096/diff/1/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/7305096/diff/1/charm/repo_test.go#newcode342
charm/repo_test.go:342: c.Assert(c.GetTestLog(), Not(Matches), "(?m).* JUJU
charm: WARNING: failed to load charm.*")
Probably best to just fix "dummy" if we can figure out how to.

[dimitern, 2013-02-15 13:35:08]

Please take a look.

https://codereview.appspot.com/7305096/diff/1/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/7305096/diff/1/charm/bundle_test.go#newcode89
charm/bundle_test.go:89: c.Assert(dir.Path, Equals, path)
On 2013/02/15 12:57:20, fwereade wrote:
> I kinda feel that this block obfuscates what we're really testing here.
I agree, these asserts do not bring any value, I'll remove them.

https://codereview.appspot.com/7305096/diff/1/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7305096/diff/1/charm/dir.go#newcode175
charm/dir.go:175: charmName := filepath.Base(zp.root)
On 2013/02/15 12:57:20, fwereade wrote:
> This isn't necessarily the charm name -- you need to get that from the Meta.
> (But: do we definitely need to log that here anyway? I'd be inclined to log
what
> charm we were bundling somewhere before this point and allow the reader to
infer
> context.)

As agreed online, I'll use meta.Name for charm name and in addition, check if
the permissions are wrong before setting them and issuing the warning.

https://codereview.appspot.com/7305096/diff/1/charm/dir.go#newcode195
charm/dir.go:195: if setExec {
On 2013/02/15 12:57:20, fwereade wrote:
> Could we just figure out setExec at this point?

As agreed, I'll move the checks here.

https://codereview.appspot.com/7305096/diff/1/charm/dir_test.go
File charm/dir_test.go (right):

https://codereview.appspot.com/7305096/diff/1/charm/dir_test.go#newcode136
charm/dir_test.go:136: c.Assert(tlog, Matches, `(?m)`+expectHooks)
On 2013/02/15 12:57:20, fwereade wrote:
> These tests seem a little bit weak -- they're testing the logging in quite a
> casual way, which is ok, but don't seem to verify that the bits have actually
> been set. I think there's some code already in here that expands a bundle
> without passing through ExpandTo -- if we could use that to check the real
> perms, I think that would be best.

I'll add additional checks for permissions.

https://codereview.appspot.com/7305096/diff/1/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/7305096/diff/1/charm/repo_test.go#newcode342
charm/repo_test.go:342: c.Assert(c.GetTestLog(), Not(Matches), "(?m).* JUJU
charm: WARNING: failed to load charm.*")
On 2013/02/15 12:57:20, fwereade wrote:
> Probably best to just fix "dummy" if we can figure out how to.
As discussed online, this is due to a mistake on my end - not checking before
setting permissions, so the warning will always be issued. I'll fix that and
revert this test how it was.

[fwereade, 2013-02-15 14:30:50]

looking nice and clean,but a couple more comments...

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode227
charm/bundle.go:227: hookName := filepath.Base(cleanName)
Sorry, only just spotted -- won't this find everything under the hooks dir, not
just the stuff directly inside it?

https://codereview.appspot.com/7305096/diff/8001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/bundle_test.go#newcode65
charm/bundle_test.go:65: bundlePath := testing.Charms.BundlePath(c.MkDir(),
"all-hooks")
Don't you need to bundle this via zip rather than BundleTo to verify ExpandTo
behaviour?

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go#newcode187
charm/dir.go:187: hookName := filepath.Base(relpath)
See ExpandTo comment?

[rog, 2013-02-15 15:26:42]

looks good. a few comments below.

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode187
charm/bundle.go:187: func (b *Bundle) expand(hooks map[string]bool, dir string,
zfile *zip.File) error {
with the new argument, this function isn't so obvious any more. how about:
// expand unpacks a charm's zip file into the given directory.
// The hooks map holds all the possible hook names in the
// charm.
?

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode227
charm/bundle.go:227: hookName := filepath.Base(cleanName)
On 2013/02/15 14:30:50, fwereade wrote:
> Sorry, only just spotted -- won't this find everything under the hooks dir,
not
> just the stuff directly inside it?

+1

i think the above test should be

    if filepath.IsDir(cleanName) == "hooks" {

(aside: we're gonna have to think hard about filepath vs path when moving to
support Windows).

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode228
charm/bundle.go:228: if _, ok := hooks[hookName]; mode&os.ModeDir == 0 && ok {
i think i'd be more conservative and use
mode&os.ModeType == 0

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode229
charm/bundle.go:229: // Bug #864164: Set all hooks executable (by owner)
No need to reference a bug number - it's not a bug any more!

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go#newcode189
charm/dir.go:189: hooks := zp.meta.Hooks()
if you stored the hooks inside zipPackage, you wouldn't have to regenerate the
possible hook list for each hook file.
in fact, if you changed the error message slightly, we wouldn't need the meta at
all.

how about:
log.Printf("charm: WARNING: making %q executable in charm", path)

i *think* that the user will find everything they need to know in the full path
name (including the charm name)

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go#newcode191
charm/dir.go:191: log.Printf("charm: WARNING: setting hook %q in charm %q to
executable.", hookName, charmName)
aside: we should really print these kinds of log messages by default.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go
File charm/dir_test.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode109
charm/dir_test.go:109: orig := log.Target
pity we can't just use LoggingSuite here. something to consider for the future.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode112
charm/dir_test.go:112: var hooks []string
hooks := []string{"install", ....}

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode113
charm/dir_test.go:113: for _, hookName := range []string{"install", "start",
"config-changed",
d

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode134
charm/dir_test.go:134: // we need to negate this with (?m) - multiline mode.
alternatively, and more readable to vanilla-regexp old fogeys like me, you could
use (.|\n)*

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode136
charm/dir_test.go:136: c.Assert(tlog, Matches, `(?m)`+expectHooks)
i'm not sure this tests what you need to test.
it checks that at least one of the hooks is mentioned in the log, but i think
you probably want to check that every hook is mentioned in the log with the
WARNING message.

something like this, perhaps:
for _, hook := range hooks {
    c.Assert(tlog, Matches, `(.|\n)*JUJU charm: WARNING: setting hook "`+hook+`"
in charm "all-hooks" to executable(.|\n)*`)`)
}

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode151
charm/dir_test.go:151: if strings.HasPrefix(cleanName, "hooks") {
i think i'd use the same filepath.Dir check suggested earlier, and perhaps even
include a directory inside the all-hooks charm to verify that files in it are
unaffected.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode154
charm/dir_test.go:154: perms := zfile.Mode() & 0777
i don't think the mask is necessary here.

[dimitern, 2013-02-15 15:28:21]

Please take a look.

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode227
charm/bundle.go:227: hookName := filepath.Base(cleanName)
On 2013/02/15 14:30:50, fwereade wrote:
> Sorry, only just spotted -- won't this find everything under the hooks dir,
not
> just the stuff directly inside it?

Right, thanks for noticing. I fixed this here and in dir.go.

https://codereview.appspot.com/7305096/diff/8001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/bundle_test.go#newcode65
charm/bundle_test.go:65: bundlePath := testing.Charms.BundlePath(c.MkDir(),
"all-hooks")
On 2013/02/15 14:30:50, fwereade wrote:
> Don't you need to bundle this via zip rather than BundleTo to verify ExpandTo
> behaviour?

Right, done.

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go#newcode187
charm/dir.go:187: hookName := filepath.Base(relpath)
On 2013/02/15 14:30:50, fwereade wrote:
> See ExpandTo comment?

Done.

[dimitern, 2013-02-15 16:13:42]

Please take a look.

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode187
charm/bundle.go:187: func (b *Bundle) expand(hooks map[string]bool, dir string,
zfile *zip.File) error {
On 2013/02/15 15:26:42, rog wrote:
> with the new argument, this function isn't so obvious any more. how about:
> // expand unpacks a charm's zip file into the given directory.
> // The hooks map holds all the possible hook names in the
> // charm.
> ?

Done.

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode227
charm/bundle.go:227: hookName := filepath.Base(cleanName)
On 2013/02/15 15:26:42, rog wrote:
> On 2013/02/15 14:30:50, fwereade wrote:
> > Sorry, only just spotted -- won't this find everything under the hooks dir,
> not
> > just the stuff directly inside it?
> 
> +1
> 
> i think the above test should be
> 
>     if filepath.IsDir(cleanName) == "hooks" {
> 
> (aside: we're gonna have to think hard about filepath vs path when moving to
> support Windows).

Changed with filepath.Dir(cleanName) == "hooks"

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode228
charm/bundle.go:228: if _, ok := hooks[hookName]; mode&os.ModeDir == 0 && ok {
On 2013/02/15 15:26:42, rog wrote:
> i think i'd be more conservative and use
> mode&os.ModeType == 0

Done.

https://codereview.appspot.com/7305096/diff/8001/charm/bundle.go#newcode229
charm/bundle.go:229: // Bug #864164: Set all hooks executable (by owner)
On 2013/02/15 15:26:42, rog wrote:
> No need to reference a bug number - it's not a bug any more!

Done.

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go#newcode189
charm/dir.go:189: hooks := zp.meta.Hooks()
On 2013/02/15 15:26:42, rog wrote:
> if you stored the hooks inside zipPackage, you wouldn't have to regenerate the
> possible hook list for each hook file.
> in fact, if you changed the error message slightly, we wouldn't need the meta
at
> all.
> 
> how about:
> log.Printf("charm: WARNING: making %q executable in charm", path)
> 
> i *think* that the user will find everything they need to know in the full
path
> name (including the charm name)

Done.

https://codereview.appspot.com/7305096/diff/8001/charm/dir.go#newcode191
charm/dir.go:191: log.Printf("charm: WARNING: setting hook %q in charm %q to
executable.", hookName, charmName)
On 2013/02/15 15:26:42, rog wrote:
> aside: we should really print these kinds of log messages by default.
We had a discussion about how logging is crack right now, but I think it's
better to log it here, and the bug mentions that as well.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go
File charm/dir_test.go (right):

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode109
charm/dir_test.go:109: orig := log.Target
On 2013/02/15 15:26:42, rog wrote:
> pity we can't just use LoggingSuite here. something to consider for the
future.
I did this the same way as repo_test.go does it.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode112
charm/dir_test.go:112: var hooks []string
On 2013/02/15 15:26:42, rog wrote:
> hooks := []string{"install", ....}

Done.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode113
charm/dir_test.go:113: for _, hookName := range []string{"install", "start",
"config-changed",
On 2013/02/15 15:26:42, rog wrote:
> d

Done.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode134
charm/dir_test.go:134: // we need to negate this with (?m) - multiline mode.
On 2013/02/15 15:26:42, rog wrote:
> alternatively, and more readable to vanilla-regexp old fogeys like me, you
could
> use (.|\n)*
Done.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode136
charm/dir_test.go:136: c.Assert(tlog, Matches, `(?m)`+expectHooks)
On 2013/02/15 15:26:42, rog wrote:
> i'm not sure this tests what you need to test.
> it checks that at least one of the hooks is mentioned in the log, but i think
> you probably want to check that every hook is mentioned in the log with the
> WARNING message.
> 
> something like this, perhaps:
> for _, hook := range hooks {
>     c.Assert(tlog, Matches, `(.|\n)*JUJU charm: WARNING: setting hook
"`+hook+`"
> in charm "all-hooks" to executable(.|\n)*`)`)
> }

Done.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode151
charm/dir_test.go:151: if strings.HasPrefix(cleanName, "hooks") {
On 2013/02/15 15:26:42, rog wrote:
> i think i'd use the same filepath.Dir check suggested earlier, and perhaps
even
> include a directory inside the all-hooks charm to verify that files in it are
> unaffected.

There is a subdir in all-hooks indeed. I'll change the the HasPrefix call to
filepath.Dir() == "hooks", as I did in the other places.

https://codereview.appspot.com/7305096/diff/8001/charm/dir_test.go#newcode154
charm/dir_test.go:154: perms := zfile.Mode() & 0777
On 2013/02/15 15:26:42, rog wrote:
> i don't think the mask is necessary here.

Done.

[fwereade, 2013-02-15 16:34:33]

LGTM

https://codereview.appspot.com/7305096/diff/17001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7305096/diff/17001/charm/bundle.go#newcode230
charm/bundle.go:230: // Process only hooks directly inside hooks/
Nice -- but not sure you even need this comment :)

https://codereview.appspot.com/7305096/diff/17001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/7305096/diff/17001/charm/bundle_test.go#newcode68
charm/bundle_test.go:68: func (s *BundleSuite) prepareBundle(c *C, charmDir
*charm.Dir, bundlePath string) {
Could have sworn there was already something like this, but if not sgtm

https://codereview.appspot.com/7305096/diff/17001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7305096/diff/17001/charm/dir.go#newcode186
charm/dir.go:186: // Process only hooks directly inside hooks/
Similarly

https://codereview.appspot.com/7305096/diff/17001/charm/meta.go
File charm/meta.go (right):

https://codereview.appspot.com/7305096/diff/17001/charm/meta.go#newcode71
charm/meta.go:71: for hookName := range m.Peers {
Good catch, thank you

[dimitern, 2013-02-15 17:56:15]

*** Submitted:

charm: Fix bug #864164 - non-exec hooks

Changed ExpandTo() and BundleTo() to set
executable permissions while processing
charm hook files. In the case of BundleTo(),
as mentioned in the bug, a warning is issued
to the log about this, while in ExpandTo()
it happens silently.
Permissions of hooks are set executable by owner.
Added tests and a testing charm with all hooks
defined.

R=fwereade, rog
CC=
https://codereview.appspot.com/7305096

https://codereview.appspot.com/7305096/diff/17001/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/7305096/diff/17001/charm/bundle.go#newcode230
charm/bundle.go:230: // Process only hooks directly inside hooks/
On 2013/02/15 16:34:33, fwereade wrote:
> Nice -- but not sure you even need this comment :)

Done.

https://codereview.appspot.com/7305096/diff/17001/charm/bundle_test.go
File charm/bundle_test.go (right):

https://codereview.appspot.com/7305096/diff/17001/charm/bundle_test.go#newcode68
charm/bundle_test.go:68: func (s *BundleSuite) prepareBundle(c *C, charmDir
*charm.Dir, bundlePath string) {
On 2013/02/15 16:34:33, fwereade wrote:
> Could have sworn there was already something like this, but if not sgtm
I couldn't quite find anything close enough to how bundles are created, to reuse
it.

https://codereview.appspot.com/7305096/diff/17001/charm/dir.go
File charm/dir.go (right):

https://codereview.appspot.com/7305096/diff/17001/charm/dir.go#newcode186
charm/dir.go:186: // Process only hooks directly inside hooks/
On 2013/02/15 16:34:33, fwereade wrote:
> Similarly

Done.