Implements relation ident support.

Implements relation ident support.



https://code.launchpad.net/~jimbaker/juju/relation-id/+merge/99148

(do not edit description out of merge proposal)

[jimbaker, 2012-03-26 01:48:21]

Please take a look.

[hazmat, 2012-03-27 12:13:51]

This looks good overall. There are two common threads through this branch that
need some rework. relation_id already has a common well known meaning the code.
Attempting to quality that usage only introduces confusion, as it is the common
case. Instead i suggest renaming relation_id to relation_ident whenever used in
the code (the hooks see the same name JUJU_RELATION_ID). Also the passing of
relation_ident throughout the rel states/invoker in addition to relation_name is
just passing the same info twice via parameters. Relation name should be removed
from constructors and computed lazily from ident.

https://codereview.appspot.com/5900067/diff/1/juju/control/tests/test_resolve...
File juju/control/tests/test_resolved.py (right):

https://codereview.appspot.com/5900067/diff/1/juju/control/tests/test_resolve...
juju/control/tests/test_resolved.py:94: service_relation.relation_name,
service_relation.relation_id,
This is way to easy to confuse with internal_relation_id, i'd suggest calling it
relation_ident, its an identity in this context not the internal id, also since
it contains relation_name, the latter can be computed as needed.

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

https://codereview.appspot.com/5900067/diff/1/juju/state/hook.py#newcode14
juju/state/hook.py:14: def __init__(self, relation_name, relation_id,
change_type, unit_name):
removing relation_name passing in favor relation_ident with lazy computation of
name avoids passing duplication information in extra params

https://codereview.appspot.com/5900067/diff/1/juju/state/hook.py#newcode25
juju/state/hook.py:25: def relation_id(self):
this would be nicer as relation_ident to avoid confusion.

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

https://codereview.appspot.com/5900067/diff/1/juju/unit/lifecycle.py#newcode382
juju/unit/lifecycle.py:382: for internal_relation_id in removed:
all of this qualification for the common use of relation_id can be removed by
renaming the public use to relation_ident

[jimbaker, 2012-03-27 21:32:10]

*** Submitted:

Implements relation ident support.

R=hazmat
CC=
https://codereview.appspot.com/5900067

[jimbaker, 2012-03-30 02:50:18]

Please take a look.

[jimbaker, 2012-03-30 06:14:10]

Please take a look at the most recent proposal

https://codereview.appspot.com/5900067/diff/1/juju/control/tests/test_resolve...
File juju/control/tests/test_resolved.py (right):

https://codereview.appspot.com/5900067/diff/1/juju/control/tests/test_resolve...
juju/control/tests/test_resolved.py:94: service_relation.relation_name,
service_relation.relation_id,
Changed to relation_ident

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

https://codereview.appspot.com/5900067/diff/1/juju/state/hook.py#newcode14
juju/state/hook.py:14: def __init__(self, relation_name, relation_id,
change_type, unit_name):
Now does lazy computation as suggested

https://codereview.appspot.com/5900067/diff/1/juju/state/hook.py#newcode25
juju/state/hook.py:25: def relation_id(self):
relation_ident ditto

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

https://codereview.appspot.com/5900067/diff/1/juju/unit/lifecycle.py#newcode382
juju/unit/lifecycle.py:382: for internal_relation_id in removed:
On 2012/03/27 12:13:51, hazmat wrote:
> all of this qualification for the common use of relation_id can be removed by
> renaming the public use to relation_ident

Done.

[hazmat, 2012-03-30 17:22:12]

LGTM, thanks for switching out the rel-id name.

https://codereview.appspot.com/5900067/diff/8001/juju/hooks/tests/test_invoke...
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/5900067/diff/8001/juju/hooks/tests/test_invoke...
juju/hooks/tests/test_invoker.py:64: `relation_ident`: relation ident of the
relation the Invoker is for.
s/relation ident/relation identity

[jimbaker, 2012-03-30 18:29:22]

https://codereview.appspot.com/5900067/diff/8001/juju/hooks/tests/test_invoke...
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/5900067/diff/8001/juju/hooks/tests/test_invoke...
juju/hooks/tests/test_invoker.py:64: `relation_ident`: relation ident of the
relation the Invoker is for.
On 2012/03/30 17:22:12, hazmat wrote:
> s/relation ident/relation identity

Done.