Merge lp:~ltrager/maas/lp1683440 into lp:~maas-committers/maas/trunk

[Lee Trager]

Properly handle grabbing the title for custom images.

[Mike Pontillo (mpontillo)]

Can you please add an additional test case for the failure condition, so that we can ensure this regression doesn't occur in the future?

review: Needs Fixing

[Mike Pontillo (mpontillo)]

To be clear, from reading the diff it's not clear if the test case is checking for the branch where the split('/') is being used, or the custom special case.

I think it should be two test cases.

[Mike Pontillo (mpontillo)]

My bad, I misread the test case; I thought you replaced the assert.

This would have been clearer with either a couple of comments, or two separate test cases. I'll let you address that as you see fit.

review: Approve

=== modified file 'src/maasserver/utils/osystems.py'
--- src/maasserver/utils/osystems.py	2017-04-14 19:02:17 +0000
+++ src/maasserver/utils/osystems.py	2017-04-17 23:46:47 +0000
@@ -25,6 +25,7 @@
 from django.core.exceptions import ValidationError
 from django.db.models import Q
 from maasserver.clusterrpc.osystems import gen_all_known_operating_systems
+from maasserver.enum import BOOT_RESOURCE_TYPE
 from maasserver.models import (
     BootResource,
     BootSourceCache,
@@ -67,7 +68,18 @@
     # the rack gives us.
     for resource in BootResource.objects.exclude(extra={}):
         if 'title' in resource.extra and resource.extra['title'] != '':
-            osystem, release = resource.name.split('/')
+            # All BootResources which originate from a SimpleStream have
+            # their name set to osystem/series
+            # (see BootResourceStore.get_or_create_boot_resource). User
+            # uploaded images set the name to whatever the user specifed.
+            # When custom images are returned by list_all_usable_osystems
+            # the osystem name is 'custom' and the release name to whatever
+            # the user specified.
+            if resource.rtype == BOOT_RESOURCE_TYPE.UPLOADED:
+                osystem = 'custom'
+                release = resource.name
+            else:
+                osystem, release = resource.name.split('/')
             if osystem not in titles:
                 titles[osystem] = {}
             titles[osystem][release] = resource.extra['title']

=== modified file 'src/maasserver/utils/tests/test_osystems.py'
--- src/maasserver/utils/tests/test_osystems.py	2017-04-14 19:02:17 +0000
+++ src/maasserver/utils/tests/test_osystems.py	2017-04-17 23:46:47 +0000
@@ -120,30 +120,7 @@
         self.assertEqual(
             releases, list_all_usable_releases([osystem])[osystem['name']])
 
-    def test_list_all_releases_requiring_keys(self):
-        releases = [
-            make_rpc_release(requires_license_key=True) for _ in range(3)]
-        release_without_license_key = make_rpc_release(
-            requires_license_key=False)
-        osystem = make_rpc_osystem(
-            releases=releases + [release_without_license_key])
-        self.assertItemsEqual(
-            releases,
-            list_all_releases_requiring_keys([osystem])[osystem['name']])
-
-    def test_list_all_releases_requiring_keys_sorts(self):
-        releases = [
-            make_rpc_release(requires_license_key=True) for _ in range(3)]
-        release_without_license_key = make_rpc_release(
-            requires_license_key=False)
-        osystem = make_rpc_osystem(
-            releases=releases + [release_without_license_key])
-        releases = sorted(releases, key=itemgetter('title'))
-        self.assertEqual(
-            releases,
-            list_all_releases_requiring_keys([osystem])[osystem['name']])
-
-    def test_list_all_releases_looks_up_title_in_boot_resource_table(self):
+    def test_list_all_usable_releases_finds_title_in_boot_resource_table(self):
         release = make_rpc_release()
         osystem = make_rpc_osystem(releases=[release])
         title = factory.make_name('title')
@@ -155,6 +132,42 @@
             title,
             list_all_usable_releases([osystem])[osystem['name']][0]['title'])
 
+    def test_list_all_usable_releases_finds_title_for_custom(self):
+        # Regression test for LP:1683440
+        release = make_rpc_release()
+        osystem = make_rpc_osystem(
+            name='custom', releases=[release])
+        title = factory.make_name('title')
+        factory.make_BootResource(
+            rtype=BOOT_RESOURCE_TYPE.UPLOADED, name=release['name'],
+            extra={'title': title})
+        self.assertEquals(
+            title,
+            list_all_usable_releases([osystem])[osystem['name']][0]['title'])
+
+    def test_list_all_releases_requiring_keys(self):
+        releases = [
+            make_rpc_release(requires_license_key=True) for _ in range(3)]
+        release_without_license_key = make_rpc_release(
+            requires_license_key=False)
+        osystem = make_rpc_osystem(
+            releases=releases + [release_without_license_key])
+        self.assertItemsEqual(
+            releases,
+            list_all_releases_requiring_keys([osystem])[osystem['name']])
+
+    def test_list_all_releases_requiring_keys_sorts(self):
+        releases = [
+            make_rpc_release(requires_license_key=True) for _ in range(3)]
+        release_without_license_key = make_rpc_release(
+            requires_license_key=False)
+        osystem = make_rpc_osystem(
+            releases=releases + [release_without_license_key])
+        releases = sorted(releases, key=itemgetter('title'))
+        self.assertEqual(
+            releases,
+            list_all_releases_requiring_keys([osystem])[osystem['name']])
+
     def test_get_release_requires_key_returns_asterisk_when_required(self):
         release = make_rpc_release(requires_license_key=True)
         self.assertEqual('*', get_release_requires_key(release))