Merge lp:~jtran/nova/lp702580 into lp:~hudson-openstack/nova/trunk

[John Tran]

Adds proper error handling for images that can't be found and a test for deregister image.

[Devin Carlen (devcamcar)]

Merge conflict:

37 +<<<<<<< TREE
38 +=======
39 +from nova.objectstore import image
40 +from nova.exception import NotEmpty, NotFound
41 +>>>>>>> MERGE-SOURCE

I don't think this is a good way to handle raising the exception. You lose the underlying stack trace this way and I'm not sure what value catching and raising a new exception of the same type is.

21 + try:
22 + image = self._get_image(context, image_id)
23 + except exception.NotFound:
24 + raise exception.NotFound(_('Image %s not found') %
25 + image_id)

Looks like the best way to handle would be to fix the underlying exceptions being raised.

review: Needs Fixing

[John Tran (jtran)]

I figured it would be good to keep consistent w/ the prior code base. See nova/api/ec2/cloud.py:describe_images.

[Vish Ishaya (vishvananda)]

Yes the issue here is that we are wrapping the underlying exception with a useful error message so that it can be sent to the client. There is handling in the wsgi app for displaying NotFound, although we should perhaps call it ApiNotFound or something.

Vish

On Mar 28, 2011, at 12:08 PM, John Tran wrote:

> I figured it would be good to keep consistent w/ the prior code base. See nova/api/ec2/cloud.py:describe_images.
> --
> https://code.launchpad.net/~jtran/nova/lp702580/+merge/55209
> You are subscribed to branch lp:nova.

[termie (termie)]

- image no longer exists in objectstore, it was removed in the evnetlet_objectstore branch
- the branch also removed the usage of NotFound, replacing with exception.NotFound as per HACKING
- you should be using comments, not triple-quoted strings to comment your code in test_deregister_image

review: Needs Fixing

[termie (termie)]

- instead of from nova.exception import NotFound you should be using from nova import exception and then referencing exception.NotFound

review: Needs Fixing

[John Tran (jtran)]

> - instead of from nova.exception import NotFound you should be using from nova
> import exception and then referencing exception.NotFound

Thanks for the feedback. I've submitted the fix.

[Vish Ishaya (vishvananda)]

lgtm

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Brian Lamar (blamar)]

Is this still valid, or did this fix the same issue:

https://code.launchpad.net/~vishvananda/nova/lp759053/+merge/57381

[Vish Ishaya (vishvananda)]

no this is different, fixes deregister and includes test. Looks like termie's concerns were addressed as well. Sending this off to Merge bot.

[OpenStack Infra (hudson-openstack)]

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

text conflict in nova/tests/test_cloud.py

[Jesse Andrews (anotherjesse)]

John - can you merge trunk and we can try again?

[termie (termie)]

lgtm, btw

review: Approve

[OpenStack Infra (hudson-openstack)]

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

text conflict in nova/tests/test_cloud.py

[lp:~jtran/nova/lp702580]

852. By John Tran on 2011-04-25

merged from trunk

[Brian Waldon (bcwaldon)]

12: Can you replace this exception with "ImageNotFound"? If you merge trunk, you'll see an example in describe_image_attributes.

review: Needs Fixing

[lp:~jtran/nova/lp702580]

853. By John Tran on 2011-05-12

merged from trunk

854. By John Tran on 2011-05-12

incorporated ImageNotFound instead of NotFound

=== modified file 'nova/tests/test_cloud.py'
--- nova/tests/test_cloud.py	2011-05-05 03:50:58 +0000
+++ nova/tests/test_cloud.py	2011-05-12 19:52:47 +0000
@@ -279,6 +279,26 @@
                                            user_group=['all'])
         self.assertEqual(True, result['is_public'])
 
+    def test_deregister_image(self):
+        deregister_image = self.cloud.deregister_image
+
+        def fake_delete(self, context, id):
+            return None
+
+        self.stubs.Set(local.LocalImageService, 'delete', fake_delete)
+        # valid image
+        result = deregister_image(self.context, 'ami-00000001')
+        self.assertEqual(result['imageId'], 'ami-00000001')
+        # invalid image
+        self.stubs.UnsetAll()
+
+        def fake_detail_empty(self, context):
+            return []
+
+        self.stubs.Set(local.LocalImageService, 'detail', fake_detail_empty)
+        self.assertRaises(exception.ImageNotFound, deregister_image,
+                          self.context, 'ami-bad001')
+
     def test_console_output(self):
         instance_type = FLAGS.default_instance_type
         max_count = 1