Code review comment for lp://qastaging/~justin-fathomdb/nova/justinsb-openstack-api-keys

A good enhancement to Nova, Justin! :) Some smallish notes:

1)

14 +# Copyright 2011 Justin Santa Barbara
115 +# All Rights Reserved.

Much of the code in the newly-added file nova/api/keys.py was written by the Anso/NASA guys that was moved from the nova/api/ec2/cloud.py file. It would be best, in my opinion, to put this above your copyright line:

# Copyright 2010 United States Government as represented by the
# Administrator of the National Aeronautics and Space Administration.

In other words, it's good faith to put both your own copyright line as well as the NASA one above it to acknowledge that some significant portion of the code in the file can be attributed to Anso. Hope that makes sense. No worries on the copyright header stuff in all the other files (like nova/api/openstack/keys.py since you wrote all of that code yourself.

2)

I'd prefer to get rid of the Keys class in nova/api/keys.py. All of the methods of Keys should be simple module-level functions since there is no object-level or class-level data to the Keys class. This would also mean you could remove this line in nova/api/ec2/cloud.py:

55 + self.keys_api = Keys()

and change the import in that same file from this:

7 +from nova.api.keys import Keys

to simply this:

from nova.api import keys

Then, in the Controller methods, instead of something like this:

64 + key_pairs = self.keys_api.get_all_keys(context)

just do this:

key_pairs = keys.get_all_keys(context)

The above solves two problems:

a) from nova.api.keys import Keys breaks the rule in the HACKING file that you should only import modules, not objects (and yes, I know that rule is broken regularly in various parts of the source code, but still, it's a documented coding rule...)
b) Removes the unnecessary Keys class that does nothing more than provide a class-level namespace instead of the more appropriate module-level namespacing.

3)

218 + mapper.resource("key", "keys", controller=keys.Controller(),
219 + collection={'detail': 'GET'})

While I think your addition of the keys resource to the OpenStack REST API is very beneficial, it's up to the mailing list and community to decide on changes to the API. Please propose this change/enhancement to the mailing list for approval. We can't approve this patch until this community approval is gained, regardless of how we personally feel the API would be improved. Hope you understand.

4)

Please add some test cases for the OpenStack API /keys stuff, too :)

Cheers!
jay