Code review comment for lp:~gnuoy/charm-helpers/status-set

Revision history for this message
Stuart Bishop (stub) wrote :

Looks good. Would have looked even better if I had seen it before repeating your work (see https://code.launchpad.net/~stub/charm-helpers/status-set/+merge/258618 ).

One minor issue - you need to add 'universal_newlines=True' to the subprocess.check_output command or you will get back a byte string under Python3.

I'm approving your work, pending the Py3 fix. However, before landing you might want to check out my branch and see if you prefer my approach. The major departure is that I chose to make status_set implicitly terminate on waiting and blocked, which also allowed me to put the unit into an error state as a fallback if the charm attempts to set blocked and status-set is unavailable. However, I just realized that this will not play well with Config's implicit saves so more work would be needed.

review: Approve

« Back to merge proposal