|
|
|
juju/control should be aware of subordinates
Includes support for add-unit, deploy, status, remove-unit
https://code.launchpad.net/~bcsaller/juju/subordinate-control-status/+merge/100489
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : juju/control should be aware of subordinates #
Total comments: 7
Total messages: 4
|
bcsaller
Please take a look.
|
13 years, 9 months ago (2012年04月02日 18:06:11 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
Please take a look.
This looks good. The refactoring of status isn't complete though.. it needs some more docs, this area of the codebase is frequently touched, and it needs more introduction to what the responsibility of the various methods are, esp. given their use of shared instance state variables in a functional style. https://codereview.appspot.com/5970068/diff/2002/juju/control/status.py File juju/control/status.py (right): https://codereview.appspot.com/5970068/diff/2002/juju/control/status.py#newco... juju/control/status.py:136: s = StatusCommand(client, provider, log) it shouldn't exist if its only purpose is for existing tests.. i'd just update the tests to the new api.. that's part of the refactoring, else its just dead code. https://codereview.appspot.com/5970068/diff/2002/juju/control/status.py#newco... juju/control/status.py:155: self.service_manager = ServiceStateManager(client) these where documented in a self referential way post the diff, but saying 'service_name' to 'state' isn't very clear, if its referring to the actual state object (unlikely), or what structure the dictionary termed state has. https://codereview.appspot.com/5970068/diff/2002/juju/control/status.py#newco... juju/control/status.py:252: def _process_units(self, service, relations, rel_svc_map): this should have some docs, what are the params https://codereview.appspot.com/5970068/diff/2002/juju/control/status.py#newco... juju/control/status.py:271: u = self.unit_data[unit.unit_name] = dict() this method could use some documentation, what are the params https://codereview.appspot.com/5970068/diff/2002/juju/control/status.py#newco... juju/control/status.py:307: relation_data = self.service_data[service.service_name]["relations"] please document the return value. https://codereview.appspot.com/5970068/diff/2002/juju/control/status.py#newco... juju/control/status.py:339: relation_data = self.service_data[service.service_name]["relations"] please document what the method is doing.. ie. inspects all the relations of a service and...?
https://codereview.appspot.com/5970068/diff/2002/juju/control/tests/test_stat... File juju/control/tests/test_status.py (right): https://codereview.appspot.com/5970068/diff/2002/juju/control/tests/test_stat... juju/control/tests/test_status.py:788: state = yield self.build_topology() this method is a crutch, its creating way more stuff than what is actually tested for. its better to simplify and model an api for creation of whats needed, instead of relying on a megamethod to build world every test. i'd like to see a test here of removing a primary unit with a subordinate attached, as that broke status on my previous usage of this feature.