Code review comment for lp:~cisco-openstack/neutron/l2network-plugin-persistence

Thanks for the review and comments Brad.

> 1171 + return vlanid
> 1172 + except exc.NoResultFound:
> 1173 + pass
>
> Indentation is off there
>
> 1244 + except exc.NoResultFound:
> 1245 + pass
>
> Same here
>
> 1315 + return pp
> 1316 + except exc.NoResultFound:
> 1317 + pass
>
> And here
>
> 1320 +def update_portprofile(tenantid, ppid, newppname=None,
> newvlanid=None, \
> 1321 + newqos=None):
>
> You don't need the "\" there
>

Fixed.

> 2454 + res[const.NET_PORTS] = ports
>
> I think you want const.NETWORKPORTS there?
>
> 1975 + const.NET_PORTS: []}
>
> (and here)
>
> 4911 + def _make_net_dict(self, net_id, net_name, ports):
> 4912 + res = {const.NET_ID: net_id, const.NET_NAME: net_name}
> 4913 + res[const.NET_PORTS] = ports
> 4914 + return res
>
> (and here)
>
> 2460 + res[const.NET_ID] = net_id
>
> There is const.NET_ID used in a lot of places when I think it should be
> const.NETWORKID .. but I could be missing something here. Maybe const.NET_ID
> is still defined somewher else.
>

NETWORKPORTS = 'ports'
NET_PORTS = 'net-ports'

NETWORKID = 'network_id'
NET_ID = 'net-id'

There are two different constants defined in cisco/common/cisco_constants.py.
As you would have seen, the make_dict methods create a dictionary with keys as excepted by the api (net-id in this case)
We couldn't define net-id (and others containing '-' ) as variables for the database models and therefore had to use different variables.

> 3182 +"""Unittest runner for quantum OVS plugin
>
> Might want to change that to Cisco plugin ;)
>
> 3438 +#from quantum.plugins.openvswitch.tests.test_vlan_map import
> VlanMapTest
>
> Might as well nuke that line.. don't think its ever going to be used in this
> file.
>
> 3740 +class QuantumDB(object):
> 3741 + """Class conisting of methods to call Quantum db methods"""
> 3742 + def get_all_networks(self, tenant_id):
>
> We should probably move this type of stuff into a library since other plugins
> will want to use it. Not an issue for this merge though.

Sure, we'll take this an action item.