Merge lp:~citrix-openstack/glance/iso-boot into lp:~hudson-openstack/glance/trunk

[Donal Lafferty]

Added new disk_format type of 'iso'. Nova can use this information to identify images that have to be booted from a CDROM.

Updated unittests so that next available image id is no longer a magic number. Previously, it was not possible to add new records into the image database stub without breaking many, many tests.

Updated docs in three locations where 'raw' disk format was expalined. However, no examples of 'iso' were in headers or API calls added.

[Jay Pipes (jaypipes)]

Nice work, Donal! Thanks for your work in refactoring the unit tests to make them more flexible.

One tiny thing, though. Please update the documentation for disk_formats for both these places:

http://glance.openstack.org/formats.html
http://glance.openstack.org/glanceapi.html

You can find these in the source tree under /doc/source/formats.rst and /doc/source/glanceapi.rst.

Thanks!
jay

review: Needs Fixing

[Donal Lafferty (donal-lafferty)]

Updated docs in three locations where 'raw' disk format was expalined. However, no examples of 'iso' were in headers or API calls added.

[Jay Pipes (jaypipes)]

lgtm.

review: Approve

[Brian Waldon (bcwaldon)]

64: Make this say ":returns:" (per sphinx docstring guidelines)

To avoid going through the codebase and updating tests that depended on the existing set of fixtures, I add temporary fixtures within each of my tests. This allows tests to be much more robust and explicit about what they are expecting. Notice we do not have a fixture that represents every combination of disk_format/container_format. You could make a database api create call at the start of the tests you added rather than add something to the core fixtures list. I know it didn't affect any other tests with regards to logic (because you made it private), but it did cause you to have to implement the next_image_id concept.

128,146,164,182,200...: if you decide to keep the extra fixture (and the concept of next_image_id), it seems counter-intuitive to have to add a '+ n' to the end of next_image_id after adding 'n' images. I feel like next_image_id should be incremented for you (maybe it should be a function). Being static doesn't really give us much.

review: Needs Fixing

[Jay Pipes (jaypipes)]

Hi again, Donal.

After further consideration, I agree with Brian's assessment. A more appropriate method would be to add a specific fixture to the test case you added around the iso disk_format.

-jay

review: Needs Fixing

[Donal Lafferty (donal-lafferty)]

Hi guys,

Brian's point seems to be that the tests should operate in isolation. This offers robustness and explicitness. Take the example of his work with temporary fixures. The robustness seems to come from there not being any code shared with other tests. Explicitness is due to colocating test and fixtures, IMO.

The advantage in temporary fixtures is offset by copy/paste reuse and magic numbers. Temporary fixtures are used in a number of locations, but their reuse requries a copy/paste between test routines. Perhaps this is a good idea of distributed development, because it allows each test to be siloed. However, the usual problems of copy/paste apply. You can't easily change the image schema without modification of every single test. Also, the way the current fixutres database is structured, the only way to obtain a unused image number is through magic numbers. This prevents the quick aggregation of disk_format/container_format test cases. Therefore, our temporary fixtures seem to violate important programming concepts.

Removing next_image_id does not improve on the code on these points. Removing the next_image_id introduces global magic numbers where only local magic numbers existed. The opportunity to avoid copy/paste uses cases is removed, because the central fixtures database cannot aggregate new tests. For these reasons, next_image_id represented a step forward, albeit a limited one.

That said, I'm new to unittest writing for Python and in an open source environment. I'm happy to revise the proposed code. On the odd chance that anyone changes their mind, I'll wait until tomorrow to revise my changes.

Cheers,

DL

[Brian Waldon (bcwaldon)]

> The advantage in temporary fixtures is offset by copy/paste reuse and magic
> numbers. Temporary fixtures are used in a number of locations, but their
> reuse requries a copy/paste between test routines. Perhaps this is a good
> idea of distributed development, because it allows each test to be siloed.
> However, the usual problems of copy/paste apply. You can't easily change the
> image schema without modification of every single test. Also, the way the
> current fixutres database is structured, the only way to obtain a unused image
> number is through magic numbers. This prevents the quick aggregation of
> disk_format/container_format test cases. Therefore, our temporary fixtures
> seem to violate important programming concepts.

These are great points that I did not fully consider in my initial review. Thanks for bringing them up.

> Removing next_image_id does not improve on the code on these points. Removing
> the next_image_id introduces global magic numbers where only local magic
> numbers existed. The opportunity to avoid copy/paste uses cases is removed,
> because the central fixtures database cannot aggregate new tests. For these
> reasons, next_image_id represented a step forward, albeit a limited one.

