From ef05bf335d8e3fa18f22868f33c0b92df9b3aa9e Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 4 Jan 2017 11:31:44 -0800 Subject: [PATCH] Allow Harbormaster builds to publish to a different object Summary: Fixes T9276. Fixes T8650. The story so far: - We once published build updates to Revisions. - An unrelated fix (D10911) sent them to the Diffs instead of Revisions, which isn't useful, since you can't see a diff's timeline anywhere. - This also caused a race condition, where the RevisionEditor and DiffEditor would update the diff simultaneously (T8650). - The diff update was just disabled to avoid the race (part of D13441). - Instead, allow the updates to go somewhere else. In this case, we send commit updates to the commit but send diff updates to the revision so you can see 'em. - Since everything will be using the revision editor now, we should either get proper lock behavior for free or it should be easy to add if something whack is still happening. - Overall, this should pretty much put us back in working order like we were before D10911. This behavior is undoubtedly refinable, but this should let us move forward. Test Plan: Saw a build failure in timeline: {F2304575} Reviewers: chad Reviewed By: chad Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T9276, T8650 Differential Revision: https://secure.phabricator.com/D17139 --- .../differential/storage/DifferentialDiff.php | 4 +++ .../storage/DifferentialRevision.php | 4 +++ .../engine/HarbormasterBuildEngine.php | 25 +++++++++++++------ .../HarbormasterBuildableInterface.php | 15 +++++++++++ .../storage/HarbormasterBuildable.php | 4 +++ .../storage/PhabricatorRepositoryCommit.php | 4 +++ 6 files changed, 49 insertions(+), 7 deletions(-) diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index 3ca6ecca18..891af35982 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -502,6 +502,10 @@ final class DifferentialDiff return null; } + public function getHarbormasterPublishablePHID() { + return $this->getHarbormasterContainerPHID(); + } + public function getBuildVariables() { $results = array(); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index ab8b8ddc3a..1e90b12dc4 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -513,6 +513,10 @@ final class DifferentialRevision extends DifferentialDAO return $this->getPHID(); } + public function getHarbormasterPublishablePHID() { + return $this->getPHID(); + } + public function getBuildVariables() { return array(); } diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index 893f8fc546..0616e77b4e 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -497,17 +497,28 @@ final class HarbormasterBuildEngine extends Phobject { return; } - if (!($object instanceof PhabricatorApplicationTransactionInterface)) { + $publish_phid = $object->getHarbormasterPublishablePHID(); + if (!$publish_phid) { return; } - // TODO: Publishing these transactions is causing a race. See T8650. - // We shouldn't be publishing to diffs anyway. - if ($object instanceof DifferentialDiff) { + if ($publish_phid === $object->getPHID()) { + $publish = $object; + } else { + $publish = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($publish_phid)) + ->executeOne(); + if (!$publish) { + return; + } + } + + if (!($publish instanceof PhabricatorApplicationTransactionInterface)) { return; } - $template = $object->getApplicationTransactionTemplate(); + $template = $publish->getApplicationTransactionTemplate(); if (!$template) { return; } @@ -526,7 +537,7 @@ final class HarbormasterBuildEngine extends Phobject { $daemon_source = PhabricatorContentSource::newForSource( PhabricatorDaemonContentSource::SOURCECONST); - $editor = $object->getApplicationTransactionEditor() + $editor = $publish->getApplicationTransactionEditor() ->setActor($viewer) ->setActingAsPHID($harbormaster_phid) ->setContentSource($daemon_source) @@ -534,7 +545,7 @@ final class HarbormasterBuildEngine extends Phobject { ->setContinueOnMissingFields(true); $editor->applyTransactions( - $object->getApplicationTransactionObject(), + $publish->getApplicationTransactionObject(), array($template)); } diff --git a/src/applications/harbormaster/interface/HarbormasterBuildableInterface.php b/src/applications/harbormaster/interface/HarbormasterBuildableInterface.php index 0e852b72a3..598f2b61f0 100644 --- a/src/applications/harbormaster/interface/HarbormasterBuildableInterface.php +++ b/src/applications/harbormaster/interface/HarbormasterBuildableInterface.php @@ -18,6 +18,21 @@ interface HarbormasterBuildableInterface { public function getHarbormasterBuildablePHID(); public function getHarbormasterContainerPHID(); + + /** + * Get the object PHID which build status should be published to. + * + * In some cases (like commits), this is the object itself. In other cases, + * it is a different object: for example, diffs publish builds to revisions. + * + * This method can return `null` to disable publishing. + * + * @return phid|null Build status updates will be published to this object's + * transaction timeline. + */ + public function getHarbormasterPublishablePHID(); + + public function getBuildVariables(); public function getAvailableBuildVariables(); diff --git a/src/applications/harbormaster/storage/HarbormasterBuildable.php b/src/applications/harbormaster/storage/HarbormasterBuildable.php index 5e55d38824..9761fb287f 100644 --- a/src/applications/harbormaster/storage/HarbormasterBuildable.php +++ b/src/applications/harbormaster/storage/HarbormasterBuildable.php @@ -317,6 +317,10 @@ final class HarbormasterBuildable extends HarbormasterDAO return $this->getContainerPHID(); } + public function getHarbormasterPublishablePHID() { + return $this->getBuildableObject()->getHarbormasterPublishablePHID(); + } + public function getBuildVariables() { return array(); } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index f2ae90d9f3..58b9057449 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -413,6 +413,10 @@ final class PhabricatorRepositoryCommit return $this->getRepository()->getPHID(); } + public function getHarbormasterPublishablePHID() { + return $this->getPHID(); + } + public function getBuildVariables() { $results = array();