On Wed, Oct 9, 2019 at 2:06 AM Frode Nordahl <frode.nordahl@canonical.com> wrote:
On Fri, Oct 4, 2019 at 3:46 PM Corey Bryant <corey.bryant@canonical.com> wrote:
Hi All,

Hey Corey,

Great to see the charm coming along!


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()



--
Frode Nordahl


Hey Frode,

Thanks very much for the input. I have these up in gerrit now with the changes you mentioned so I think we can move further reviews to gerrit: https://review.opendev.org/#/q/topic:charms-train-placement+(status:open+OR+status:merged)

Thanks,
Corey