Merge ~ltrager/maas:script_result_skip into maas:master

[Lee Trager]

LP: #1760919 - Allow ScriptResults to be skipped

Scripts may now signal 'skipped' in the result YAML. This tells MAAS the
script was unable to run but does not change the state of the node. The SMART
tests use 'skipped' when SMART is unavailable on a device.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b script_result_skip lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: f7b546b38aee71d52902b23f60f4910c28ce53aa

review: Approve

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

diff --git a/src/maasserver/static/js/angular/directives/script_status.js b/src/maasserver/static/js/angular/directives/script_status.js
index 10d6316..fd8e793 100644
--- a/src/maasserver/static/js/angular/directives/script_status.js
+++ b/src/maasserver/static/js/angular/directives/script_status.js
@@ -1,4 +1,4 @@
-/* Copyright 2017 Canonical Ltd.  This software is licensed under the
+/* Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
  * GNU Affero General Public License version 3 (see the file LICENSE).
  *
  * Script status icon select directive.
@@ -51,6 +51,10 @@ angular.module('MAAS').directive('maasScriptStatus', function() {
                     case 4:
                         $scope.icon = 'p-icon--timed-out';
                         break;
+                    // SCRIPT_STATUS.SKIPPED
+                    case 9:
+                        $scope.icon = 'p-icon--warning';
+                        break;
                     case -1:
                         // No scripts have been run.
                         $scope.show = false;
diff --git a/src/maasserver/static/js/angular/directives/tests/test_script_status.js b/src/maasserver/static/js/angular/directives/tests/test_script_status.js
index c43d192..d51b0d4 100644
--- a/src/maasserver/static/js/angular/directives/tests/test_script_status.js
+++ b/src/maasserver/static/js/angular/directives/tests/test_script_status.js
@@ -87,6 +87,12 @@ describe("maasScriptStatus", function() {
         expect(select.attr("class")).toBe("p-icon--timed-out");
     });
 
+    it("SCRIPT_STATUS.SKIPPED", function() {
+        var directive = compileDirective("9");
+        var select = directive.find("span");
+        expect(select.attr("class")).toBe("p-icon--warning");
+    });
+
     it("NONE", function() {
         var directive = compileDirective("-1");
         var select = directive.find("span");
diff --git a/src/maasserver/static/partials/script-results-list.html b/src/maasserver/static/partials/script-results-list.html
index 1c1fa4c..187a8cb 100644
--- a/src/maasserver/static/partials/script-results-list.html
+++ b/src/maasserver/static/partials/script-results-list.html
@@ -32,8 +32,8 @@
                                 <td class="col-3" aria-label="Date"><span data-ng-hide="result.showing_history">{$ result.updated $}</span></td>
                                 <td class="col-2" aria-label="Result">
                                     <span data-ng-hide="result.showing_history">
-                                        <!-- Only link to the testing result when we've received it. This is indicated with status 2(passed), 3(failed), 4(timedout), 6(degraded), 8(failed installing)-->
-                                        {$ result.status_name $} <a data-ng-if="result.status === 2 || result.status === 3 || result.status === 4 || result.status === 6 || result.status === 8" href="#/{$ type_name $}/{$ node.system_id $}/{$ section.area $}/{$ result.id $}">View log</a>
+                                        <!-- Only link to the testing result when we've received it. This is indicated with status 2(passed), 3(failed), 4(timedout), 6(degraded), 8(failed installing), 9(skipped)-->
+                                        {$ result.status_name $} <a data-ng-if="result.status === 2 || result.status === 3 || result.status === 4 || result.status === 6 || result.status === 8 || result.status === 9" href="#/{$ type_name $}/{$ node.system_id $}/{$ section.area $}/{$ result.id $}">View log</a>
                                     </span>
                                 </td>
                                 <td class="col-1">
@@ -84,8 +84,8 @@
                                                         <td class="col-1" aria-label="Runtime"><span data-maas-script-run-time="script-runtime" data-start-time="item.starttime" data-run-time="{{item.runtime}}" data-estimated-run-time="{{item.estimated_runtime}}" data-script-status="item.status"></span></td>
                                                         <td class="col-3" aria-label="Date">{$ item.updated $}</td>
                                                         <td class="col-2" aria-label="Status">
-                                                            <!-- Only link to the testing result when we've received it. This is indicated with status 2(passed), 3(failed), 4(timedout), 6(degraded), 8(failed installing)-->
-                                                            {$ item.status_name $} <a data-ng-if="item.status === 2 || item.status === 3 || item.status === 4 || item.status === 6 || item.status === 8" href="#/{$ type_name $}/{$ node.system_id $}/{$ section.area $}/{$ item.id $}">View log</a>
+                                                            <!-- Only link to the testing result when we've received it. This is indicated with status 2(passed), 3(failed), 4(timedout), 6(degraded), 8(failed installing), 9(skipped)-->
+                                                            {$ item.status_name $} <a data-ng-if="item.status === 2 || item.status === 3 || item.status === 4 || item.status === 6 || item.status === 8 || item.status === 9" href="#/{$ type_name $}/{$ node.system_id $}/{$ section.area $}/{$ item.id $}">View log</a>
                                                         </td>
                                                     </tr>
                                                 </tbody>
diff --git a/src/metadataserver/builtin_scripts/smartctl.py b/src/metadataserver/builtin_scripts/smartctl.py
index 5cff6d8..6b747f7 100644
--- a/src/metadataserver/builtin_scripts/smartctl.py
+++ b/src/metadataserver/builtin_scripts/smartctl.py
@@ -35,6 +35,7 @@
 # --- End MAAS 1.0 script metadata ---
 
 import argparse
+import os
 import re
 from subprocess import (
     CalledProcessError,
@@ -49,6 +50,8 @@ from subprocess import (
 import sys
 from time import sleep
 
+import yaml
+
 # We're just reading the SMART data or asking the drive to run a self test.
 # If this takes more then a minute there is something wrong the with drive.
 TIMEOUT = 60
@@ -84,6 +87,10 @@ def check_SMART_support(storage):
         print(
             'INFO: Unable to run test. The following drive '
             'does not support SMART: %s\n' % storage)
+        result_path = os.environ.get('RESULT_PATH')
+        if result_path is not None:
+            with open(result_path, 'w') as results_file:
+                yaml.safe_dump({'status': 'skipped'}, results_file)
         sys.exit()
     print('INFO: SMART support is available; continuing...')
 
diff --git a/src/metadataserver/builtin_scripts/tests/test_smartctl.py b/src/metadataserver/builtin_scripts/tests/test_smartctl.py
index 9781549..11767a3 100644
--- a/src/metadataserver/builtin_scripts/tests/test_smartctl.py
+++ b/src/metadataserver/builtin_scripts/tests/test_smartctl.py
@@ -1,10 +1,11 @@
-# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2017-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test smartctl functions."""
 
 __all__ = []
 
+import io
 from subprocess import (
     CalledProcessError,
     DEVNULL,
@@ -81,6 +82,11 @@ class TestSmartCTL(MAASTestCase):
         mock_check_output = self.patch(smartctl, "check_output")
         mock_check_output.return_value = b"SMART support is not available."
         mock_print = self.patch(smartctl, "print")
+        result_path = factory.make_name("result_path")
+        self.patch(smartctl.os, "environ", {"RESULT_PATH": result_path})
+        mock_open = self.patch(smartctl, "open")
+        mock_open.return_value = io.StringIO()
+        mock_yaml_safe_dump = self.patch(smartctl.yaml, "safe_dump")
 
         self.assertRaises(SystemExit, smartctl.check_SMART_support, storage)
         self.assertThat(mock_check_output, MockCalledOnceWith(
@@ -95,6 +101,9 @@ class TestSmartCTL(MAASTestCase):
                      'sudo -n smartctl --all %s\n' % storage),
                 call('INFO: Unable to run test. The following drive does '
                      'not support SMART: %s\n' % storage)))
+        self.assertThat(mock_open, MockCalledOnceWith(result_path, "w"))
+        self.assertThat(mock_yaml_safe_dump, MockCalledOnceWith(
+            {'status': 'skipped'}, mock_open.return_value))
 
     def test_run_smartctl_selftest(self):
         storage = factory.make_name('storage')
diff --git a/src/metadataserver/enum.py b/src/metadataserver/enum.py
index 7188914..d9a362c 100644
--- a/src/metadataserver/enum.py
+++ b/src/metadataserver/enum.py
@@ -76,6 +76,7 @@ class SCRIPT_STATUS:
     DEGRADED = 6
     INSTALLING = 7
     FAILED_INSTALLING = 8
+    SKIPPED = 9
 
 
 SCRIPT_STATUS_CHOICES = (
@@ -87,7 +88,8 @@ SCRIPT_STATUS_CHOICES = (
     (SCRIPT_STATUS.ABORTED, "Aborted"),
     (SCRIPT_STATUS.DEGRADED, "Degraded"),
     (SCRIPT_STATUS.INSTALLING, "Installing dependencies"),
-    (SCRIPT_STATUS.FAILED_INSTALLING, "Failed installing dependencies")
+    (SCRIPT_STATUS.FAILED_INSTALLING, "Failed installing dependencies"),
+    (SCRIPT_STATUS.SKIPPED, "Skipped"),
 )
 
 
diff --git a/src/metadataserver/migrations/builtin/0018_script_result_skipped.py b/src/metadataserver/migrations/builtin/0018_script_result_skipped.py
new file mode 100644
index 0000000..0e3eca6
--- /dev/null
+++ b/src/metadataserver/migrations/builtin/0018_script_result_skipped.py
@@ -0,0 +1,23 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.11 on 2018-04-12 20:44
+from __future__ import unicode_literals
+
+from django.db import (
+    migrations,
+    models,
+)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('metadataserver', '0017_store_requested_scripts'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='scriptresult',
+            name='status',
+            field=models.IntegerField(choices=[(0, 'Pending'), (1, 'Running'), (2, 'Passed'), (3, 'Failed'), (4, 'Timed out'), (5, 'Aborted'), (6, 'Degraded'), (7, 'Installing dependencies'), (8, 'Failed installing dependencies'), (9, 'Skipped')], default=0),
+        ),
+    ]
diff --git a/src/metadataserver/models/scriptresult.py b/src/metadataserver/models/scriptresult.py
index 9cd657f..add7c94 100644
--- a/src/metadataserver/models/scriptresult.py
+++ b/src/metadataserver/models/scriptresult.py
@@ -189,10 +189,10 @@ class ScriptResult(CleanSave, TimestampedModel):
             raise ValidationError('YAML must be a dictionary.')
 
         if parsed_yaml.get('status') not in [
-                'passed', 'failed', 'degraded', 'timedout', None]:
+                'passed', 'failed', 'degraded', 'timedout', 'skipped', None]:
             raise ValidationError(
-                'status must be "passed", "failed", "degraded", or '
-                '"timedout".')
+                'status must be "passed", "failed", "degraded", '
+                '"timedout", or "skipped".')
 
         results = parsed_yaml.get('results')
         if results is None:
@@ -274,6 +274,8 @@ class ScriptResult(CleanSave, TimestampedModel):
                     self.status = SCRIPT_STATUS.DEGRADED
                 elif status == 'timedout':
                     self.status = SCRIPT_STATUS.TIMEDOUT
+                elif status == 'skipped':
+                    self.status = SCRIPT_STATUS.SKIPPED
 
         if self.script:
             if script_version_id is not None:
@@ -351,7 +353,8 @@ class ScriptResult(CleanSave, TimestampedModel):
         elif self.ended is None and self.status in {
                 SCRIPT_STATUS.PASSED, SCRIPT_STATUS.FAILED,
                 SCRIPT_STATUS.TIMEDOUT, SCRIPT_STATUS.ABORTED,
-                SCRIPT_STATUS.DEGRADED, SCRIPT_STATUS.FAILED_INSTALLING}:
+                SCRIPT_STATUS.DEGRADED, SCRIPT_STATUS.FAILED_INSTALLING,
+                SCRIPT_STATUS.SKIPPED}:
             self.ended = datetime.now()
             if 'update_fields' in kwargs:
                 kwargs['update_fields'].append('ended')
diff --git a/src/metadataserver/models/scriptset.py b/src/metadataserver/models/scriptset.py
index 6836b57..f84399f 100644
--- a/src/metadataserver/models/scriptset.py
+++ b/src/metadataserver/models/scriptset.py
@@ -68,6 +68,8 @@ def get_status_from_qs(qs):
     if count == 0:
         return -1
     # The status order below represents the order of precedence.
+    # Skipped is omitted here otherwise one skipped test will show
+    # a warning icon in the UI on the test tab.
     for status in (
             SCRIPT_STATUS.RUNNING, SCRIPT_STATUS.INSTALLING,
             SCRIPT_STATUS.PENDING, SCRIPT_STATUS.ABORTED,
diff --git a/src/metadataserver/models/tests/test_scriptresult.py b/src/metadataserver/models/tests/test_scriptresult.py
index fedeb6d..ca72e72 100644
--- a/src/metadataserver/models/tests/test_scriptresult.py
+++ b/src/metadataserver/models/tests/test_scriptresult.py
@@ -196,6 +196,7 @@ class TestScriptResult(MAASServerTestCase):
             'failed': SCRIPT_STATUS.FAILED,
             'degraded': SCRIPT_STATUS.DEGRADED,
             'timedout': SCRIPT_STATUS.TIMEDOUT,
+            'skipped': SCRIPT_STATUS.SKIPPED,
         }
         status = random.choice(list(status_choices.keys()))
         status_yaml = {'status': status}
@@ -351,7 +352,7 @@ class TestScriptResult(MAASServerTestCase):
         script_result.status = random.choice([
             SCRIPT_STATUS.PASSED, SCRIPT_STATUS.FAILED, SCRIPT_STATUS.TIMEDOUT,
             SCRIPT_STATUS.ABORTED, SCRIPT_STATUS.DEGRADED,
-            SCRIPT_STATUS.FAILED_INSTALLING])
+            SCRIPT_STATUS.FAILED_INSTALLING, SCRIPT_STATUS.SKIPPED])
         script_result.save(update_fields=['status'])
         self.assertIsNotNone(reload_object(script_result).ended)
 
@@ -362,7 +363,7 @@ class TestScriptResult(MAASServerTestCase):
         script_result.status = random.choice([
             SCRIPT_STATUS.PASSED, SCRIPT_STATUS.FAILED, SCRIPT_STATUS.TIMEDOUT,
             SCRIPT_STATUS.ABORTED, SCRIPT_STATUS.DEGRADED,
-            SCRIPT_STATUS.FAILED_INSTALLING])
+            SCRIPT_STATUS.FAILED_INSTALLING, SCRIPT_STATUS.SKIPPED])
         script_result.save(update_fields=['status'])
         script_result = reload_object(script_result)
         self.assertIsNotNone(script_result.started)
@@ -502,7 +503,7 @@ class TestScriptResult(MAASServerTestCase):
     def test_read_results(self):
         results = {
             'status': random.choice(
-                ['passed', 'failed', 'degraded', 'timedout']),
+                ['passed', 'failed', 'degraded', 'timedout', 'skipped']),
             'results': {
                 factory.make_name('key'): factory.make_name('value'),
                 factory.make_name('key'): [
@@ -524,7 +525,7 @@ class TestScriptResult(MAASServerTestCase):
 
     def test_read_results_does_not_require_results(self):
         result = {'status': random.choice(
-            ['passed', 'failed', 'degraded', 'timedout'])}
+            ['passed', 'failed', 'degraded', 'timedout', 'skipped'])}
         script_result = factory.make_ScriptResult(
             result=yaml.safe_dump(result).encode())
         self.assertDictEqual(result, script_result.read_results())
@@ -547,8 +548,8 @@ class TestScriptResult(MAASServerTestCase):
             result=yaml.safe_dump(result).encode())
         with self.assertRaisesRegex(
                 ValidationError,
-                'status must be "passed", "failed", "degraded", or '
-                '"timedout".'):
+                'status must be "passed", "failed", "degraded", '
+                '"timedout", or "skipped".'):
             script_result.read_results()
 
     def test_read_results_errors_when_dict_keys_not_str(self):