Support for container relations

Support for container relations

Support in state for container relations. This version reuses as much of the existing watch code as possible.

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

(do not edit description out of merge proposal)

[bcsaller, 2012-03-03 02:36:14]

Please take a look.

[hazmat, 2012-03-16 02:05:37]

lot of moving bits here, looks good for the most part.

https://codereview.appspot.com/5732048/diff/1/juju/lib/pick.py
File juju/lib/pick.py (right):

https://codereview.appspot.com/5732048/diff/1/juju/lib/pick.py#newcode5
juju/lib/pick.py:5: """Return all element having key/value pairs listed in
kwargs."""
this method is obsolete with pick_all_attr.

https://codereview.appspot.com/5732048/diff/1/juju/lib/pick.py#newcode16
juju/lib/pick.py:16: """Return the first element of iterable with all key/value
pairs.
Afaics this entire module is just used for unit tests, it would be nice to fold
these into juju/lib/testing if that's case, debatable though.

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

https://codereview.appspot.com/5732048/diff/1/juju/state/charm.py#newcode115
juju/state/charm.py:115: """Is this a subordinate charm."""
Tests?

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

https://codereview.appspot.com/5732048/diff/1/juju/state/endpoint.py#newcode25
juju/state/endpoint.py:25: return super(cls, RelationEndpoint).__new__(
nice use of __new__

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

https://codereview.appspot.com/5732048/diff/1/juju/state/errors.py#newcode238
juju/state/errors.py:238: class UnitMissingContainer(StateError):
Missing tests?

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

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode62
juju/state/relation.py:62: for endpoint in endpoints:
its not really the minimum.. its just the scope. 
s/min_scope/scope

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode64
juju/state/relation.py:64: endpoint.relation_scope == "container"):
isn't if any of the endpoints have a scope =="container" the scope is container?
ie. why is this filtering on 'client'

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode182
juju/state/relation.py:182: relations = yield
self.get_relations_for_service(service_state)
This scope filter could be inlined more efficiently and cleanly in
get_relations_for_service

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode188
juju/state/relation.py:188: service_state = yield
service_unit.get_service_state()
This method is suspect, its always returning an empty dict

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode284
juju/state/relation.py:284: relation_scope_value = container.internal_id
this would be better with unit_name instead of internal_id

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode292
juju/state/relation.py:292: if relation_scope_value:
is this an attempt at backwards compatibility? the topology version is changing,
so its not worth doing that.

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode681
juju/state/relation.py:681: 
Afaics, the unit mapping should also be on the scope container, else your its
the scope isn't self contained, and units need to filter this.

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

https://codereview.appspot.com/5732048/diff/1/juju/state/service.py#newcode351
juju/state/service.py:351: if container is not None:
please verify the container is a serviceunitstate before using its internal id,
lots of things have internal ids.

https://codereview.appspot.com/5732048/diff/1/juju/state/tests/test_relation.py
File juju/state/tests/test_relation.py (right):

https://codereview.appspot.com/5732048/diff/1/juju/state/tests/test_relation....
juju/state/tests/test_relation.py:379: 
i'm only seeing one type of container relation tested, between a server global
and a client container. I'd like to see additional coverage here of
client/server w/ container 2 container and global 2 container

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

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode72
juju/state/topology.py:72: # this will raise if it cannot update the topology
A general comment of which this is an example. Its always good to use proper
syntax on comments. Ie starts with a Capitalized word, ends with punctuation.

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode193
juju/state/topology.py:193: if container_id is not None:
This block has several code style issues related 80col.

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode194
juju/state/topology.py:194: subordinate_unit_service =
self.get_service_unit_service(container_id)
its the principal not the subordinate

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode201
juju/state/topology.py:201: self.get_relation_scope(relation_id) ==
"container"):
Its more than the principal having any container relation, it needs to have a
container relation to the subordinate service.

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode269
juju/state/topology.py:269: def get_service_unit_container(self, unit_id):
This is returning too much information, and doing so in a way that will blow up
if anyone gets a non tuple value (ie it returns None).

It could just return container_id or container_name, the caller can introspect
any additional information as needed.

also this appears to be a testing method? in which case it would be better as a
test helper than in the impl.

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode544
juju/state/topology.py:544: """Migrate topology version 1 to version 2.
This migration block is untested.

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode609
juju/state/topology.py:609: while current_version < VERSION:
more untested migrations.

