remove hook package; hooks are not really important at this stage

remove hook package; hooks are not really important at this stage

The new cmd/jujuc/server package contains (so far) the Context type; it
defines the server-side state with which proxied tool invocations can
interact. It is also responsible for running hooks, but that's because it
is the source of truth for much of the hook's desired environment; I'm not
*sure* RunHook will always be attached to Context, but I think that's
something to decide once we have a client.

Context shares some features with the existing hook context classes in
Python, but should not be confused with them. A Context holds all the data
that is needed to execute any jujuc tool [0], plus an Id which is not yet
used directly. It is not concerned with those variables that are purely
related to the actual execution of the hook (ie socketPath and charmDir);
therefore, those are passed separately to RunHook.

[0] Not strictly true, yet; we'll add data when we handle relation contexts,but that's all in the future for now :).

https://code.launchpad.net/~fwereade/juju/go-stateless-hook-context/+merge/97676

(do not edit description out of merge proposal)

[fwereade, 2012-03-15 15:16:15]

Please take a look.

[rog, 2012-03-15 15:29:11]

https://codereview.appspot.com/5832045/diff/1/hook/context.go
File hook/context.go (right):

https://codereview.appspot.com/5832045/diff/1/hook/context.go#newcode19
hook/context.go:19: func NewLocalContext(state *state.State, unitName string)
*Context {
are these constructors worth it?
can't we just export the members of Context - then the caller has the advantage
of being able to use field names in the constructor.

but perhaps you expect more substantial initialisation in the future?

https://codereview.appspot.com/5832045/diff/1/hook/context.go#newcode38
hook/context.go:38: func (ctx *Context) Vars() []string {
can't we just put UnitName and Relation as fields into ExecInfo?

https://codereview.appspot.com/5832045/diff/1/hook/context.go#newcode70
hook/context.go:70: func (ctx *Context) Members() []string {
i don't see the need for this method.
for one, a nil slice is an empty slice, so the implementation
would be come just "return ctx.members".

given that, why not just export the field?

[niemeyer, 2012-03-15 19:15:37]

We've covered that one online. Marked the branch as WIP just so it gets back
into view after a re-propose is made.

[fwereade, 2012-03-16 11:49:54]

Please take a look.

[rog, 2012-03-16 12:25:52]

much happier with this, thanks!

https://codereview.appspot.com/5832045/diff/4001/hook/context.go
File hook/context.go (right):

https://codereview.appspot.com/5832045/diff/4001/hook/context.go#newcode17
hook/context.go:17: ContextId   string
as discussed on IRC, i think it's a bit odd that the context id can't be taken
from the Context itself, but i'm ok with it for now.

https://codereview.appspot.com/5832045/diff/4001/hook/context.go#newcode24
hook/context.go:24: func (info *ExecInfo) Vars() []string {
i don't think this needs to be exported.

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go
File hook/context_test.go (right):

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode23
hook/context_test.go:23: func saveLog(debug bool) (*bytes.Buffer, func()) {
nice.
perhaps name the return values - the func return gave me a few moments' pause
until i saw the name "restore" being used below.

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode25
hook/context_test.go:25: buf := bytes.NewBuffer([]byte{})
buf := new(bytes.Buffer)

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode73
hook/context_test.go:73: `
this might be slightly more more elegant (and less fragile) using the actual
template package:

var hookTemplate = template.Must(template.New("").Parse(
`#!/bin/bash
printenv > {{.OutPath}}
exit {{.ExitCode}}
`))

and below (omitting error checks):

hook, err := os.Create(hookPath)
err = hookTemplate.Execute(hook, struct{OutPath string, ExitCode int}{outPath,
code})
hook.Close()

YMMV of course.

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode96
hook/context_test.go:96: for i := 0; i < len(lines); i++ {
for _, line := range lines {
    if line == sought {
        found = true
        continue
    }
}

[niemeyer, 2012-03-16 23:26:31]

For the record, we've debated live today about some ideas for reorganizing this.

[fwereade, 2012-03-17 00:12:15]

Please take a look.

https://codereview.appspot.com/5832045/diff/4001/hook/context.go
File hook/context.go (right):

https://codereview.appspot.com/5832045/diff/4001/hook/context.go#newcode17
hook/context.go:17: ContextId   string
On 2012/03/16 12:25:52, rog wrote:
> as discussed on IRC, i think it's a bit odd that the context id can't be taken
> from the Context itself, but i'm ok with it for now.

Thanks :).

https://codereview.appspot.com/5832045/diff/4001/hook/context.go#newcode24
hook/context.go:24: func (info *ExecInfo) Vars() []string {
On 2012/03/16 12:25:52, rog wrote:
> i don't think this needs to be exported.

Good point, done.

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go
File hook/context_test.go (right):

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode23
hook/context_test.go:23: func saveLog(debug bool) (*bytes.Buffer, func()) {
On 2012/03/16 12:25:52, rog wrote:
> nice.
> perhaps name the return values - the func return gave me a few moments' pause
> until i saw the name "restore" being used below.

Done.

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode25
hook/context_test.go:25: buf := bytes.NewBuffer([]byte{})
On 2012/03/16 12:25:52, rog wrote:
> buf := new(bytes.Buffer)

Done.

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode25
hook/context_test.go:25: buf := bytes.NewBuffer([]byte{})
On 2012/03/16 12:25:52, rog wrote:
> buf := new(bytes.Buffer)

Done.

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode73
hook/context_test.go:73: `
On 2012/03/16 12:25:52, rog wrote:
> this might be slightly more more elegant (and less fragile) using the actual
> template package:
> 
> var hookTemplate = template.Must(template.New("").Parse(
> `#!/bin/bash
> printenv > {{.OutPath}}
> exit {{.ExitCode}}
> `))
> 
> and below (omitting error checks):
> 
> hook, err := os.Create(hookPath)
> err = hookTemplate.Execute(hook, struct{OutPath string, ExitCode int}{outPath,
> code})
> hook.Close()
> 
> YMMV of course.

Not sure I buy the elegance argument entirely, but I'm sold on the robustness
one. Done.

https://codereview.appspot.com/5832045/diff/4001/hook/context_test.go#newcode96
hook/context_test.go:96: for i := 0; i < len(lines); i++ {
On 2012/03/16 12:25:52, rog wrote:
> for _, line := range lines {
>     if line == sought {
>         found = true
>         continue
>     }
> }

/hangs-head-in-shame

Done :).

[fwereade, 2012-03-19 11:48:22]

Please take a look.

[niemeyer, 2012-03-20 13:55:09]

LGTM

For the record:

<niemeyer> fwereade_: Sure.. I was actually going to ask you about these
branches in review
<fwereade_> niemeyer, cool
<niemeyer> fwereade_: In one of them you seem to have reverted back to kill the
whole idea?
<fwereade_> niemeyer, the hook context one?
<niemeyer> fwereade_: Yeah
<fwereade_> niemeyer, it became clear after sleeping on our discussion that it
is *not* a *hook* context
<fwereade_> niemeyer, it's a tool context
<fwereade_> niemeyer, which happens to be useful for hooks
<fwereade_> niemeyer, the hook package itself became unjustifiable; but it fits
really nicely IMO as one of a number of things in a cmd/jujuc/server package
<fwereade_> niemeyer, jujuc is the dumb main.main that calls the server package
<fwereade_> niemeyer, the server package contains the various tools implemented
as Command, and the RPC server that executes them
<fwereade_> niemeyer, it's a good place for that particular set of, er,
synergistic functionality
<niemeyer> fwereade_: Sounds reasonable
<fwereade_> niemeyer, and it'll be as convenient as anything to use once we have
an agent far enough along that it's *actually* needing to run hooks
<niemeyer> fwereade_: +1
<fwereade_> niemeyer, sweet
<fwereade_> niemeyer, well, that's the point of that change :)
<niemeyer> fwereade_: Cool, thanks

[niemeyer, 2012-03-20 14:16:42]

LGTM still, just a few extra comments I had missed:

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:36: full := fmt.Sprintf("Context<%s>: %s",
strings.Join(s, ", "), msg)
This feels a bit noisy for a user-oriented log message. Can we please have one
of these depending on the availability of information:

1) "foo"
2) "mysql/2: foo"
3) "mysql/2 relation-bar: foo"

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context_tes...
File cmd/jujuc/server/context_test.go (right):

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:84: func makeCharm(c *C, hookName string, perm
os.FileMode, code int) (string, string) {
This would benefit form a comment, and from using named results to document what
we're getting back. (string, string) is not informative.

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:90: filepath.Join(hooksDir, hookName),
os.O_CREATE|os.O_WRONLY, perm,
FWIW, that's a slightly non-conventional way to break lines. You can keep things
in the same line, as we don't have a 80 columns restriction.

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:115: func (s *ExecSuite) AssertEnv(c *C,
outPath string, env map[string]string) {
This doesn't seem to be using s.

[fwereade, 2012-03-20 15:05:36]

Please take a look.

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context.go
File cmd/jujuc/server/context.go (right):

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:36: full := fmt.Sprintf("Context<%s>: %s",
strings.Join(s, ", "), msg)
On 2012/03/20 14:16:42, niemeyer wrote:
> This feels a bit noisy for a user-oriented log message. Can we please have one
> of these depending on the availability of information:
> 
> 1) "foo"
> 2) "mysql/2: foo"
> 3) "mysql/2 relation-bar: foo"

Done.

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context_tes...
File cmd/jujuc/server/context_test.go (right):

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:84: func makeCharm(c *C, hookName string, perm
os.FileMode, code int) (string, string) {
On 2012/03/20 14:16:42, niemeyer wrote:
> This would benefit form a comment, and from using named results to document
what
> we're getting back. (string, string) is not informative.

Done.

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:90: filepath.Join(hooksDir, hookName),
os.O_CREATE|os.O_WRONLY, perm,
On 2012/03/20 14:16:42, niemeyer wrote:
> FWIW, that's a slightly non-conventional way to break lines. You can keep
things
> in the same line, as we don't have a 80 columns restriction.

Done.

https://codereview.appspot.com/5832045/diff/9001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:115: func (s *ExecSuite) AssertEnv(c *C,
outPath string, env map[string]string) {
On 2012/03/20 14:16:42, niemeyer wrote:
> This doesn't seem to be using s.

Done.

[niemeyer, 2012-03-20 19:10:18]

LGTM

https://codereview.appspot.com/5832045/diff/4015/cmd/jujuc/server/context_tes...
File cmd/jujuc/server/context_test.go (right):

https://codereview.appspot.com/5832045/diff/4015/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:96: err = hookTemplate.Execute(hook,
hookArgs{outPath, code})
Not sure if you intend to grow this, but FWIW:

err = fmt.Fprintf(hook, "#!/bin/sh\nenv > %s; exit %d", outPath, code)

[fwereade, 2012-03-20 23:13:27]

https://codereview.appspot.com/5832045/diff/4015/cmd/jujuc/server/context_tes...
File cmd/jujuc/server/context_test.go (right):

https://codereview.appspot.com/5832045/diff/4015/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:96: err = hookTemplate.Execute(hook,
hookArgs{outPath, code})
On 2012/03/20 19:10:18, niemeyer wrote:
> Not sure if you intend to grow this, but FWIW:
> 
> err = fmt.Fprintf(hook, "#!/bin/sh\nenv > %s; exit %d", outPath, code)

Heh, I had been at one stage, but I don't think that'll be called for now. Done.

[fwereade, 2012-03-20 23:57:23]

*** Submitted:

remove hook package; hooks are not really important at this stage

The new cmd/jujuc/server package contains (so far) the Context type; it
defines the server-side state with which proxied tool invocations can
interact. It is also responsible for running hooks, but that's because it
is the source of truth for much of the hook's desired environment; I'm not
*sure* RunHook will always be attached to Context, but I think that's
something to decide once we have a client.

Context shares some features with the existing hook context classes in
Python, but should not be confused with them. A Context holds all the data
that is needed to execute any jujuc tool [0], plus an Id which is not yet
used directly. It is not concerned with those variables that are purely
related to the actual execution of the hook (ie socketPath and charmDir);
therefore, those are passed separately to RunHook.

[0] Not strictly true, yet; we'll add data when we handle relation contexts,but
that's all in the future for now :).

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

[niemeyer, 2012-03-21 00:05:11]

On 2012/03/20 23:57:23, fwereade wrote:
> *** Submitted:
> 
> remove hook package; hooks are not really important at this stage

The subject was still incorrect.