Record build success or failure on buildable objects

Summary:
Fixes T4810. When a buildable completes, make an effort to update the corresponding object with a success or failure message. Commits don't support this yet, but revisions do.

{F144614}

Test Plan:
  - Used `bin/harbormaster build` and `bin/harbormaster update` to run a pile of builds.
  - Tried good/bad builds.
  - Sent some normal mail to make sure the mail reentrancy change didn't break stuff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4810

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

src/applications/harbormaster/engine/HarbormasterBuildEngine.php

@@ -360,11 +360,13 @@ final class HarbormasterBuildEngine extends Phobject {
    * @return  void
    */
   private function updateBuildable(HarbormasterBuildable $buildable) {
+    $viewer = $this->getViewer();
+
     $lock_key = 'harbormaster.buildable:'.$buildable->getID();
     $lock = PhabricatorGlobalLock::newLock($lock_key)->lock(15);
 
     $buildable = id(new HarbormasterBuildableQuery())
-      ->setViewer($this->getViewer())
+      ->setViewer($viewer)
       ->withIDs(array($buildable->getID()))
       ->needBuilds(true)
       ->executeOne();
@@ -389,12 +391,62 @@ final class HarbormasterBuildEngine extends Phobject {
       $new_status = HarbormasterBuildable::STATUS_BUILDING;
     }
 
-    if ($buildable->getBuildableStatus() != $new_status) {
+    $old_status = $buildable->getBuildableStatus();
+    $did_update = ($old_status != $new_status);
+    if ($did_update) {
       $buildable->setBuildableStatus($new_status);
       $buildable->save();
     }
 
     $lock->unlock();
+
+    // If we changed the buildable status, try to post a transaction to the
+    // object about it. We can safely do this outside of the locked region.
+
+    // NOTE: We only post transactions for automatic buildables, not for
+    // manual ones: manual builds are test builds, whoever is doing tests
+    // can look at the results themselves, and other users generally don't
+    // care about the outcome.
+
+    if ($did_update && !$buildable->getIsManualBuildable()) {
+
+      $object = id(new PhabricatorObjectQuery())
+        ->setViewer($viewer)
+        ->withPHIDs(array($buildable->getBuildablePHID()))
+        ->executeOne();
+
+      if ($object instanceof PhabricatorApplicationTransactionInterface) {
+        $template = $object->getApplicationTransactionTemplate();
+        if ($template) {
+          $template
+            ->setTransactionType(PhabricatorTransactions::TYPE_BUILDABLE)
+            ->setMetadataValue(
+              'harbormaster:buildablePHID',
+              $buildable->getPHID())
+            ->setOldValue($old_status)
+            ->setNewValue($new_status);
+
+          $harbormaster_phid = id(new PhabricatorApplicationHarbormaster())
+            ->getPHID();
+
+          $daemon_source = PhabricatorContentSource::newForSource(
+            PhabricatorContentSource::SOURCE_DAEMON,
+            array());
+
+          $editor = $object->getApplicationTransactionEditor()
+            ->setActor($viewer)
+            ->setActingAsPHID($harbormaster_phid)
+            ->setContentSource($daemon_source)
+            ->setContinueOnNoEffect(true)
+            ->setContinueOnMissingFields(true);
+
+          $editor->applyTransactions(
+            $object->getApplicationTransactionObject(),
+            array($template));
+        }
+      }
+    }
+
   }
 
 }

src/applications/metamta/storage/PhabricatorMetaMTAMail.php

@@ -292,19 +292,32 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
     return $this->save();
   }
 
-  protected function didWriteData() {
+  public function save() {
-    parent::didWriteData();
+    if ($this->getID()) {
+      return parent::save();
+    }
 
-    if (!$this->getWorkerTaskID()) {
+    // NOTE: When mail is sent from CLI scripts that run tasks in-process, we
+    // may re-enter this method from within scheduleTask(). The implementation
+    // is intended to avoid anything awkward if we end up reentering this
+    // method.
+
+    $this->openTransaction();
+      // Save to generate a task ID.
+      parent::save();
+
+      // Queue a task to send this mail.
       $mailer_task = PhabricatorWorker::scheduleTask(
         'PhabricatorMetaMTAWorker',
         $this->getID());
 
+      // Save again to update the task ID.
       $this->setWorkerTaskID($mailer_task->getID());
-      $this->save();
+      $result = parent::save();
-    }
+    $this->saveTransaction();
-  }
 
+    return $result;
+  }
 
   public function buildDefaultMailer() {
     return PhabricatorEnv::newObjectFromConfig('metamta.mail-adapter');

src/applications/transactions/constants/PhabricatorTransactions.php

@@ -9,6 +9,7 @@ final class PhabricatorTransactions {
   const TYPE_JOIN_POLICY  = 'core:join-policy';
   const TYPE_EDGE         = 'core:edge';
   const TYPE_CUSTOMFIELD  = 'core:customfield';
+  const TYPE_BUILDABLE    = 'harbormaster:buildable';
 
   const COLOR_RED         = 'red';
   const COLOR_ORANGE      = 'orange';

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -155,6 +155,10 @@ abstract class PhabricatorApplicationTransactionEditor
       $types[] = PhabricatorTransactions::TYPE_CUSTOMFIELD;
     }
 
+    if ($this->object instanceof HarbormasterBuildableInterface) {
+      $types[] = PhabricatorTransactions::TYPE_BUILDABLE;
+    }
+
     return $types;
   }
 
