Page MenuHomePhabricator
Files F15412709

D20239.diff
No OneTemporary
Actions

D20239.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -653,6 +653,7 @@
'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 @@
'DifferentialRevisionUpdateTransaction' => 'DifferentialRevisionTransactionType',
'DifferentialRevisionViewController' => 'DifferentialController',
'DifferentialRevisionVoidTransaction' => 'DifferentialRevisionTransactionType',
+ 'DifferentialRevisionWrongBuildsTransaction' => 'DifferentialRevisionTransactionType',
'DifferentialRevisionWrongStateTransaction' => 'DifferentialRevisionTransactionType',
'DifferentialSchemaSpec' => 'PhabricatorConfigSchemaSpec',
'DifferentialSetDiffPropertyConduitAPIMethod' => 'DifferentialConduitAPIMethod',
diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
--- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
+++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php
@@ -285,6 +285,24 @@
->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 @@
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;
+ }
+
}
diff --git a/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php b/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php
new file mode 100644
--- /dev/null
+++ b/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;
+ }
+
+}
diff --git a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php
--- a/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php
+++ b/src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php
@@ -27,6 +27,12 @@
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 @@
$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 @@
'"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 @@
'"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 @@
'"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 @@
'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 @@
'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)

File Metadata

Mime Type
text/plain
Expires
Thu, Mar 20, 2:31 PM (2 d, 5 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7711962
Default Alt Text
D20239.diff (9 KB)

Event Timeline

Phabricator · Built by Phacility