Code review comment for lp:~gmb/maas/fix-commissioning-page-distro-list-bug-1312844

Thanks for the review and for sanity-checking me :)

On 4 June 2014 15:19, Gavin Panella <email address hidden> wrote:
> Looks good, but I have several comments.
>
> Also, there probably ought to be a test to demonstrate that DeployForm has fields that reflect the current state of the system, rather than that as import time.

Good point. I'll work on that.

> Rapheël mentioned using callables for parts of Django's form machinery. Did you investigate that?

Yeah, doesn't work (there's an open ticket for it in the Django Trac).
Well, it does for `initial` values, but not for `choices`. Because,
why would you want something that useful.

> Diff comments:
>
>> === modified file 'src/maasserver/forms.py'
>> --- src/maasserver/forms.py 2014-05-29 18:20:58 +0000
>> +++ src/maasserver/forms.py 2014-06-04 09:49:20 +0000
>> @@ -230,7 +230,10 @@
>> for nodegroup in NodeGroup.objects.all():
>> osystems = osystems.union(
>> BootImage.objects.get_usable_osystems(nodegroup))
>> - osystems = [OperatingSystemRegistry[osystem] for osystem in osystems]
>> + osystems = [
>> + OperatingSystemRegistry[osystem] for osystem in osystems
>> + if osystem in OperatingSystemRegistry
>
> I suspect this needs to be changed to query the cluster. All information on what OS/release combos could be supported are in the simplestream that describes boot images. The information on what OS/release combos that *can be supported right now* is in the list of boot resources downloaded. Both of those originate in the cluster, though the latter is also cached in BootImage.
>
> This doesn't need to be changed in this branch, but there ought to be a plan to address it.

Right. I'll file a bug once this is landed.

>> + ]
>> return sorted(osystems, key=lambda osystem: osystem.title)
>>
>>
>> @@ -1023,9 +1026,33 @@
>>
>> class DeployForm(ConfigForm):
>> """Settings page, Deploy section."""
>> +
>> default_osystem = get_config_field('default_osystem')
>> default_distro_series = get_config_field('default_distro_series')
>>
>> + def __init__(self, *args, **kwargs):
>> + super(DeployForm, self).__init__(*args, **kwargs)
>> + # There's a race condition of some descrption that causes an
>> + # error when walking the BootImage table at import time (as
>> + # get_usable{os|release}_choices ultimately do). Setting the
>> + # choices here is less expensive than trying to find the
>> + # underlying race, which might be a Django thing.
>> + usable_oses = list_all_usable_osystems()
>> + os_choices = list_osystem_choices(usable_oses)
>> + self.fields['default_osystem'].choices = os_choices
>> + self.fields['default_osystem'].error_messages = {
>> + 'invalid_choice': compose_invalid_choice_text(
>> + 'osystem', os_choices)
>> + }
>> +
>> + release_choices = list_release_choices(
>> + list_all_usable_releases(usable_oses))
>> + self.fields['default_distro_series'].choices = release_choices
>> + self.fields['default_distro_series'].error_messages = {
>> + 'invalid_choice': compose_invalid_choice_text(
>> + 'osystem', release_choices)
>
> s/osystem/release/ or something like that.

Yup, ta.

>> === modified file 'src/maasserver/testing/factory.py'
>> --- src/maasserver/testing/factory.py 2014-06-03 08:24:52 +0000
>> +++ src/maasserver/testing/factory.py 2014-06-04 09:49:20 +0000
>> @@ -64,7 +64,7 @@
>> # XXX 2014-05-13 blake-rouse bug=1319143
>> # Need to not import directly, use RPC to info from cluster.
>> from provisioningserver.driver import OperatingSystemRegistry
>> -from provisioningserver.driver.os_ubuntu import UbuntuOS
>> +from provisioningserver.driver.os_ubuntu import UbuntuOS, DISTRO_SERIES_CHOICES
>
> Format imports.

Gaah. I'll do a lint check too.

>>
>> # We have a limited number of public keys:
>> # src/maasserver/tests/data/test_rsa{0, 1, 2, 3, 4}.pub
>> @@ -148,6 +148,10 @@
>>
>> def getRandomOS(self):
>> """Pick a random operating system from the registry."""
>> + # Make sure there's a boot image for each of the existing Ubuntu
>> + # OS / release combinations.
>
> Could you fabricate a random new OS here?

Sure.

>> + for series in DISTRO_SERIES_CHOICES.keys():
>
> No need for .keys().

K.

>> + self.make_boot_image(release=series, osystem="ubuntu")
>> osystems = [obj for _, obj in OperatingSystemRegistry]
>> return random.choice(osystems)
>>
>>
>> === modified file 'src/maasserver/tests/test_forms.py'
>> --- src/maasserver/tests/test_forms.py 2014-05-30 07:10:20 +0000
>> +++ src/maasserver/tests/test_forms.py 2014-06-04 09:49:20 +0000
>> @@ -238,6 +238,28 @@
>> self.assertEqual(
>> [osystem], list_all_usable_osystems())
>>
>> + def test_list_all_usable_osystems_omits_oses_without_boot_images(self):
>> + usable_os_name = factory.make_name('os')
>> + unusable_os_name = factory.make_name('os')
>> + self.make_usable_boot_images(osystem=usable_os_name)
>> + usable_os = make_usable_osystem(self, usable_os_name)
>> + unusable_os = make_usable_osystem(self, unusable_os_name)
>> +
>> + usable_os_list = list_all_usable_osystems()
>> + self.assertIn(usable_os, usable_os_list)
>> + self.assertNotIn(unusable_os, usable_os_list)
>> +
>> + def test_list_all_usable_osystems_omits_oses_not_supported(self):
>> + usable_os_name = factory.make_name('os')
>> + unusable_os_name = factory.make_name('os')
>> + self.make_usable_boot_images(osystem=usable_os_name)
>> + self.make_usable_boot_images(osystem=unusable_os_name)
>> + usable_os = make_usable_osystem(self, usable_os_name)
>> +
>> + usable_os_list = list_all_usable_osystems()
>> + self.assertIn(usable_os, usable_os_list)
>> + self.assertNotIn(unusable_os_name, [os.name for os in usable_os_list])
>> +
>> def test_list_all_usable_releases_combines_nodegroups(self):
>> expected = {}
>> osystems = []
>>
>
>
> --
> https://code.launchpad.net/~gmb/maas/fix-commissioning-page-distro-list-bug-1312844/+merge/222010
> You are the owner of lp:~gmb/maas/fix-commissioning-page-distro-list-bug-1312844.