Merge lp:~citrix-openstack/glance/iso-boot into lp:~hudson-openstack/glance/trunk
- iso-boot
- Merge into trunk
Status: | Superseded |
---|---|
Proposed branch: | lp:~citrix-openstack/glance/iso-boot |
Merge into: | lp:~hudson-openstack/glance/trunk |
Diff against target: |
151 lines (+75/-4) 6 files modified
doc/source/formats.rst (+4/-0) doc/source/glanceapi.rst (+1/-1) doc/source/registries.rst (+1/-1) glance/registry/db/api.py (+2/-1) tests/stubs.py (+0/-1) tests/unit/test_clients.py (+67/-0) |
To merge this branch: | bzr merge lp:~citrix-openstack/glance/iso-boot |
Related bugs: | |
Related blueprints: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jay Pipes (community) | Needs Fixing | ||
Brian Waldon | Pending | ||
Review via email: mp+62642@code.launchpad.net |
This proposal supersedes a proposal from 2011-05-19.
This proposal has been superseded by a proposal from 2011-05-27.
Commit message
Description of the change
Added new disk_format type of 'iso'. Nova can use this information to identify images that have to be booted from a CDROM.
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) wrote : Posted in a previous version of this proposal | # |
Donal Lafferty (donal-lafferty) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
lgtm.
Brian Waldon (bcwaldon) wrote : Posted in a previous version of this proposal | # |
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/
128,146,
Jay Pipes (jaypipes) wrote : Posted in a previous version of this proposal | # |
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
Donal Lafferty (donal-lafferty) wrote : Posted in a previous version of this proposal | # |
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/
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) wrote : Posted in a previous version of this proposal | # |
> 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/
> 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
Donal Lafferty (donal-lafferty) wrote : Posted in a previous version of this proposal | # |
> > 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/
> > 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.
Jay Pipes (jaypipes) wrote : | # |
Hi!
One little nit:
133 + """Tests that we can add image metadata with iso disk format"""
That's a copy/paste from the previous test. I think you need to update that docstring to something like:
"""Test that ISO disk format and VHD container format is invalid."""
Cheers!
jay
Donal Lafferty (donal-lafferty) wrote : | # |
> Hi!
>
> One little nit:
>
> 133 + """Tests that we can add image metadata with iso disk format"""
>
> That's a copy/paste from the previous test. I think you need to update that
> docstring to something like:
>
> """Test that ISO disk format and VHD container format is invalid."""
>
> Cheers!
> jay
Fixed :)
- 126. By Donal Lafferty
-
Correct documentation.
Unmerged revisions
Preview Diff
1 | === modified file 'doc/source/formats.rst' |
2 | --- doc/source/formats.rst 2011-03-05 17:04:43 +0000 |
3 | +++ doc/source/formats.rst 2011-05-27 14:54:25 +0000 |
4 | @@ -49,6 +49,10 @@ |
5 | A disk format supported by VirtualBox virtual machine monitor and the QEMU |
6 | emulator |
7 | |
8 | +* **iso** |
9 | + |
10 | + An archive format for the data contents of an optical disc (e.g. CDROM). |
11 | + |
12 | * **qcow2** |
13 | |
14 | A disk format supported by the QEMU emulator that can expand dynamically and |
15 | |
16 | === modified file 'doc/source/glanceapi.rst' |
17 | --- doc/source/glanceapi.rst 2011-05-25 15:03:16 +0000 |
18 | +++ doc/source/glanceapi.rst 2011-05-27 14:54:25 +0000 |
19 | @@ -308,7 +308,7 @@ |
20 | * ``x-image-meta-disk-format`` |
21 | |
22 | This header is optional. Valid values are one of ``aki``, ``ari``, ``ami``, |
23 | - ``raw``, ``vhd``, ``vdi``, ``qcow2``, or ``vmdk``. |
24 | + ``raw``, ``iso``, ``vhd``, ``vdi``, ``qcow2``, or ``vmdk``. |
25 | |
26 | For more information, see :doc:`About Disk and Container Formats <formats>` |
27 | |
28 | |
29 | === modified file 'doc/source/registries.rst' |
30 | --- doc/source/registries.rst 2011-05-25 15:03:16 +0000 |
31 | +++ doc/source/registries.rst 2011-05-27 14:54:25 +0000 |
32 | @@ -106,7 +106,7 @@ |
33 | **queued**, or **killed** |
34 | |
35 | * ``disk_format`` must be non-empty, and must be one of **ari**, **aki**, |
36 | - **ami**, **raw**, **vhd**, **vdi**, **qcow2**, or **vmdk** |
37 | + **ami**, **raw**, **iso**, **vhd**, **vdi**, **qcow2**, or **vmdk** |
38 | |
39 | * ``container_format`` must be non-empty, and must be on of **ari**, |
40 | **aki**, **ami**, **bare**, or **ovf** |
41 | |
42 | === modified file 'glance/registry/db/api.py' |
43 | --- glance/registry/db/api.py 2011-05-16 13:20:53 +0000 |
44 | +++ glance/registry/db/api.py 2011-05-27 14:54:25 +0000 |
45 | @@ -47,7 +47,8 @@ |
46 | 'is_public', 'location', 'checksum']) |
47 | |
48 | CONTAINER_FORMATS = ['ami', 'ari', 'aki', 'bare', 'ovf'] |
49 | -DISK_FORMATS = ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2', 'vdi'] |
50 | +DISK_FORMATS = ['ami', 'ari', 'aki', 'vhd', 'vmdk', 'raw', 'qcow2', 'vdi', |
51 | + 'iso'] |
52 | STATUSES = ['active', 'saving', 'queued', 'killed'] |
53 | |
54 | |
55 | |
56 | === modified file 'tests/stubs.py' |
57 | --- tests/stubs.py 2011-05-16 13:20:53 +0000 |
58 | +++ tests/stubs.py 2011-05-27 14:54:25 +0000 |
59 | @@ -267,7 +267,6 @@ |
60 | The "datastore" always starts with this set of image fixtures. |
61 | |
62 | :param stubs: Set of stubout stubs |
63 | - |
64 | """ |
65 | class FakeDatastore(object): |
66 | |
67 | |
68 | === modified file 'tests/unit/test_clients.py' |
69 | --- tests/unit/test_clients.py 2011-05-24 19:07:19 +0000 |
70 | +++ tests/unit/test_clients.py 2011-05-27 14:54:25 +0000 |
71 | @@ -578,6 +578,30 @@ |
72 | for k, v in fixture.items(): |
73 | self.assertEquals(v, data[k]) |
74 | |
75 | + def test_get_image_iso_meta(self): |
76 | + """Tests that the detailed info about an ISO image is returned""" |
77 | + fixture = {'id': 3, |
78 | + 'name': 'fake iso image', |
79 | + 'is_public': False, |
80 | + 'disk_format': 'iso', |
81 | + 'container_format': 'bare', |
82 | + 'status': 'active', |
83 | + 'size': 19, |
84 | + 'location': "file:///tmp/glance-tests/3", |
85 | + 'properties': {}} |
86 | + |
87 | + new_image = self.client.add_image(fixture) |
88 | + new_image_id = new_image['id'] |
89 | + |
90 | + # Test ID auto-assigned properly |
91 | + self.assertEquals(3, new_image_id) |
92 | + |
93 | + # Test all other attributes set |
94 | + data = self.client.get_image_meta(3) |
95 | + |
96 | + for k, v in fixture.items(): |
97 | + self.assertEquals(v, data[k]) |
98 | + |
99 | def test_get_image_non_existing(self): |
100 | """Tests that NotFound is raised when getting a non-existing image""" |
101 | |
102 | @@ -647,6 +671,49 @@ |
103 | self.assertTrue('status' in data) |
104 | self.assertEquals('active', data['status']) |
105 | |
106 | + def test_add_image_with_iso_properties(self): |
107 | + """Tests that we can add image metadata with ISO disk format""" |
108 | + fixture = {'name': 'fake public iso', |
109 | + 'is_public': True, |
110 | + 'disk_format': 'iso', |
111 | + 'container_format': 'bare', |
112 | + 'size': 19, |
113 | + 'location': "file:///tmp/glance-tests/2", |
114 | + 'properties': {'install': 'Bindows Heaven'}, |
115 | + } |
116 | + new_image = self.client.add_image(fixture) |
117 | + new_image_id = new_image['id'] |
118 | + |
119 | + # Test ID auto-assigned properly |
120 | + self.assertEquals(3, new_image_id) |
121 | + |
122 | + # Test all other attributes set |
123 | + data = self.client.get_image_meta(3) |
124 | + |
125 | + for k, v in fixture.items(): |
126 | + self.assertEquals(v, data[k]) |
127 | + |
128 | + # Test status was updated properly |
129 | + self.assertTrue('status' in data) |
130 | + self.assertEquals('active', data['status']) |
131 | + |
132 | + def test_add_image_with_bad_iso_properties(self): |
133 | + """Verify that ISO with invalid container format is rejected. |
134 | + Intended to exercise error path once rather than be exhaustive |
135 | + set of mismatches""" |
136 | + fixture = {'name': 'fake public iso', |
137 | + 'is_public': True, |
138 | + 'disk_format': 'iso', |
139 | + 'container_format': 'vhd', |
140 | + 'size': 19, |
141 | + 'location': "file:///tmp/glance-tests/3", |
142 | + 'properties': {'install': 'Bindows Heaven'}, |
143 | + } |
144 | + |
145 | + self.assertRaises(exception.Invalid, |
146 | + self.client.add_image, |
147 | + fixture) |
148 | + |
149 | def test_add_image_already_exists(self): |
150 | """Tests proper exception is raised if image with ID already exists""" |
151 | fixture = {'id': 2, |
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 glance. openstack. org/glanceapi. html
http://
You can find these in the source tree under /doc/source/ formats. rst and /doc/source/ glanceapi. rst.
Thanks!
jay