Merge lp:~retr0h/nova/lp705131 into lp:~hudson-openstack/nova/trunk

[John Dewey]

Make unit tests clean up their mess in /tmp after themselves.

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Vish Ishaya (vishvananda)]

Although the old code wasn't doing this, I feel like the directory (and any leftover files) should be cleaned up after they are used. I don't like the tests leaving around random tmp dirs.

review: Needs Fixing

[Vish Ishaya (vishvananda)]

Nice. In general we prefer no space before the """ comments, so
"""This comment is better."""
"""
   Than this one
"""

On Jan 25, 2011, at 5:27 PM, John Dewey wrote:

> John Dewey has proposed merging lp:~retr0h/nova/lp705131 into lp:nova.
>
> Requested reviews:
> Devin Carlen (devcamcar)
> Vish Ishaya (vishvananda)
> Related bugs:
> #705131 LocalImageService tests fail for user "B" if user "A" has previously run test on same machine
> https://bugs.launchpad.net/bugs/705131
>
> For more details, see:
> https://code.launchpad.net/~retr0h/nova/lp705131/+merge/47491
> --
> https://code.launchpad.net/~retr0h/nova/lp705131/+merge/47491
> You are requested to review the proposed merge of lp:~retr0h/nova/lp705131 into lp:nova.
> === modified file 'nova/image/local.py'
> --- nova/image/local.py 2010-12-03 21:50:30 +0000
> +++ nova/image/local.py 2011-01-26 01:27:27 +0000
> @@ -18,6 +18,7 @@
> import cPickle as pickle
> import os.path
> import random
> +import tempfile
>
> from nova import exception
> from nova.image import service
> @@ -30,11 +31,7 @@
> It assumes that image_ids are integers."""
>
> def __init__(self):
> - self._path = "/tmp/nova/images"
> - try:
> - os.makedirs(self._path)
> - except OSError: # Exists
> - pass
> + self._path = tempfile.mkdtemp()
>
> def _path_to(self, image_id):
> return os.path.join(self._path, str(image_id))
> @@ -65,7 +62,9 @@
> return id
>
> def update(self, context, image_id, data):
> - """Replace the contents of the given image with the new data."""
> + """
> + Replace the contents of the given image with the new data.
> + """
> try:
> pickle.dump(data, open(self._path_to(image_id), 'w'))
> except IOError:
> @@ -82,7 +81,13 @@
>
> def delete_all(self):
> """
> - Clears out all images in local directory
> + Clears out all images in local directory.
> """
> for id in self._ids():
> os.unlink(self._path_to(id))
> +
> + def delete_imagedir(self):
> + """
> + Deletes the local directory. Raises OSError if directory is not empty.
> + """
> + os.rmdir(self._path)
>
> === modified file 'nova/tests/api/openstack/test_images.py'
> --- nova/tests/api/openstack/test_images.py 2011-01-12 19:20:05 +0000
> +++ nova/tests/api/openstack/test_images.py 2011-01-26 01:27:27 +0000
> @@ -143,6 +143,7 @@
>
> def tearDown(self):
> self.service.delete_all()
> + self.service.delete_imagedir()
> self.stubs.UnsetAll()
>
>
>

[Rick Harris (rconradharris)]

lgtm

review: Approve

[Soren Hansen (soren)]

lgtm, although I took the liberty of adding a commit message. Can't merge stuff without it.

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

=== modified file 'nova/image/local.py'
--- nova/image/local.py	2010-12-03 21:50:30 +0000
+++ nova/image/local.py	2011-01-26 02:21:46 +0000
@@ -18,6 +18,7 @@
 import cPickle as pickle
 import os.path
 import random
+import tempfile
 
 from nova import exception
 from nova.image import service
@@ -26,15 +27,12 @@
 class LocalImageService(service.BaseImageService):
 
     """Image service storing images to local disk.
+    It assumes that image_ids are integers.
 
-    It assumes that image_ids are integers."""
+    """
 
     def __init__(self):
-        self._path = "/tmp/nova/images"
-        try:
-            os.makedirs(self._path)
-        except OSError:  # Exists
-            pass
+        self._path = tempfile.mkdtemp()
 
     def _path_to(self, image_id):
         return os.path.join(self._path, str(image_id))
@@ -56,9 +54,7 @@
             raise exception.NotFound
 
     def create(self, context, data):
-        """
-        Store the image data and return the new image id.
-        """
+        """Store the image data and return the new image id."""
         id = random.randint(0, 2 ** 31 - 1)
         data['id'] = id
         self.update(context, id, data)
@@ -72,8 +68,9 @@
             raise exception.NotFound
 
     def delete(self, context, image_id):
-        """
-        Delete the given image.  Raises OSError if the image does not exist.
+        """Delete the given image.
+        Raises OSError if the image does not exist.
+
         """
         try:
             os.unlink(self._path_to(image_id))
@@ -81,8 +78,13 @@
             raise exception.NotFound
 
     def delete_all(self):
-        """
-        Clears out all images in local directory
-        """
+        """Clears out all images in local directory."""
         for id in self._ids():
             os.unlink(self._path_to(id))
+
+    def delete_imagedir(self):
+        """Deletes the local directory.
+        Raises OSError if directory is not empty.
+
+        """
+        os.rmdir(self._path)

=== modified file 'nova/tests/api/openstack/test_images.py'
--- nova/tests/api/openstack/test_images.py	2011-01-12 19:20:05 +0000
+++ nova/tests/api/openstack/test_images.py	2011-01-26 02:21:46 +0000
@@ -143,6 +143,7 @@
 
     def tearDown(self):
         self.service.delete_all()
+        self.service.delete_imagedir()
         self.stubs.UnsetAll()