Rudimentary hook execution functionality

https://code.launchpad.net/~fwereade/juju/go-hook-package/+merge/96166

(do not edit description out of merge proposal)

[fwereade, 2012-03-06 16:13:58]

Please take a look.

[fwereade, 2012-03-06 16:16:46]

Please take a look.

[niemeyer, 2012-03-12 18:33:33]

https://codereview.appspot.com/5753057/diff/2002/hook/exec.go
File hook/exec.go (right):

https://codereview.appspot.com/5753057/diff/2002/hook/exec.go#newcode17
hook/exec.go:17: var JUJUC_PATH = "/usr/bin/jujuc"
If nothing else, this shouldn't be exported as the responsibility to define
where the command lives doesn't belong to this package, so at least
s/JUJUC_PATH/jujucPath/

https://codereview.appspot.com/5753057/diff/2002/hook/exec.go#newcode98
hook/exec.go:98: func Exec(ctx Context, hookName string) error {
I'm missing some design background for what's above this method. It feels like
all we need is the hook.Context type, and this function. Why is all of the
previous functionality necessary? Why is it creating and removing symlinks on
demand, and why the separation of Env from Context?

[fwereade, 2012-03-13 12:56:40]

Please take a look.

[niemeyer, 2012-03-13 22:05:40]

That's much better, thanks William. A few additional details:

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go
File hook/exec.go (right):

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go#newcode12
hook/exec.go:12: // Env exposes the values of the environment variables that are
always expected
I'd prefer to invert it, and keep Context (or some other name) rather than Env,
to avoid the name clash with the other environment types we have.

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go#newcode16
hook/exec.go:16: ContextId() string
Why the change from fields to methods? All the environments must expose a
consistent view to the hooks, which means the set of environment variables must
be the same. I hope we only ever need one of those.

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go#newcode17
hook/exec.go:17: AgentSock() string
s/Sock/Socket/

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go#newcode20
hook/exec.go:20: Vars() map[string]string
Do we need this as a method? Exec can take the Env/Context/Whatever and build
the list of variables internally via the vars function. Anything we need to put
in such a bag of data, we can also put more properly in a field here.

[rog, 2012-03-14 08:39:55]

LGTM with one minor query.

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go
File hook/exec.go (right):

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go#newcode55
hook/exec.go:55: stat, err := os.Stat(path)
is it worth calling Stat here? can't we just rely on the error from calling Run?

[fwereade, 2012-03-14 15:11:17]

Please take a look.

[rog, 2012-03-14 15:58:21]

https://codereview.appspot.com/5753057/diff/14001/hook/exec.go
File hook/exec.go (right):

https://codereview.appspot.com/5753057/diff/14001/hook/exec.go#newcode17
hook/exec.go:17: Flush() error
i'm not sure why this method needs to be called by Exec.
the caller of Exec (the creator of the context) could do that just as well
AFAICS. then Vars could be a field inside ExecInfo (ExtraVars?), and the code in
this file becomes more direct.

[fwereade, 2012-03-14 17:31:48]

Please take a look.

[niemeyer, 2012-03-14 17:41:46]

LGTM, sorry for the trouble on this one.

[rog, 2012-03-14 17:43:11]

LGTM

https://codereview.appspot.com/5753057/diff/17001/hook/exec.go
File hook/exec.go (right):

https://codereview.appspot.com/5753057/diff/17001/hook/exec.go#newcode38
hook/exec.go:38: func isImportant(err error) bool {
i think i'd probably name this function in a more specific way, isNotExist for
example, but i don't mind much.

https://codereview.appspot.com/5753057/diff/17001/hook/exec.go#newcode46
hook/exec.go:46: // Exec executes the named hook in the environment defined by
ctx and info.
s/by ctx//

[fwereade, 2012-03-14 17:54:48]

*** Submitted:

Rudimentary hook execution functionality

R=niemeyer, rog
CC=
https://codereview.appspot.com/5753057

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go
File hook/exec.go (right):

https://codereview.appspot.com/5753057/diff/6001/hook/exec.go#newcode55
hook/exec.go:55: stat, err := os.Stat(path)
On 2012/03/14 08:39:55, rog wrote:
> is it worth calling Stat here? can't we just rely on the error from calling
Run?

I don't want to have to distinguish between different kinds of errors out of
Run() to detect the "it's ok, hook didn't exist, that's perfectly normal" case;
and then, since we've got the stat already, we may as well check and bomb out
early if we know it won't be executable.