When a build is aborted, fail the buildable

Summary:
Ref T13054. Fixes T10746. Fixes T11154. This is really a one-line fix (include `ABORTED` in `BuildEngine->updateBuildable()`) but try to structure the code a little more clearly too and reduce (at least slightly) the number of random lists of status attributes spread throughout the codebase.

Also add a header tag for buildable status.

Test Plan: Aborted a build, saw buildable fail properly.

Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13054, T11154, T10746

Differential Revision: https://secure.phabricator.com/D19055

src/applications/harbormaster/constants/HarbormasterBuildStatus.php

@@ -2,51 +2,56 @@
 
 final class HarbormasterBuildStatus extends Phobject {
 
-  /**
-   * Not currently being built.
-   */
   const STATUS_INACTIVE = 'inactive';
-
-  /**
-   * Pending pick up by the Harbormaster daemon.
-   */
   const STATUS_PENDING = 'pending';
-
-  /**
-   * Current building the buildable.
-   */
   const STATUS_BUILDING = 'building';
-
-  /**
-   * The build has passed.
-   */
   const STATUS_PASSED = 'passed';
-
-  /**
-   * The build has failed.
-   */
   const STATUS_FAILED = 'failed';
-
-  /**
-   * The build has aborted.
-   */
   const STATUS_ABORTED = 'aborted';
-
-  /**
-   * The build encountered an unexpected error.
-   */
   const STATUS_ERROR = 'error';
-
-  /**
-   * The build has been paused.
-   */
   const STATUS_PAUSED = 'paused';
-
-  /**
-   * The build has been deadlocked.
-   */
   const STATUS_DEADLOCKED = 'deadlocked';
 
+  private $key;
+  private $properties;
+
+  public function __construct($key, array $properties) {
+    $this->key = $key;
+    $this->properties = $properties;
+  }
+
+  public static function newBuildStatusObject($status) {
+    $spec = self::getBuildStatusSpec($status);
+    return new self($status, $spec);
+  }
+
+  private function getProperty($key) {
+    if (!array_key_exists($key, $this->properties)) {
+      throw new Exception(
+        pht(
+          'Attempting to access unknown build status property ("%s").',
+          $key));
+    }
+
+    return $this->properties[$key];
+  }
+
+  public function isBuilding() {
+    return $this->getProperty('isBuilding');
+  }
+
+  public function isPaused() {
+    return ($this->key === self::STATUS_PAUSED);
+  }
+
+  public function isComplete() {
+    return $this->getProperty('isComplete');
+  }
+
+  public function isPassed() {
+    return ($this->key === self::STATUS_PASSED);
+  }
+
 
   /**
    * Get a human readable name for a build status constant.
@@ -56,7 +61,7 @@ final class HarbormasterBuildStatus extends Phobject {
    */
   public static function getBuildStatusName($status) {
     $spec = self::getBuildStatusSpec($status);
-    return idx($spec, 'name', pht('Unknown ("%s")', $status));
+    return $spec['name'];
   }
 
   public static function getBuildStatusMap() {
@@ -66,17 +71,17 @@ final class HarbormasterBuildStatus extends Phobject {
 
   public static function getBuildStatusIcon($status) {
     $spec = self::getBuildStatusSpec($status);
-    return idx($spec, 'icon', 'fa-question-circle');
+    return $spec['icon'];
   }
 
   public static function getBuildStatusColor($status) {
     $spec = self::getBuildStatusSpec($status);
-    return idx($spec, 'color', 'bluegrey');
+    return $spec['color'];
   }
 
   public static function getBuildStatusANSIColor($status) {
     $spec = self::getBuildStatusSpec($status);
-    return idx($spec, 'color.ansi', 'magenta');
+    return $spec['color.ansi'];
   }
 
   public static function getWaitingStatusConstants() {
@@ -110,60 +115,90 @@ final class HarbormasterBuildStatus extends Phobject {
         'icon' => 'fa-circle-o',
         'color' => 'dark',
         'color.ansi' => 'yellow',
+        'isBuilding' => false,
+        'isComplete' => false,
       ),
       self::STATUS_PENDING => array(
         'name' => pht('Pending'),
         'icon' => 'fa-circle-o',
         'color' => 'blue',
         'color.ansi' => 'yellow',
+        'isBuilding' => true,
+        'isComplete' => false,
       ),
       self::STATUS_BUILDING => array(
         'name' => pht('Building'),
         'icon' => 'fa-chevron-circle-right',
         'color' => 'blue',
         'color.ansi' => 'yellow',
+        'isBuilding' => true,
+        'isComplete' => false,
       ),
       self::STATUS_PASSED => array(
         'name' => pht('Passed'),
         'icon' => 'fa-check-circle',
         'color' => 'green',
         'color.ansi' => 'green',
+        'isBuilding' => false,
+        'isComplete' => true,
       ),
       self::STATUS_FAILED => array(
         'name' => pht('Failed'),
         'icon' => 'fa-times-circle',
         'color' => 'red',
         'color.ansi' => 'red',
+        'isBuilding' => false,
+        'isComplete' => true,
       ),
       self::STATUS_ABORTED => array(
         'name' => pht('Aborted'),
         'icon' => 'fa-minus-circle',
         'color' => 'red',
         'color.ansi' => 'red',
+        'isBuilding' => false,
+        'isComplete' => true,
       ),
       self::STATUS_ERROR => array(
         'name' => pht('Unexpected Error'),
         'icon' => 'fa-minus-circle',
         'color' => 'red',
         'color.ansi' => 'red',
+        'isBuilding' => false,
+        'isComplete' => true,
       ),
       self::STATUS_PAUSED => array(
         'name' => pht('Paused'),
         'icon' => 'fa-minus-circle',
         'color' => 'dark',
         'color.ansi' => 'yellow',
+        'isBuilding' => false,
+        'isComplete' => false,
       ),
       self::STATUS_DEADLOCKED => array(
         'name' => pht('Deadlocked'),
         'icon' => 'fa-exclamation-circle',
         'color' => 'red',
         'color.ansi' => 'red',
+        'isBuilding' => false,
+        'isComplete' => true,
       ),
     );
   }
 
   private static function getBuildStatusSpec($status) {
-    return idx(self::getBuildStatusSpecMap(), $status, array());
+    $map = self::getBuildStatusSpecMap();
+    if (isset($map[$status])) {
+      return $map[$status];
+    }
+
+    return array(
+      'name' => pht('Unknown ("%s")', $status),
+      'icon' => 'fa-question-circle',
+      'color' => 'bluegrey',
+      'color.ansi' => 'magenta',
+      'isBuilding' => false,
+      'isComplete' => false,
+    );
   }
 
 }

src/applications/harbormaster/controller/HarbormasterBuildableViewController.php

@@ -36,6 +36,10 @@ final class HarbormasterBuildableViewController
       ->setHeader($title)
       ->setUser($viewer)
       ->setPolicyObject($buildable)
+      ->setStatus(
+        $buildable->getStatusIcon(),
+        $buildable->getStatusColor(),
+        $buildable->getStatusDisplayName())
       ->setHeaderIcon('fa-recycle');
 
     $timeline = $this->buildTransactionTimeline(

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -443,15 +443,11 @@ final class HarbormasterBuildEngine extends Phobject {
     $all_pass = true;
     $any_fail = false;
     foreach ($buildable->getBuilds() as $build) {
-      if ($build->getBuildStatus() != HarbormasterBuildStatus::STATUS_PASSED) {
+      if (!$build->isPassed()) {
         $all_pass = false;
       }
-      if (in_array($build->getBuildStatus(), array(
-          HarbormasterBuildStatus::STATUS_FAILED,
-          HarbormasterBuildStatus::STATUS_ERROR,
-          HarbormasterBuildStatus::STATUS_DEADLOCKED,
-        ))) {
 
+      if ($build->isComplete() && !$build->isPassed()) {
         $any_fail = true;
       }
     }

src/applications/harbormaster/storage/HarbormasterBuildable.php

@@ -58,6 +58,18 @@ final class HarbormasterBuildable extends HarbormasterDAO
     }
   }
 
+  public function getStatusIcon() {
+    return self::getBuildableStatusIcon($this->getBuildableStatus());
+  }
+
+  public function getStatusDisplayName() {
+    return self::getBuildableStatusName($this->getBuildableStatus());
+  }
+
+  public function getStatusColor() {
+    return self::getBuildableStatusColor($this->getBuildableStatus());
+  }
+
   public static function initializeNewBuildable(PhabricatorUser $actor) {
     return id(new HarbormasterBuildable())
       ->setIsManualBuildable(0)

src/applications/harbormaster/storage/build/HarbormasterBuild.php

@@ -108,9 +108,7 @@ final class HarbormasterBuild extends HarbormasterDAO
   }
 
   public function isBuilding() {
-    return
-      $this->getBuildStatus() === HarbormasterBuildStatus::STATUS_PENDING ||
-      $this->getBuildStatus() === HarbormasterBuildStatus::STATUS_BUILDING;
+    return $this->getBuildStatusObject()->isBuilding();
   }
 
   public function isAutobuild() {
@@ -173,13 +171,15 @@ final class HarbormasterBuild extends HarbormasterDAO
   }
 
   public function isComplete() {
-    return in_array(
-      $this->getBuildStatus(),
-      HarbormasterBuildStatus::getCompletedStatusConstants());
+    return $this->getBuildStatusObject()->isComplete();
   }
 
   public function isPaused() {
-    return ($this->getBuildStatus() == HarbormasterBuildStatus::STATUS_PAUSED);
+    return $this->getBuildStatusObject()->isPaused();
+  }
+
+  public function isPassed() {
+    return $this->getBuildStatusObject()->isPassed();
   }
 
   public function getURI() {
@@ -187,6 +187,11 @@ final class HarbormasterBuild extends HarbormasterDAO
     return "/harbormaster/build/{$id}/";
   }
 
+  protected function getBuildStatusObject() {
+    $status_key = $this->getBuildStatus();
+    return HarbormasterBuildStatus::newBuildStatusObject($status_key);
+  }
+
 
 /* -(  Build Commands  )----------------------------------------------------- */