Merge lp:~allenap/maas/dont-crash-when-selecting-fpi--bug-1293676--1.5 into lp:maas/1.5

[Gavin Panella]

Backport from trunk r2221: Inhibit the controls to change between fast and default installers when the use-fastpath-installer tag is defined with an expression.

[Jeroen T. Vermeulen (jtv)]

This looks like a straight backport. Feel free to self-approve those, assuming there are no conflicts.

However I do have a few notes about the code changes that also apply to what's already in trunk. It might be worth addressing them, and then backporting the updated version of the changes:

.

The big warning strings strike me as a bit confusing. Updated? In what way? By whom? Why? These situations often start with the passive voice. Don't do it. The imperative is fine when you're telling the user something they need to do in order to accomplish something — you don't even need to say “please.”

.

Both the ‘inhibit’ methods in node_action.py have this weird indentation:

            return dedent("""\
            The use-fastpath-installer tag is defined with an

Did tabs sneak in? I would expect the text inside an argument to the function call to be indented further than the line that starts the function call.

If that makes the indentation too deep for comfortable text flow, why not extract these two identical-looking methods, or parts thereof, into a function outside the classes? In fact, with these two near-identical explanations both repeated, why not extract them into global constants?

Jeroen

review: Approve

[Gavin Panella (allenap)]

> This looks like a straight backport.  Feel free to self-approve those,
> assuming there are no conflicts.

I was thinking that Julian ought to be gatekeeper on what we're going to
spend time trying to get into Trusty from here on, but a consensus is
good too.

>
> However I do have a few notes about the code changes that also apply to what's
> already in trunk.  It might be worth addressing them, and then backporting the
> updated version of the changes:
>
> .
>
> The big warning strings strike me as a bit confusing.  Updated?  In what way?
> By whom?  Why?  These situations often start with the passive voice.  Don't do
> it.  The imperative is fine when you're telling the user something they need
> to do in order to accomplish something — you don't even need to say “please.”

I can see that might be an improvement, but it also strikes me as a tiny
one. I don't know if that's because English is my first language, or
because I'm me. The passive voice here comes across as more polite; I'm
not telling you what to do, but I am telling you what must be done to
achieve your goal.

>
> .
>
> Both the ‘inhibit’ methods in node_action.py have this weird indentation:
>
>             return dedent("""\
>             The use-fastpath-installer tag is defined with an
>
> Did tabs sneak in?  I would expect the text inside an argument to the function
> call to be indented further than the line that starts the function call.
>
> If that makes the indentation too deep for comfortable text flow, why not
> extract these two identical-looking methods, or parts thereof, into a function
> outside the classes?  In fact, with these two near-identical explanations both
> repeated, why not extract them into global constants?

I just reflexively used python-mode's default indentation for this, and
I'm happy with that tbh. I'd be happy with it another 4 spaces further
in too. My shed's purple, yours is aubergine :-/

I think the pain of another branch and another back-port far outweighs
the gain of fixing either or both of these issues. Sorry.

Thanks for being so thorough though!

[Julian Edwards (julian-edwards)]

On Thursday 03 Apr 2014 20:19:01 you wrote:
> > This looks like a straight backport. Feel free to self-approve those,
> > assuming there are no conflicts.
>
> I was thinking that Julian ought to be gatekeeper on what we're going to
> spend time trying to get into Trusty from here on, but a consensus is
> good too.

It's probably a good idea to run it by me, but yes if I am not around and you
all agree please JFDI, don't block on me.

Anything landing in the 1.5 branch from now on will get CPed in packaging
anyway, so sadly the package in trusty won't necessarily entirely reflect
what's in the 1.5 branch.

> > The big warning strings strike me as a bit confusing. Updated? In what
> > way? By whom? Why? These situations often start with the passive voice.
> > Don't do it. The imperative is fine when you're telling the user
> > something they need to do in order to accomplish something — you don't
> > even need to say “please.”
> I can see that might be an improvement, but it also strikes me as a tiny
> one. I don't know if that's because English is my first language, or
> because I'm me. The passive voice here comes across as more polite; I'm
> not telling you what to do, but I am telling you what must be done to
> achieve your goal.

I agree with jtv here, UI instructions should generally be more direct and
explicit. Using the passive voice is usually frowned upon IIRC.

Dammit, I wish we had a UI expert on the team still :(

J

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2014-03-27 17:52:53 +0000
+++ src/maasserver/models/node.py	2014-04-02 19:45:27 +0000
@@ -837,8 +837,8 @@
         if uti_tag.is_defined:
             raise RuntimeError(
                 "The use-fastpath-installer tag is defined with an "
-                "expression. This expression must be updated to prevent "
-                "this node from booting with the Fast Path installer.")
+                "expression. This expression must instead be updated to set "
+                "this node to install with the default installer.")
         self.tags.remove(uti_tag)
 
     def use_fastpath_installer(self):
@@ -856,6 +856,6 @@
         if uti_tag.is_defined:
             raise RuntimeError(
                 "The use-fastpath-installer tag is defined with an "
-                "expression. This expression must be updated to make this "
-                "node boot with the Fast Path Installer.")
+                "expression. This expression must instead be updated to set "
+                "this node to install with the fast installer.")
         self.tags.add(uti_tag)

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2013-11-15 18:15:51 +0000
+++ src/maasserver/node_action.py	2014-04-02 19:45:27 +0000
@@ -44,6 +44,7 @@
     Node,
     SSHKey,
     )
