Merge lp://qastaging/~raxnetworking/nova/melange into lp://qastaging/~hudson-openstack/nova/trunk

As discussed in last week meeting, this should wait for Essex opening -- September 8.

This is quite a lengthy review for a lengthy merge request.

I tried to be as pedantic as possible, so you're going to see a LOT of me repeating myself. Overall the code looks pretty good from a style perspective although I haven't even started testing the code for functionality, so this review is all about style.

Top issues:

1) Newlines between imports and the copyright header. For some reason no separation here makes me bonkers. I don't think this is a PEP8 requirement, but it seems to be consistent with Nova to have a space here.

2) Importing classes and methods. Having imports only deal with modules makes things easier down the line.

-----------------

96 +# under the License.
97 +import gettext

Please separate with an empty line.

112 +gettext.install('melange', unicode=1)

In nova, we put this in nova/__init__.py because it was used so frequently. I think it makes sense for melange to do the same.

119 +def create_options(parser):
120 + """
121 + Sets up the CLI and config-file options that may be
122 + parsed and program commands.
123 + :param parser: The option parser
124 + """

According the HACKING, this docstring should read something along the lines of:

def create_options(parser):
    """Adds needed options to the given parser.

    :param parser: The option parser to add options to.
    :returns: None

    """

134 + oparser = optparse.OptionParser(version='%%prog VERSION')

Should there be a VERSION substitute here?

144 + except RuntimeError, e:

As a rule for Python future compatibility, I would suggets this be replaced with:

except RuntimeError as error:

In general replace all commas in 'except' statements with "as" and try not to use single letter names.

168 +"""
169 + CLI interface for IPAM.
170 +"""
171 +import gettext

Space to look like:

# under the License.

"""CLI interface for IPAM."""

import gettext

185 +gettext.install("melange", unicode=1)

Same gettext suggestion as earlier.

188 +from melange.common.auth import KeystoneClient
189 +from melange.common.client import HTTPClient
190 +from melange.common.utils import MethodInspector
191 +from melange.ipam.client import (IpBlockClient, SubnetClient, PolicyClient,
192 + UnusableIpOctetsClient,
193 + UnusableIpRangesClient)

No importing classes, and only one import per line.

196 +def create_options(parser):
197 + """
198 + Sets up the CLI and config-file options that may be
199 + parsed and program commands.
200 +
201 + :param parser: The option parser
202 + """

Fix docstring according to HACKING.

237 + """
238 + Returns the parsed CLI options, command to run and its arguments, merged
239 + with any same-named options found in a configuration file
240 +
241 + :param parser: The option parser
242 + """

Fix docstring according to HACKING.

252 +%prog category action [args] [options]"

Not sure if the " should be at the end there.

262 +categories = dict(ip_block=IpBlockClient, subnet=SubnetClient,
263 + policy=PolicyClient, unusable_ip_range=UnusableIpRangesClient,
264 + unusable_ip_octet=Unu...

Thanks Brain, for taking the time out for such a thorough review.
Wish I had adhered to the HACKING guide better :-)

We are halfway through fixing the style and other issues you raised.
Also fixing these style issues for the rest of the melange code that you
couldnt go through.

I though I would elaborate on only those issues that need a little more
discussion and not mention the ones that we are fixing anyway:

> 967 +from openstack.common.config import (parse_options,
> 968 + add_log_options,
> 969 + add_common_options,
> 970 + load_paste_config,
> 971 + setup_logging,
> 972 + load_paste_app, get_option)
>
> I'm not sure how you're running this right now as there isn't an official
> openstack.common package/module. I'm guessing you're just using the openstack-
> skeleton project? I hope to get the openstack-common initiative on the road
> soon so.

We are using openstack.common from the skeleton project. We made a few changes to
it so that we could use it in Melange. We are also looking to move extensions and
default serializers to openstack.common so that we can remove most of the copy-paste
code that sits in Melange today.
To allow Melange to access the skeleton projects, we have done the following in
tools/install_venv.py:
+ run_command(['tools/with_venv.sh', 'pip', 'install', '-E', venv, '-e',
+ "git+https://github.com/rajarammallya/"
+ "openstack-skeleton.git#egg=skeleton"],
+ redirect_output=False)

Of course, we will replace the reference to the personal git branch with the
main git branch for the skeleton project once we get all our changes approved
in the main git branch.
I suppose this can be changed to a pip_requires once there is a package ready for
openstack.common

> 967 +from openstack.common.config import (parse_options,
> 968 + add_log_options,
> 969 + add_common_options,
> 970 + load_paste_config,
> 971 + setup_logging,
> 972 + load_paste_app, get_option)
> These lines, however, need to import the module, not the individual methods.
> (so from openstack.common import config)
> Actually, these lines are not currently used? So this import can actually go
> away completely?