https://codereview.appspot.com/5732048/diff/1/juju/unit/lifecycle.py
File juju/unit/lifecycle.py (right):

https://codereview.appspot.com/5732048/diff/1/juju/unit/lifecycle.py#newcode438
juju/unit/lifecycle.py:438: state_dict.update((yield
relation_wf.get_relation_state()))
Why is this dumping the relation state additionally? 

All the changes in this module are untested.

https://codereview.appspot.com/5732048/diff/1/juju/unit/workflow.py
File juju/unit/workflow.py (right):

https://codereview.appspot.com/5732048/diff/1/juju/unit/workflow.py#newcode507
juju/unit/workflow.py:507: def get_relation_state(self):
is the workflow needed here? i the use case is to get the scope of the unit
relation workflow, in which case a simple property accessor to scope would
suffice. Also this is untested.

[bcsaller, 2012-03-18 07:32:58]

Thanks for the review

https://codereview.appspot.com/5732048/diff/1/juju/lib/pick.py
File juju/lib/pick.py (right):

https://codereview.appspot.com/5732048/diff/1/juju/lib/pick.py#newcode5
juju/lib/pick.py:5: """Return all element having key/value pairs listed in
kwargs."""
On 2012/03/16 02:05:37, hazmat wrote:
> this method is obsolete with pick_all_attr.

Its the access protocol x[y] vs x.y 

I think both are useful

