Merge lp://qastaging/~justin-fathomdb/nova/justinsb-openstack-api-keys into lp://qastaging/~hudson-openstack/nova/trunk

Proposed by justinsb
Status: Work in progress
Proposed branch: lp://qastaging/~justin-fathomdb/nova/justinsb-openstack-api-keys
Merge into: lp://qastaging/~hudson-openstack/nova/trunk
Diff against target: 956 lines (+679/-56)
15 files modified
nova/api/ec2/cloud.py (+14/-41)
nova/api/keys.py (+93/-0)
nova/api/openstack/__init__.py (+10/-0)
nova/api/openstack/keys.py (+122/-0)
nova/cloudpipe/pipelib.py (+2/-2)
nova/compute/api.py (+2/-1)
nova/db/api.py (+2/-2)
nova/db/sqlalchemy/api.py (+17/-6)
nova/tests/integrated/__init__.py (+20/-0)
nova/tests/integrated/api/__init__.py (+20/-0)
nova/tests/integrated/api/client.py (+130/-0)
nova/tests/integrated/integrated_helpers.py (+164/-0)
nova/tests/integrated/test_keys.py (+78/-0)
nova/tests/test_api.py (+2/-1)
nova/tests/test_cloud.py (+3/-3)
To merge this branch: bzr merge lp://qastaging/~justin-fathomdb/nova/justinsb-openstack-api-keys
Reviewer Review Type Date Requested Status
Jay Pipes Pending
Review via email: mp+50662@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2011-02-19.

Description of the change

Support user SSH keys in OpenStack API (they're already supported in EC2, needed for integration tests). Disabled by FLAG option until approved by community, but needed for testing and hence stability.

To post a comment you must log in.
Revision history for this message
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal

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

review: Needs Fixing
Revision history for this message
justinsb (justin-fathomdb) wrote :

1) Quite right Jay & very sorry NASA/Anso guys! I started with a clean file and then didn't update the copyright as I refactored the code into there.

2) The idea of the Keys class is to make it look like volumes, network, images & compute. Each of the Web services instantiates a service.API class, and then makes calls on it. Although it's in-process at the moment, and thus could be a module, the intention is that this is an implementation detail. I renamed it to API to show this and added a comment. I did fix up the imports as you suggested.

3) This is necessary to support testing, which is needed for our goal of stability. However, until it has been approved by the community, it is disabled unless specifically enabled for testing, just like FLAGS.allow_admin_api. I called this flag FLAGS.allow_testing_api

4) Done! (Taking advantage of #3!)

Unmerged revisions

710. By justinsb

Eventlet seems to screw up httplib2; switched back to httplib as used elsewhere in nova

709. By justinsb

Merged with head - resolved conflict with moving of context.user.id => context.user_id

708. By justinsb

NASA copyright was in wrong file & a few formatting issues

707. By justinsb

Clean up Keys to meet style guidelines, and add explanatory note.
The idea of the API class is to make keys 'feel' like the other services (volumes, compute, images, network). Currently it's implemented in-process, but this way the REST interfaces all look the same, irrespective of this implementation detail

706. By justinsb

Don't allow keys to be part of the OpenStack API until it is approved, but support stability by allowing it to be tested using the same approach as allow_admin_api

705. By justinsb

Added NASA copyright into file into which I pasted their copy. Sorry guys!!

704. By justinsb

Added unit tests for keys

703. By justinsb

Fix broken unit tests

702. By justinsb

pep8 fixes

701. By justinsb

Support keys in OpenStack API (needed for integration tests)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.