Merge lp:~blake-rouse/maas/fix-1581130 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Fix uploading of custom image to strip "custom/" from the name. Fix distro_series and osystem to support longer names that match the same length of a boot resource. Fix issue with raising NodeActionError in the deploy action. Fix boot resources not having a title set falling back to the name of the boot resource.

[Gavin Panella (allenap)]

This looks fine, but there's no explanation why much of it is necessary:

- Stripping "custom/" seems a fairly arbitrary thing to do. The clean_name method has a docstring and a comment which are both useful like "2 + 2 # add 2 and 2" is. There needs to be a proper explanation for future maintainers in the code.

- What was the issue with NodeActionError? One test has changed (for two distinct source changes...) and I can't tell how things are better now.

- How come boot resources don't have a title? Is that a bug in simplestreams?

+1, just explain in the code why these changes are necessary.

review: Approve

[Blake Rouse (blake-rouse)]

> This looks fine, but there's no explanation why much of it is necessary:
>
> - Stripping "custom/" seems a fairly arbitrary thing to do. The clean_name
> method has a docstring and a comment which are both useful like "2 + 2 # add
> 2 and 2" is. There needs to be a proper explanation for future maintainers in
> the code.

Add a better docstring to explain why.

>
> - What was the issue with NodeActionError? One test has changed (for two
> distinct source changes...) and I can't tell how things are better now.

The issue is that "message" attribute no longer exists on ValidationError in Django 1.8. The test was setting it up incorrect.

>
> - How come boot resources don't have a title? Is that a bug in simplestreams?

This is for uploaded images that do not come from simplestreams. A title on an uploaded image is optional. It is only used when the user wants to give it a nice title for the user to select in the WebUI.

>
> +1, just explain in the code why these changes are necessary.

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2016-05-17 20:28:10 +0000
+++ src/maasserver/forms.py	2016-06-16 15:17:48 +0000
@@ -2139,6 +2139,20 @@
             choices=choices, required=True, initial=default_arch,
             error_messages={'invalid_choice': invalid_arch_message})
 
