OpenStack Compute (nova)

Merge lp://qastaging/~rackspace-titan/nova/osapi-servers-cleanup-p1 into lp://qastaging/~hudson-openstack/nova/trunk

osapi-servers-cleanup-p1
Merge into trunk

Proposed by Brian Lamar on 2011-06-17

Status:	Work in progress
Proposed branch:	lp://qastaging/~rackspace-titan/nova/osapi-servers-cleanup-p1
Merge into:	lp://qastaging/~hudson-openstack/nova/trunk
Diff against target:	771 lines (+199/-197) 6 files modified nova/api/openstack/__init__.py (+1/-1) nova/api/openstack/create_instance_helper.py (+5/-5) nova/api/openstack/servers.py (+174/-185) nova/api/openstack/views/servers.py (+6/-3) nova/api/openstack/wsgi.py (+10/-0) nova/tests/api/openstack/test_servers.py (+3/-3)
To merge this branch:	bzr merge lp://qastaging/~rackspace-titan/nova/osapi-servers-cleanup-p1
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Brian Waldon		2011-06-17	Pending
Review via email: mp+64993@code.qastaging.launchpad.net

Commit message

Code cleanup and refactoring of OSAPI servers code.

Description of the change

In order to prepare for this BP: https://blueprints.launchpad.net/nova/+spec/efficient-limiting, I want to clean up some code and learn about how things are done.

When I learn about code I tend to do the following:

-Look for generically named functions and rename them
-Abstract out oftenly used and/or repeated functions
-Look over all tests to make sure they make sense (but don't make major modifications to tests at the same time as refactoring!)

With that being said, I have updated servers.py in the OSAPI to move helper methods to the top of the class, improve their naming, improve code readability (in my opinion), and prepare for the aforementioned query efficiency improvements.

Revision history for this message

Brian Lamar (blamar) wrote on 2011-06-17:

PEP8 fixes: 8-9, 39-40, 48-49

Explanation in exceptions: 21-22, 30-31, 56-57, 151,

Return-code fixes: 601-602, 605-606, 657-658, 661-662, these return codes didn't make sense in their current form. If you're missing 'flavorId' is a bad request, not an unprocessable entity.

I've also added "request.context" and "request.reservation_id" to the wsgi.Request object, which accounts for a good deal of this MP.

It's a pretty big prop, and I wanted to make it smaller, so let me know if you think I should remove some things or do it differently.

Revision history for this message

Brian Waldon (bcwaldon) wrote on 2011-06-17:

The pep8 and explanation stuff is fine. I'm fine with the return-code fixes as long as they match the spec. I don't even like using 422, as is a webdav-specific and not in the core set of http exceptions.

I'm curious if we actually need the context object. I have no idea what it is doing for us.

I'm not too crazy about adding the reservation_id property to wsgi.Request. It doesn't seem to get used very much and it doesn't belong on the request object if the majority of the codes doesn't use it.

Also, you changed single quotes to double quotes. Personally, I use single quotes for tokens (mainly dict keys) and double quotes for things like HTTPException explanations. What is your take on this?

Revision history for this message

Brian Lamar (blamar) wrote on 2011-06-17:

request.context > request.environ["nova.context"] everywhere because hard-coded strings are bad. I'd rather write the hard-coded string in one place rather than in 1000 places.

request.reservation_id is the same things, even if it's only used in a small number of places, hard-coding a string like that is just something I try to stay away from. What happens when we need to validate it? Do we put validation code everywhere it's used?

I made reservation_id in the Request object to pave the way for putting 'limit', 'offset', and 'marker' in the Request object as well. It allows for centralization of validation logic for request parameters.

> Also, you changed single quotes to double quotes.

I don't care one way or the other. If I ruled the world I would make everything double quotes, but I don't want to hold up code for quotation marks so I'll just change them all back.

Revision history for this message

Brian Waldon (bcwaldon) wrote on 2011-06-17:

#agree