Code review comment for lp:~johnsca/charm-helpers/multi-unit

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Thanks, notes follow.

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/services.py#newcode259
charmhelpers/core/services.py:259: order: 'wordpress/0', 'wordpress/1',
'mediawiki/0'.
I think you do a fine job of explaining this now, but the use-case for
it is still unclear to me.

If there is more than one relation with the same interface I don't think
we can silently combine those lists (the data is coming from different
relation ids). They represent different services and should expect
different orchestration.

I'd go so far as to say this is a whole in our model.

In CF mysql has relations to UAA and CC but doesn't use the services
framework to manage this. If it did and iterated those units uniformly
havoc could follow, no?

The default case isn't this and we built for it, but unless my thinking
here is muddy, we'd need to do something different here. Currently the
relation id information would be lost. We'd at minimum have to put the
rid in the data, or break the units into collections by relation.

Breaking them up by relation is to my mind similar to always processing
this as a list (rather than the default unit notion of before), its the
more complex case, but its one that some services need to be aware of.

I apologize for not picking up on this sooner. If you'd like to talk
about this in the hangout let me know.

https://codereview.appspot.com/96680043/diff/100001/charmhelpers/core/services.py#newcode268
charmhelpers/core/services.py:268: {%- endfor %}
This is good, thanks, it might have to turn into

interface[0][0]['key']

for interface[first relation][first unit][key]

lets talk.

https://codereview.appspot.com/96680043/

« Back to merge proposal