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

Proposed by Brian Lamar
Status: Merged
Approved by: Josh Kearney
Approved revision: 832
Merged at revision: 870
Proposed branch: lp:~blamar/nova/pylint-undefined
Merge into: lp:~hudson-openstack/nova/trunk
Diff against target: 67 lines (+7/-5)
3 files modified
nova/api/openstack/ (+4/-3)
nova/db/sqlalchemy/ (+2/-1)
nova/virt/ (+1/-1)
To merge this branch: bzr merge lp:~blamar/nova/pylint-undefined
Reviewer Review Type Date Requested Status
Josh Kearney (community) Approve
Devin Carlen (community) Approve
Review via email:

Commit message

Pylint 'Undefined variable' E0602 error fixes.

Description of the change

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'

To post a comment you must log in.
Revision history for this message
Brian Lamar (blamar) wrote :

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

Revision history for this message
justinsb (justin-fathomdb) wrote :

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.

Revision history for this message
Devin Carlen (devcamcar) wrote :

> 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
Revision history for this message
justinsb (justin-fathomdb) wrote :

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!)

Revision history for this message
Brian Lamar (blamar) wrote :

justinsb: 100% right on the instance_id.

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

Revision history for this message
Josh Kearney (jk0) wrote :


review: Approve
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

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

text conflict in nova/db/sqlalchemy/

Revision history for this message
Brian Lamar (blamar) wrote :

Fixed conflict.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/api/openstack/'
2--- nova/api/openstack/ 2011-03-11 19:49:32 +0000
3+++ nova/api/openstack/ 2011-03-24 17:55:38 +0000
4@@ -14,6 +14,7 @@
5 # under the License.
7 import common
8+import webob.exc
10 from nova import exception
11 from nova import flags
12@@ -51,10 +52,10 @@
13 raise exception.NotAuthorized(_("Not admin user."))
15 def index(self, req):
16- raise faults.Fault(exc.HTTPNotImplemented())
17+ raise faults.Fault(webob.exc.HTTPNotImplemented())
19 def detail(self, req):
20- raise faults.Fault(exc.HTTPNotImplemented())
21+ raise faults.Fault(webob.exc.HTTPNotImplemented())
23 def show(self, req, id):
24 """Return data about the given account id"""
25@@ -69,7 +70,7 @@
26 def create(self, req):
27 """We use update with create-or-update semantics
28 because the id comes from an external source"""
29- raise faults.Fault(exc.HTTPNotImplemented())
30+ raise faults.Fault(webob.exc.HTTPNotImplemented())
32 def update(self, req, id):
33 """This is really create or update."""
35=== modified file 'nova/db/sqlalchemy/'
36--- nova/db/sqlalchemy/ 2011-03-23 19:16:03 +0000
37+++ nova/db/sqlalchemy/ 2011-03-24 17:55:38 +0000
38@@ -2209,7 +2209,7 @@
39 filter_by(id=id).first()
40 if not result:
41 raise exception.NotFound(_("No migration found with id %s")
42- % migration_id)
43+ % id)
44 return result
47@@ -2432,6 +2432,7 @@
49 @require_admin_context
50 def zone_update(context, zone_id, values):
51+ session = get_session()
52 zone = session.query(models.Zone).filter_by(id=zone_id).first()
53 if not zone:
54 raise exception.NotFound(_("No zone with id %(zone_id)s") % locals())
56=== modified file 'nova/virt/'
57--- nova/virt/ 2011-03-23 05:58:52 +0000
58+++ nova/virt/ 2011-03-24 17:55:38 +0000
59@@ -344,7 +344,7 @@
60 Note that this function takes an instance ID, not a
61 compute.service.Instance, so that it can be called by compute.monitor.
62 """
63- return [0L, 0L, 0L, 0L, null]
64+ return [0L, 0L, 0L, 0L, None]
66 def interface_stats(self, instance_name, iface_id):
67 """