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

[Lee Trager]

Accept short hand distro series names

Allow the user to specify short hand distro series names. For example if we have centos65 and centos70 passing distro_series=centos7 will set the distro_series to centos70. If multiple matches are found MAAS will do a reverse sort and pick the first match. This ensures that short hand will versioned series will always pick the latest. e.g with centos65, centos70, centos71 and given centos MAAS will pick centos71.

[Andres Rodriguez (andreserl)]

Hi Lee,

Let me see if I understand this correctly. If we pas centos6 it will pick
centos 65, and if we pass centos7 on a system that has centos70 centos71 it
will pick the latest, centos71

On Saturday, October 24, 2015, Lee Trager <email address hidden> wrote:

> Lee Trager has proposed merging lp:~ltrager/maas/allow_short_release_names
> into lp:maas.
>
> Commit message:
> Accept short hand distro series names
>
> Requested reviews:
> MAAS Maintainers (maas-maintainers)
> Related bugs:
> Bug #1507359 in MAAS: "MAAS 1.9 CentOS image names and Juju streams
> disagree"
> https://bugs.launchpad.net/maas/+bug/1507359
>
> For more details, see:
>
> https://code.launchpad.net/~ltrager/maas/allow_short_release_names/+merge/275619
>
> Allow the user to specify short hand distro series names. For example if
> we have centos65 and centos70 passing distro_series=centos7 will set the
> distro_series to centos70. If multiple matches are found MAAS will do a
> reverse sort and pick the first match. This ensures that short hand will
> versioned series will always pick the latest. e.g with centos65, centos70,
> centos71 and given centos MAAS will pick centos71.
> --
> You are subscribed to branch lp:maas.
>

--
Andres Rodriguez (RoAkSoAx)
Ubuntu Server Developer
MSc. Telecom & Networking
Systems Engineer

[Lee Trager (ltrager)]

Yes, thats correct.

> Hi Lee,
>
> Let me see if I understand this correctly. If we pas centos6 it will pick
> centos 65, and if we pass centos7 on a system that has centos70 centos71 it
> will pick the latest, centos71
>
> On Saturday, October 24, 2015, Lee Trager <email address hidden> wrote:
>
> > Lee Trager has proposed merging lp:~ltrager/maas/allow_short_release_names
> > into lp:maas.
> >
> > Commit message:
> > Accept short hand distro series names
> >
> > Requested reviews:
> > MAAS Maintainers (maas-maintainers)
> > Related bugs:
> > Bug #1507359 in MAAS: "MAAS 1.9 CentOS image names and Juju streams
> > disagree"
> > https://bugs.launchpad.net/maas/+bug/1507359
> >
> > For more details, see:
> >
> > https://code.launchpad.net/~ltrager/maas/allow_short_release_names/+merge/27
> 5619
> >
> > Allow the user to specify short hand distro series names. For example if
> > we have centos65 and centos70 passing distro_series=centos7 will set the
> > distro_series to centos70. If multiple matches are found MAAS will do a
> > reverse sort and pick the first match. This ensures that short hand will
> > versioned series will always pick the latest. e.g with centos65, centos70,
> > centos71 and given centos MAAS will pick centos71.
> > --
> > You are subscribed to branch lp:maas.
> >
>
>
> --
> Andres Rodriguez (RoAkSoAx)
> Ubuntu Server Developer
> MSc. Telecom & Networking
> Systems Engineer

[Andres Rodriguez (andreserl)]

Comments inline.

review: Needs Fixing

[Lee Trager (ltrager)]

I've changed the varialbe name to possible_short_names, let me know if you want something else. I've also modifed the patch to only accept short names for non-Ubuntu OSes. So centos7 will give you centos70 but trust will fail.

