Code review comment for lp:~rohitagarwalla/neutron/l2network-plugin-db

Revision history for this message
Sumit Naiksatam (snaiksat) wrote :

Thanks, great job! Looks mostly good to me. I ran the tests provided with this code and they executed fine. Just a few small fixes suggested (some are nits):

1. As discussed in a separate thread, please move the code to the new "db' directory.

2. The quantum component is following the convention of "getting" a logger component and logging to that (so that the logs get flagged with the name of the component). Can you please do this in the modules in which you are writing to the logs? This is how to do it:

from quantum.plugins.cisco.common import cisco_constants as const

LOG.basicConfig(level=LOG.WARN)
LOG.getLogger(const.LOGGER_COMPONENT_NAME)

3. The requirement for the creation of "cisco_naas" DB should be documented in the README file. Please also provide instructions on how to create it.

4. Specifically on the name "cisco_naas", Ram earlier had the suggestion that it be called something else. I would definitely recommend getting away from "naas" since we are not using it any more. Maybe call it "quantum_l2network" since we are calling our plugin "l2network-plugin".

5. The nova coding guidelines require that imports be arranged in the alphabetical order. The following order does not seem correct to me, please check this (and other places as well):

+import ConfigParser
33 +import os
34 +import logging as LOG
35 +import unittest
36 +from optparse import OptionParser

More info on imports order can be found in the HACKING file in the nova branch.

review: Needs Fixing

« Back to merge proposal