@@ -222,6 +226,7 @@ abstract class PhabricatorApplicationTransactionEditor
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
       case PhabricatorTransactions::TYPE_EDIT_POLICY:
       case PhabricatorTransactions::TYPE_JOIN_POLICY:
+      case PhabricatorTransactions::TYPE_BUILDABLE:
         return $xaction->getNewValue();
       case PhabricatorTransactions::TYPE_EDGE:
         return $this->getEdgeTransactionNewValue($xaction);
@@ -311,6 +316,8 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        return;
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
         $object->setViewPolicy($xaction->getNewValue());
         break;
@@ -321,6 +328,7 @@ abstract class PhabricatorApplicationTransactionEditor
         $field = $this->getCustomFieldForTransaction($object, $xaction);
         return $field->applyApplicationTransactionInternalEffects($xaction);
     }
+
     return $this->applyCustomInternalTransaction($object, $xaction);
   }
 
@@ -328,6 +336,8 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
     switch ($xaction->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        return;
       case PhabricatorTransactions::TYPE_SUBSCRIBERS:
         $subeditor = id(new PhabricatorSubscriptionsEditor())
           ->setObject($object)

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -52,6 +52,8 @@ abstract class PhabricatorApplicationTransaction
 
   public function shouldGenerateOldValue() {
     switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        return false;
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
         return false;
     }
@@ -234,6 +236,12 @@ abstract class PhabricatorApplicationTransaction
           $phids[] = array($new);
         }
         break;
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        $phid = $this->getMetadataValue('harbormaster:buildablePHID');
+        if ($phid) {
+          $phids[] = array($phid);
+        }
+        break;
     }
 
     return array_mergev($phids);
@@ -325,12 +333,24 @@ abstract class PhabricatorApplicationTransaction
         return 'lock';
       case PhabricatorTransactions::TYPE_EDGE:
         return 'link';
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        return 'wrench';
     }
 
     return 'edit';
   }
 
   public function getColor() {
+    switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        switch ($this->getNewValue()) {
+          case HarbormasterBuildable::STATUS_PASSED:
+            return 'green';
+          case HarbormasterBuildable::STATUS_FAILED:
+            return 'red';
+        }
+        break;
+    }
     return null;
   }
 
@@ -379,6 +399,17 @@ abstract class PhabricatorApplicationTransaction
   }
 
   public function shouldHideForMail(array $xactions) {
+    switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        switch ($this->getNewValue()) {
+          case HarbormasterBuildable::STATUS_PASSED:
+            // For now, just never send mail when builds pass. We might let
+            // you customize this later, but in most cases this is probably
+            // completely uninteresting.
+            return true;
+        }
+    }
+
     return $this->shouldHide();
   }
 
@@ -537,6 +568,24 @@ abstract class PhabricatorApplicationTransaction
             $this->renderHandleLink($author_phid));
         }
 
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        switch ($this->getNewValue()) {
+          case HarbormasterBuildable::STATUS_PASSED:
+            return pht(
+              '%s completed building %s.',
+              $this->renderHandleLink($author_phid),
+              $this->renderHandleLink(
+                $this->getMetadataValue('harbormaster:buildablePHID')));
+          case HarbormasterBuildable::STATUS_FAILED:
+            return pht(
+              '%s failed to build %s!',
+              $this->renderHandleLink($author_phid),
+              $this->renderHandleLink(
+                $this->getMetadataValue('harbormaster:buildablePHID')));
+          default:
+            return null;
+        }
+
       default:
         return pht(
           '%s edited this %s.',
@@ -596,6 +645,25 @@ abstract class PhabricatorApplicationTransaction
             $this->renderHandleLink($author_phid),
             $this->renderHandleLink($object_phid));
         }
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        switch ($this->getNewValue()) {
+          case HarbormasterBuildable::STATUS_PASSED:
+            return pht(
+              '%s completed building %s for %s.',
+              $this->renderHandleLink($author_phid),
+              $this->renderHandleLink(
+                $this->getMetadataValue('harbormaster:buildablePHID')),
+              $this->renderHandleLink($object_phid));
+          case HarbormasterBuildable::STATUS_FAILED:
+            return pht(
+              '%s failed to build %s for %s.',
+              $this->renderHandleLink($author_phid),
+              $this->renderHandleLink(
+                $this->getMetadataValue('harbormaster:buildablePHID')),
+              $this->renderHandleLink($object_phid));
+          default:
+            return null;
+        }
 
     }
 
@@ -649,6 +717,15 @@ abstract class PhabricatorApplicationTransaction
         return pht('Changed Policy');
       case PhabricatorTransactions::TYPE_SUBSCRIBERS:
         return pht('Changed Subscribers');
+      case PhabricatorTransactions::TYPE_BUILDABLE:
+        switch ($this->getNewValue()) {
+          case HarbormasterBuildable::STATUS_PASSED:
+            return pht('Build Passed');
+          case HarbormasterBuildable::STATUS_FAILED:
+            return pht('Build Failed');
+          default:
+            return pht('Build Status');
+        }
       default:
         return pht('Updated');
     }