Refactor charm/hooks,worker/uniter/hook

Refactor charm/hooks,worker/uniter/hook

As discussed, this splits worker/uniter/hook
module into uniter-related stuff (Info and
validation) and generic charm hooks stuff
(defined hooks kinds). Adds UnitHooks
and RelationHooks func, which return
their respective Kinds as slices. Changed
uniter code to match the code split. Added
a Meta method Hooks() to return map[string]bool
of all (possible) hooks according to the metadata.
This is needed for a follow-up CL to fix
bug #864164.

https://code.launchpad.net/~dimitern/juju-core/refactor-charm-hooks/+merge/148512

(do not edit description out of merge proposal)

[dimitern, 2013-02-14 17:10:38]

Please take a look.

[fwereade, 2013-02-14 17:54:16]

LGTM assuming the following.

https://codereview.appspot.com/7317045/diff/1/charm/meta.go
File charm/meta.go (right):

https://codereview.appspot.com/7317045/diff/1/charm/meta.go#newcode49
charm/meta.go:49: func generateRelationHooks(relName string, allHooks
map[string]bool) map[string]bool {
The map is changed in place, no need to return it.

https://codereview.appspot.com/7317045/diff/1/charm/meta.go#newcode66
charm/meta.go:66: for hookName, _ := range m.Provides {
s/, _//

https://codereview.appspot.com/7317045/diff/1/charm/meta.go#newcode69
charm/meta.go:69: for hookName, _ := range m.Requires {
s/, _//

https://codereview.appspot.com/7317045/diff/1/charm/meta.go#newcode72
charm/meta.go:72: for hookName, _ := range m.Provides {
s/, _//

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

https://codereview.appspot.com/7317045/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:6: chook "launchpad.net/juju-core/charm/hook"
You can just use "hook" here.

[rog, 2013-02-14 18:20:16]

LGTM assuming some resolution
to the following minor issues.

does this really justify a new charm/hook
package? can't the charm package itself know
about hook names?

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

https://codereview.appspot.com/7317045/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:6: chook "launchpad.net/juju-core/charm/hook"
On 2013/02/14 17:54:16, fwereade wrote:
> You can just use "hook" here.

yeah, s/chook //

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

https://codereview.appspot.com/7317045/diff/1/worker/uniter/hook/hook_test.go...
worker/uniter/hook/hook_test.go:6: uhook
"launchpad.net/juju-core/worker/uniter/hook"
i think i'd go the other way around here and alias the package that's furthest
away from this code. then "hook.X" means the same in all the uniter code.

same comment applies to the other places in uniter.

and "chook" is quite a cool name too. :-)

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

https://codereview.appspot.com/7317045/diff/1/worker/uniter/modes.go#newcode13
worker/uniter/modes.go:13: uhook "launchpad.net/juju-core/worker/uniter/hook"
suggest aliasing charm/hook instead.

[dimitern, 2013-02-15 10:09:27]

Please take a look.

https://codereview.appspot.com/7317045/diff/1/charm/meta.go
File charm/meta.go (right):

https://codereview.appspot.com/7317045/diff/1/charm/meta.go#newcode49
charm/meta.go:49: func generateRelationHooks(relName string, allHooks
map[string]bool) map[string]bool {
On 2013/02/14 17:54:16, fwereade wrote:
> The map is changed in place, no need to return it.

At first I did it in place with no return, but then I thought of append() and
did it like this. I'll change it, I agree it's better in place.

https://codereview.appspot.com/7317045/diff/1/charm/meta.go#newcode66
charm/meta.go:66: for hookName, _ := range m.Provides {
On 2013/02/14 17:54:16, fwereade wrote:
> s/, _//

Done.

https://codereview.appspot.com/7317045/diff/1/charm/meta.go#newcode69
charm/meta.go:69: for hookName, _ := range m.Requires {
On 2013/02/14 17:54:16, fwereade wrote:
> s/, _//

Done.

https://codereview.appspot.com/7317045/diff/1/charm/meta.go#newcode72
charm/meta.go:72: for hookName, _ := range m.Provides {
On 2013/02/14 17:54:16, fwereade wrote:
> s/, _//

Done.

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

https://codereview.appspot.com/7317045/diff/1/worker/uniter/hook/hook.go#newc...
worker/uniter/hook/hook.go:6: chook "launchpad.net/juju-core/charm/hook"
On 2013/02/14 18:20:16, rog wrote:
> On 2013/02/14 17:54:16, fwereade wrote:
> > You can just use "hook" here.
> 
> yeah, s/chook //

As agreed online, I renamed charm/hook to charm/hooks to avoid name clash and
changed the relevant code in the uniter as well.

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

https://codereview.appspot.com/7317045/diff/1/worker/uniter/hook/hook_test.go...
worker/uniter/hook/hook_test.go:6: uhook
"launchpad.net/juju-core/worker/uniter/hook"
On 2013/02/14 18:20:16, rog wrote:
> i think i'd go the other way around here and alias the package that's furthest
> away from this code. then "hook.X" means the same in all the uniter code.
> 
> same comment applies to the other places in uniter.
> 
> and "chook" is quite a cool name too. :-)

See above

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

https://codereview.appspot.com/7317045/diff/1/worker/uniter/modes.go#newcode13
worker/uniter/modes.go:13: uhook "launchpad.net/juju-core/worker/uniter/hook"
On 2013/02/14 18:20:16, rog wrote:
> suggest aliasing charm/hook instead.

See above

[fwereade, 2013-02-15 10:15:36]

LGTM, only trivials/remarks for consideration

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go
File charm/hooks/hooks.go (right):

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode32
charm/hooks/hooks.go:32: var UnitHooks = []Kind{
I wish we could declare this to be constant. Maybe private, with a method that
copies it?

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode40
charm/hooks/hooks.go:40: // RelationBroken contains all known relation hook
kinds.
s/Broken/Hooks/

[rog, 2013-02-15 11:17:31]

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go
File charm/hooks/hooks.go (right):

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode32
charm/hooks/hooks.go:32: var UnitHooks = []Kind{
On 2013/02/15 10:15:36, fwereade wrote:
> I wish we could declare this to be constant. Maybe private, with a method that
> copies it?

-1
constant-by-convention is fine, i think, as discussed online.

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode48
charm/hooks/hooks.go:48: // IsRelation will return true if the Kind represents a
relation hook.
convention would have this:

// IsRelation returns whether the Kind represents a relation hook.

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode57
charm/hooks/hooks.go:57: // IsUnit will return true if the Kind does represents
an unit hook.
hmm, even the relation hooks are unit hooks, and the sense of this method should
be precisely the opposite of IsRelation.
i vote for deleting it.

[dimitern, 2013-02-15 11:38:37]

Please take a look.

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go
File charm/hooks/hooks.go (right):

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode32
charm/hooks/hooks.go:32: var UnitHooks = []Kind{
On 2013/02/15 10:15:36, fwereade wrote:
> I wish we could declare this to be constant. Maybe private, with a method that
> copies it?

Done.

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode32
charm/hooks/hooks.go:32: var UnitHooks = []Kind{
On 2013/02/15 11:17:31, rog wrote:
> On 2013/02/15 10:15:36, fwereade wrote:
> > I wish we could declare this to be constant. Maybe private, with a method
that
> > copies it?
> 
> -1
> constant-by-convention is fine, i think, as discussed online.

As finally agreed online.

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode40
charm/hooks/hooks.go:40: // RelationBroken contains all known relation hook
kinds.
On 2013/02/15 10:15:36, fwereade wrote:
> s/Broken/Hooks/

Hmm.. emacs intelligent code completion at it's best :)
Done

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode48
charm/hooks/hooks.go:48: // IsRelation will return true if the Kind represents a
relation hook.
On 2013/02/15 11:17:31, rog wrote:
> convention would have this:
> 
> // IsRelation returns whether the Kind represents a relation hook.

Done.

https://codereview.appspot.com/7317045/diff/7001/charm/hooks/hooks.go#newcode57
charm/hooks/hooks.go:57: // IsUnit will return true if the Kind does represents
an unit hook.
On 2013/02/15 11:17:31, rog wrote:
> hmm, even the relation hooks are unit hooks, and the sense of this method
should
> be precisely the opposite of IsRelation.
> i vote for deleting it.

Deleted.

[dimitern, 2013-02-15 11:43:50]

Please take a look.

[rog, 2013-02-15 11:46:31]

On 2013/02/15 11:43:50, dimitern wrote:
> Please take a look.

LGTM

with some reluctance on the can't-have-a-global-variable thing. thanks for your
patience dimiter!

[dimitern, 2013-02-15 11:47:49]

*** Submitted:

Refactor charm/hooks,worker/uniter/hook

As discussed, this splits worker/uniter/hook
module into uniter-related stuff (Info and
validation) and generic charm hooks stuff
(defined hooks kinds). Adds UnitHooks
and RelationHooks func, which return
their respective Kinds as slices. Changed
uniter code to match the code split. Added
a Meta method Hooks() to return map[string]bool
of all (possible) hooks according to the metadata.
This is needed for a follow-up CL to fix
bug #864164.

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