https://codereview.appspot.com/5732048/diff/1/juju/lib/pick.py#newcode16
juju/lib/pick.py:16: """Return the first element of iterable with all key/value
pairs.
On 2012/03/16 02:05:37, hazmat wrote:
> Afaics this entire module is just used for unit tests, it would be nice to
fold
> these into juju/lib/testing if that's case, debatable though.

I use it again in the later lifecycle branch

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

https://codereview.appspot.com/5732048/diff/1/juju/state/charm.py#newcode115
juju/state/charm.py:115: """Is this a subordinate charm."""
On 2012/03/16 02:05:37, hazmat wrote:
> Tests?

I don't know if this was in a later pipeline phase or what but I'm pretty sure
this one was tested, it simple. I added it back in.

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

https://codereview.appspot.com/5732048/diff/1/juju/state/endpoint.py#newcode25
juju/state/endpoint.py:25: return super(cls, RelationEndpoint).__new__(
On 2012/03/16 02:05:37, hazmat wrote:
> nice use of __new__

That was actually Jim's idea.

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

https://codereview.appspot.com/5732048/diff/1/juju/state/errors.py#newcode238
juju/state/errors.py:238: class UnitMissingContainer(StateError):
On 2012/03/16 02:05:37, hazmat wrote:
> Missing tests?

added

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

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode62
juju/state/relation.py:62: for endpoint in endpoints:
On 2012/03/16 02:05:37, hazmat wrote:
> its not really the minimum.. its just the scope. 
> s/min_scope/scope

I was thinking it was tracking the most restricted scope we'd found, but by the
time we use it for assignment it is just 'scope' so I'll change it.

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode64
juju/state/relation.py:64: endpoint.relation_scope == "container"):
On 2012/03/16 02:05:37, hazmat wrote:
> isn't if any of the endpoints have a scope =="container" the scope is
container?
> ie. why is this filtering on 'client'

I think this came from an earlier version of the spec where the principal side
would only treat this as a hint, but now we can enforce it, so changed.

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode188
juju/state/relation.py:188: service_state = yield
service_unit.get_service_state()
On 2012/03/16 02:05:37, hazmat wrote:
> This method is suspect, its always returning an empty dict

The last two blocks are dead code. I thought this was needed to support status
but that turns out not to be the case, ServiceUnitState.get_container and
is_subordinate are enough.

Removed.

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode284
juju/state/relation.py:284: relation_scope_value = container.internal_id
On 2012/03/16 02:05:37, hazmat wrote:
> this would be better with unit_name instead of internal_id

This would need mapping the unit/0 format to unit-0. This isn't typically human
introspected data and if a person is looking they'll have access to the topology
I think.

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode292
juju/state/relation.py:292: if relation_scope_value:
On 2012/03/16 02:05:37, hazmat wrote:
> is this an attempt at backwards compatibility? the topology version is
changing,
> so its not worth doing that.

No, global relation can have their ZK structure built on creation. Container
relations don't get built until a container is added so we can place it in the
structure. This is the late construction path.

https://codereview.appspot.com/5732048/diff/1/juju/state/relation.py#newcode681
juju/state/relation.py:681: 
On 2012/03/16 02:05:37, hazmat wrote:
> Afaics, the unit mapping should also be on the scope container, else your its
> the scope isn't self contained, and units need to filter this.

I may misunderstand this. I've looked this over again and as I see it the single
mapping serves as a cache over the whole relationship. _cb_container_children
gets passed only the contained units now and then resolves only those ids from
the central map. If you're read on this is different let me know (I might be
missing some usage aspect somewhere?).

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

https://codereview.appspot.com/5732048/diff/1/juju/state/service.py#newcode351
juju/state/service.py:351: if container is not None:
On 2012/03/16 02:05:37, hazmat wrote:
> please verify the container is a serviceunitstate before using its internal
id,
> lots of things have internal ids.

Done, I added some other checks as well. Some of this was being done in the
topology but it could make sense here as well

https://codereview.appspot.com/5732048/diff/1/juju/state/tests/test_relation.py
File juju/state/tests/test_relation.py (right):

https://codereview.appspot.com/5732048/diff/1/juju/state/tests/test_relation....
juju/state/tests/test_relation.py:379: 
On 2012/03/16 02:05:37, hazmat wrote:
> i'm only seeing one type of container relation tested, between a server global
> and a client container. I'd like to see additional coverage here of
> client/server w/ container 2 container and global 2 container

Done.

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

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode194
juju/state/topology.py:194: subordinate_unit_service =
self.get_service_unit_service(container_id)
On 2012/03/16 02:05:37, hazmat wrote:
> its the principal not the subordinate

Wow, thinko, thanks.

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode201
juju/state/topology.py:201: self.get_relation_scope(relation_id) ==
"container"):
On 2012/03/16 02:05:37, hazmat wrote:
> Its more than the principal having any container relation, it needs to have a
> container relation to the subordinate service.

I think that is what this says. It should look at all the relationships seeing
it this relationship (of the service we are adding) is to the service of the
container. If that relationship exists the next line checks if the scope of that
relation is container.

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode269
juju/state/topology.py:269: def get_service_unit_container(self, unit_id):
On 2012/03/16 02:05:37, hazmat wrote:
> This is returning too much information, and doing so in a way that will blow
up
> if anyone gets a non tuple value (ie it returns None).
> 
> It could just return container_id or container_name, the caller can introspect
> any additional information as needed.
> 
> also this appears to be a testing method? in which case it would be better as
a
> test helper than in the impl.

This is used in ServiceUnitState.get_container (which is important :) I added
more information to the doc string indicating that it will return None when no
container is present. I'd be willing to use a namedtuple or a dict for the
return but this is basically all the information needed to return a
ServiceUnitState in one call to the topology.

https://codereview.appspot.com/5732048/diff/1/juju/state/topology.py#newcode544
juju/state/topology.py:544: """Migrate topology version 1 to version 2.
On 2012/03/16 02:05:37, hazmat wrote:
> This migration block is untested.

I added a test that invokes parse on dumped v1 data and verifies the output is
updated to v2.

