<p dir="ltr"><br>
> Hi Jamie!<br>
><br>
> > That looks like it will go through, I hope<br>
> > you don't mind me making changes to your review i just had them there<br>
> > already.<br>
> No problem!<br>
><br>
> > The next thing i think we need to figure out is the usage of HTTPClient.<br>
> > Nova and Quantum take the opinion that a HTTPClient is a standalone<br>
> > object and something that is share-able. This actually explains to me<br>
> > some of the intention behind what happens in our test code that i felt<br>
> > didn't work correctly. However keystone, heat and others don't work this<br>
> > way, they inherit httpclient.<br>
> Yep, and inheritance makes them less flexible.<br>
> I strongly recommend class composition, that's why I used it in oslo-apiclient.<br>
><br>
> > This patch was approved and should probably go into the base.py in oslo:<br>
> > <a href="https://review.openstack.org/#/c/36019/">https://review.openstack.org/#/c/36019/</a><br>
> Would you like to write that patch for oslo or I will mention you in<br>
> Co-authored-by field in commit message?<br>
><br>
> > The Keystone Auth plugins should be seperate. One for v2 and one for v3<br>
> > that probably inherit from a base keystone auth. I'm a little concerned<br>
> > with how this is going to work because unlike nova the operation of the<br>
> > client depends on the content of the token but we should be able to<br>
> > figure something out.<br>
> Well, first I wrote two separate plugins for v2 and v3, but I decided<br>
> to join them just for command-line tool convenience: user can omit<br>
> "--os-auth-system=keystone" option and the system will be chosen<br>
> automatically based on available arguments (in<br>
> auth.load_plugin_from_args function).<br>
> And v3 and v2 are distinguished by --os-identity-api-version option of<br>
> the plugin.<br>
><br>
> I understand the will to use separate plugins. Could you describe how<br>
> can we distinguish keystone versions? My proposal:<br>
> KeystoneV2AuthPlugin and KeystoneV3AuthPlugin raise an exception in<br>
> their sufficient_options() methods if os-identity-api-version is<br>
> incorrect.<br>
><br>
> > The handling of json decoding is a standalone change. Taking this out of<br>
> > the core request mechanism and pushing it up a layer. I have some<br>
> > reviews at the moment that have wanted this and it will be good to see.<br>
> Ok, could you give me a link?<br>
> I can propose now to move this decoding to a separate strategy using<br>
> class composition (add a new member field to HttpClient).<br>
><br>
> > Another standalone change is that v2 and v3 both currently authenticate<br>
> > to keystone immediately upon creation, under the apiclient they won't.<br>
> > I've got no real opinion either way but it may cause some discussion.<br>
> I have developed the first version of common API client library more<br>
> than a year ago for our own Web dashboard in Grid Dynamics and I<br>
> definitely vote for lazy authentication. It is more convenient for<br>
> user; also, authentication can be perfectly called explicitly by<br>
> HttpClient.authenticate()<br>
><br>
> > This turned into a bit more of a rambling than i intended, but its after<br>
> > 5 on a friday so i'm just writing out thoughts :) All of the above<br>
> > should be considered points of discussion.<br>
> That's nice, I like long letters :)<br>
><br>
> I add <a href="mailto:openstack@lists.openstack.org">openstack@lists.openstack.org</a> to make our discussion public. We<br>
> could also use <a href="https://etherpad.openstack.org/apiclient-marconi">https://etherpad.openstack.org/apiclient-marconi</a><br>
> (marconiclient was the first project that accepted my library).<br>
><br>
> Thank you for interest to my code!<br>
><br>
> Best regards,<br>
> --<br>
> Alessio Ababilov<br>
> Senior Software Engineer<br>
> Grid Dynamics<br>
</p>