reworked ClientContext to HookContext

reworked ClientContext to HookContext

HookContext now offers facilities sufficient to implement relation-get,
relation-set, relation-ids, and relation-list. Expect followups
implementing those shortly; for now, sigh, and observe the annoyingly
large amount of test changes that support what STM to be a worthwhile
simplification of the hook context as a whole.

https://code.launchpad.net/~fwereade/juju-core/jujuc-server-unit-context/+merge/117253

Requires: https://code.launchpad.net/~fwereade/juju-core/jujuc-server-relation-context/+merge/117202

(do not edit description out of merge proposal)

[fwereade, 2012-07-30 12:45:45]

Please take a look.

[rog, 2012-07-30 13:20:08]

This is looking lovely. As discussed online, I reckon HookContext reads better
than UnitContext. A few other minor comments below.

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

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:21: // tools that need this specific UnitContext).
This comment seems somewhat more involved than it could be.
Given our discussion about UnitContext vs HookContext:

// HookContext holds the context for running hooks
// for a unit.

could be sufficient?

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:39: // Relations must always contain a
*RelationContext for every relation the
i'm not sure the "must" is useful here.

// Relations contains the context for every relation
// the unit is a member of, keyed on relation id.
?

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:106: log.Printf("cannot flush context for relation
%d: %v", id, e)
i'm not sure about this conditional logging. if we're going to log the second
relation-flushed error, i think we should log the first one too.
also, the log message should identify the source.
log.Printf("jujuc/server: cannot flush context for relation %d: %v", id, e)

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/ports.go
File cmd/jujuc/server/ports.go (right):

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/ports.go#newco...
cmd/jujuc/server/ports.go:18: action   func(*portCommand) error
not sure why this changed. using the method directly seemed to work ok.

[fwereade, 2012-07-30 14:31:24]

Please take a look.

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

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:21: // tools that need this specific UnitContext).
On 2012/07/30 13:20:08, rog wrote:
> This comment seems somewhat more involved than it could be.
> Given our discussion about UnitContext vs HookContext:
> 
> // HookContext holds the context for running hooks
> // for a unit.
> 
> could be sufficient?

SGTM. Done.

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:39: // Relations must always contain a
*RelationContext for every relation the
On 2012/07/30 13:20:08, rog wrote:
> i'm not sure the "must" is useful here.
> 
> // Relations contains the context for every relation
> // the unit is a member of, keyed on relation id.
> ?

SGTM. Done.

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:106: log.Printf("cannot flush context for relation
%d: %v", id, e)
On 2012/07/30 13:20:08, rog wrote:
> i'm not sure about this conditional logging. if we're going to log the second
> relation-flushed error, i think we should log the first one too.
> also, the log message should identify the source.
> log.Printf("jujuc/server: cannot flush context for relation %d: %v", id, e)

Huh, looks like I changed that without meaning to; the intent was that we log
the errors that would otherwise be ignored. Still, logging them all is probably
better. Done.

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/ports.go
File cmd/jujuc/server/ports.go (right):

https://codereview.appspot.com/6445058/diff/1/cmd/jujuc/server/ports.go#newco...
cmd/jujuc/server/ports.go:18: action   func(*portCommand) error
On 2012/07/30 13:20:08, rog wrote:
> not sure why this changed. using the method directly seemed to work ok.

It seemed to stop working for no reason I could determine, so I invoked rational
ignorance and changed to something I knew would work so I didn't get distracted.
Will investigate.

~~~~~

cmd/jujuc/server/ports.go:76: unknown export symbol: state.(*Unit).OpenPort
cmd/jujuc/server/ports.go:88: unknown export symbol: state.(*Unit).ClosePort

It turns out that the above happens, for no reason that I can make out; keeping
as is, will go and try to construct a minimal example elsewhere.

[niemeyer, 2012-07-31 20:29:21]

Outstanding. LGTM.

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

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:21: // tools that need this specific HookContext).
Great doc.

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:31: // otherwise, its value must be a valid key into
the Relations map.
Great doc too.

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:36: // or if it is running a relation-broken hook.
OMG, I can understand everything. :-)

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:40: // of, keyed on relation id.
Ok, I'll stop now. But.. <3 it!

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:103: log.Printf("jujuc/server: failed to flush
context for relation %d: %v", id, e)
s,jujuc/server: ,,; we're getting rid of those prefixes

Also, would be good to make the message slightly more oriented towards a
non-developer.

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:113: // hooks as JUJU_RELATION and JUJU_RELATION_ID
respectively. It will panic
Nice, that's a great place for those.

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

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:102: var runHookTests = []struct {
I suggest "summary string" here, and see below..

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:110: // Missing hook is not an error.
We might move this onto the test itself, and make them more clear:

{
    summary: "Missing hook is not an error",
    relid: -1,
}, {
    summary: "Report failure to execute hook",
    relid:   -1,
    perms:   0600,
    err:     `exec: .*something-happened": permission denied`,
}, {
...
}

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:137: c.Logf("test %d", i)
This can then show a summary, which is generally quite handy to see what is
failing:

c.Logf("test %d: %s", i, t.summary)

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/juju-log.go
File cmd/jujuc/server/juju-log.go (right):

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/juju-log.go...
cmd/jujuc/server/juju-log.go:40: s := []string{c.Unit.Name()}
Seems a bit involved for what's actually going on:

relId := ""
if c.RelationId != -1 {
    _, id := c.relationIdentifiers()
    relId = " " + id
}
msg := fmt.Sprintf("%s%s: %s", c.Unit.Name(), relId, c.Message)

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/ports.go
File cmd/jujuc/server/ports.go (left):

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/ports.go#ol...
cmd/jujuc/server/ports.go:68: return c.action(unit, c.Protocol, c.Port)
Why all the changes here? Wasn't it just a matter of c.action(c.Unit,
c.Protocol, c.Port)?

[fwereade, 2012-07-31 21:54:41]

*** Submitted:

reworked ClientContext to HookContext

HookContext now offers facilities sufficient to implement relation-get,
relation-set, relation-ids, and relation-list. Expect followups
implementing those shortly; for now, sigh, and observe the annoyingly
large amount of test changes that support what STM to be a worthwhile
simplification of the hook context as a whole.

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

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

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context.go#...
cmd/jujuc/server/context.go:103: log.Printf("jujuc/server: failed to flush
context for relation %d: %v", id, e)
On 2012/07/31 20:29:21, niemeyer wrote:
> s,jujuc/server: ,,; we're getting rid of those prefixes
> 
> Also, would be good to make the message slightly more oriented towards a
> non-developer.

Done.

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

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:102: var runHookTests = []struct {
On 2012/07/31 20:29:21, niemeyer wrote:
> I suggest "summary string" here, and see below..

Done.

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:110: // Missing hook is not an error.
On 2012/07/31 20:29:21, niemeyer wrote:
> We might move this onto the test itself, and make them more clear:
> 
> {
>     summary: "Missing hook is not an error",
>     relid: -1,
> }, {
>     summary: "Report failure to execute hook",
>     relid:   -1,
>     perms:   0600,
>     err:     `exec: .*something-happened": permission denied`,
> }, {
> ...
> }

Very nice. Done.

https://codereview.appspot.com/6445058/diff/4001/cmd/jujuc/server/context_tes...
cmd/jujuc/server/context_test.go:137: c.Logf("test %d", i)
On 2012/07/31 20:29:21, niemeyer wrote:
> This can then show a summary, which is generally quite handy to see what is
> failing:
> 
> c.Logf("test %d: %s", i, t.summary)

Done.