Merge lp:~blamar/nova/pylint-undefined into lp:~hudson-openstack/nova/trunk

[Brian Lamar]

Pylint 'Undefined variable' E0602 error fixes.

Fixes for the following pylint E0602 errors:

************* Module nova.compute.api
E0602:482:API.confirm_resize: Undefined variable 'migration_id'
************* Module nova.api.openstack.accounts
E0602: 54:Controller.index: Undefined variable 'exc'
E0602: 57:Controller.detail: Undefined variable 'exc'
E0602: 72:Controller.create: Undefined variable 'exc'
************* Module nova.virt.libvirt_conn
E0602:1200:LibvirtConnection.live_migration: Undefined variable 'greenthread'
************* Module nova.virt.fake
E0602:326:FakeConnection.block_stats: Undefined variable 'null'
************* Module nova.db.sqlalchemy.migrate_repo.versions.008_add_instance_types
E0602: 58:upgrade: Undefined variable 'table'
E0602: 79:upgrade: Undefined variable 'table'
************* Module nova.db.sqlalchemy.api
E0602:2207:migration_get: Undefined variable 'migration_id'
E0602:2219:migration_get_by_instance_and_status: Undefined variable 'migration_id'
E0602:2430:zone_update: Undefined variable 'session'

[Brian Lamar (blamar)]

Removed my old proposal which had a bunch of different pylint fixes in favor of a couple smaller ones as per community suggestion.

[justinsb (justin-fathomdb)]

8 +import webob.exc

I think people prefer "from webob import exc", and then using exc.X instead of webob.exc.X, but this isn't my rule ;-)

63 raise exception.NotFound(_("No migration found with instance id %s")
64 - % migration_id)
65 + % id)

Should that be instance_id? (Note this is on the second migration_id error, not the first one)

Otherwise, lgtm. Thank you for this - it's pretty thankless work, but you're saving us from a lot of real bugs here. Fixing them like this is a lot 'cheaper' than the alternative of finding them in the wild.

[Devin Carlen (devcamcar)]

> 8 +import webob.exc
>
> I think people prefer "from webob import exc", and then using exc.X instead of
> webob.exc.X, but this isn't my rule ;-)

We don't have a hard rule on this that I'm aware of. I think there are places where it makes sense to use this form, and importing a module's exceptions is one of my favorites.

For readability's sake, I think:

except webob.exc.HttpNotImplemented():

is superior to

except exc.HttpNotImplemented()

because now I don't have to wonder which module's exceptions I'm working with.

Anyway it's all very minor and doesn't make a lot of difference in the end, but it's fine for us to use.

review: Approve

[justinsb (justin-fathomdb)]

I stand corrected - sorry Brian. Thanks for explaining Devin - I'll use that style of import where it clarifies ambiguity in future.

(I do think the second migration_id should be instance_id though!)

[Brian Lamar (blamar)]

justinsb: 100% right on the instance_id.

Sorry, I was on a brief vacation, but that should be fixed now. Thanks!

[Josh Kearney (jk0)]

lgtm

review: Approve

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in nova/db/sqlalchemy/api.py

[Brian Lamar (blamar)]

Fixed conflict.

=== modified file 'nova/api/openstack/accounts.py'
--- nova/api/openstack/accounts.py	2011-03-11 19:49:32 +0000
+++ nova/api/openstack/accounts.py	2011-03-24 17:55:38 +0000
@@ -14,6 +14,7 @@
 #    under the License.
 
 import common
+import webob.exc
 
 from nova import exception
 from nova import flags
@@ -51,10 +52,10 @@
             raise exception.NotAuthorized(_("Not admin user."))
 
     def index(self, req):
-        raise faults.Fault(exc.HTTPNotImplemented())
+        raise faults.Fault(webob.exc.HTTPNotImplemented())
 
     def detail(self, req):
-        raise faults.Fault(exc.HTTPNotImplemented())
+        raise faults.Fault(webob.exc.HTTPNotImplemented())
 
     def show(self, req, id):
         """Return data about the given account id"""
@@ -69,7 +70,7 @@
     def create(self, req):
         """We use update with create-or-update semantics
            because the id comes from an external source"""
-        raise faults.Fault(exc.HTTPNotImplemented())
+        raise faults.Fault(webob.exc.HTTPNotImplemented())
 
     def update(self, req, id):
         """This is really create or update."""

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-03-23 19:16:03 +0000
+++ nova/db/sqlalchemy/api.py	2011-03-24 17:55:38 +0000
@@ -2209,7 +2209,7 @@
                      filter_by(id=id).first()
     if not result:
         raise exception.NotFound(_("No migration found with id %s")
-                % migration_id)
+                % id)
     return result
 
 
@@ -2432,6 +2432,7 @@
 
 @require_admin_context
 def zone_update(context, zone_id, values):
+    session = get_session()
     zone = session.query(models.Zone).filter_by(id=zone_id).first()
     if not zone:
         raise exception.NotFound(_("No zone with id %(zone_id)s") % locals())

=== modified file 'nova/virt/fake.py'
--- nova/virt/fake.py	2011-03-23 05:58:52 +0000
+++ nova/virt/fake.py	2011-03-24 17:55:38 +0000
@@ -344,7 +344,7 @@
         Note that this function takes an instance ID, not a
         compute.service.Instance, so that it can be called by compute.monitor.
         """
-        return [0L, 0L, 0L, 0L, null]
+        return [0L, 0L, 0L, 0L, None]
 
     def interface_stats(self, instance_name, iface_id):
         """