Add a warning to revision timelines when changes land with ongoing or failed builds

Summary:
Ref T13258. The general idea here is "if arc land prompted you and you hit 'y', you get a warning about it on the timeline".

This is similar to the existing warning about landing revisions in the wrong state and hitting "y" to get through that. See D18808, previously.

These warnings make it easier to catch process issues at a glance, especially because the overall build status is now more complicated (and may legally include some failures on tests which are marked as unimportant).

The transaction stores which builds had problems, but I'm not doing anything to render that for now. I think you can usually figure it out from the UI already; if not, we could refine this.

Test Plan:
  - Used `bin/differential attach-commit` to trigger extraction/attachment.
  - Attached a commit to a revision with various build states, and various build plan "Warn When Landing" flags.
  - Got sensible warnings and non-warnings based on "Warn When Landing" setting.

{F6251631}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13258

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

src/__phutil_library_map__.php

@@ -653,6 +653,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionUpdateTransaction' => 'applications/differential/xaction/DifferentialRevisionUpdateTransaction.php',
     'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php',
     'DifferentialRevisionVoidTransaction' => 'applications/differential/xaction/DifferentialRevisionVoidTransaction.php',
+    'DifferentialRevisionWrongBuildsTransaction' => 'applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php',
     'DifferentialRevisionWrongStateTransaction' => 'applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php',
     'DifferentialSchemaSpec' => 'applications/differential/storage/DifferentialSchemaSpec.php',
     'DifferentialSetDiffPropertyConduitAPIMethod' => 'applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php',
@@ -6181,6 +6182,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionUpdateTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionViewController' => 'DifferentialController',
     'DifferentialRevisionVoidTransaction' => 'DifferentialRevisionTransactionType',
+    'DifferentialRevisionWrongBuildsTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialRevisionWrongStateTransaction' => 'DifferentialRevisionTransactionType',
     'DifferentialSchemaSpec' => 'PhabricatorConfigSchemaSpec',
     'DifferentialSetDiffPropertyConduitAPIMethod' => 'DifferentialConduitAPIMethod',

src/applications/differential/engine/DifferentialDiffExtractionEngine.php

@@ -285,6 +285,24 @@ final class DifferentialDiffExtractionEngine extends Phobject {
         ->setNewValue($revision->getModernRevisionStatus());
     }
 
+    $concerning_builds = $this->loadConcerningBuilds($revision);
+    if ($concerning_builds) {
+      $build_list = array();
+      foreach ($concerning_builds as $build) {
+        $build_list[] = array(
+          'phid' => $build->getPHID(),
+          'status' => $build->getBuildStatus(),
+        );
+      }
+
+      $wrong_builds =
+        DifferentialRevisionWrongBuildsTransaction::TRANSACTIONTYPE;
+
+      $xactions[] = id(new DifferentialTransaction())
+        ->setTransactionType($wrong_builds)
+        ->setNewValue($build_list);
+    }
+
     $type_update = DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE;
 
     $xactions[] = id(new DifferentialTransaction())
@@ -322,4 +340,81 @@ final class DifferentialDiffExtractionEngine extends Phobject {
     return $result_data;
   }
 
+  private function loadConcerningBuilds(DifferentialRevision $revision) {
+    $viewer = $this->getViewer();
+    $diff = $revision->getActiveDiff();
+
+    $buildables = id(new HarbormasterBuildableQuery())
+      ->setViewer($viewer)
+      ->withBuildablePHIDs(array($diff->getPHID()))
+      ->needBuilds(true)
+      ->withManualBuildables(false)
+      ->execute();
+    if (!$buildables) {
+      return array();
+    }
+
+
+    $land_key = HarbormasterBuildPlanBehavior::BEHAVIOR_LANDWARNING;
+    $behavior = HarbormasterBuildPlanBehavior::getBehavior($land_key);
+
+    $key_never = HarbormasterBuildPlanBehavior::LANDWARNING_NEVER;
+    $key_building = HarbormasterBuildPlanBehavior::LANDWARNING_IF_BUILDING;
+    $key_complete = HarbormasterBuildPlanBehavior::LANDWARNING_IF_COMPLETE;
+
+    $concerning_builds = array();
+    foreach ($buildables as $buildable) {
+      $builds = $buildable->getBuilds();
+      foreach ($builds as $build) {
+        $plan = $build->getBuildPlan();
+        $option = $behavior->getPlanOption($plan);
+        $behavior_value = $option->getKey();
+
+        $if_never = ($behavior_value === $key_never);
+        if ($if_never) {
+          continue;
+        }
+
+        $if_building = ($behavior_value === $key_building);
+        if ($if_building && $build->isComplete()) {
+          continue;
+        }
+
+        $if_complete = ($behavior_value === $key_complete);
+        if ($if_complete) {
+          if (!$build->isComplete()) {
+            continue;
+          }
+
+          // TODO: If you "arc land" and a build with "Warn: If Complete"
+          // is still running, you may not see a warning, and push the revision
+          // in good faith. The build may then complete before we get here, so
+          // we now see a completed, failed build.
+
+          // For now, just err on the side of caution and assume these builds
+          // were in a good state when we prompted the user, even if they're in
+          // a bad state now.
+
+          // We could refine this with a rule like "if the build finished
+          // within a couple of minutes before the push happened, assume it was
+          // in good faith", but we don't currently have an especially
+          // convenient way to check when the build finished or when the commit
+          // was pushed or discovered, and this would create some issues in
+          // cases where the repository is observed and the fetch pipeline
+          // stalls for a while.
+
+          continue;
+        }
+
+        if ($build->isPassed()) {
+          continue;
+        }
+
+        $concerning_builds[] = $build;
+      }
+    }
+
+    return $concerning_builds;
+  }
+
 }

