<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 9, 2019 at 2:06 AM Frode Nordahl <<a href="mailto:frode.nordahl@canonical.com">frode.nordahl@canonical.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Fri, Oct 4, 2019 at 3:46 PM Corey Bryant <<a href="mailto:corey.bryant@canonical.com" target="_blank">corey.bryant@canonical.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Hi All,</div></div></blockquote><div><br></div><div>Hey Corey,</div><div><br></div><div>Great to see the charm coming along!</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Code is located at:<div><a href="https://github.com/coreycb/charm-placement" target="_blank">https://github.com/coreycb/charm-placement</a></div><div><a href="https://github.com/coreycb/charm-interface-placement" target="_blank">https://github.com/coreycb/charm-interface-placement</a></div><div><a href="https://review.opendev.org/#/q/topic:charms-train-placement+(status:open+OR+status:merged)" target="_blank">https://review.opendev.org/#/q/topic:charms-train-placement+(status:open+OR+status:merged)</a></div></div></blockquote><div><br></div><div>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].</div><div><br></div><div>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.<br></div><div><br></div><div>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]</div><div><br></div><div><br></div><div>    import charms_openstack.bus<br>    import charms_openstack.charm as charm<br><br>    charms_openstack.bus.discover()<br></div><div><br></div><div><br></div><div>0: <a href="https://github.com/search?q=org%3Aopenstack+%22from+charms.reactive+import+Endpoint%22&type=Code" target="_blank">https://github.com/search?q=org%3Aopenstack+%22from+charms.reactive+import+Endpoint%22&type=Code</a></div><div>1: <a href="https://github.com/search?q=org%3Aopenstack+charms_openstack.bus&type=Code" target="_blank">https://github.com/search?q=org%3Aopenstack+charms_openstack.bus&type=Code</a></div></div><br>-- <br><div dir="ltr"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div>Frode Nordahl</div></div></div></div></div></div></div></div></div></div></div></div></blockquote><div><br></div><div><br></div><div>Hey Frode,</div><div><br></div><div>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: <a href="https://review.opendev.org/#/q/topic:charms-train-placement+(status:open+OR+status:merged)">https://review.opendev.org/#/q/topic:charms-train-placement+(status:open+OR+status:merged)</a></div><div><br></div><div>Thanks,<br></div><div>Corey<br> </div></div></div>