juju/control should be aware of subordinates

juju/control should be aware of subordinates

This includes support for deploy, add-unit, remove-unit and status.

https://code.launchpad.net/~bcsaller/juju/subordinate-control-status/+merge/100486

Requires: https://code.launchpad.net/~bcsaller/juju/subordinate-relation-type/+merge/95717

(do not edit description out of merge proposal)

[bcsaller, 2012-03-18 07:47:23]

Please take a look.

[hazmat, 2012-03-25 21:23:33]

This should probably be get rebased off the status branch

https://codereview.appspot.com/5845061/diff/1/juju/control/add_unit.py
File juju/control/add_unit.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/add_unit.py#newcode52
juju/control/add_unit.py:52: raise JujuError("Subordinate services acquire units
with their principal service.")
The immediate error intent is more clear to an end user using s/with/from i
think.

https://codereview.appspot.com/5845061/diff/1/juju/control/deploy.py
File juju/control/deploy.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/deploy.py#newcode149
juju/control/deploy.py:149: "suggests: juju add-relation %s <principal service
name>" % (service_name, service_name))
this feels a bit repetitive after the first one, i'd suggest just simplifying it
to. We'll need end user docs on subordinate usages.

Subordinate %r awaiting relationship to principal for deployment.

https://codereview.appspot.com/5845061/diff/1/juju/control/remove_unit.py
File juju/control/remove_unit.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/remove_unit.py#new...
juju/control/remove_unit.py:45: raise JujuError("Subordinate services acquire
units with their principal service.")
same as add-unit re message.

https://codereview.appspot.com/5845061/diff/1/juju/control/status.py
File juju/control/status.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/status.py#newcode149
juju/control/status.py:149: unit_data = {}  # un :unit_info
#un: unit_info is really indicative of anything.

It feels like its time refactor this method instead of adding additional
complexity to it.. ie add helper methods for _process_service, _process_unit,
_process_machine.

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_deploy.py
File juju/control/tests/test_deploy.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_deploy....
juju/control/tests/test_deploy.py:411: log.getvalue())
The additional message is checked here.. re suggest: but its probably better
removed anyways.

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_status.py
File juju/control/tests/test_status.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_status....
juju/control/tests/test_status.py:99: unit = yield
service.add_unit_state(container=container)
assert container or machine

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_status....
juju/control/tests/test_status.py:104: if units is not None:
This seems out of place, grafting internal bookeeping into external data
structures.

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_status....
juju/control/tests/test_status.py:174: wpu = yield self.add_unit(wordpress, m1,
with_unit, units)
This functionality should be exposed separately and used in the tests that use
it instead of adding more to the this multi-page beast.

https://codereview.appspot.com/5845061/diff/1/juju/state/charm.py
File juju/state/charm.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/state/charm.py#newcode94
juju/state/charm.py:94: return self._metadata.is_subordinate
is_x implies a method name to me. for a property just 'subordinate' is clear.

[bcsaller, 2012-04-01 03:51:18]

https://codereview.appspot.com/5845061/diff/1/juju/control/add_unit.py
File juju/control/add_unit.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/add_unit.py#newcode52
juju/control/add_unit.py:52: raise JujuError("Subordinate services acquire units
with their principal service.")
On 2012/03/25 21:23:33, hazmat wrote:
> The immediate error intent is more clear to an end user using s/with/from i
> think.

Done.

https://codereview.appspot.com/5845061/diff/1/juju/control/deploy.py
File juju/control/deploy.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/deploy.py#newcode149
juju/control/deploy.py:149: "suggests: juju add-relation %s <principal service
name>" % (service_name, service_name))
On 2012/03/25 21:23:33, hazmat wrote:
> this feels a bit repetitive after the first one, i'd suggest just simplifying
it
> to. We'll need end user docs on subordinate usages.
> 
> Subordinate %r awaiting relationship to principal for deployment.

Done.

https://codereview.appspot.com/5845061/diff/1/juju/control/remove_unit.py
File juju/control/remove_unit.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/remove_unit.py#new...
juju/control/remove_unit.py:45: raise JujuError("Subordinate services acquire
units with their principal service.")
On 2012/03/25 21:23:33, hazmat wrote:
> same as add-unit re message.

Done.

https://codereview.appspot.com/5845061/diff/1/juju/control/status.py
File juju/control/status.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/status.py#newcode149
juju/control/status.py:149: unit_data = {}  # un :unit_info
On 2012/03/25 21:23:33, hazmat wrote:
> #un: unit_info is really indicative of anything.
> 
> It feels like its time refactor this method instead of adding additional
> complexity to it.. ie add helper methods for _process_service, _process_unit,
> _process_machine.

I converted it to a class (to easy the argument passing, some of these things
use quite a bit of state) and provider a wrapper for backwards compat. Slightly
less indentation, and easier to follow I think.

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_deploy.py
File juju/control/tests/test_deploy.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_deploy....
juju/control/tests/test_deploy.py:411: log.getvalue())
On 2012/03/25 21:23:33, hazmat wrote:
> The additional message is checked here.. re suggest: but its probably better
> removed anyways.

Done.

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_status.py
File juju/control/tests/test_status.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/control/tests/test_status....
juju/control/tests/test_status.py:174: wpu = yield self.add_unit(wordpress, m1,
with_unit, units)
On 2012/03/25 21:23:33, hazmat wrote:
> This functionality should be exposed separately and used in the tests that use
> it instead of adding more to the this multi-page beast.

Build topology returns a collection of the components its built so that tests
can manipulate it further. This just eases that. 

The subordinate status test uses it (minimally) to keep track of a previously
allocated unit. If this really bothers you I'm happy to do it a different way.

https://codereview.appspot.com/5845061/diff/1/juju/state/charm.py
File juju/state/charm.py (right):

https://codereview.appspot.com/5845061/diff/1/juju/state/charm.py#newcode94
juju/state/charm.py:94: return self._metadata.is_subordinate
On 2012/03/25 21:23:33, hazmat wrote:
> is_x implies a method name to me. for a property just 'subordinate' is clear.

Actually this was overwritten by a latter declaration in the same class w/o the
property. You're spot on that it shouldn't have been like that, cleaned up.

[bcsaller, 2012-04-01 03:58:32]

Please take a look.

[bcsaller, 2012-04-02 17:15:58]

Please take a look.

[bcsaller, 2012-04-02 17:26:08]

Please take a look.

[bcsaller, 2012-04-02 17:31:14]

Please take a look.