Add "Revision has passing builds" Herald rules for commit content (pushes) and commits (discovery)

Summary:
Depends on D20469. Ref T13276. See PHI1159. See PHI953. See PHI901.

Allow Herald to detect when "arc land" would (or did) warn users about failed or ongoing builds. This respects the "Warn on Landing" build plan behavior.

To accomplish this:

  - When we close a revision, set a "wrong build state" flag if it lands in the wrong build state.
  - If the revision is closed when we hit Herald, look for the flag.
  - If not (common for push rules, can happen for commit rules if we race against the revision update worker), hit Harbormaster ourselves and check the current state.

Test Plan:

  - Wrote a "Require Green" rule.
  - Ran it against various commits with various build states (good, not good).
  - Fiddled with "Warn on Landing" and saw the effect in rule evaluation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

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

src/__phutil_library_map__.php

@@ -773,6 +773,7 @@ phutil_register_library_map(array(
     'DiffusionCommitTimelineEngine' => 'applications/diffusion/engine/DiffusionCommitTimelineEngine.php',
     'DiffusionCommitTransactionType' => 'applications/diffusion/xaction/DiffusionCommitTransactionType.php',
     'DiffusionCommitVerifyTransaction' => 'applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php',
+    'DiffusionCommitWrongBuildsHeraldField' => 'applications/diffusion/herald/DiffusionCommitWrongBuildsHeraldField.php',
     'DiffusionCompareController' => 'applications/diffusion/controller/DiffusionCompareController.php',
     'DiffusionConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionConduitAPIMethod.php',
     'DiffusionController' => 'applications/diffusion/controller/DiffusionController.php',
@@ -904,6 +905,7 @@ phutil_register_library_map(array(
     'DiffusionPreCommitContentRevisionHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitContentRevisionHeraldField.php',
     'DiffusionPreCommitContentRevisionReviewersHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitContentRevisionReviewersHeraldField.php',
     'DiffusionPreCommitContentRevisionSubscribersHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitContentRevisionSubscribersHeraldField.php',
+    'DiffusionPreCommitContentWrongBuildsHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitContentWrongBuildsHeraldField.php',
     'DiffusionPreCommitRefChangeHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefChangeHeraldField.php',
     'DiffusionPreCommitRefHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefHeraldField.php',
     'DiffusionPreCommitRefHeraldFieldGroup' => 'applications/diffusion/herald/DiffusionPreCommitRefHeraldFieldGroup.php',
@@ -6438,6 +6440,7 @@ phutil_register_library_map(array(
     'DiffusionCommitTimelineEngine' => 'PhabricatorTimelineEngine',
     'DiffusionCommitTransactionType' => 'PhabricatorModularTransactionType',
     'DiffusionCommitVerifyTransaction' => 'DiffusionCommitAuditTransaction',
+    'DiffusionCommitWrongBuildsHeraldField' => 'DiffusionCommitHeraldField',
     'DiffusionCompareController' => 'DiffusionController',
     'DiffusionConduitAPIMethod' => 'ConduitAPIMethod',
     'DiffusionController' => 'PhabricatorController',
@@ -6572,6 +6575,7 @@ phutil_register_library_map(array(
     'DiffusionPreCommitContentRevisionHeraldField' => 'DiffusionPreCommitContentHeraldField',
     'DiffusionPreCommitContentRevisionReviewersHeraldField' => 'DiffusionPreCommitContentHeraldField',
     'DiffusionPreCommitContentRevisionSubscribersHeraldField' => 'DiffusionPreCommitContentHeraldField',
+    'DiffusionPreCommitContentWrongBuildsHeraldField' => 'DiffusionPreCommitContentHeraldField',
     'DiffusionPreCommitRefChangeHeraldField' => 'DiffusionPreCommitRefHeraldField',
     'DiffusionPreCommitRefHeraldField' => 'HeraldField',
     'DiffusionPreCommitRefHeraldFieldGroup' => 'HeraldFieldGroup',

src/applications/differential/engine/DifferentialDiffExtractionEngine.php

@@ -281,7 +281,11 @@ final class DifferentialDiffExtractionEngine extends Phobject {
         ->setNewValue($revision->getModernRevisionStatus());
     }
 
-    $concerning_builds = $this->loadConcerningBuilds($revision);
+    $concerning_builds = self::loadConcerningBuilds(
+      $this->getViewer(),
+      $revision,
+      $strict = false);
+
     if ($concerning_builds) {
       $build_list = array();
       foreach ($concerning_builds as $build) {
@@ -328,8 +332,11 @@ final class DifferentialDiffExtractionEngine extends Phobject {
     $editor->applyTransactions($revision, $xactions);
   }
 
-  private function loadConcerningBuilds(DifferentialRevision $revision) {
-    $viewer = $this->getViewer();
+  public static function loadConcerningBuilds(
+    PhabricatorUser $viewer,
+    DifferentialRevision $revision,
+    $strict) {
+
     $diff = $revision->getActiveDiff();
 
     $buildables = id(new HarbormasterBuildableQuery())
@@ -342,7 +349,6 @@ final class DifferentialDiffExtractionEngine extends Phobject {
       return array();
     }
 
-
     $land_key = HarbormasterBuildPlanBehavior::BEHAVIOR_LANDWARNING;
     $behavior = HarbormasterBuildPlanBehavior::getBehavior($land_key);
 
@@ -391,8 +397,14 @@ final class DifferentialDiffExtractionEngine extends Phobject {
           // cases where the repository is observed and the fetch pipeline
           // stalls for a while.
 
+          // If we're in strict mode (from a pre-commit content hook), we do
+          // not ignore these, since we're doing an instantaneous check against
+          // the current state.
+
+          if (!$strict) {
             continue;
           }
+        }
 
         if ($build->isPassed()) {
           continue;

src/applications/differential/storage/DifferentialRevision.php

@@ -62,6 +62,7 @@ final class DifferentialRevision extends DifferentialDAO
   const PROPERTY_LINES_ADDED = 'lines.added';
   const PROPERTY_LINES_REMOVED = 'lines.removed';
   const PROPERTY_BUILDABLES = 'buildables';
+  const PROPERTY_WRONG_BUILDS = 'wrong.builds';
 
   public static function initializeNewRevision(PhabricatorUser $actor) {
     $app = id(new PhabricatorApplicationQuery())

src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php

@@ -13,6 +13,10 @@ final class DifferentialRevisionWrongBuildsTransaction
     return $value;
   }
 
+  public function applyInternalEffects($object, $value) {
+    $object->setProperty(DifferentialRevision::PROPERTY_WRONG_BUILDS, true);
+  }
+
   public function getIcon() {
     return 'fa-exclamation';
   }

src/applications/diffusion/herald/DiffusionCommitWrongBuildsHeraldField.php

@@ -0,0 +1,49 @@
+<?php
+
+final class DiffusionCommitWrongBuildsHeraldField
+  extends DiffusionCommitHeraldField {
+
+  const FIELDCONST = 'diffusion.commit.builds.wrong';
+
+  public function getHeraldFieldName() {
+    return pht('Revision has build warning');
+  }
+
+  public function getFieldGroupKey() {
+    return HeraldRelatedFieldGroup::FIELDGROUPKEY;
+  }
+
+  public function getHeraldFieldValue($object) {
+    $adapter = $this->getAdapter();
+    $viewer = $adapter->getViewer();
+
+    $revision = $adapter->loadDifferentialRevision();
+    if (!$revision) {
+      return false;
+    }
+
+    if ($revision->isPublished()) {
+      $wrong_builds = DifferentialRevision::PROPERTY_WRONG_BUILDS;
+      return !$revision->getProperty($wrong_builds, false);
+    }
+
+    // Reload the revision to pick up active diffs.
+    $revision = id(new DifferentialRevisionQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($revision->getPHID()))
+      ->needActiveDiffs(true)
+      ->executeOne();
+
+    $concerning = DifferentialDiffExtractionEngine::loadConcerningBuilds(
+      $viewer,
+      $revision,
+      $strict = false);
+
+    return (bool)$concerning;
+  }
+
+  protected function getHeraldFieldStandardType() {
+    return self::STANDARD_BOOL;
+  }
+
+}

src/applications/diffusion/herald/DiffusionPreCommitContentWrongBuildsHeraldField.php

@@ -0,0 +1,49 @@
+<?php
+
+final class DiffusionPreCommitContentWrongBuildsHeraldField
+  extends DiffusionPreCommitContentHeraldField {
+
+  const FIELDCONST = 'diffusion.pre.content.builds.wrong';
+
+  public function getHeraldFieldName() {
+    return pht('Revision has build warning');
+  }
+
+  public function getFieldGroupKey() {
+    return HeraldRelatedFieldGroup::FIELDGROUPKEY;
+  }
+
+  public function getHeraldFieldValue($object) {
+    $adapter = $this->getAdapter();
+    $viewer = $adapter->getViewer();
+
+    $revision = $adapter->getRevision();
+    if (!$revision) {
+      return false;
+    }
+
+    if ($revision->isPublished()) {
+      $wrong_builds = DifferentialRevision::PROPERTY_WRONG_BUILDS;
+      return !$revision->getProperty($wrong_builds, false);
+    }
+
+    // Reload the revision to pick up active diffs.
+    $revision = id(new DifferentialRevisionQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($revision->getPHID()))
+      ->needActiveDiffs(true)
+      ->executeOne();
+
+    $concerning = DifferentialDiffExtractionEngine::loadConcerningBuilds(
+      $viewer,
+      $revision,
+      $strict = true);
+
+    return (bool)$concerning;
+  }
+
+  protected function getHeraldFieldStandardType() {
+    return self::STANDARD_BOOL;
+  }
+
+}