Code review comment for lp:~gundlach/nova/controllers-in-api

Thanks so much for the review! Comments inline. A major theme is that some
things look weird because I intentionally did the minimal amount of touching
the old code while moving it into the new framework. I'd like to leave them
untouched for this merge, as the purpose is to move code off of Tornado onto
Eventlet, not to clean up all the style issues in the old code. Please push
back if you think this is inappropriate.

On Wed, Sep 8, 2010 at 9:37 AM, termie <email address hidden> wrote:

> 1. Please kill extra newlines at:
>

Done. Many of these are just newlines in the old code that I moved around
and purposefully didn't touch (to minimize the changes I was making), but I
took care of them anyway.

> 2. Line up the arguments inside the parens in all the added methods in
> nova/image/service.py.

I didn't modify the spacing of arguments in the old code.

> Additionally it doesn't look like the "qs" function exists that file in the
> branch I am looking at, are there tests written for these methods?.
>

Good catch! There are *no* tests written covering this part of the EC2
Images-related API, apparently, since the unit tests passed with this broken
code.

This change actually snuck in to my branch accidentally, and I left it in
because I thought it was harmless. As I'm not in the business of cleaning
up (or particularly understanding!) the EC2 API right now, but rather moving
it to Eventlet, I have simply fixed the qs() reference. If you disagree
with this I will just excise the S3ImageService from this mergeprop and
leave it for a separate branch. Let me know.

> 3. Same as #2 but for nova/api/ec2/images.py, however it _does_ have a qs
> function defined, which seems like it should be replaced by urllib.urlencode
> anyway.
>

Probably -- again, I'm not in the business of improving the old code, but
just moving it.

> 4. Extra newline at nova/api/ec2/context.py line 1020
>

Done

> 5. In nova/api/ec2/cloud.py please make the formatting in create_volume
> match that of the other calls in that file, namely moving the {"method"...
> section to the next line and double indenting it.
>

This is old code, and I don't actually see any pattern in the layout of the
many rpc.call()s in that file (there are several layouts). I lined it up to
look less egregious.

> 6. Also in nova/api/ec2/cloud.py please line up the indentation in
> create_key_pair's return statement.
>

Done

> 7. Please include your name in the TODO added in APIRequest's send method.
>
> e.g. TODO(gundlach): foo
>
> Unless this is just a renamed file being treated as added, in which case I
> guess
> we leave it as an unclaimed TODO.
>

It is a renamed file being treated as added. AKA, I didn't write that TODO.

> 8. Please format your docstring for the Router, Authorizer, Executor class
> with the format
>
> """Single line summary of this class.
>
> Additional details about this class.
> """
>

Oops, duh, thanks. Done. Also,
http://www.python.org/dev/peps/pep-0257/ claims
that you can put a \n before the word "Single" in your example above... is
that not kosher? For expediency I've changed the file to put the summary on
the same line as the triple-quote.

> 9. If the args in Authenticate's __call__ method could all fit indented to
> the end of manager.AuthManager().authenticate( that would be grand, but if
> they can't could you double indent them and make the last parens either be
> on the last line or lined up with rest of the words (as it stands it looks
> like the end of the "(user" parens.
>

This is old code that I didn't touch. Good call, though, and I made the
change.

> Aaaaanyway, besides all that minor stuff it looks pretty stellar, please
> take the time to make sure that the stuff in #2 is being covered by tests,
> because it seems like any test using it would be failing since qs is not
> defined.
>

As I said above, I fixed the reference errors and left it without unit
tests, as this isn't my code (I just extracted some of it into a class.) If
that's not satisfactory I'll just revert the change for the sake of this
mergeprop -- let me know.

> Also consider replacing qs with urllib.urlencode.
>

Not in this branch! ;)

> Really happy to be getting rid of Tornado and having better names for our
> stuff :)
>

Yeah, me too. Thanks for doing this epic code review.

Michael

Confidentiality Notice: This e-mail message (including any attached or
embedded documents) is intended for the exclusive and confidential use of the
individual or entity to which this message is addressed, and unless otherwise
expressly indicated, is confidential and privileged information of Rackspace.
Any dissemination, distribution or copying of the enclosed material is prohibited.
If you receive this transmission in error, please notify us immediately by e-mail
at <email address hidden>, and delete the original message.
Your cooperation is appreciated.