Maybe to cut down on the copy/paste issues we could use an image factory that allows us to build image objects and add them to the fixtures without having to define the image object interface all over the place. We could allow that factory to handle the magic id problem, while also allowing us to override attributes we need for a specific test.

I am also not in love with having a set of "fixed" fixtures to deal with like we do now. Maybe making it easy enough to add fixtures without copy/pasting dictionaries every where would allow us to remove them? I know it is convenient to not have to define your fixtures in every test, but I see it as being much more powerful to have total control over it. We could also do it in the setUp/tearDown, not every test method.

> That said, I'm new to unittest writing for Python and in an open source
> environment. I'm happy to revise the proposed code. On the odd chance that
> anyone changes their mind, I'll wait until tomorrow to revise my changes.

I do want to point out that we definitely appreciate anything you are willing to contribute. Sorry if we seemed to jump on you a bit here :)

Brian

[lp:~citrix-openstack/glance/iso-boot]

122. By Donal Lafferty on 2011-05-26

Sync with trunk.

123. By Donal Lafferty on 2011-05-26

Remove additions to FIXTURES in test/stubs.py, which requried changes elsewhere.

124. By Donal Lafferty on 2011-05-26

Fix accidental delete.

125. By Donal Lafferty on 2011-05-27

Remove changes to stub database.

[Donal Lafferty (donal-lafferty)]

> > The advantage in temporary fixtures is offset by copy/paste reuse and magic
> > numbers. Temporary fixtures are used in a number of locations, but their
> > reuse requries a copy/paste between test routines. Perhaps this is a good
> > idea of distributed development, because it allows each test to be siloed.
> > However, the usual problems of copy/paste apply. You can't easily change
> the
> > image schema without modification of every single test. Also, the way the
> > current fixutres database is structured, the only way to obtain a unused
> image
> > number is through magic numbers. This prevents the quick aggregation of
> > disk_format/container_format test cases. Therefore, our temporary fixtures
> > seem to violate important programming concepts.
>
> These are great points that I did not fully consider in my initial review.
> Thanks for bringing them up.
>
> > Removing next_image_id does not improve on the code on these points.
> Removing
> > the next_image_id introduces global magic numbers where only local magic
> > numbers existed. The opportunity to avoid copy/paste uses cases is removed,
> > because the central fixtures database cannot aggregate new tests. For these
> > reasons, next_image_id represented a step forward, albeit a limited one.
>
> Maybe to cut down on the copy/paste issues we could use an image factory that
> allows us to build image objects and add them to the fixtures without having
> to define the image object interface all over the place. We could allow that
> factory to handle the magic id problem, while also allowing us to override
> attributes we need for a specific test.
>
> I am also not in love with having a set of "fixed" fixtures to deal with like
> we do now. Maybe making it easy enough to add fixtures without copy/pasting
> dictionaries every where would allow us to remove them? I know it is
> convenient to not have to define your fixtures in every test, but I see it as
> being much more powerful to have total control over it. We could also do it in
> the setUp/tearDown, not every test method.
>
> > That said, I'm new to unittest writing for Python and in an open source
> > environment. I'm happy to revise the proposed code. On the odd chance that
> > anyone changes their mind, I'll wait until tomorrow to revise my changes.
>
> I do want to point out that we definitely appreciate anything you are willing
> to contribute. Sorry if we seemed to jump on you a bit here :)
>
> Brian

Cool. Took a bit longer than expected to get the changes out, while I got used to using bzr's plugins/features (explorer, revert). I beleive that I've rolled back the testing changes.

[lp:~citrix-openstack/glance/iso-boot]

126. By Donal Lafferty on 2011-05-27

Correct documentation.

Unmerged revisions

=== modified file 'doc/source/formats.rst'
--- doc/source/formats.rst	2011-03-05 17:04:43 +0000
+++ doc/source/formats.rst	2011-05-27 09:46:36 +0000
@@ -49,6 +49,10 @@
   A disk format supported by VirtualBox virtual machine monitor and the QEMU
   emulator
 
+* **iso**
+
+  An archive format for the data contents of an optical disc (e.g. CDROM). 
+
 * **qcow2**
 
   A disk format supported by the QEMU emulator that can expand dynamically and

=== modified file 'doc/source/glanceapi.rst'
--- doc/source/glanceapi.rst	2011-05-25 15:03:16 +0000
+++ doc/source/glanceapi.rst	2011-05-27 09:46:36 +0000
@@ -308,7 +308,7 @@
 * ``x-image-meta-disk-format``
 
   This header is optional. Valid values are one of ``aki``, ``ari``, ``ami``,
-  ``raw``, ``vhd``, ``vdi``, ``qcow2``, or ``vmdk``.
+  ``raw``, ``iso``, ``vhd``, ``vdi``, ``qcow2``, or ``vmdk``.
 
   For more information, see :doc:`About Disk and Container Formats <formats>`
 

