From c20b4e365b765ee5c31e12066140451affe3141e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 3 Apr 2018 05:48:40 -0700 Subject: [PATCH] Move structural build publishing logic to BuildEngine, provide "bin/harbormaster publish" Summary: Depends on D19278. Ref T13110. This moves most of the structural logic for publishing builds to BuildableEngine and provides a `bin/harbormaster publish` to make publishing easy to retry/debug. This intentionally removes the bit which actually does anything when builds publish. Followup changes will implement application-specific versions of the publishing logic in Differential and Diffusion. Test Plan: Ran `bin/harbormaster publish Bxxx`, saw it do nothing (but not crash). Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13110 Differential Revision: https://secure.phabricator.com/D19279 --- src/__phutil_library_map__.php | 2 + .../engine/HarbormasterBuildEngine.php | 85 +++++----------- .../engine/HarbormasterBuildableEngine.php | 99 ++++++++++++++++++- .../HarbormasterManagementPublishWorkflow.php | 87 ++++++++++++++++ 4 files changed, 212 insertions(+), 61 deletions(-) create mode 100644 src/applications/harbormaster/management/HarbormasterManagementPublishWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 32c80d75a1..7de5918a42 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1324,6 +1324,7 @@ phutil_register_library_map(array( 'HarbormasterLogWorker' => 'applications/harbormaster/worker/HarbormasterLogWorker.php', 'HarbormasterManagementArchiveLogsWorkflow' => 'applications/harbormaster/management/HarbormasterManagementArchiveLogsWorkflow.php', 'HarbormasterManagementBuildWorkflow' => 'applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php', + 'HarbormasterManagementPublishWorkflow' => 'applications/harbormaster/management/HarbormasterManagementPublishWorkflow.php', 'HarbormasterManagementRebuildLogWorkflow' => 'applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php', 'HarbormasterManagementRestartWorkflow' => 'applications/harbormaster/management/HarbormasterManagementRestartWorkflow.php', 'HarbormasterManagementUpdateWorkflow' => 'applications/harbormaster/management/HarbormasterManagementUpdateWorkflow.php', @@ -6675,6 +6676,7 @@ phutil_register_library_map(array( 'HarbormasterLogWorker' => 'HarbormasterWorker', 'HarbormasterManagementArchiveLogsWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow', + 'HarbormasterManagementPublishWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementRebuildLogWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementRestartWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementUpdateWorkflow' => 'HarbormasterManagementWorkflow', diff --git a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php index f3589bfc53..170e4c8a5c 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildEngine.php @@ -489,6 +489,8 @@ final class HarbormasterBuildEngine extends Phobject { } } + $old = clone $buildable; + // Don't update the buildable status if we're still preparing builds: more // builds may still be scheduled shortly, so even if every build we know // about so far has passed, that doesn't mean the buildable has actually @@ -515,8 +517,7 @@ final class HarbormasterBuildEngine extends Phobject { $new_status = HarbormasterBuildableStatus::STATUS_BUILDING; } - $old_status = $buildable->getBuildableStatus(); - $did_update = ($old_status != $new_status); + $did_update = ($old->getBuildableStatus() !== $new_status); if ($did_update) { $buildable->setBuildableStatus($new_status); $buildable->save(); @@ -530,81 +531,45 @@ final class HarbormasterBuildEngine extends Phobject { return; } - // 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. + $this->publishBuildable($old, $buildable); + } - // 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. + public function publishBuildable( + HarbormasterBuildable $old, + HarbormasterBuildable $new) { - $should_publish = - ($did_update) && - ($new_status != HarbormasterBuildableStatus::STATUS_BUILDING) && - (!$buildable->getIsManualBuildable()); + $viewer = $this->getViewer(); - if (!$should_publish) { - return; - } + // Publish the buildable. We publish buildables even if they haven't + // changed status in Harbormaster because applications may care about + // different things than Harbormaster does. For example, Differential + // does not care about local lint and unit tests when deciding whether + // a revision should move out of draft or not. + + // NOTE: We're publishing both automatic and manual buildables. Buildable + // objects should generally ignore manual buildables, but it's up to them + // to decide. $object = id(new PhabricatorObjectQuery()) ->setViewer($viewer) - ->withPHIDs(array($buildable->getBuildablePHID())) + ->withPHIDs(array($new->getBuildablePHID())) ->executeOne(); if (!$object) { return; } - $publish_phid = $object->getHarbormasterPublishablePHID(); - if (!$publish_phid) { - return; - } - - 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 = $publish->getApplicationTransactionTemplate(); - if (!$template) { - return; - } - - $template - ->setTransactionType(PhabricatorTransactions::TYPE_BUILDABLE) - ->setMetadataValue( - 'harbormaster:buildablePHID', - $buildable->getPHID()) - ->setOldValue($old_status) - ->setNewValue($new_status); - - $harbormaster_phid = id(new PhabricatorHarbormasterApplication()) - ->getPHID(); + $engine = HarbormasterBuildableEngine::newForObject($object, $viewer); $daemon_source = PhabricatorContentSource::newForSource( PhabricatorDaemonContentSource::SOURCECONST); - $editor = $publish->getApplicationTransactionEditor() - ->setActor($viewer) + $harbormaster_phid = id(new PhabricatorHarbormasterApplication()) + ->getPHID(); + + $engine ->setActingAsPHID($harbormaster_phid) ->setContentSource($daemon_source) - ->setContinueOnNoEffect(true) - ->setContinueOnMissingFields(true); - - $editor->applyTransactions( - $publish->getApplicationTransactionObject(), - array($template)); + ->publishBuildable($old, $new); } private function releaseAllArtifacts(HarbormasterBuild $build) { diff --git a/src/applications/harbormaster/engine/HarbormasterBuildableEngine.php b/src/applications/harbormaster/engine/HarbormasterBuildableEngine.php index 5e35f6223e..993ca5fc6d 100644 --- a/src/applications/harbormaster/engine/HarbormasterBuildableEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterBuildableEngine.php @@ -1,4 +1,101 @@ viewer = $viewer; + return $this; + } + + final public function getViewer() { + return $this->viewer; + } + + final public function setActingAsPHID($acting_as_phid) { + $this->actingAsPHID = $acting_as_phid; + return $this; + } + + final public function getActingAsPHID() { + return $this->actingAsPHID; + } + + final public function setContentSource( + PhabricatorContentSource $content_source) { + $this->contentSource = $content_source; + return $this; + } + + final public function getContentSource() { + return $this->contentSource; + } + + final public function setObject(HarbormasterBuildableInterface $object) { + $this->object = $object; + return $this; + } + + final public function getObject() { + return $this->object; + } + + final public function publishBuildable( + HarbormasterBuildable $old, + HarbormasterBuildable $new) { + return; + } + + final public static function newForObject( + HarbormasterBuildableInterface $object, + PhabricatorUser $viewer) { + return $object->newBuildableEngine() + ->setViewer($viewer) + ->setObject($object); + } + + final protected function newEditor() { + $publishable = $this->getObject(); + + $viewer = $this->getViewer(); + + $editor = $publishable->getApplicationTransactionEditor() + ->setActor($viewer) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true); + + $acting_as_phid = $this->getActingAsPHID(); + if ($acting_as_phid !== null) { + $editor->setActingAsPHID($acting_as_phid); + } + + $content_source = $this->getContentSource(); + if ($content_source !== null) { + $editor->setContentSource($content_source); + } + + return $editor; + } + + final protected function newTransaction() { + $publishable = $this->getObject(); + + return $publishable->getApplicationTransactionTemplate(); + } + + final protected function applyTransactions(array $xactions) { + $publishable = $this->getObject(); + $editor = $this->newEditor(); + + $editor->applyTransactions( + $publishable->getApplicationTransactionObject(), + $xactions); + } + + +} diff --git a/src/applications/harbormaster/management/HarbormasterManagementPublishWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementPublishWorkflow.php new file mode 100644 index 0000000000..df42d8632f --- /dev/null +++ b/src/applications/harbormaster/management/HarbormasterManagementPublishWorkflow.php @@ -0,0 +1,87 @@ +setName('publish') + ->setExamples(pht('**publish** __buildable__ ...')) + ->setSynopsis( + pht( + 'Publish a buildable. This is primarily useful for developing '. + 'and debugging applications which have buildable objects.')) + ->setArguments( + array( + array( + 'name' => 'buildable', + 'wildcard' => true, + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $buildable_names = $args->getArg('buildable'); + if (!$buildable_names) { + throw new PhutilArgumentUsageException( + pht( + 'Name one or more buildables to publish, like "B123".')); + } + + $query = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames($buildable_names); + + $query->execute(); + + $result_map = $query->getNamedResults(); + + foreach ($buildable_names as $name) { + if (!isset($result_map[$name])) { + throw new PhutilArgumentUsageException( + pht( + 'Argument "%s" does not name a buildable. Provide one or more '. + 'valid buildable monograms or PHIDs.', + $name)); + } + } + + foreach ($result_map as $name => $result) { + if (!($result instanceof HarbormasterBuildable)) { + throw new PhutilArgumentUsageException( + pht( + 'Object "%s" is not a HarbormasterBuildable (it is a "%s"). '. + 'Name one or more buildables to publish, like "B123".', + get_class($result))); + } + } + + foreach ($result_map as $buildable) { + echo tsprintf( + "%s\n", + pht( + 'Publishing "%s"...', + $buildable->getMonogram())); + + // Reload the buildable to pick up builds. + $buildable = id(new HarbormasterBuildableQuery()) + ->setViewer($viewer) + ->withIDs(array($buildable->getID())) + ->needBuilds(true) + ->executeOne(); + + $engine = id(new HarbormasterBuildEngine()) + ->setViewer($viewer) + ->publishBuildable($buildable, $buildable); + } + + echo tsprintf( + "%s\n", + pht('Done.')); + + return 0; + } + +}