Code review comment for lp:~chad.smith/landscape-client/ha-manager-skeleton

[1] + def run_parts(script_dir):

You have an inner run_parts method that doesn't need to exist; you can just inline the code.

[2] + def _respond_failure(self, failure, opid):

You should use landscape.lib.log.log_failure here to log the failure.

[3] _format_exception and the following code at the end of handle_change_ha_service is unnecessary:

+ except Exception, e:
+ self._respond_failure(self._format_exception(e), opid)

Instead, do

except:
    self._respond_failure(Failure(), opid)

Failure(), when constructed with no arguments, automatically grabs the "current" exception and traceback.

[4] I recommend separating _respond_failure into two different functions, one for handling failure instances and another for handling string messages.

[5] In the places where you invoke getProcessValue, I think you'll still need to provide an environment in case the script relies on basic things like PATH, etc. It should be reasonable to just pass through os.environ like you do for the getProcessOutputAndValue call.

[6]

+ def validate_exit_code(code, script):
+ if code != 0:
+ return fail(CharmScriptError(script, code))
+ else:
+ return succeed("%s succeeded." % script)

This could be rewritten a bit nicer as

if code != 0:
    raise CharmScriptError(script, code)
else:
    return "%s succeeded." % script

[7] Same for parse_output.