Oh, Thanks Dougal, i am now clear since it is your TripleO use case. So yes, in this case, IMHO, we should keep the _get_client() as before but change it to the classmethod. May be other methods too like _create_client(), etc. We can think this change to be an alternative to the solution of creating extra Minxin classes.
Br, Tuan On Tue, Mar 14, 2017 at 11:38 AM, Dougal Matthews <dou...@redhat.com> wrote: > > > On 14 March 2017 at 10:21, lương hữu tuấn <tuantulu...@gmail.com> wrote: > >> >> >> On Tue, Mar 14, 2017 at 10:28 AM, Dougal Matthews <dou...@redhat.com> >> wrote: >> >>> >>> >>> On 13 March 2017 at 09:49, lương hữu tuấn <tuantulu...@gmail.com> wrote: >>> >>>> >>>> >>>> On Mon, Mar 13, 2017 at 9:34 AM, Thomas Herve <the...@redhat.com> >>>> wrote: >>>> >>>>> On Fri, Mar 10, 2017 at 9:52 PM, Ryan Brady <rbr...@redhat.com> wrote: >>>>> > >>>>> > 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. >>>>> >>>>> Is there any reason why those methods aren't functions? AFAICT they >>>>> don't use the instance, they could live top level in the action module >>>>> and be accessible by all actions. If you can avoid multiple >>>>> inheritance (or inheritance!) you'll simplify the design. You could >>>>> also do client = NovaAction().get_client() in your own action (if >>>>> get_client was a public method). >>>>> >>>>> -- >>>>> Thomas >>>>> >>>>> If you want to do that, you need to change the whole structure of base >>>> action and the whole way of creating an action >>>> as you have described and IMHO, i myself do not like this idea: >>>> >>>> 1. Mistral is working well (at the standpoint of creating action) and >>>> changing it is not a short term work. >>>> 2. Using base class to create base action is actually a good idea in >>>> order to control and make easy to action developers. >>>> The base class will define the whole mechanism to execute an action, >>>> developers do not need to take care of it, just only >>>> providing OpenStack clients (the _create_client() method). >>>> 3. From the #2 point of view, the alternative to >>>> NovaAction().get_client() does not make sense since the problem here is >>>> subclass mechanism, >>>> not the way to call get_client(). >>>> >>> >> Hi, >> >> It is hard to me to understand what Thomas wants to say but i just >> understood based on what he wrote:). Sorry for my misunderstanding. >> >> >>> I might be wrong, but I think you read that Thomas wants to use >>> functions for actions, not classes. I don't think that is the case. I think >>> he is referring to the get_client method which is also what rbrady is >>> referring to. At the moment multiple inheritance wont work if you want to >>> inherit from NovaAction and KeyStone action because they both provide a >>> "_get_client" method. If they has a unique name "get_keystone_client" and >>> "get_nova_client" then the multiple inheritance wouldn't clash. >>> >>> Sorry Dougal but i do not get your point. Why the get_client could not >> be used through instance since it has context? >> > > In Mistral we have various OpenStack action classes. For example > NovaAction[1] and GlanceAction[2] (and many others in that file). If I want > to write an action that uses either Nova or Glance I can inherit from them, > for example: > > class MyNovaAction(NovaAction): > > def run(self): > > client = self._create_client() > # ... do something with the client and return > > However, if I wanted to use use two OpenStack clients, which I admit is a > special case and I think one that only TripleO uses (that we know of). > > class MyNovaAndGlanceActioin(NovaAction, GlanceAction): > > def run(self): > > nova = self._create_client() > glance = self._create_client() <- doesn't work because they both > access the same method on NovaAction. > > > If the method was called "create_nova_client" and "create_glance_client" > then you could inherit from both without any conflict. > > However, based on the reply Thomas sent earlier, I think we should > consider something like this when the OpenStack actions are moved to > mistral-extra. > > nova = NovaAction.client(context) > > This is slight adaptation changes "_create_client" to "client" and makes > it a class method that accepts the context. I think this would provide a > very clear interface. I also can't think of any advantage of inheriting > from NovaAction, there is no state shared with it, so we only want it to > create the class for us. > > > [1]: https://github.com/openstack/mistral/blob/master/mistral/ > actions/openstack/actions.py#L75 > [2]: https://github.com/openstack/mistral/blob/master/mistral/ > actions/openstack/actions.py#L109 > > >> >> >> >>> Thomas - The difficulty with these methods is that they need to access >>> the context - the context is going to be added to the action class, and >>> thus while the get_client methods don't use the instance now, they will >>> soon - unless we change direction. >>> >>> >>> >>>> @Renat: I myself not against to multiple inheritance too, the only >>>> thing is if we want to make it multiple inheritance, we should think about >>>> it more thoroughly for the hierarchy of inheritance, what each inheritance >>>> layer does, etc. These work will make the multiple inheritance easy to >>>> understand and for action developers as well easy to develop. So, IMHO, i >>>> vote for make it simple, easy to understand first (if you continue with >>>> mistral-lib) and then do the next thing later. >>>> >>>> Br, >>>> >>>> Tuan/Nokia >>>> >>>>> ____________________________________________________________ >>>>> ______________ >>>>> OpenStack Development Mailing List (not for usage questions) >>>>> Unsubscribe: openstack-dev-requ...@lists.op >>>>> enstack.org?subject:unsubscribe >>>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >>>>> >>>> >>>> >>>> ____________________________________________________________ >>>> ______________ >>>> OpenStack Development Mailing List (not for usage questions) >>>> Unsubscribe: openstack-dev-requ...@lists.op >>>> enstack.org?subject:unsubscribe >>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >>>> >>>> >>> >>> ____________________________________________________________ >>> ______________ >>> OpenStack Development Mailing List (not for usage questions) >>> Unsubscribe: openstack-dev-requ...@lists.op >>> enstack.org?subject:unsubscribe >>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >>> >>> >> >> ____________________________________________________________ >> ______________ >> OpenStack Development Mailing List (not for usage questions) >> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscrib >> e >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> >> > > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev