Merge lp:~openstack-gd/nova/lp783478 into lp:~hudson-openstack/nova/trunk

[Eldar Nugaev]

print information about nova-manage project problems

Added catching NotFound errors and print information
User %user% could not be found
Project %project% could not be found

[Brian Lamar (blamar)]

While pedantic, pylint is going to complain about single letter "e" variables. I think many people have been using "ex". Also, can you add example text to the bug report or here showing what this looks like before and after. I think that should suffice as I'm not sure it's practical to automatically test bin/nova-manage.

[Ed Leafe (ed-leafe)]

> While pedantic, pylint is going to complain about single letter "e" variables.
> I think many people have been using "ex".

Catching an exception using the name 'e' is probably the most common usage in Python; the second most common is 'err', although I dislike that as it confuses errors and exceptions. I can't say that I've ever seen 'ex' used outside of some places in nova. I would be much more concerned about using the older structure of 'except SomeException, e:' (separating the class and the exception object with a comma); this syntax should not be used; instead, it would be much more preferable to use the 'except SomeException as e:'.

[Vish Ishaya (vishvananda)]

Yes, the ex change was because of pylint. Maybe we should just tell pylint to shut up about single letter variable names?

On May 16, 2011, at 10:51 AM, Ed Leafe wrote:

>> While pedantic, pylint is going to complain about single letter "e" variables.
>> I think many people have been using "ex".
>
> Catching an exception using the name 'e' is probably the most common usage in Python; the second most common is 'err', although I dislike that as it confuses errors and exceptions. I can't say that I've ever seen 'ex' used outside of some places in nova. I would be much more concerned about using the older structure of 'except SomeException, e:' (separating the class and the exception object with a comma); this syntax should not be used; instead, it would be much more preferable to use the 'except SomeException as e:'.
> --
> https://code.launchpad.net/~openstack-gd/nova/lp783478/+merge/61126
> You are subscribed to branch lp:nova.

[Ilya Alekseyev (ilyaalekseyev)]

Not sure is it important to disable usage of single letter variables, but seems patch solving the problem.
So, lgtm.

review: Approve

[Brian Lamar (blamar)]

> Yes, the ex change was because of pylint. Maybe we should just tell pylint to
> shut up about single letter variable names?

I'd be opposed to allowing single-letter variable names everywhere though, it just makes for poorly readable code. Exception handling should be short and sweet in most cases and I don't want pylint one-off ignores next to all of the "except Exception, e" so my recommendation for "ex" stands.

[Eldar Nugaev (reldan)]

Ok. No problem. Fixed

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Rick Harris (rconradharris)]

Generally agree w/ Brian that single-letter vars are bad form; though, to me at least, it seems like there is a lazy-consensus in the Python community that the following cases are acceptable:

 * Exception handlers (except Exception as `e`)
 * List/Generator comprehensions (e.g. [h.name for h in hosts])
 * Index variables (`i` and `j`)

Since pylint doesn't seem to be smart enough to distinguish between these cases, I'd vote we disable that rule and continue our current process of keeping it the reviewer's call.

It'd be a shame if pylint led to *less* idiomatic code.

[Matt Dietz (cerberus)]

Also agree on the issue of single letter variable names. While there are cases where I think it makes sense, as detailed above, I don't want to open the floodgates to lazy developers.

review: Approve

[Eldar Nugaev (reldan)]

Thank you, Matt.
Could you please change status to approved?

[OpenStack Infra (hudson-openstack)]

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

text conflict in bin/nova-manage

[Vish Ishaya (vishvananda)]

looks like this needs a trunk merge.

review: Needs Fixing

[Eldar Nugaev (reldan)]

Thanks Vish.
Merged with trunk and conflicts resolved

[lp:~openstack-gd/nova/lp783478]

1076. By Eldar Nugaev on 2011-05-20

Merge and conflict resolving

[Vish Ishaya (vishvananda)]

review: Approve

=== modified file 'bin/nova-manage'
--- bin/nova-manage	2011-05-17 16:15:21 +0000
+++ bin/nova-manage	2011-05-20 09:34:30 +0000
@@ -362,27 +362,47 @@
     def add(self, project_id, user_id):
         """Adds user to project
         arguments: project_id user_id"""
-        self.manager.add_to_project(user_id, project_id)
+        try:
+            self.manager.add_to_project(user_id, project_id)
+        except exception.UserNotFound as ex:
+            print ex
+            raise
 
     def create(self, name, project_manager, description=None):
         """Creates a new project
         arguments: name project_manager [description]"""
-        self.manager.create_project(name, project_manager, description)
+        try:
+            self.manager.create_project(name, project_manager, description)
+        except exception.UserNotFound as ex:
+            print ex
+            raise
 
     def modify(self, name, project_manager, description=None):
         """Modifies a project
         arguments: name project_manager [description]"""
-        self.manager.modify_project(name, project_manager, description)
+        try:
+            self.manager.modify_project(name, project_manager, description)
+        except exception.UserNotFound as ex:
+            print ex
+            raise
 
     def delete(self, name):
         """Deletes an existing project
         arguments: name"""
-        self.manager.delete_project(name)
+        try:
+            self.manager.delete_project(name)
+        except exception.ProjectNotFound as ex:
+            print ex
+            raise
 
     def environment(self, project_id, user_id, filename='novarc'):
         """Exports environment variables to an sourcable file
         arguments: project_id user_id [filename='novarc]"""
-        rc = self.manager.get_environment_rc(user_id, project_id)
+        try:
+            rc = self.manager.get_environment_rc(user_id, project_id)
+        except (exception.UserNotFound, exception.ProjectNotFound) as ex:
+            print ex
+            raise
         with open(filename, 'w') as f:
             f.write(rc)
 
@@ -399,7 +419,7 @@
         if key:
             try:
                 db.quota_update(ctxt, project_id, key, value)
-            except exception.NotFound:
+            except exception.ProjectQuotaNotFound:
                 db.quota_create(ctxt, project_id, key, value)
         project_quota = quota.get_quota(ctxt, project_id)
         for key, value in project_quota.iteritems():
@@ -408,7 +428,11 @@
     def remove(self, project_id, user_id):
         """Removes user from project
         arguments: project_id user_id"""
-        self.manager.remove_from_project(user_id, project_id)
+        try:
+            self.manager.remove_from_project(user_id, project_id)
+        except (exception.UserNotFound, exception.ProjectNotFound) as ex:
+            print ex
+            raise
 
     def scrub(self, project_id):
         """Deletes data associated with project
@@ -427,6 +451,9 @@
             zip_file = self.manager.get_credentials(user_id, project_id)
             with open(filename, 'w') as f:
                 f.write(zip_file)
+        except (exception.UserNotFound, exception.ProjectNotFound) as ex:
+            print ex
+            raise
         except db.api.NoMoreNetworks:
             print _('No more networks available. If this is a new '
                     'installation, you need\nto call something like this:\n\n'