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

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

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.

[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?

[3]

         # Queue up our wait condition, of 3 hooks firing

s/3/4/

[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.

[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.

review: Approve

« Back to merge proposal