https://codereview.appspot.com/5732048/diff/1/juju/unit/lifecycle.py
File juju/unit/lifecycle.py (right):

https://codereview.appspot.com/5732048/diff/1/juju/unit/lifecycle.py#newcode438
juju/unit/lifecycle.py:438: state_dict.update((yield
relation_wf.get_relation_state()))
On 2012/03/16 02:05:37, hazmat wrote:
> Why is this dumping the relation state additionally? 

Because reconstruct_workflow creates the UnitRelationState objects which need
scope as an argument.

> 
> All the changes in this module are untested.

These changes are only to preserve the existing functionality which is already
tested. The test_rememeber_relation_removes for examples loads/stores relations
using this code.

https://codereview.appspot.com/5732048/diff/1/juju/unit/workflow.py
File juju/unit/workflow.py (right):

https://codereview.appspot.com/5732048/diff/1/juju/unit/workflow.py#newcode507
juju/unit/workflow.py:507: def get_relation_state(self):
On 2012/03/16 02:05:37, hazmat wrote:
> is the workflow needed here? i the use case is to get the scope of the unit
> relation workflow, in which case a simple property accessor to scope would
> suffice. Also this is untested.

I was thinking the thing to ask what relation information to store relative to
the workflow was the workflow but your point is a fair one. In the meantime I've
added the usage of this method as part of the validation down by tests already
checking workflow state after transitions.

[bcsaller, 2012-03-18 07:36:09]

Please take a look.

[bcsaller, 2012-03-18 23:44:09]

Please take a look.

[bcsaller, 2012-03-21 23:35:05]

Please take a look.

[hazmat, 2012-03-22 13:23:12]

So it still feels like there's significant testing missing here, lots of
relation usage that don't properly take into account scope paths. i've fixed a
few in branch lp:~hazmat/juju/subordinate-relation-type

but there are others.. as far as tests go i'd like to see

 - a subordinate unit relation watch that fires on settings change.
 - a hook context tests with subordinate doing a get from another unit and set
locally. 
 - a departed hook test with a subordinate.

https://codereview.appspot.com/5732048/diff/13001/juju/state/errors.py
File juju/state/errors.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/state/errors.py#newcod...
juju/state/errors.py:137: return ("Illegal attempt to assign %s to machine. %s
is a subordinate unit and "
perhaps simplify to .. Cannot assign subordinate unit '%s' to machine.

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py
File juju/state/relation.py (left):

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py#oldc...
juju/state/relation.py:649: 
This has no corresponding subordinate test.

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py
File juju/state/relation.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py#newc...
juju/state/relation.py:65: 
not particularly meaningful, but seems like it should break after discovery in
the loop

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py#newc...
juju/state/relation.py:108: # this is done for each container in a container
scoped relation
This comment formatting/content could use a cleanup.

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py#newc...
juju/state/relation.py:259: #                else:
This get yanked, reinstated, or moved. this check feels better suited to the
relation creation than repeated per unit.

https://codereview.appspot.com/5732048/diff/13001/juju/state/service.py
File juju/state/service.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/state/service.py#newco...
juju/state/service.py:864: self._service_name)
Where is this method used?

https://codereview.appspot.com/5732048/diff/13001/juju/state/tests/test_relat...
File juju/state/tests/test_relation.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/state/tests/test_relat...
juju/state/tests/test_relation.py:825: 
There should also be some verification of settings nodes.

https://codereview.appspot.com/5732048/diff/13001/juju/state/tests/test_topol...
File juju/state/tests/test_topology.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/state/tests/test_topol...
juju/state/tests/test_topology.py:1168: def test_mirgation_v1_to_v2(self):
s/migration

https://codereview.appspot.com/5732048/diff/13001/juju/state/tests/test_topol...
juju/state/tests/test_topology.py:1177: TOPOLOGY_V2_EXPECTED.strip())
There should also be a test of an unsupported migration.

