aababilov at griddynamics
Aug 9, 2013, 11:06 AM
Post #1 of 2
(26 views)
Permalink
|
> 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 [at] lists 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
|
