On Fri, Oct 4, 2019 at 3:46 PM Corey Bryant <corey.bryant at canonical.com> wrote: > Hi All, > Hey Corey, Great to see the charm coming along! Code is located at: > https://github.com/coreycb/charm-placement > https://github.com/coreycb/charm-interface-placement > > https://review.opendev.org/#/q/topic:charms-train-placement+(status:open+OR+status:merged) > 1) Since the interface is new I would love to see it based on the ``Endpoint`` class instead of the aging ``RelationBase`` class. Also the interface code needs unit tests. We have multiple examples of interface implementations with both in place you can get inspiration from [0]. Also consider having both a ``connected`` and ``available`` state, the available state could be set on the first relation-changed event. This increases the probability of your charm detecting a live charm in the other end of the relation, both states are also required to use the ``charms.openstack`` required relation gating code. 2) In the reactive handler you do a bespoke import of the charm class module just to activate the code, this is no longer necessary as there has been implemented a module that does automatic search and import of the class for you. Please use that instead. [1] import charms_openstack.bus import charms_openstack.charm as charm charms_openstack.bus.discover() 0: https://github.com/search?q=org%3Aopenstack+%22from+charms.reactive+import+Endpoint%22&type=Code 1: https://github.com/search?q=org%3Aopenstack+charms_openstack.bus&type=Code -- Frode Nordahl -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.openstack.org/pipermail/openstack-discuss/attachments/20191009/ef25367f/attachment.html>