src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php

@@ -0,0 +1,37 @@
+<?php
+
+final class DifferentialRevisionWrongBuildsTransaction
+  extends DifferentialRevisionTransactionType {
+
+  const TRANSACTIONTYPE = 'differential.builds.wrong';
+
+  public function generateOldValue($object) {
+    return null;
+  }
+
+  public function generateNewValue($object, $value) {
+    return $value;
+  }
+
+  public function getIcon() {
+    return 'fa-exclamation';
+  }
+
+  public function getColor() {
+    return 'pink';
+  }
+
+  public function getActionStrength() {
+    return 4;
+  }
+
+  public function getTitle() {
+    return pht(
+      'This revision was landed with ongoing or failed builds.');
+  }
+
+  public function shouldHideForFeed() {
+    return true;
+  }
+
+}

src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php

@@ -27,6 +27,12 @@ final class HarbormasterBuildPlanBehavior
   const BUILDABLE_IF_BUILDING = 'building';
   const BUILDABLE_NEVER = 'never';
 
+  const BEHAVIOR_LANDWARNING = 'arc-land';
+  const LANDWARNING_ALWAYS = 'always';
+  const LANDWARNING_IF_BUILDING = 'building';
+  const LANDWARNING_IF_COMPLETE = 'complete';
+  const LANDWARNING_NEVER = 'never';
+
   public function setKey($key) {
     $this->key = $key;
     return $this;
@@ -176,7 +182,7 @@ final class HarbormasterBuildPlanBehavior
 
     $land_options = array(
       id(new HarbormasterBuildPlanBehaviorOption())
-        ->setKey('always')
+        ->setKey(self::LANDWARNING_ALWAYS)
         ->setIcon('fa-check-circle-o green')
         ->setName(pht('Always'))
         ->setIsDefault(true)
@@ -185,7 +191,7 @@ final class HarbormasterBuildPlanBehavior
             '"arc land" warns if the build is still running or has '.
             'failed.')),
       id(new HarbormasterBuildPlanBehaviorOption())
-        ->setKey('building')
+        ->setKey(self::LANDWARNING_IF_BUILDING)
         ->setIcon('fa-pause-circle-o yellow')
         ->setName(pht('If Building'))
         ->setDescription(
@@ -193,7 +199,7 @@ final class HarbormasterBuildPlanBehavior
             '"arc land" warns if the build is still running, but ignores '.
             'the build if it has failed.')),
       id(new HarbormasterBuildPlanBehaviorOption())
-        ->setKey('complete')
+        ->setKey(self::LANDWARNING_IF_COMPLETE)
         ->setIcon('fa-dot-circle-o yellow')
         ->setName(pht('If Complete'))
         ->setDescription(
@@ -201,7 +207,7 @@ final class HarbormasterBuildPlanBehavior
             '"arc land" warns if the build has failed, but ignores the '.
             'build if it is still running.')),
       id(new HarbormasterBuildPlanBehaviorOption())
-        ->setKey('never')
+        ->setKey(self::LANDWARNING_NEVER)
         ->setIcon('fa-circle-o red')
         ->setName(pht('Never'))
         ->setDescription(
@@ -296,7 +302,7 @@ final class HarbormasterBuildPlanBehavior
             'revision, even if builds have failed or are still in progress.'))
         ->setOptions($draft_options),
       id(new self())
-        ->setKey('arc-land')
+        ->setKey(self::BEHAVIOR_LANDWARNING)
         ->setName(pht('Warn When Landing'))
         ->setEditInstructions(
           pht(
@@ -312,7 +318,10 @@ final class HarbormasterBuildPlanBehavior
             'for it) or the outcome is not important.'.
             "\n\n".
             'This warning is only advisory. Users may always elect to ignore '.
-            'this warning and continue, even if builds have failed.'))
+            'this warning and continue, even if builds have failed.'.
+            "\n\n".
+            'This setting also affects the warning that is published to '.
+            'revisions when commits land with ongoing or failed builds.'))
         ->setOptions($land_options),
       id(new self())
         ->setKey(self::BEHAVIOR_BUILDABLE)