<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 10, 2017 at 5:16 AM, Renat Akhmerov <span dir="ltr"><<a href="mailto:renat.akhmerov@gmail.com" target="_blank">renat.akhmerov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><div style="color:rgb(0,0,0);letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;word-wrap:break-word"><div><br></div></div></div><div><span class="m_4695271997112051141m_-7192563608475669884gmail-"><blockquote type="cite"><div>On 10 Mar 2017, at 15:09, Dougal Matthews <<a href="mailto:dougal@redhat.com" target="_blank">dougal@redhat.com</a>> wrote:</div><br class="m_4695271997112051141m_-7192563608475669884gmail-m_-7636940306865310761Apple-interchange-newline"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 10 March 2017 at 04:22, Renat Akhmerov <span dir="ltr"><<a href="mailto:renat.akhmerov@gmail.com" target="_blank">renat.akhmerov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div>Hi,</div><div><br></div><div>I probably like the base class approach better too.</div><div><br></div><div>However, I’m trying to understand if we need this variety of classes.</div><div><ul class="m_4695271997112051141m_-7192563608475669884gmail-m_-7636940306865310761m_4542071899119995468MailOutline"><li>Do we need a separate class for asynchronous actions? IMO, since is_sync() is just an instance method that can potentially return both True and False based on the instance state shouldn’t be introduced by a special class. Otherwise it’s confusing that a classes declared as AsyncAction can actually be synchronous (if its is_sync() returns True). So maybe we should just leave this method in the base class.</li><li>I”m also wondering if we should just always pass “context” into run() method. Those action implementations that don’t need it could just ignore it. Not sure though.</li></ul></div></div></blockquote><div>This is a good point. I had originally thought it would be backwards incompatible to make this change - however, users will need to update their actions to inherit from mistral-lib so they will need to opt in. Then in mistral we can do something like...<br><br></div><div>if isinstance(action, mistral_lib.Action):<br></div><div>    action.run(ctx)<br></div><div>else:<br></div><div>    # deprecation warning about action now inheriting from mistral_lib and taking a context etc.<br></div><div>    action.run()<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 style="word-wrap:break-word"></div></blockquote></div></div></div></div></blockquote><div><br></div></span><div>Yes, right.</div></div></div></blockquote><div><br></div><div>The example here by Dougal looks like the way to move forward.  Thanks for all of the feedback.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word"><div><span class="m_4695271997112051141m_-7192563608475669884gmail-"><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><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 style="word-wrap:break-word"><div>As far as mixin approach, I’d say I’d be ok with having mixing for context-based actions. Although, like Dougal said, it may be a little harder to read, this approach gives a huge flexibility for long term. Imagine if we want to have a class of actions that some different kind of information. Just making it up… For example, some of my actions need to be aware of some policies (Congress-like) or information about metrics of the current operating system (this is probably a bad example because it’s easy to use standard Python modules but I’m just trying to illustrate the idea). In this case we could have PolicyMixin and OperatingSystemMixin that would set required info into the instance state or provide with handle interfaces for more advanced uses.</div></div></blockquote><div><br></div><div>I like the idea of mixins if we can see a future with many small components that can be included in an action class. However, like you I didn't manage to think of any real examples.<br><br></div><div>It should be possible to migrate to a mixin approach later if we have the need.<br></div></div></div></div></div></blockquote></span></div><div><br></div>Well, I didn’t manage to find real use cases probably because I don’t develop lots of actions :) Although the example with policies seems almost real to me. This is something that was raised several times during design sessions in the past. Anyway, I agree with you that seems like we can add mixins later if we want to. I don’t see any reasons now why not.<span class="m_4695271997112051141m_-7192563608475669884gmail-HOEnZb"><font color="#888888"><div><br></div></font></span></div></blockquote><div><br></div><div>One of the pain points for me as an action developer is the OpenStack actions[1].  Since they all use the same method name to retrieve the underlying client, you cannot simply inherit from more than one so you are forced to rewrite the client access methods.  We saw this in creating actions for TripleO[2].  In the base action in TripleO, we have actions that make calls to more than one OpenStack client and so we end up re-writing and maintaining code.  IMO the idea of using multiple inheritance there would be helpful.  It may not require the mixin approach here, but rather a design change in the generator to ensure the method names don't match.</div><div><br></div><div><div>[1] <a href="https://github.com/openstack/mistral/blob/master/mistral/actions/openstack/actions.py" target="_blank">https://github.com/opensta<wbr>ck/mistral/blob/master/mistral<wbr>/actions/openstack/actions.py</a></div><div>[2] <a href="https://github.com/openstack/tripleo-common/blob/master/tripleo_common/actions/base.py#L27" target="_blank">https://github.com/opensta<wbr>ck/tripleo-common/blob/master/<wbr>tripleo_common/actions/base.py<wbr>#L27</a></div></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 style="word-wrap:break-word"><span class="m_4695271997112051141m_-7192563608475669884gmail-HOEnZb"><font color="#888888"><div><div><br><div><div><div style="word-wrap:break-word"><div>Renat Akhmerov</div><div>@Nokia</div><div><br></div></div></div></div><div><br></div></div></div></font></span></div><br>______________________________<wbr>______________________________<wbr>______________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.op<wbr>enstack.org?subject:unsubscrib<wbr>e</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi<wbr>-bin/mailman/listinfo/openstac<wbr>k-dev</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="m_4695271997112051141m_-7192563608475669884gmail_signature"><div dir="ltr">Ryan Brady<div>Cloud Engineering</div><div><a href="mailto:rbrady@redhat.com" target="_blank">rbrady@redhat.com</a> </div><div><a href="tel:(919)%20890-8925" value="+19198908925" target="_blank">919.890.8925</a></div></div></div>
</div></div>