From 7ffea0463e428a79a08999daab597d9feda967cf Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Fri, 8 Nov 2013 16:48:17 -0800 Subject: [PATCH] Use herald to trigger builds of revisions and commits. Summary: Depends on D7500. This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code). Test Plan: Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley CC: Korvin, epriestley, aran, hwinkel Maniphest Tasks: T1049 Differential Revision: https://secure.phabricator.com/D7501 --- .../editor/DifferentialRevisionEditor.php | 5 ++ .../HarbormasterBuildableApplyController.php | 15 +--- .../HarbormasterBuildableViewController.php | 8 +- .../phid/HarbormasterPHIDTypeBuildPlan.php | 2 + .../query/HarbormasterBuildableQuery.php | 52 +++++++++--- .../storage/HarbormasterBuildable.php | 79 +++++++++++++++++++ .../herald/adapter/HeraldAdapter.php | 5 ++ .../herald/adapter/HeraldCommitAdapter.php | 17 +++- .../HeraldDifferentialRevisionAdapter.php | 15 ++++ .../controller/HeraldRuleController.php | 1 + ...habricatorRepositoryCommitHeraldWorker.php | 5 ++ ...torTypeaheadCommonDatasourceController.php | 15 ++++ .../js/application/herald/HeraldRuleEditor.js | 1 + 13 files changed, 194 insertions(+), 26 deletions(-) diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index 028948d32b..beefc3f114 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/DifferentialRevisionEditor.php @@ -273,6 +273,11 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { $rem_ccs = $adapter->getCCsRemovedByHerald(); $blocking_reviewers = array_keys( $adapter->getBlockingReviewersAddedByHerald()); + + HarbormasterBuildable::applyBuildPlans( + $diff->getPHID(), + $revision->getPHID(), + $adapter->getBuildPlans()); } else { $sub = array( 'rev' => array(), diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php b/src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php index 215e117953..b1eff91f15 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php @@ -31,17 +31,10 @@ final class HarbormasterBuildableApplyController ->withIDs(array($request->getInt('build-plan'))) ->executeOne(); - $build = HarbormasterBuild::initializeNewBuild($viewer); - $build->setBuildablePHID($buildable->getPHID()); - $build->setBuildPlanPHID($plan->getPHID()); - $build->setBuildStatus(HarbormasterBuild::STATUS_PENDING); - $build->save(); - - PhabricatorWorker::scheduleTask( - 'HarbormasterBuildWorker', - array( - 'buildID' => $build->getID() - )); + HarbormasterBuildable::applyBuildPlans( + $buildable->getBuildablePHID(), + $buildable->getContainerPHID(), + array($plan->getPHID())); return id(new AphrontRedirectResponse())->setURI($buildable_uri); } diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php index a5c9c0b34d..c1eb56496a 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php @@ -19,7 +19,7 @@ final class HarbormasterBuildableViewController ->setViewer($viewer) ->withIDs(array($id)) ->needBuildableHandles(true) - ->needContainerObjects(true) + ->needContainerHandles(true) ->executeOne(); if (!$buildable) { return new Aphront404Response(); @@ -139,6 +139,12 @@ final class HarbormasterBuildableViewController pht('Buildable'), $buildable->getBuildableHandle()->renderLink()); + if ($buildable->getContainerHandle() !== null) { + $properties->addProperty( + pht('Container'), + $buildable->getContainerHandle()->renderLink()); + } + } } diff --git a/src/applications/harbormaster/phid/HarbormasterPHIDTypeBuildPlan.php b/src/applications/harbormaster/phid/HarbormasterPHIDTypeBuildPlan.php index c378902f5d..0a18c73d85 100644 --- a/src/applications/harbormaster/phid/HarbormasterPHIDTypeBuildPlan.php +++ b/src/applications/harbormaster/phid/HarbormasterPHIDTypeBuildPlan.php @@ -31,6 +31,8 @@ final class HarbormasterPHIDTypeBuildPlan extends PhabricatorPHIDType { foreach ($handles as $phid => $handle) { $build_plan = $objects[$phid]; + $handles[$phid]->setName($build_plan->getName()); + $handles[$phid]->setURI('/harbormaster/plan/'.$build_plan->getID()); } } diff --git a/src/applications/harbormaster/query/HarbormasterBuildableQuery.php b/src/applications/harbormaster/query/HarbormasterBuildableQuery.php index decf9d8a40..1aae1b4240 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildableQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildableQuery.php @@ -9,6 +9,7 @@ final class HarbormasterBuildableQuery private $containerPHIDs; private $needContainerObjects; + private $needContainerHandles; private $needBuildableHandles; private $needBuilds; @@ -37,6 +38,11 @@ final class HarbormasterBuildableQuery return $this; } + public function needContainerHandles($need) { + $this->needContainerHandles = $need; + return $this; + } + public function needBuildableHandles($need) { $this->needBuildableHandles = $need; return $this; @@ -88,22 +94,42 @@ final class HarbormasterBuildableQuery } protected function didFilterPage(array $page) { - if ($this->needContainerObjects) { - $containers = array(); - + if ($this->needContainerObjects || $this->needContainerHandles) { $container_phids = array_filter(mpull($page, 'getContainerPHID')); - if ($container_phids) { - $containers = id(new PhabricatorObjectQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs($container_phids) - ->setParentQuery($this) - ->execute(); - $containers = mpull($containers, null, 'getPHID'); + + if ($this->needContainerObjects) { + $containers = array(); + + if ($container_phids) { + $containers = id(new PhabricatorObjectQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($container_phids) + ->setParentQuery($this) + ->execute(); + $containers = mpull($containers, null, 'getPHID'); + } + + foreach ($page as $key => $buildable) { + $container_phid = $buildable->getContainerPHID(); + $buildable->attachContainerObject(idx($containers, $container_phid)); + } } - foreach ($page as $key => $buildable) { - $container_phid = $buildable->getContainerPHID(); - $buildable->attachContainerObject(idx($containers, $container_phid)); + if ($this->needContainerHandles) { + $handles = array(); + + if ($container_phids) { + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($container_phids) + ->setParentQuery($this) + ->execute(); + } + + foreach ($page as $key => $buildable) { + $container_phid = $buildable->getContainerPHID(); + $buildable->attachContainerHandle(idx($handles, $container_phid)); + } } } diff --git a/src/applications/harbormaster/storage/HarbormasterBuildable.php b/src/applications/harbormaster/storage/HarbormasterBuildable.php index ebaf15842f..0dcb9e86c5 100644 --- a/src/applications/harbormaster/storage/HarbormasterBuildable.php +++ b/src/applications/harbormaster/storage/HarbormasterBuildable.php @@ -11,6 +11,7 @@ final class HarbormasterBuildable extends HarbormasterDAO private $buildableObject = self::ATTACHABLE; private $containerObject = self::ATTACHABLE; private $buildableHandle = self::ATTACHABLE; + private $containerHandle = self::ATTACHABLE; private $builds = self::ATTACHABLE; const STATUS_WHATEVER = 'whatever'; @@ -21,6 +22,75 @@ final class HarbormasterBuildable extends HarbormasterDAO ->setBuildableStatus(self::STATUS_WHATEVER); } + /** + * Returns an existing buildable for the object's PHID or creates a + * new buildable implicitly if needed. + */ + public static function createOrLoadExisting( + PhabricatorUser $actor, + $buildable_object_phid, + $container_object_phid) { + + $buildable = id(new HarbormasterBuildableQuery()) + ->setViewer($actor) + ->withBuildablePHIDs(array($buildable_object_phid)) + ->executeOne(); + if ($buildable) { + return $buildable; + } + $buildable = HarbormasterBuildable::initializeNewBuildable($actor) + ->setBuildablePHID($buildable_object_phid) + ->setContainerPHID($container_object_phid); + $buildable->save(); + return $buildable; + } + + /** + * Looks up the plan PHIDs and applies the plans to the specified + * object identified by it's PHID. + */ + public static function applyBuildPlans( + $phid, + $container_phid, + array $plan_phids) { + + if (count($plan_phids) === 0) { + return; + } + + // Skip all of this logic if the Harbormaster application + // isn't currently installed. + + $harbormaster_app = 'PhabricatorApplicationHarbormaster'; + if (!PhabricatorApplication::isClassInstalled($harbormaster_app)) { + return; + } + + $buildable = HarbormasterBuildable::createOrLoadExisting( + PhabricatorUser::getOmnipotentUser(), + $phid, + $container_phid); + + $plans = id(new HarbormasterBuildPlanQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($plan_phids) + ->execute(); + foreach ($plans as $plan) { + $build = HarbormasterBuild::initializeNewBuild( + PhabricatorUser::getOmnipotentUser()); + $build->setBuildablePHID($buildable->getPHID()); + $build->setBuildPlanPHID($plan->getPHID()); + $build->setBuildStatus(HarbormasterBuild::STATUS_PENDING); + $build->save(); + + PhabricatorWorker::scheduleTask( + 'HarbormasterBuildWorker', + array( + 'buildID' => $build->getID() + )); + } + } + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, @@ -50,6 +120,15 @@ final class HarbormasterBuildable extends HarbormasterDAO return $this->assertAttached($this->containerObject); } + public function attachContainerHandle($container_handle) { + $this->containerHandle = $container_handle; + return $this; + } + + public function getContainerHandle() { + return $this->assertAttached($this->containerHandle); + } + public function attachBuildableHandle($buildable_handle) { $this->buildableHandle = $buildable_handle; return $this; diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 041e01adef..40cd9c8aaa 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -54,6 +54,7 @@ abstract class HeraldAdapter { const ACTION_ADD_PROJECTS = 'addprojects'; const ACTION_ADD_REVIEWERS = 'addreviewers'; const ACTION_ADD_BLOCKING_REVIEWERS = 'addblockingreviewers'; + const ACTION_APPLY_BUILD_PLANS = 'applybuildplans'; const VALUE_TEXT = 'text'; const VALUE_NONE = 'none'; @@ -67,6 +68,7 @@ abstract class HeraldAdapter { const VALUE_FLAG_COLOR = 'flagcolor'; const VALUE_CONTENT_SOURCE = 'contentsource'; const VALUE_USER_OR_PROJECT = 'userorproject'; + const VALUE_BUILD_PLAN = 'buildplan'; private $contentSource; @@ -490,6 +492,7 @@ abstract class HeraldAdapter { self::ACTION_ADD_PROJECTS => pht('Add projects'), self::ACTION_ADD_REVIEWERS => pht('Add reviewers'), self::ACTION_ADD_BLOCKING_REVIEWERS => pht('Add blocking reviewers'), + self::ACTION_APPLY_BUILD_PLANS => pht('Apply build plans'), ); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: return array( @@ -657,6 +660,8 @@ abstract class HeraldAdapter { case self::ACTION_ADD_REVIEWERS: case self::ACTION_ADD_BLOCKING_REVIEWERS: return self::VALUE_USER_OR_PROJECT; + case self::ACTION_APPLY_BUILD_PLANS: + return self::VALUE_BUILD_PLAN; default: throw new Exception("Unknown or invalid action '{$action}'."); } diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index 5b56446ccf..2c9d7090d8 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -22,6 +22,7 @@ final class HeraldCommitAdapter extends HeraldAdapter { protected $emailPHIDs = array(); protected $addCCPHIDs = array(); protected $auditMap = array(); + protected $buildPlans = array(); protected $affectedPaths; protected $affectedRevision; @@ -120,7 +121,8 @@ final class HeraldCommitAdapter extends HeraldAdapter { self::ACTION_ADD_CC, self::ACTION_EMAIL, self::ACTION_AUDIT, - self::ACTION_NOTHING, + self::ACTION_APPLY_BUILD_PLANS, + self::ACTION_NOTHING ); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: return array( @@ -176,6 +178,10 @@ final class HeraldCommitAdapter extends HeraldAdapter { return $this->auditMap; } + public function getBuildPlans() { + return $this->buildPlans; + } + public function getHeraldName() { return 'r'. @@ -422,6 +428,15 @@ final class HeraldCommitAdapter extends HeraldAdapter { true, pht('Triggered an audit.')); break; + case self::ACTION_APPLY_BUILD_PLANS: + foreach ($effect->getTarget() as $phid) { + $this->buildPlans[] = $phid; + } + $result[] = new HeraldApplyTranscript( + $effect, + true, + pht('Applied build plans.')); + break; case self::ACTION_FLAG: $result[] = parent::applyFlagEffect( $effect, diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 2b0c7ee391..7a9116d3a4 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -14,6 +14,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { protected $emailPHIDs = array(); protected $addReviewerPHIDs = array(); protected $blockingReviewerPHIDs = array(); + protected $buildPlans = array(); protected $repository; protected $affectedPackages; @@ -117,6 +118,10 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { return $this->blockingReviewerPHIDs; } + public function getBuildPlans() { + return $this->buildPlans; + } + public function getPHID() { return $this->revision->getPHID(); } @@ -349,6 +354,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { self::ACTION_EMAIL, self::ACTION_ADD_REVIEWERS, self::ACTION_ADD_BLOCKING_REVIEWERS, + self::ACTION_APPLY_BUILD_PLANS, self::ACTION_NOTHING, ); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: @@ -468,6 +474,15 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { true, pht('Added blocking reviewers.')); break; + case self::ACTION_APPLY_BUILD_PLANS: + foreach ($effect->getTarget() as $phid) { + $this->buildPlans[] = $phid; + } + $result[] = new HeraldApplyTranscript( + $effect, + true, + pht('Applied build plans.')); + break; default: throw new Exception("No rules to handle action '{$action}'."); } diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index adfa8fef49..b9e46d58a6 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -511,6 +511,7 @@ final class HeraldRuleController extends HeraldController { 'package' => '/typeahead/common/packages/', 'project' => '/typeahead/common/projects/', 'userorproject' => '/typeahead/common/accountsorprojects/', + 'buildplan' => '/typeahead/common/buildplans/', ), 'markup' => $template, ); diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index eaa9f682e8..3427628f5b 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -66,6 +66,11 @@ final class PhabricatorRepositoryCommitHeraldWorker $this->createAudits($commit, $audit_phids, $cc_phids, $rules); } + HarbormasterBuildable::applyBuildPlans( + $commit->getPHID(), + $repository->getPHID(), + $adapter->getBuildPlans()); + $explicit_auditors = $this->createAuditsFromCommitMessage($commit, $data); if ($repository->getDetail('herald-disabled')) { diff --git a/src/applications/typeahead/controller/PhabricatorTypeaheadCommonDatasourceController.php b/src/applications/typeahead/controller/PhabricatorTypeaheadCommonDatasourceController.php index ce2bcce99f..d9830ad85c 100644 --- a/src/applications/typeahead/controller/PhabricatorTypeaheadCommonDatasourceController.php +++ b/src/applications/typeahead/controller/PhabricatorTypeaheadCommonDatasourceController.php @@ -35,6 +35,7 @@ final class PhabricatorTypeaheadCommonDatasourceController $need_noproject = false; $need_symbols = false; $need_jump_objects = false; + $need_build_plans = false; switch ($this->type) { case 'mainsearch': $need_users = true; @@ -93,6 +94,9 @@ final class PhabricatorTypeaheadCommonDatasourceController case 'arcanistprojects': $need_arcanist_projects = true; break; + case 'buildplans': + $need_build_plans = true; + break; } $results = array(); @@ -219,6 +223,17 @@ final class PhabricatorTypeaheadCommonDatasourceController } } + if ($need_build_plans) { + $plans = id(new HarbormasterBuildPlanQuery()) + ->setViewer($viewer) + ->execute(); + foreach ($plans as $plan) { + $results[] = id(new PhabricatorTypeaheadResult()) + ->setName($plan->getName()) + ->setPHID($plan->getPHID()); + } + } + if ($need_projs) { $projs = id(new PhabricatorProjectQuery()) ->setViewer($viewer) diff --git a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js index 35b7a01e90..7d8cde287e 100644 --- a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js +++ b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js @@ -221,6 +221,7 @@ JX.install('HeraldRuleEditor', { case 'package': case 'project': case 'userorproject': + case 'buildplan': var tokenizer = this._newTokenizer(type); input = tokenizer[0]; get_fn = tokenizer[1];