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