From 2728a9f9649e3be6abef5001d81da536a2106415 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 2 Oct 2015 06:32:08 -0700 Subject: [PATCH] Allow builds to have parameters Summary: Ref T9352. See D13635. Build targets can have variables already, but let builds have them too. This mostly enables future use cases (sub-builds, more sophisticated build triggers). Test Plan: With a custom Herald rule + action like the one in T9352, updated a revision and saw it generate multiple builds with varying parameters. Reviewers: chad, hach-que Reviewed By: hach-que Maniphest Tasks: T9352 Differential Revision: https://secure.phabricator.com/D14222 --- .../20151002.harbormaster.bparam.1.sql | 5 +++ src/__phutil_library_map__.php | 2 + .../HeraldDifferentialRevisionAdapter.php | 11 ++--- .../diffusion/herald/HeraldCommitAdapter.php | 11 ++--- .../HarbormasterPlanRunController.php | 2 +- .../engine/HarbormasterBuildRequest.php | 37 ++++++++++++++++ .../engine/HarbormasterTargetEngine.php | 2 +- .../HarbormasterBuildableAdapterInterface.php | 5 ++- .../HarbormasterRunBuildPlansHeraldAction.php | 4 +- .../HarbormasterManagementBuildWorkflow.php | 2 +- .../HarbormasterBuildStepImplementation.php | 2 +- .../storage/HarbormasterBuildable.php | 42 +++++++++++++++---- .../storage/build/HarbormasterBuild.php | 8 ++++ ...habricatorApplicationTransactionEditor.php | 2 +- 14 files changed, 108 insertions(+), 27 deletions(-) create mode 100644 resources/sql/autopatches/20151002.harbormaster.bparam.1.sql create mode 100644 src/applications/harbormaster/engine/HarbormasterBuildRequest.php diff --git a/resources/sql/autopatches/20151002.harbormaster.bparam.1.sql b/resources/sql/autopatches/20151002.harbormaster.bparam.1.sql new file mode 100644 index 0000000000..0deb71c503 --- /dev/null +++ b/resources/sql/autopatches/20151002.harbormaster.bparam.1.sql @@ -0,0 +1,5 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_build + ADD buildParameters LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL; + +UPDATE {$NAMESPACE}_harbormaster.harbormaster_build + SET buildParameters = '{}' WHERE buildParameters = ''; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8f8bc085fd..6fed2b21bd 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -972,6 +972,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanTransaction' => 'applications/harbormaster/storage/configuration/HarbormasterBuildPlanTransaction.php', 'HarbormasterBuildPlanTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildPlanTransactionQuery.php', 'HarbormasterBuildQuery' => 'applications/harbormaster/query/HarbormasterBuildQuery.php', + 'HarbormasterBuildRequest' => 'applications/harbormaster/engine/HarbormasterBuildRequest.php', 'HarbormasterBuildStep' => 'applications/harbormaster/storage/configuration/HarbormasterBuildStep.php', 'HarbormasterBuildStepCoreCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php', 'HarbormasterBuildStepCustomField' => 'applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php', @@ -4757,6 +4758,7 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanTransaction' => 'PhabricatorApplicationTransaction', 'HarbormasterBuildPlanTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'HarbormasterBuildQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildRequest' => 'Phobject', 'HarbormasterBuildStep' => array( 'HarbormasterDAO', 'PhabricatorApplicationTransactionInterface', diff --git a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php index a47d433357..d7b8c5dd33 100644 --- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php @@ -10,7 +10,7 @@ final class HeraldDifferentialRevisionAdapter protected $changesets; private $haveHunks; - private $buildPlanPHIDs = array(); + private $buildRequests = array(); public function getAdapterApplicationClass() { return 'PhabricatorDifferentialApplication'; @@ -139,12 +139,13 @@ final class HeraldDifferentialRevisionAdapter return $this->getObject()->getPHID(); } - public function getQueuedHarbormasterBuildPlanPHIDs() { - return $this->buildPlanPHIDs; + public function getQueuedHarbormasterBuildRequests() { + return $this->buildRequests; } - public function queueHarbormasterBuildPlanPHID($phid) { - $this->buildPlanPHIDs[] = $phid; + public function queueHarbormasterBuildRequest( + HarbormasterBuildRequest $request) { + $this->buildRequests[] = $request; } } diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php index ed82e90820..6530b72e6f 100644 --- a/src/applications/diffusion/herald/HeraldCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php @@ -17,7 +17,7 @@ final class HeraldCommitAdapter protected $affectedPackages; protected $auditNeededPackages; - private $buildPlanPHIDs = array(); + private $buildRequests = array(); public function getAdapterApplicationClass() { return 'PhabricatorDiffusionApplication'; @@ -308,12 +308,13 @@ final class HeraldCommitAdapter return $this->getObject()->getRepository()->getPHID(); } - public function getQueuedHarbormasterBuildPlanPHIDs() { - return $this->buildPlanPHIDs; + public function getQueuedHarbormasterBuildRequests() { + return $this->buildRequests; } - public function queueHarbormasterBuildPlanPHID($phid) { - $this->buildPlanPHIDs[] = $phid; + public function queueHarbormasterBuildRequest( + HarbormasterBuildRequest $request) { + $this->buildRequests[] = $request; } } diff --git a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php index 980c7fad4b..be6d138cf7 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanRunController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanRunController.php @@ -62,7 +62,7 @@ final class HarbormasterPlanRunController extends HarbormasterController { if (!$errors) { $buildable->save(); - $buildable->applyPlan($plan); + $buildable->applyPlan($plan, array()); $buildable_uri = '/B'.$buildable->getID(); return id(new AphrontRedirectResponse())->setURI($buildable_uri); diff --git a/src/applications/harbormaster/engine/HarbormasterBuildRequest.php b/src/applications/harbormaster/engine/HarbormasterBuildRequest.php new file mode 100644 index 0000000000..1874f08b27 --- /dev/null +++ b/src/applications/harbormaster/engine/HarbormasterBuildRequest.php @@ -0,0 +1,37 @@ +buildPlanPHID = $build_plan_phid; + return $this; + } + + public function getBuildPlanPHID() { + return $this->buildPlanPHID; + } + + public function setBuildParameters(array $build_parameters) { + $this->buildParameters = $build_parameters; + return $this; + } + + public function getBuildParameters() { + return $this->buildParameters; + } + +} diff --git a/src/applications/harbormaster/engine/HarbormasterTargetEngine.php b/src/applications/harbormaster/engine/HarbormasterTargetEngine.php index a8403385ac..b201891b6b 100644 --- a/src/applications/harbormaster/engine/HarbormasterTargetEngine.php +++ b/src/applications/harbormaster/engine/HarbormasterTargetEngine.php @@ -206,7 +206,7 @@ final class HarbormasterTargetEngine extends Phobject { // resource and "own" it, so we don't try to handle this, but may need // to be more careful here if use of autotargets expands. - $build = $buildable->applyPlan($plan); + $build = $buildable->applyPlan($plan, array()); PhabricatorWorker::setRunAllTasksInProcess(false); } catch (Exception $ex) { PhabricatorWorker::setRunAllTasksInProcess(false); diff --git a/src/applications/harbormaster/herald/HarbormasterBuildableAdapterInterface.php b/src/applications/harbormaster/herald/HarbormasterBuildableAdapterInterface.php index cf6cda8f95..6933306145 100644 --- a/src/applications/harbormaster/herald/HarbormasterBuildableAdapterInterface.php +++ b/src/applications/harbormaster/herald/HarbormasterBuildableAdapterInterface.php @@ -4,8 +4,9 @@ interface HarbormasterBuildableAdapterInterface { public function getHarbormasterBuildablePHID(); public function getHarbormasterContainerPHID(); - public function getQueuedHarbormasterBuildPlanPHIDs(); - public function queueHarbormasterBuildPlanPHID($phid); + public function getQueuedHarbormasterBuildRequests(); + public function queueHarbormasterBuildRequest( + HarbormasterBuildRequest $request); } diff --git a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php index 830598e812..db75b36d2f 100644 --- a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php +++ b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php @@ -31,7 +31,9 @@ final class HarbormasterRunBuildPlansHeraldAction $phids = array_fuse(array_keys($targets)); foreach ($phids as $phid) { - $adapter->queueHarbormasterBuildPlanPHID($phid); + $request = id(new HarbormasterBuildRequest()) + ->setBuildPlanPHID($phid); + $adapter->queueHarbormasterBuildRequest($request); } $this->logEffect(self::DO_BUILD, $phids); diff --git a/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php index fc0f670633..bc6a52018c 100644 --- a/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php +++ b/src/applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php @@ -89,7 +89,7 @@ final class HarbormasterManagementBuildWorkflow PhabricatorEnv::getProductionURI('/B'.$buildable->getID())); PhabricatorWorker::setRunAllTasksInProcess(true); - $buildable->applyPlan($plan); + $buildable->applyPlan($plan, array()); $console->writeOut("%s\n", pht('Done.')); diff --git a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php index b47f1d00d1..87148b0830 100644 --- a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php @@ -194,7 +194,7 @@ abstract class HarbormasterBuildStepImplementation extends Phobject { * @return string String with variables replaced safely into it. */ protected function mergeVariables($function, $pattern, array $variables) { - $regexp = '/\\$\\{(?P[a-z\\.]+)\\}/'; + $regexp = '@\\$\\{(?P[a-z\\./-]+)\\}@'; $matches = null; preg_match_all($regexp, $pattern, $matches); diff --git a/src/applications/harbormaster/storage/HarbormasterBuildable.php b/src/applications/harbormaster/storage/HarbormasterBuildable.php index 0376ebc890..e74471e879 100644 --- a/src/applications/harbormaster/storage/HarbormasterBuildable.php +++ b/src/applications/harbormaster/storage/HarbormasterBuildable.php @@ -96,15 +96,21 @@ final class HarbormasterBuildable extends HarbormasterDAO } /** - * Looks up the plan PHIDs and applies the plans to the specified - * object identified by it's PHID. + * Start builds for a given buildable. + * + * @param phid PHID of the object to build. + * @param phid Container PHID for the buildable. + * @param list List of builds to perform. + * @return void */ public static function applyBuildPlans( $phid, $container_phid, - array $plan_phids) { + array $requests) { - if (!$plan_phids) { + assert_instances_of($requests, 'HarbormasterBuildRequest'); + + if (!$requests) { return; } @@ -116,31 +122,49 @@ final class HarbormasterBuildable extends HarbormasterDAO return; } + $viewer = PhabricatorUser::getOmnipotentUser(); + $buildable = self::createOrLoadExisting( - PhabricatorUser::getOmnipotentUser(), + $viewer, $phid, $container_phid); + $plan_phids = mpull($requests, 'getBuildPlanPHID'); $plans = id(new HarbormasterBuildPlanQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->setViewer($viewer) ->withPHIDs($plan_phids) ->execute(); - foreach ($plans as $plan) { + $plans = mpull($plans, null, 'getPHID'); + + foreach ($requests as $request) { + $plan_phid = $request->getBuildPlanPHID(); + $plan = idx($plans, $plan_phid); + + if (!$plan) { + throw new Exception( + pht( + 'Failed to load build plan ("%s").', + $plan_phid)); + } + if ($plan->isDisabled()) { // TODO: This should be communicated more clearly -- maybe we should // create the build but set the status to "disabled" or "derelict". continue; } - $buildable->applyPlan($plan); + $parameters = $request->getBuildParameters(); + $buildable->applyPlan($plan, $parameters); } } - public function applyPlan(HarbormasterBuildPlan $plan) { + public function applyPlan(HarbormasterBuildPlan $plan, array $parameters) { + $viewer = PhabricatorUser::getOmnipotentUser(); $build = HarbormasterBuild::initializeNewBuild($viewer) ->setBuildablePHID($this->getPHID()) ->setBuildPlanPHID($plan->getPHID()) + ->setBuildParameters($parameters) ->setBuildStatus(HarbormasterBuild::STATUS_PENDING); $auto_key = $plan->getPlanAutoKey(); diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 1c012b1842..154681d93f 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -9,6 +9,7 @@ final class HarbormasterBuild extends HarbormasterDAO protected $buildPlanPHID; protected $buildStatus; protected $buildGeneration; + protected $buildParameters = array(); protected $planAutoKey; private $buildable = self::ATTACHABLE; @@ -156,6 +157,9 @@ final class HarbormasterBuild extends HarbormasterDAO protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, + self::CONFIG_SERIALIZATION => array( + 'buildParameters' => self::SERIALIZATION_JSON, + ), self::CONFIG_COLUMN_SCHEMA => array( 'buildStatus' => 'text32', 'buildGeneration' => 'uint32', @@ -258,6 +262,10 @@ final class HarbormasterBuild extends HarbormasterDAO 'build.id' => null, ); + foreach ($this->getBuildParameters() as $key => $value) { + $results['build/'.$key] = $value; + } + $buildable = $this->getBuildable(); $object = $buildable->getBuildableObject(); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 851cc24cc0..2efd19a000 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -2837,7 +2837,7 @@ abstract class PhabricatorApplicationTransactionEditor HarbormasterBuildable::applyBuildPlans( $adapter->getHarbormasterBuildablePHID(), $adapter->getHarbormasterContainerPHID(), - $adapter->getQueuedHarbormasterBuildPlanPHIDs()); + $adapter->getQueuedHarbormasterBuildRequests()); } return array_merge(