I changed series to release['name'] because if you look in set_distro_series(src/maasserver/forms.py:681) it accepts the argument series then passes that to find_osystem_and_release_from_release_name which validates the given release is available and returns two dictionaries describing the osystem and release. Previously all we were really doing was finding the osystem name and the series name never changed. Now we're allowing the series name to change so we need to use the found value, not the value passed in.

[Blake Rouse (blake-rouse)]

You should add a unit test for the ubuntu code, to make sure that it doesn't accept the startswith for Ubuntu releases.

review: Needs Fixing

[Lee Trager (ltrager)]

I've added a test to ensure that short names can't be used with Ubuntu releases.

[Blake Rouse (blake-rouse)]

Thanks for adding that test.

review: Approve

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2015-10-23 22:06:42 +0000
+++ src/maasserver/forms.py	2015-10-27 02:41:10 +0000
@@ -364,10 +364,36 @@
 def find_osystem_and_release_from_release_name(name):
     """Return os and release for the given release name."""
     osystems = list_all_usable_osystems()
+    possible_short_names = []
     for osystem in osystems:
         for release in osystem['releases']:
             if release['name'] == name:
                 return osystem, release
+            elif osystem['name'] == name:
+                # If the given release matches the osystem name add it to
+                # our list of possibilities. This allows a user to specify
+                # Ubuntu and get the latest release available.
+                possible_short_names.append({
+                    'osystem': osystem,
+                    'release': release})
+            elif (osystem['name'] != "ubuntu"
+                  and release['name'].startswith(name)):
+                # Check if the given name is a shortened version of a known
+                # name. e.g centos7 for centos70.  We don't allow short names
+                # for Ubuntu releases
+                possible_short_names.append({
+                    'osystem': osystem,
+                    'release': release})
+    if len(possible_short_names) > 0:
+        # Do a reverse sort of all the possibilities and pick the top one.
+        # This allows a user to do a short hand with versioning to pick the
+        # latest release. e.g we have centos70, centos71 given centos7 this
+        # will pick centos71
+        sorted_list = sorted(
+            possible_short_names,
+            key=lambda os_release: os_release['release']['name'],
+            reverse=True)
+        return sorted_list[0]['osystem'], sorted_list[0]['release']
     return None, None
 
 
@@ -671,7 +697,7 @@
                 self.data['osystem'] = osystem['name']
                 self.data['distro_series'] = '%s/%s%s' % (
                     osystem['name'],
-                    series,
+                    release['name'],
                     key_required,
                     )
             else:

=== modified file 'src/maasserver/tests/test_forms_node.py'
--- src/maasserver/tests/test_forms_node.py	2015-10-21 23:36:45 +0000
+++ src/maasserver/tests/test_forms_node.py	2015-10-27 02:41:10 +0000
@@ -205,6 +205,36 @@
         self.assertFalse(form.is_valid())
         self.assertItemsEqual(['distro_series'], form._errors.keys())
 
+    def test_set_distro_series_accepts_short_distro_series(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
+        release = factory.make_name('release')
+        make_usable_osystem(
+            self, releases=[release + '6', release + '0', release + '3'])
+        form = NodeForm(data={
+            'hostname': factory.make_name('host'),
+            'architecture': make_usable_architecture(self),
+            },
+            instance=node)
+        form.set_distro_series(release)
+        form.save()
+        self.assertEquals(release + '6', node.distro_series)
+
+    def test_set_distro_series_doesnt_allow_short_ubuntu_series(self):
+        self.client_log_in()
+        node = factory.make_Node(owner=self.logged_in_user)
+        make_usable_osystem(
+            self,
+            osystem_name='ubuntu',
+            releases=['trusty'])
+        form = NodeForm(data={
+            'hostname': factory.make_name('host'),
+            'architecture': make_usable_architecture(self),
+            },
+            instance=node)
+        form.set_distro_series('trust')
+        self.assertFalse(form.is_valid())
+
     def test_starts_with_default_distro_series(self):
         self.client_log_in()
         node = factory.make_Node(owner=self.logged_in_user)