Merge lp://staging/~johannes.erdfelt/nova/lp844905 into lp://staging/~hudson-openstack/nova/trunk

Finding it hard to see that the new code is being properly covered by the tests.

Ideally I'd like to see explicit unit tests on each of the new methods added (and ensuring they hit each of the conditional branches).

At the very least, some description in the unit tests as to what they are actually testing and which methods are being touched. But ideally, simple single-method unit tests.

I'm not sure I understand what needs to be changed.

The unit tests check the two different methods to determine the best content type and the three different methods to determine the API version.

Each unit test checks a single piece of functionality, so I'm not sure I understand what you mean by "single-method unit tests".

Are you saying some code isn't being tested? The existing code being tested isn't tested well enough?

Wanted to echo removing comments and some of the formatting the others mentioned here.

I got some 3 failures with the S3APITestCase. Not sure if this is related or not yet.

Please also add yourself to the Authors file. This failed on the unit tests for me.

======================================================================
FAIL: test_authors_up_to_date (nova.tests.test_misc.ProjectTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/bzr/TrustedComputingPools/nova/tests/test_misc.py", line 93, in test_authors_up_to_date
    '%r not listed in Authors' % missing)
AssertionError: set([u'<email address hidden>']) not listed in Authors

meh, too many windows open.

I'm very curious why nova/api/openstack/urlmap.py is necessary. I feel like we can get most of this functionality out of webob, is there something I'm missing?

Since we have to do content-type checking up front now, can we guarantee it *always* gets done up front? It seems like best_match_content_type is now designed to allow late checking of that header.

paste deploy determines which versions of the API are available by using the configuration in etc/nova/api-paste.ini. This all happens before webob gets into the picture.

My original stab at this task modified nova.api.openstack.versions (which is the handler for / and thusly the default for any URL paths not prefixed with a version) instead. However, I had to duplicate the configuration of API versions, which I wanted to avoid.

The reason for best_match_content_type is covered in the description for the merge prop. As a summary, it's because the direct API uses some of the same WSGI stack but not paste deploy, and there are tests that deliberately operate on only part of the WSGI stack (for instance nova/tests/api/openstack/test_faults.py) and partially test the same functionality of urlmap (suffix content type tests).

I can't say I'm pleased with having two different methods of determining content type either. It might be possible to fix the direct API to use a different request object or maybe even use paste deploy. It might also be possible to mock up a different request object for the tests. I think there might be other problems too, I ran into a lot when I tried developing a patch that left best_match_content_type out.

It's guaranteed to get done up front if the paste configuration is correct. I'm not a fan of requiring configuration to get correct behavior, but there's already precedent for that (see the auth middleware) and I didn't want the scope to grow beyond the original bug.

Okay, I can definitely understand many of your concerns here. Maybe one day we will get away from paste and our problems will be solved ;)

As for the accept header parsing: I feel like the code in urlmap.py is duplicating functionality provided in webob.acceptparse (http://docs.webob.org/en/latest/modules/webob.html). Can you take a look at that and see if you agree with me?

It *does* duplicate the code in webob, but only because webob ignores all parameters except for 'q'. We need the 'version' parameter as well (which is kind of the purpose of the branch).

I should also mention that the Accept parsing code in this branch is based off of the code in Werkzeug. It's BSD licensed and appears to be compatible with the Apache license Openstack uses. I'm thinking I might need to move that code to a separate file to keep the license separate.

Okay, I didn't look close enough at acceptparse to notice it didn't return values other than quality. I'm going to approve with the understanding that some of this will get simplified in the future.

Moved to gerrit.