Phew, took me a while to get through this. It mostly looks great, some minor improvements can be made, but [4] and [5] are blockers for landing. It's heading in the right direction though! [1] === modified file 'src/maasserver/templates/maasserver/bootimage-list.html' --- src/maasserver/templates/maasserver/bootimage-list.html 2014-04-23 16:09:39 +0000 +++ src/maasserver/templates/maasserver/bootimage-list.html 2014-04-28 05:57:14 +0000 @@ -33,7 +33,7 @@ {% for bootimage in bootimage_list %} {{ bootimage.id }} - {{ bootimage.osystem }} + {{ bootimage.osystem_title }} {{ bootimage.release }} {{ bootimage.architecture }} {{ bootimage.subarchitecture }} This change needs a test in test_contains_boot_image_list(). [2] +def make_osystem_with_releases(testcase, with_releases=True, osystem_name=None, + releases=None): + """Generate an arbitrary operating system. + + :param with_releases: Should the operating system include releases? + Defaults to `True`. There's a few params left to document :) + """ + if osystem_name is None: + osystem_name = factory.make_name('os') + if with_releases: + if releases is None: + releases = [factory.make_name('release') for _ in range(3)] I would simplify the parameters here and omit the "with_releases" parameter and just allow callers to say releases=[]. Then the code becomes: if releases is None: releases = [factory.make_name('release') for _ in range(3)] ... if releases is not None and releases != []: osystem.fake_list = releases If osystem.fake_list works with [] as well as None then you can omit the conditional of course. (Also used in make_usable_osystem) [3] +def patch_usable_osystems(testcase, osystems=None, allow_empty=True): + """Set a fixed list of usable oeprating systems. Typo: "oeprating" + A usable operating system is one for which boot images are available. + + :param testcase: A `TestCase` whose `patch` this function can use. + :param architectures: Optional list of oprating systems. If omitted, "architectures" does not exist, did you mean osystems? Typo: "oprating" + defaults to a list (which may be empty) of random operating systems. + """ + start = 0 + if allow_empty is False: + start = 1 + if osystems is None: + osystems = [ + make_osystem_with_releases(testcase) + for _ in range(randint(start, 2)) + ] + distro_series = {} + for osystem in osystems: + distro_series[osystem.name] = osystem.get_supported_releases() + patch = testcase.patch(forms, 'list_all_usable_osystems') + patch.return_value = osystems + patch = testcase.patch(forms, 'list_all_usable_releases') + patch.return_value = distro_series You can simplify this a bit like this: testcase.patch(forms, 'list_all_usable_osystems').return_value = osystems testcase.patch( forms, 'list_all_usable_releases').return_value = distro_series This is the only place list_all_usable_osystems and list_all_usable_releases are mentioned in your two branches so far, as is make_usable_osystem. I guess this is an artefact of how you split up your huge branch, but this is making things a little harder to review than necessary. No need to change anything here but please bear this in mind in future. We are much happier reviewing lots of small branches rather than big ones - it's actually less work! It's also more likely that you tested everything properly ;) (A good rule of thumb is at least one branch per day, you'll also *feel* more productive because of it) [4] === modified file 'src/maasserver/views/clusters.py' --- src/maasserver/views/clusters.py 2014-04-23 16:09:39 +0000 +++ src/maasserver/views/clusters.py 2014-04-28 05:57:14 +0000 @@ -51,6 +51,7 @@ NodeGroupInterface, ) from maasserver.views import PaginatedListView +from provisioningserver.osystems import OperatingSystemRegistry You cannot do this, maasserver cannot import registry items from provisioningserver, instead you need to define an RPC where the region calls out to the relevant cluster controller. The reason is because each cluster may have different OS capabilities (think rolling upgrades). Have a look at src/provisioningserver/rpc/cluster.py and src/provisioningserver/rpc/clusterservice.py which already defines some RPC functions (we use AMP) and define your own "ListSupportedOSes" function. Have a chat with Gavin if you get stuck, this is all pretty new stuff. Also, please, do that as a separate branch to make reviewing easier and then when it's landed you can just use it here without fuss. [5] + def get_osystem_title(self, osystem): + osystem_obj = OperatingSystemRegistry.get_item(osystem, default=None) + if osystem_obj is None: + return osystem + return osystem_obj.title This needs a test. It will obviously depend on your required RPC function. def get_context_data(self, **kwargs): context = super( BootImagesListView, self).get_context_data(**kwargs) context['nodegroup'] = self.get_nodegroup() + for bootimage in context['bootimage_list']: + bootimage.osystem_title = self.get_osystem_title(bootimage.osystem) return context We should add osystem_title to the BootImage table instead of adding the (what will be) extra round trip like this because, again, the rolling upgrade problem. In the long term we possibly want to get rid of BootImage entirely and just rely on the ListBootImages RPC. [6] In src/maasserver/views/tests/test_boot_image_list.py: + [make_osystem(bi.osystem, ['install'], self) for bi in images] ... + [make_osystem(bi.osystem, ['install'], self) for bi in boot_images] ... + [make_osystem(bi.osystem, ['install'], self) for bi in boot_images] Rather than making a list and then throwing it away, can you do real "for" loops here please, it will improve readability. Also, please try to be consistent with images vs boot_images. (Also just saw this in src/maasserver/views/tests/test_clusters.py) [7] In src/provisioningserver/boot/tests/test_tftppath.py: +def make_osystem(osystem, purpose, testcase): You put "testcase" as the first param in similar functions, can you be consistent with that here please. However, see below, as I think we can get rid of it. You could also improve this by making purpose a "purposes" array instead and defaulting to all the purposes. Most tests won't care that they all exist and the minority of ones that do can supply their own purposes. Finally, this function is getting used in a few places outside of here, so I would put it in the Factory in src/maastesting/factory.py. Please consider doing that as a separate branch. [8] + class FakeOS(OperatingSystem): Please move this to the top level of the module, it is really obfuscating the rest of the code in this function. [9] + fake = FakeOS(purpose) + OperatingSystemRegistry.register_item(fake.name, fake) + + testcase.addCleanup( + OperatingSystemRegistry.unregister_item, osystem) You can avoid this dance if you use RegistryFixture in your tests. See src/provisioningserver/utils/testing.py and the code that uses it in src/provisioningserver/utils/tests/test_registry.py. Using the fixture will also improve your test isolation. + else: + + obj = OperatingSystemRegistry[osystem] + old_func = obj.get_boot_image_purposes + testcase.patch(obj, 'get_boot_image_purposes').return_value = purpose + + def reset_func(obj, old_func): + obj.get_boot_image_purposes = old_func + + testcase.addCleanup(reset_func, obj, old_func) + + return obj Once you've done that you can get rid of this --^ patch and its cleanup code as well and then remove "testcase" from the params. [10] def extract_image_params(path): ... - purposes = ['commissioning', 'install', 'xinstall'] + osystem_obj = OperatingSystemRegistry.get_item(osystem, default=None) + if osystem_obj is None: + raise OperatingSystemError( + "Unsupported operating system: %s" % osystem) + + purposes = osystem_obj.get_boot_image_purposes( + arch, subarch, release, label) These changes need a test. [11] === added directory 'src/provisioningserver/osystems' === added file 'src/provisioningserver/osystems/__init__.py' The other registries are all in src/provisioningserver/driver/__init__.py At the very least, please put this new code in driver/osystems.py (We want all the enablement stuff in there to make it easy to find and relatively self-contained) [12] +class OperatingSystemError(Exception): + """Exception raised for errors from a OperatingSystem.""" + + +class OperatingSystem: + """Skeleton for a operating system.""" Grammar: s/a/an/ +# Import the supported operating systems after defining OperatingSystem. +from provisioningserver.osystems.ubuntu import UbuntuOS If you split the "class OperatingSystem" out of this file into its own, you can put this import back at the top where it belongs. There's probably too much code in __init__.py here anyway, we avoid that where possible. [13] +class OperatingSystemRegistry(Registry): + """Registry for operating system classes.""" Please add a basic test for this as for the others in src/provisioningserver/driver/tests/test_registries.py [14] === added file 'src/provisioningserver/osystems/tests/test_ubuntu.py' You're missing tests for quite a few of the class methods in UbuntuOS.