|
|
|
Remove LXC restart partial support
This branch includes the bug fix (and a few trivials). The extra checks and features are in
the history but not included here.
https://code.launchpad.net/~bcsaller/juju/lxc-repairs/+merge/115206
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove LXC restart partial support #
Total comments: 6
Total messages: 4
|
bcsaller
Please take a look.
|
13 years, 6 months ago (2012年07月16日 18:55:51 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
Please take a look.
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.... 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.p... 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
Changes made, test add, merging. 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.... juju/lib/tests/test_service.py:45: On 2012年07月26日 13:03:13, hazmat wrote: > there should also be a real tests here, twistd has several builtin services. Added test using twistd to spawn a webserver, some of the arg handling of twistd is more specific than the manager expected, this was corrected in the code, the result is more flexible.