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

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

Thanks for this. I include some thoughts below, I hope they are
agreeable.

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

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py#newcode239
charmhelpers/core/services.py:239:
I make suggestions below on altering this process. Let me know what you
think.

https://codereview.appspot.com/96680043/diff/20001/charmhelpers/core/services.py#newcode298
charmhelpers/core/services.py:298: over the units, not the items of the
default unit.
There is a rule I try to use when generating/process data these days, no
data should live in the key side of the mapping. In the context of a
template for iteration is simpler to iterate

context[interface][{_unit: 'unit/0', 'foo': 'bar'},
                    {_unit: 'unit/1', 'foo': bar}]

This rule makes additional sense when thinking that knowing the unit
names to request them would imply a broken charm and thus shouldn't be
an index in this context. I'd suggest transitioning to that. We could
also make the rule (and document here) that only elements with complete
data appear in the list.

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

« Back to merge proposal