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

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 = '';

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',

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() {
+  public function getQueuedHarbormasterBuildRequests() {
-    return $this->buildPlanPHIDs;
+    return $this->buildRequests;
   }
 
-  public function queueHarbormasterBuildPlanPHID($phid) {
+  public function queueHarbormasterBuildRequest(
-    $this->buildPlanPHIDs[] = $phid;
+    HarbormasterBuildRequest $request) {
+    $this->buildRequests[] = $request;
   }
 
 }

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() {
+  public function getQueuedHarbormasterBuildRequests() {
-    return $this->buildPlanPHIDs;
+    return $this->buildRequests;
   }
 
-  public function queueHarbormasterBuildPlanPHID($phid) {
+  public function queueHarbormasterBuildRequest(
-    $this->buildPlanPHIDs[] = $phid;
+    HarbormasterBuildRequest $request) {
+    $this->buildRequests[] = $request;
   }
 
 }

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);

src/applications/harbormaster/engine/HarbormasterBuildRequest.php

@@ -0,0 +1,37 @@
+<?php
+
+/**
+ * Structure used to ask Harbormaster to start a build.
+ *
+ * Requests to start builds sometimes originate several layers away from where
+ * they are processed. For example, Herald rules which start builds pass the
+ * requests through the adapter and then through the editor before they reach
+ * Harbormaster.
+ *
+ * This class is just a thin wrapper around these requests so we can make them
+ * more complex later without needing to rewrite any APIs.
+ */
+final class HarbormasterBuildRequest extends Phobject {
+
+  private $buildPlanPHID;
+  private $buildParameters = array();
+
+  public function setBuildPlanPHID($build_plan_phid) {
+    $this->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;
+  }
+
+}

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);

src/applications/harbormaster/herald/HarbormasterBuildableAdapterInterface.php

@@ -4,8 +4,9 @@ interface HarbormasterBuildableAdapterInterface {
 
   public function getHarbormasterBuildablePHID();
   public function getHarbormasterContainerPHID();
-  public function getQueuedHarbormasterBuildPlanPHIDs();
+  public function getQueuedHarbormasterBuildRequests();
-  public function queueHarbormasterBuildPlanPHID($phid);
+  public function queueHarbormasterBuildRequest(
+    HarbormasterBuildRequest $request);
 
 }
 

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);

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.'));
 

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<name>[a-z\\.]+)\\}/';
+    $regexp = '@\\$\\{(?P<name>[a-z\\./-]+)\\}@';
 
     $matches = null;
     preg_match_all($regexp, $pattern, $matches);

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
+   * Start builds for a given buildable.
-   * object identified by it's PHID.
+   *
+   * @param phid PHID of the object to build.
+   * @param phid Container PHID for the buildable.
+   * @param list<HarbormasterBuildRequest> 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();

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();
 

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(