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
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 service. py:56: return self._output_path
juju/lib/
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 service. py:72: if "--pidfile" not in command:
juju/lib/
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 service. py:33: class ServiceManager( object) :
juju/lib/
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 service. py:55: return "/tmp/%s.output" % self._name
juju/lib/
this should use the data dir for output.
https:/ /codereview. appspot. com/6404052/ diff/2001/ juju/lib/ service. py#newcode139 service. py:139: return False
juju/lib/
nice
https:/ /codereview. appspot. com/6404052/ diff/2001/ juju/lib/ tests/test_ service. py tests/test_ service. py (right):
File juju/lib/
https:/ /codereview. appspot. com/6404052/ diff/2001/ juju/lib/ tests/test_ service. py#newcode45 tests/test_ service. py:45:
juju/lib/
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 local/agent. py (right):
File juju/providers/
https:/ /codereview. appspot. com/6404052/ diff/2001/ juju/providers/ local/agent. py#newcode54 local/agent. py:54: pidfile = filename( juju_unit_ namespace)
juju/providers/
get_temp_
these can go into the data-dir instead of /tmp, and can be named instead
of mkstemp
https:/ /codereview. appspot. com/6404052/