+from maasserver.models.tag import Tag
 from maasserver.utils import map_enum
 
 # All node statuses.
@@ -198,6 +199,18 @@
         self.node.use_fastpath_installer()
         return "Node marked as using curtin for install."
 
+    def inhibit(self):
+        """Inhibit if ``use-fastpath-installer`` uses an expression."""
+        tag, _ = Tag.objects.get_or_create(name="use-fastpath-installer")
+        if tag.is_defined:
+            return dedent("""\
+            The use-fastpath-installer tag is defined with an
+            expression. This expression must instead be updated to set
+            this node to install with the fast installer.
+            """)
+        else:
+            return None
+
 
 class UseDI(NodeAction):
     """Set this node to use d-i for installation."""
@@ -216,6 +229,18 @@
         self.node.use_traditional_installer()
         return "Node marked as using the default installer."
 
+    def inhibit(self):
+        """Inhibit if ``use-fastpath-installer`` uses an expression."""
+        tag, _ = Tag.objects.get_or_create(name="use-fastpath-installer")
+        if tag.is_defined:
+            return dedent("""\
+            The use-fastpath-installer tag is defined with an
+            expression. This expression must instead be updated to set
+            this node to install with the default installer.
+            """)
+        else:
+            return None
+
 
 class StartNode(NodeAction):
     """Acquire and start a node."""

=== modified file 'src/maasserver/tests/test_node_action.py'
--- src/maasserver/tests/test_node_action.py	2014-01-14 00:43:21 +0000
+++ src/maasserver/tests/test_node_action.py	2014-04-02 19:45:27 +0000
@@ -23,6 +23,7 @@
     NODE_STATUS_CHOICES_DICT,
     )
 from maasserver.exceptions import Redirect
+from maasserver.models import Tag
 from maasserver.node_action import (
     Commission,
     compile_node_actions,
@@ -264,6 +265,11 @@
             self.celery.tasks[0]['task'].name)
 
 
+def make_use_fastpath_installer_tag_with_expression():
+    Tag.objects.get_or_create(
+        name="use-fastpath-installer", definition="true()")
+
+
 class TestUseCurtinNodeAction(MAASServerTestCase):
 
     def test_sets_tag(self):
@@ -287,6 +293,18 @@
         user = factory.make_admin()
         self.assertFalse(UseCurtin(node, user).is_permitted())
 
+    def test_inhibited_if_use_fastpath_installer_tag_uses_expr(self):
+        make_use_fastpath_installer_tag_with_expression()
+        node = factory.make_node()
+        user = factory.make_admin()
+        self.assertDocTestMatches(
+            """\
+            The use-fastpath-installer tag is defined with an
+            expression. This expression must instead be updated to set
+            this node to install with the fast installer.
+            """,
+            UseCurtin(node, user).inhibit())
+
 
 class TestUseDINodeAction(MAASServerTestCase):
 
@@ -310,3 +328,15 @@
         node.use_traditional_installer()
         user = factory.make_admin()
         self.assertFalse(UseDI(node, user).is_permitted())
+
+    def test_inhibited_if_use_fastpath_installer_tag_uses_expr(self):
+        make_use_fastpath_installer_tag_with_expression()
+        node = factory.make_node()
+        user = factory.make_admin()
+        self.assertDocTestMatches(
+            """\
+            The use-fastpath-installer tag is defined with an
+            expression. This expression must instead be updated to set
+            this node to install with the default installer.
+            """,
+            UseDI(node, user).inhibit())