=== modified file 'doc/source/registries.rst'
--- doc/source/registries.rst	2011-05-25 15:03:16 +0000
+++ doc/source/registries.rst	2011-05-27 09:46:36 +0000
@@ -106,7 +106,7 @@
   **queued**, or **killed**
 
 * ``disk_format`` must be non-empty, and must be one of **ari**, **aki**,
-  **ami**, **raw**, **vhd**, **vdi**, **qcow2**, or **vmdk**
+  **ami**, **raw**, **iso**, **vhd**, **vdi**, **qcow2**, or **vmdk**
 
 * ``container_format`` must be non-empty, and must be on of **ari**,
   **aki**, **ami**, **bare**, or **ovf**

=== modified file 'glance/registry/db/api.py'
--- glance/registry/db/api.py	2011-05-16 13:20:53 +0000
+++ glance/registry/db/api.py	2011-05-27 09:46:36 +0000
@@ -47,7 +47,8 @@
                                       'is_public', 'location', 'checksum'])
 
 CONTAINER_FORMATS = ['ami', 'ari', 'aki', 'bare', 'ovf']
-DISK_FORMATS = ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2', 'vdi']
+DISK_FORMATS = ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2', 'vdi',
+               'iso']
 STATUSES = ['active', 'saving', 'queued', 'killed']
 
 

=== modified file 'tests/stubs.py'
--- tests/stubs.py	2011-05-16 13:20:53 +0000
+++ tests/stubs.py	2011-05-27 09:46:36 +0000
@@ -267,7 +267,6 @@
     The "datastore" always starts with this set of image fixtures.
 
     :param stubs: Set of stubout stubs
-
     """
     class FakeDatastore(object):
 

=== modified file 'tests/unit/test_clients.py'
--- tests/unit/test_clients.py	2011-05-24 19:07:19 +0000
+++ tests/unit/test_clients.py	2011-05-27 09:46:36 +0000
@@ -578,6 +578,30 @@
         for k, v in fixture.items():
             self.assertEquals(v, data[k])
 
+    def test_get_image_iso_meta(self):
+        """Tests that the detailed info about an iso image is returned"""
+        fixture = {'id': 3,
+                   'name': 'fake iso image',
+                   'is_public': False,
+                   'disk_format': 'iso',
+                   'container_format': 'bare',
+                   'status': 'active',
+                   'size': 19,
+                   'location': "file:///tmp/glance-tests/3",
+                   'properties': {}}
+
+        new_image = self.client.add_image(fixture)
+        new_image_id = new_image['id']
+
+        # Test ID auto-assigned properly
+        self.assertEquals(3, new_image_id)
+
+        # Test all other attributes set
+        data = self.client.get_image_meta(3)
+
+        for k, v in fixture.items():
+            self.assertEquals(v, data[k])
+
     def test_get_image_non_existing(self):
         """Tests that NotFound is raised when getting a non-existing image"""
 
@@ -647,6 +671,47 @@
         self.assertTrue('status' in data)
         self.assertEquals('active', data['status'])
 
+    def test_add_image_with_iso_properties(self):
+        """Tests that we can add image metadata with iso disk format"""
+        fixture = {'name': 'fake public iso',
+                   'is_public': True,
+                   'disk_format': 'iso',
+                   'container_format': 'bare',
+                   'size': 19,
+                   'location': "file:///tmp/glance-tests/2",
+                   'properties': {'install': 'Bindows Heaven'},
+                  }
+        new_image = self.client.add_image(fixture)
+        new_image_id = new_image['id']
+
+        # Test ID auto-assigned properly
+        self.assertEquals(3, new_image_id)
+
+        # Test all other attributes set
+        data = self.client.get_image_meta(3)
+
+        for k, v in fixture.items():
+            self.assertEquals(v, data[k])
+
+        # Test status was updated properly
+        self.assertTrue('status' in data)
+        self.assertEquals('active', data['status'])
+
+    def test_add_image_with_bad_iso_properties(self):
+        """Tests that we can add image metadata with iso disk format"""
+        fixture = {'name': 'fake public iso',
+                   'is_public': True,
+                   'disk_format': 'iso',
+                   'container_format': 'vhd',
+                   'size': 19,
+                   'location': "file:///tmp/glance-tests/3",
+                   'properties': {'install': 'Bindows Heaven'},
+                  }
+
+        self.assertRaises(exception.Invalid,
+            self.client.add_image,
+            fixture)
+
     def test_add_image_already_exists(self):
         """Tests proper exception is raised if image with ID already exists"""
         fixture = {'id': 2,