This is an area we wanted some feedback on. We weren't sure if this way of doing things is
very unorthodox in python.
Importing openstack.common classes/functions into the corresponding melange.common module
allows the rest of the application to use the openstack common methods through melange.common
The rest of the app is unaware of wether the common stuff is coming from
openstack-common or is an overridden version from melange-common.
We will always have some classes from openstack.common that would be overriden to tailor it to
a particular project's needs. This may cause some confusion and ...

> We are halfway through fixing the style and other issues you raised.
> Also fixing these style issues for the rest of the melange code that you
> couldnt go through.

It's very much appreciated! Thank you.

> I though I would elaborate on only those issues that need a little more
> discussion and not mention the ones that we are fixing anyway:

Perfect, some things were personal preference so this is appreciated.

> This is an area we wanted some feedback on. We weren't sure if this way of
> doing things is
> very unorthodox in python.
> Importing openstack.common classes/functions into the corresponding
> melange.common module
> allows the rest of the application to use the openstack common methods through
> melange.common
> The rest of the app is unaware of wether the common stuff is coming from
> openstack-common or is an overridden version from melange-common.
> We will always have some classes from openstack.common that would be overriden
> to tailor it to
> a particular project's needs. This may cause some confusion and erroneous
> usage in the application
> if it ends up using the openstack.common class directly instead of an
> overriden class in
> melange.common. That is why we have done this. Is this too unconventional or
> do you see problems in this approach?

Limiting coupling points like this is something that I do myself and I didn't quite catch on that you were doing it for that reason. I'm fine with that strategy in general, although I don't know you gain a lot by doing it here.

I would suggest going this route to comply with HACKING:

from openstack.common import config

parse_options = config.parse_options
add_log_options = config.add_log_options
add_common_options = config.add_common_options
load_paste_config = config.load_paste_config
setup_logging = config.setup_logging
load_paste_app = config.load_paste_app
get_option = config.get_option

Although at this point you're not pylint variable name compliant because module-level labels need to be UpperCamelCase I believe.

> This is done for unit tests. We have a StubConfig class in unit tests that is
> used
> as follows:
> with unit.StubConfig(some_config_key="blah"):
> #access functions which read some_config_key
> #and assert that they ended up with the "blah" value
>
> StubConfig is able to override the value of some_config_key by resetting
> Config.instance within the "with" block.
> In the actual code, we are only setting Config.instance once, during app
> creation, so I dont see this becoming a problem. Do you think otherwise?

I'm fine with this for now. I'd love to have a hardcore revist when we put configuration management into common, but this is a valid strategy for the time being.

> HTTPNotAcceptable because the version might be coming from the ACCEPT headers
> as well.
> (The openstack Api1.1 spec mentioned that versions can come from a header,
> hence
> we also parse accept headers looking for a version)

This is a fine error if it is talking about headers but I don't think the method knows if it's parsed the API version from headers or from the URL right? If the API version is given in the Accept header and it's w...

> Limiting coupling points like this is something that I do myself and I didn't
> quite catch on that you were doing it for that reason. I'm fine with that
> strategy in general, although I don't know you gain a lot by doing it here.
>
> I would suggest going this route to comply with HACKING:
>
> from openstack.common import config
>
>
> parse_options = config.parse_options
> add_log_options = config.add_log_options
> add_common_options = config.add_common_options
> load_paste_config = config.load_paste_config
> setup_logging = config.setup_logging
> load_paste_app = config.load_paste_app
> get_option = config.get_option
>
>
> Although at this point you're not pylint variable name compliant because
> module-level labels need to be UpperCamelCase I believe.

Nice approach, and more than HACKING compliance, I like the way this makes things more explicit.
With this, there wont be any misunderstandings about these being unused imports.
We'll use this approach, even though it would cause pylint voilations.

> > HTTPNotAcceptable because the version might be coming from the ACCEPT
> headers
> > as well.
> > (The openstack Api1.1 spec mentioned that versions can come from a header,
> > hence
> > we also parse accept headers looking for a version)
>
> This is a fine error if it is talking about headers but I don't think the
> method knows if it's parsed the API version from headers or from the URL
> right? If the API version is given in the Accept header and it's wrong then
> this is valid, if the API version is parsed from the URL then this isn't a
> valid exception. Not sure if that case is covered but I did forget that the
> version might be specified in the Accept header so there are absolutely cases
> where this works.

The HTTPNotAcceptable is raised only when accept headers contain the versions.
If the version is specified in the url, then it doesn't get into this code path at all.
Sorry if my comments might have led you to misunderstand this.

Essex is open !