juju/control should be aware of subordinates

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)

[bcsaller, 2012-04-02 18:06:11]

Please take a look.

[bcsaller, 2012-04-02 22:32:11]

Please take a look.

[hazmat, 2012-04-03 16:38:22]

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...?

[hazmat, 2012-04-03 16:42:38]

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.