tweaks to make Uniter easier

tweaks to make Uniter easier

* added hook.Kind.IsRelation()
* made hook.StateFile.Read return a State, not a *State

https://code.launchpad.net/~fwereade/juju-core/uniter-support-tweaks/+merge/120293

Requires: https://code.launchpad.net/~fwereade/juju-core/uniter-charm-package/+merge/120200

(do not edit description out of merge proposal)

[fwereade, 2012-08-19 13:04:08]

Please take a look.

[niemeyer, 2012-08-20 15:57:16]

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

https://codereview.appspot.com/6454169/diff/1/charm/bundle_test.go#newcode63
charm/bundle_test.go:63: other := filepath.Join(path, "arbitrary")
Can we have this in a different test properly named and commented? It doesn't
look like the new content here is adding anything to the above test.

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

https://codereview.appspot.com/6454169/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:82: if ctxId != ctx.Id {
Nothing here seems to make much sense to me. The comment says it verifies
something against itself, and it returns a closure that does something extremely
trivial, and then calls something else. This isn't looking right.

https://codereview.appspot.com/6454169/diff/1/worker/uniter/charm/charm.go
File worker/uniter/charm/charm.go (right):

https://codereview.appspot.com/6454169/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:25: UpgradingForced Status = "upgrading-forced"
I'm not sure if we talked about this and I forgot, but I still don't understand
why we have "forced" as part of the status? Why does the charm directory care?

https://codereview.appspot.com/6454169/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:39: func (s Status) IsChange() bool {
I suggest

    s != Installed 

at the call site instead of introducing this method.

https://codereview.appspot.com/6454169/diff/1/worker/uniter/hook/hook.go
File worker/uniter/hook/hook.go (right):

https://codereview.appspot.com/6454169/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:61: // IsCharmChange will return true if the Kind
represents a hook associated
IsRelation sounds clear, but this doesn't sound like a good building block to
have in the API of Kind. Any hook can change the charm directory, and at the
same time neither of these hooks are actually involved in *changing* the charm
directory (they may be completely empty). It sounds like this is describing a
very particular feature of another piece of the puzzle that doesn't belong here.

[fwereade, 2012-08-20 17:12:26]

Bit of pushback on HookContext.CmdGetter; others SGTM, thanks.

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

https://codereview.appspot.com/6454169/diff/1/charm/bundle_test.go#newcode63
charm/bundle_test.go:63: other := filepath.Join(path, "arbitrary")
On 2012/08/20 15:57:16, niemeyer wrote:
> Can we have this in a different test properly named and commented? It doesn't
> look like the new content here is adding anything to the above test.

Sure, I'm happy to split them up a bit.

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

https://codereview.appspot.com/6454169/diff/1/cmd/jujuc/server/context.go#new...
cmd/jujuc/server/context.go:82: if ctxId != ctx.Id {
On 2012/08/20 15:57:16, niemeyer wrote:
> Nothing here seems to make much sense to me. The comment says it verifies
> something against itself, and it returns a closure that does something
extremely
> trivial, and then calls something else. This isn't looking right.

Hm, on reflection, I think it is right(ish) -- it's better to have a single
weird little method on server.HookContext, in the service of clean integration
with server.Server, than it is to have the same glue code dropped in the middle
of Uniter.runHook.

There's another option, which is surely correct in the long term, which would be
to make the Uniter maintain a single Server for its whole lifetime, rather then
creating one per context in the interest of simplicity.

This would be good because it would open a path towards running hook commands
outside hooks; but, IMO, it's premature, because all we need right now is to
have a server running whenever a hook is running, and we don't see any benefit
from the extra complexity. Sane?

https://codereview.appspot.com/6454169/diff/1/worker/uniter/charm/charm.go
File worker/uniter/charm/charm.go (right):

https://codereview.appspot.com/6454169/diff/1/worker/uniter/charm/charm.go#ne...
worker/uniter/charm/charm.go:39: func (s Status) IsChange() bool {
On 2012/08/20 15:57:16, niemeyer wrote:
> I suggest
> 
>     s != Installed 
> 
> at the call site instead of introducing this method.

Yeah, this could live elsewhere, probably even exactly as you suggest.

https://codereview.appspot.com/6454169/diff/1/worker/uniter/hook/hook.go
File worker/uniter/hook/hook.go (right):

https://codereview.appspot.com/6454169/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:61: // IsCharmChange will return true if the Kind
represents a hook associated
On 2012/08/20 15:57:16, niemeyer wrote:
> IsRelation sounds clear, but this doesn't sound like a good building block to
> have in the API of Kind. Any hook can change the charm directory, and at the
> same time neither of these hooks are actually involved in *changing* the charm
> directory (they may be completely empty). It sounds like this is describing a
> very particular feature of another piece of the puzzle that doesn't belong
here.

Yeah, this can/should probably move too.

[fwereade, 2012-08-20 23:24:02]

Please take a look.

[niemeyer, 2012-08-20 23:24:46]

LGTM

[fwereade, 2012-08-20 23:27:44]

*** Submitted:

tweaks to make Uniter easier

* added hook.Kind.IsRelation()
* made hook.StateFile.Read return a State, not a *State

R=niemeyer
CC=
https://codereview.appspot.com/6454169