https://codereview.appspot.com/5732048/diff/13001/juju/unit/workflow.py
File juju/unit/workflow.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/unit/workflow.py#newco...
juju/unit/workflow.py:508: """Return relation states for persistence."""
Its not returning a state object, just a dictionary, probably better named
get_relation_info()

https://codereview.appspot.com/5732048/diff/13001/juju/unit/workflow.py#newco...
juju/unit/workflow.py:510: relation_state = yield self.get_state()
why is this being returned? it shouldn't be afaics.

[bcsaller, 2012-03-27 08:34:33]

Please take a look.

[bcsaller, 2012-03-27 09:34:00]

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py
File juju/state/relation.py (left):

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py#oldc...
juju/state/relation.py:649: 
On 2012/03/22 13:23:13, hazmat wrote:
> This has no corresponding subordinate test.

test_watch_server_client_with_new_subordinate_service shows container level
matches on both sides of the relationship. I added a settings change watch to
verify_watch which is called in all cases.

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py
File juju/state/relation.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py#newc...
juju/state/relation.py:65: 
On 2012/03/22 13:23:13, hazmat wrote:
> not particularly meaningful, but seems like it should break after discovery in
> the loop

Done.

https://codereview.appspot.com/5732048/diff/13001/juju/state/tests/test_topol...
File juju/state/tests/test_topology.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/state/tests/test_topol...
juju/state/tests/test_topology.py:1177: TOPOLOGY_V2_EXPECTED.strip())
On 2012/03/22 13:23:13, hazmat wrote:
> There should also be a test of an unsupported migration.

Done.

https://codereview.appspot.com/5732048/diff/13001/juju/unit/workflow.py
File juju/unit/workflow.py (right):

https://codereview.appspot.com/5732048/diff/13001/juju/unit/workflow.py#newco...
juju/unit/workflow.py:508: """Return relation states for persistence."""
On 2012/03/22 13:23:13, hazmat wrote:
> Its not returning a state object, just a dictionary, probably better named
> get_relation_info()

Done.

https://codereview.appspot.com/5732048/diff/13001/juju/unit/workflow.py#newco...
juju/unit/workflow.py:510: relation_state = yield self.get_state()
On 2012/03/22 13:23:13, hazmat wrote:
> why is this being returned? it shouldn't be afaics.

removed, seems to function fine, though some tests previously expected it.

[bcsaller, 2012-03-27 09:34:47]

Please take a look.

[hazmat, 2012-03-27 12:38:33]

This branch seems to include a diffs for branches already merged to trunk.

Please merge trunk and resubmit, that clears up the diff considerably 3.1k to
1.8k

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py
File juju/state/relation.py (left):

https://codereview.appspot.com/5732048/diff/13001/juju/state/relation.py#oldc...
juju/state/relation.py:649: 
On 2012/03/27 09:34:00, bcsaller wrote:
> On 2012/03/22 13:23:13, hazmat wrote:
> > This has no corresponding subordinate test.
> 
> test_watch_server_client_with_new_subordinate_service shows container level
> matches on both sides of the relationship. I added a settings change watch to
> verify_watch which is called in all cases.

cool, previously it was the wrong address, so it was clear it didn't work or get
tested

[bcsaller, 2012-03-27 16:11:37]

Please take a look.

[hazmat, 2012-03-27 22:51:30]

On 2012/03/27 16:11:37, bcsaller wrote:
> Please take a look.

There looks like some sort of merge problem here, i see this removing some of
the tests/code that where previously on trunk.

[hazmat, 2012-03-28 14:17:02]

LGTM, the problems with the trunk merge need to be resolved though, there is at
least one trunk commit that's being redacted by this branch (status-changes).
perhaps an unpumped pipeline?

https://codereview.appspot.com/5732048/diff/26001/juju/lib/testing.py
File juju/lib/testing.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/lib/testing.py#newcode161
juju/lib/testing.py:161: def get_debug_client(self):
should get yanked.