+    def clean_name(self):
+        """Clean the name field.
+
+        The 'custom/' is reserved for custom uploaded images and should not
+        be present in the name field when uploaded. This allows users to
+        provide 'custom/' where it will be removed and the image will be marked
+        uploaded. Without this the image would be uploaded as Generated for
+        a custom OS which is an invalid boot resource.
+        """
+        name = self.cleaned_data['name']
+        if name.startswith('custom/'):
+            name = name[7:]
+        return name
+
     def get_existing_resource(self, resource):
         """Return existing resource if avaliable.
 

=== added file 'src/maasserver/migrations/builtin/maasserver/0065_larger_osystem_and_distro_series.py'
--- src/maasserver/migrations/builtin/maasserver/0065_larger_osystem_and_distro_series.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/migrations/builtin/maasserver/0065_larger_osystem_and_distro_series.py	2016-06-16 15:17:48 +0000
@@ -0,0 +1,27 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import (
+    migrations,
+    models,
+)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('maasserver', '0064_remove_unneeded_event_triggers'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='node',
+            name='distro_series',
+            field=models.CharField(max_length=255, blank=True, default=''),
+        ),
+        migrations.AlterField(
+            model_name='node',
+            name='osystem',
+            field=models.CharField(max_length=255, blank=True, default=''),
+        ),
+    ]

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2016-06-16 01:30:23 +0000
+++ src/maasserver/models/node.py	2016-06-16 15:17:48 +0000
@@ -717,10 +717,10 @@
     bios_boot_method = CharField(max_length=31, blank=True, null=True)
 
     osystem = CharField(
-        max_length=20, blank=True, default='')
+        max_length=255, blank=True, default='')
 
     distro_series = CharField(
-        max_length=20, blank=True, default='')
+        max_length=255, blank=True, default='')
 
     architecture = CharField(max_length=31, blank=True, null=True)
 

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2016-06-08 20:20:40 +0000
+++ src/maasserver/node_action.py	2016-06-16 15:17:48 +0000
@@ -301,7 +301,7 @@
                     validate_osystem_and_distro_series(osystem, distro_series))
                 self.node.save()
             except ValidationError as e:
-                raise NodeActionError(e.message)
+                raise NodeActionError(e)
 
         try:
             self.node.hwe_kernel = validate_hwe_kernel(
@@ -310,7 +310,7 @@
                 self.node.distro_series)
             self.node.save()
         except ValidationError as e:
-            raise NodeActionError(e.message)
+            raise NodeActionError(e)
 
         try:
             self.node.start(self.user)

=== modified file 'src/maasserver/tests/test_forms_bootresource.py'
--- src/maasserver/tests/test_forms_bootresource.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/tests/test_forms_bootresource.py	2016-06-16 15:17:48 +0000
@@ -42,7 +42,7 @@
         upload_name = factory.make_name('filename')
         uploaded_file = SimpleUploadedFile(content=content, name=upload_name)
         data = {
-            'name': name,
+            'name': 'custom/' + name,
             'title': title,
             'architecture': architecture,
             'filetype': upload_type,

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2016-06-08 20:20:40 +0000
+++ src/maasserver/tests/test_node_action.py	2016-06-16 15:17:48 +0000
@@ -451,7 +451,7 @@
         error = self.assertRaises(
             NodeActionError, Deploy(node, user).execute, **extra)
         self.assertEqual(
-            "%s is not a support operating system." % os_name,
+            "['%s is not a support operating system.']" % os_name,
             str(error))
 
     def test_Deploy_sets_osystem_and_series(self):

=== modified file 'src/maasserver/utils/osystems.py'
--- src/maasserver/utils/osystems.py	2015-12-01 18:12:59 +0000
+++ src/maasserver/utils/osystems.py	2016-06-16 15:17:48 +0000
@@ -151,9 +151,15 @@
                 requires_key = get_release_requires_key(release)
             else:
                 requires_key = ''
+            title = release['title']
+            if not title:
+                # Uploaded boot resources are not required to have a title.
+                # Fallback to the name of the release when the title is
+                # missing.
+                title = release['name']
             choices.append((
                 '%s/%s%s' % (os_name, release['name'], requires_key),
-                release['title']
+                title
                 ))
     return choices
 

=== modified file 'src/maasserver/utils/tests/test_osystems.py'
--- src/maasserver/utils/tests/test_osystems.py	2016-05-13 12:01:58 +0000
+++ src/maasserver/utils/tests/test_osystems.py	2016-06-16 15:17:48 +0000
@@ -85,9 +85,12 @@
 
     def make_release_choice(self, osystem, release, include_asterisk=False):
         key = '%s/%s' % (osystem['name'], release['name'])
+        title = release['title']
+        if not title:
+            title = release['name']
         if include_asterisk:
-            return ('%s*' % key, release['title'])
-        return (key, release['title'])
+            return ('%s*' % key, title)
+        return (key, title)
 
     def test_list_all_usable_releases(self):
         releases = [make_rpc_release() for _ in range(3)]
@@ -154,6 +157,21 @@
                 list_all_usable_releases([osystem]),
                 include_default=False))
 
+    def test_list_release_choices_fallsback_to_name(self):
+        releases = [make_rpc_release() for _ in range(3)]
+        for release in releases:
+            release['title'] = ""
+        osystem = make_rpc_osystem(releases=releases)
+        choices = [
+            self.make_release_choice(osystem, release)
+            for release in releases
+            ]
+        self.assertItemsEqual(
+            choices,
+            list_release_choices(
+                list_all_usable_releases([osystem]),
+                include_default=False))
+
     def test_list_release_choices_sorts(self):
         releases = [make_rpc_release() for _ in range(3)]
         osystem = make_rpc_osystem(releases=releases)