Code review comment for lp:~johnsca/charm-helpers/multi-unit
Revision history for this message
| Cory Johns (johnsca) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
On 2014/06/06 16:43:12, benjamin.saller wrote:
https:/
> charmhelpers/
['address',
> 'port', 'user', 'password']
> You have a follow on for the charms updating the templates as well?
Yes, as part of the port conflict resolution. Just testing and hashing
out bugs currently.
https:/
> charmhelpers/
['hostname',
> 'port']
> I am a little cautious about this one as this isn't really a CF
component, it
> would be interesting if down the road we had something like
> EtcdInterface = charmhelpers.
> To produce this class from a interface registry. We'll want that sort
of thing
> for the interface testing plans anyway.
I had considered having RelationContext
name and required keys as parameters so that you don't have to create
stub subclasses, and only need to subclass if you want to add extra
behavior (e.g., the MysqlRelation). I think this is the right way to
go, and will do so.
https:/
> charmhelpers/
> This feels a bit magical. I'm fine with letting it in but the 'why'
you'd use it
> both ways should be explained a little more below. See my following
comment
> about the example being a bit unclear.
I agree. I am very on the fence as to whether to stick with the
"magical" implementation vs creating separate classes, or even just
forcing it to always be a list to force charm authors to always consider
the possibility that there may be more units.
The reason I didn't go with separate classes is because I didn't want to
have to duplicate all the stub RelationContext subclasses, but if we
make it so the interface and keys can be passed in, we can avoid
creating 95% of the stub classes, so that'd be ok.
But now I'm thinking that forcing it to be a list might be better, since
charm authors really should keep in mind that the admin could throw more
units at the relation.
https:/