https://codereview.appspot.com/5732048/diff/26001/juju/state/errors.py
File juju/state/errors.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/state/errors.py#newcod...
juju/state/errors.py:137: return ("Unable to assign %s to machine." %
self.unit_name)
s/assign/assign subordinate

https://codereview.appspot.com/5732048/diff/26001/juju/state/relation.py
File juju/state/relation.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/state/relation.py#newc...
juju/state/relation.py:129: "role": endpoint.relation_role})
the node data should be the same regardless of rel ctx

https://codereview.appspot.com/5732048/diff/26001/juju/state/relation.py#newc...
juju/state/relation.py:470: 
its starting to feel like unit rel state should receive scope_path and role in
its constructor, not for this branch though, but we jump through hoops to
calculate in a few places to obtain then, when the service rel state has the
path, role already.

https://codereview.appspot.com/5732048/diff/26001/juju/state/tests/test_topol...
File juju/state/tests/test_topology.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/state/tests/test_topol...
juju/state/tests/test_topology.py:224: 
not assuming the implementation, there should be a test for removing a unit with
a subordinate rel.

https://codereview.appspot.com/5732048/diff/26001/juju/unit/tests/test_workfl...
File juju/unit/tests/test_workflow.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/unit/tests/test_workfl...
juju/unit/tests/test_workflow.py:675: relation_state = yield
self.workflow.get_relation_info()
the yields are unesc now.

https://codereview.appspot.com/5732048/diff/26001/juju/unit/workflow.py
File juju/unit/workflow.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/unit/workflow.py#newco...
juju/unit/workflow.py:507: """Return relation states for persistence."""
s/states/info

[bcsaller, 2012-03-29 08:02:47]

Made changes and attempted to clean up merge issues. As we talked about I am
pushing this again.

https://codereview.appspot.com/5732048/diff/26001/juju/lib/testing.py
File juju/lib/testing.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/lib/testing.py#newcode161
juju/lib/testing.py:161: def get_debug_client(self):
On 2012/03/28 14:17:03, hazmat wrote:
> should get yanked.

Done, also changed the decoder not to check for {}

https://codereview.appspot.com/5732048/diff/26001/juju/state/errors.py
File juju/state/errors.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/state/errors.py#newcod...
juju/state/errors.py:137: return ("Unable to assign %s to machine." %
self.unit_name)
On 2012/03/28 14:17:03, hazmat wrote:
> s/assign/assign subordinate

Done.

https://codereview.appspot.com/5732048/diff/26001/juju/state/relation.py
File juju/state/relation.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/state/relation.py#newc...
juju/state/relation.py:129: "role": endpoint.relation_role})
On 2012/03/28 14:17:03, hazmat wrote:
> the node data should be the same regardless of rel ctx

I could find nothing that used node_data, I've removed it, its in the topology
and I believe usage has switched to there. All tests pass w/o it.

https://codereview.appspot.com/5732048/diff/26001/juju/state/tests/test_topol...
File juju/state/tests/test_topology.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/state/tests/test_topol...
juju/state/tests/test_topology.py:224: 
On 2012/03/28 14:17:03, hazmat wrote:
> not assuming the implementation, there should be a test for removing a unit
with
> a subordinate rel.

Added tests around remove_principal_service, remove_subordinate_service,
remove_principal_service_unit, remove_subordinate_service_unit

https://codereview.appspot.com/5732048/diff/26001/juju/unit/tests/test_workfl...
File juju/unit/tests/test_workflow.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/unit/tests/test_workfl...
juju/unit/tests/test_workflow.py:675: relation_state = yield
self.workflow.get_relation_info()
On 2012/03/28 14:17:03, hazmat wrote:
> the yields are unesc now.

Done.

https://codereview.appspot.com/5732048/diff/26001/juju/unit/workflow.py
File juju/unit/workflow.py (right):

https://codereview.appspot.com/5732048/diff/26001/juju/unit/workflow.py#newco...
juju/unit/workflow.py:507: """Return relation states for persistence."""
On 2012/03/28 14:17:03, hazmat wrote:
> s/states/info

Done.