Code review comment for lp://qastaging/~raxnetworking/nova/melange

> 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 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.

>
> > 1911 + LOG.debug(traceback.format_exc())
> >
> > LOG.exception should work here.
> > 1915 + LOG.debug(traceback.format_exc())
> >
> > LOG.exception should work here.
> In both these places, we dont want to log the message as an application
> exception.
> These could be errors such as HTTPNotFound, validation errors etc that
> shouldn't look
> like there was an exception while processing the request.

Understood.

>
> > 2523 +# template repository default module
> >
> > Unsure what this means.
> Same here :-). This was a remnant from the sqlalchemy migration repo folder we
> copied from glance. Ill remove this comment, it doenst seem to mean anything.

I figured it was just an existing thing, thanks!