Code review comment for lp:~bcsaller/pyjuju/lxc-repairs

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

lgtm, with two minors.

https://codereview.appspot.com/6404052/diff/1/juju/lib/service.py
File juju/lib/service.py (right):

https://codereview.appspot.com/6404052/diff/1/juju/lib/service.py#newcode56
juju/lib/service.py:56: return self._output_path
feels like this should be required, if pid file is. hardcoded /tmp files
are a red flag.

https://codereview.appspot.com/6404052/diff/1/juju/lib/service.py#newcode72
juju/lib/service.py:72: if "--pidfile" not in command:
class should probably get renamed twisted daemon service, or class
string documentation of this the parameter requirement.

https://codereview.appspot.com/6404052/diff/2001/juju/lib/service.py
File juju/lib/service.py (right):

https://codereview.appspot.com/6404052/diff/2001/juju/lib/service.py#newcode33
juju/lib/service.py:33: class ServiceManager(object):
naming nitpick, service manager is a bit ambiguious, and sounds like its
managing multiple services. Given the description it seems more like an
AgentService.

https://codereview.appspot.com/6404052/diff/2001/juju/lib/service.py#newcode55
juju/lib/service.py:55: return "/tmp/%s.output" % self._name
this should use the data dir for output.

https://codereview.appspot.com/6404052/diff/2001/juju/lib/service.py#newcode139
juju/lib/service.py:139: return False
nice

https://codereview.appspot.com/6404052/diff/2001/juju/lib/tests/test_service.py
File juju/lib/tests/test_service.py (right):

https://codereview.appspot.com/6404052/diff/2001/juju/lib/tests/test_service.py#newcode45
juju/lib/tests/test_service.py:45:
there should also be a real tests here, twistd has several builtin
services.

https://codereview.appspot.com/6404052/diff/2001/juju/providers/local/agent.py
File juju/providers/local/agent.py (right):

https://codereview.appspot.com/6404052/diff/2001/juju/providers/local/agent.py#newcode54
juju/providers/local/agent.py:54: pidfile =
get_temp_filename(juju_unit_namespace)
these can go into the data-dir instead of /tmp, and can be named instead
of mkstemp

https://codereview.appspot.com/6404052/

« Back to merge proposal