Code review comment for lp://staging/~fwereade/pyjuju/upstartify-services

+1, and approving again with just the following minors. Also this should have the real process coverage that Kapil raised that was finding issues around paths and parent dirs.

[2]

-def run_lxc_tests():
+def skip_sudo_tests():
     if os.environ.get("TEST_LXC"):
- return None
- return "TEST_LXC=1 to include lxc tests"
+ return False
+ return "TEST_LXC=1 to include lxc tests (and others which use sudo)"

Maybe we should rename this env var so it reflects the more general usage?

[3]

+def uses_sudo(f):
+ f.skip = skip_sudo_tests()
+ return f

Simple but nice decorator!

[4]

+def _sleep(secs):
+ d = Deferred()
+ reactor.callLater(secs, d.callback, None)
+ return d
+

Use juju.lib.twistutils.sleep instead

[5]

+ @inlineCallbacks
+ def _trash_output(self):
+ if os.path.exists(self.output_path):
+ yield self._call("rm", self.output_path)
+

Should be no need to have a single file removal to be done async; just use os.unlink as used elsewhere.