Code review comment for lp:~jimbaker/pyjuju/new-hook-semantics-1-departed-hook

On Wed, Apr 6, 2011 at 9:40 AM, Gustavo Niemeyer <email address hidden>wrote:

> Review: Approve
> It looks good to me, but please wait for Kapil's review since he
> knows more about this area.
>
> Some minor comments as well:
>
>
> [1]
>
> + self._log.debug("%s: executing change hook",
> self._relation_name)
>
> This log message seems incorrect now. It should probably be something like
>
> "Executing hook %s" % hook_name
>
> Please do something similar to the other log messages.
>
>
Cleaned up

>
> [2]
>
> 51 + if not self._error_handler:
> 52 + raise
>
> It's not entirely clear to me that this is correct. Do we really want
> to ignore the other hook when this happens?
>
>
Kapil comments on this in his review; additionally, there has been no change
in this logic except that it has now part of an enclosing loop to
accommodate the fact that one or two hooks will be invoked for a change
event

>
> [3]
>
> # Queue up our wait condition, of 3 hooks firing
>
> s/3/4/
>

Changed

>
> [4]
>
> 84 + "app-relation-changed",
> 85 + "app-relation-changed",
> "app-relation-departed"])
> (...)
> 96 + "start",
> 97 + "app-relation-changed", "app-relation-departed",
> 98 + "app-relation-changed"])
>
> Please organize the lists evenly.
>
>
I reformatted this and also added comments to make more clear why I was
doing this organization to begin with, namely to show that the two hooks on
one line were actually resulting from one event.

>
> [5]
>
>
> + ("#!/bin/bash\n" "echo departed >> %s\n" % (file_path)))
>
> Please remove the parenthesis around file_path in both cases. They're
> are not doing anything.
>

Done