> Looks good. I have lots of comments, but they're mostly around the UI,
> and not your changes. Pick out those ones that you want to fix; the rest
> is just my commentary on the feature.

Thanks for going through it so thoroughly!  I made lots of changes in response, and they brought me to the point where I'm confident enough to drop the feature flag.


> [1]
> 
> From IRC:
> 
> <jtv> allenap: if you want to play with the new feature, edit
>   src/maasserver/views/nodes.py and change ENABLE_NODECOMMISSIONRESULTS
>   to be True.
> 
> Why are we using feature flags in trunk?

Because the change would have been much more contentious otherwise.  It was a mad-dash personal initiative, not a carefully planned feature.  Planned as a feature it would have taken longer, and therefore, been lower on the priority list and impossible to fit in before the sprint.  As a mad dash, it was at risk of bogging down in design/review changes, and we might still have ended up getting something great later instead of something good now.  The feature flag made it possible to land the initial branch and make changes later.


> [2]
> 
> +              {% if result_item.script_result == 0 %}
> +              <span>OK: {{ result_item.script_result }}</span>
> +              {% else %}
> 
> I know this probably comes under "systemtic UI review", but I don't
> think this needs to include the return code. Just "OK" is enough.

Exactly the sort of feedback I needed.  Done.


> +              <span class="warning">
> +                <img src="{{ STATIC_URL }}img/warning.png" title="Failed" />
> +                FAILED: {{ result_item.script_result }}
> +              </span>
> +              {% endif %}
> 
> Likewise, this doesn't need the return code (it ought to be on the
> details page though).

Also done.


> [3]
> 
> On the details page, instead of having the 2-column thing at the top,
> which takes up a lot of space without telling us much, how about
> integrating it into the output:
> 
>   <breadcrumbs>
>     Nodes > moon > Commissioning results > 00-maas-01-lshw.out
>   </breadcrumbs>
> 
>   <heading>
>     FAILED
>   </heading>
> 
>   <output>
>     <italics>
>       Recorded 12:34 on February 17th, 2014.
>     </italics>
>     Error while installing lldp: Network unreachable.
>     <italics>
>       Exited 1
>     </italics>
>   </output>
> 
> Maybe that's no good either. I just know that I dislike that 2-column
> layout; it's too sparse and light on useful information.

I agree, but it's what we had available...  Maybe we can have a proper redesign for this sometime after the sprint.  It'd be nice if it didn't look quite so much like a Node page.  For now though I hope this will remain a fairly obscure page.


> [4]
> 
> On http://localhost:5240/commissioning-results/?node=... I expected to
> be able to click on the "/!\ FAILED" bit to see the logs.  If I had
> wanted to see the logs from an "OK" result I would have wanted to click
> in the same column; it is called "Script result" after all.

Ironically, that's what I had in my original branch.  But Raphaël preferred the link on the left-hand column, so I changed it.  And now I have a solution that I hope will satisfy everyone: put the result column on the left, and make that the link to the details page.


> [4.1]
> 
> Same goes for http://localhost:5240/nodes/; I want to be able to click
> the "Failed tests" status line or the warning triangle to see what's
> wrong.

Quite — and that was one of those niceties that I deliberately avoided because niceties might have made it impossible to get the feature at all.  We can file it as a separate task now.


> [4.2]
> 
> The URL http://localhost:5240/commissioning-results/?node=... is
> slightly disorientating. It would be good to have a view under
> /nodes/$id/ to show a page with that node's own results.

It would.  We'll have to see what we end up wanting from this feature (results per nodegroup? for just the failed nodes?  filtered by arbitrary node constraints?) and then prioritise further improvements.


> [5]
> 
> I'm also not sure I care what file it wrote to:
> 00-maas-02-virtuality.out means little to me, and I know the
> implementation.

Unfortunately it's the best identification we have, below the node granularity.  I was hoping maybe someday we could link this column to commissioning scripts.


> [6]
> 
> I'm also not sure the Node column is useful on that page. I see that
> it's a general commissioning results page, narrowed to that particular
> node, but I got there by finding "moon" then clicking "3 output files".
> To be told that it's for node-29d7ad70-4671-11e1-93b8-00225f89f211 is
> confusing. Once I understood, it was superfluous.
> 
> I guess it needs to be there when showing more than just a single node's
> results, but it should then show the node's given name, rather than its
> system-generated name.

Right.  I made it show the hostname.  And it's off further to the right now.


> [7]
> 
> On http://localhost:5240/nodes/.../view/ the "Commissioning output" cell
> should probably show if commissioning failed too, but perhaps that's
> coming later.

I don't follow.  It does show if commissioning failed — the conditions are that the user be an administrator, and that the node have commissioning results.


> [8]
> 
> It's not a change you made, but the <pre> block for the output has
> excess whitespace within it. <pre> is so anal that it's necessary to
> forego nice formatting in the template to make sure it renders
> correctly.

OK, I tightened up the whitespace.  In addition, I made the output block disappear if there was no output.


> [9]
> 
> Fwiw, MAASTestCase has assertDocTestMatches(), which uses the flags
> ELLIPSIS and NORMALIZE_WHITESPACE, so if you want to punt on how best to
> normalise whitespace in a test, give that a go.

Not too keen on writing doctests again though!

Thanks again.  This really is a big help.  I'm enabling the feature in this very branch.