Allow Harbormaster build plans to request additional working copies

Summary:
Ref T9123. To run upstream builds in Harbormaster/Drydock, we need to be able to check out `libphutil`, `arcanist` and `phabricator` next to one another.

This adds an "Also Clone: ..." field to Harbormaster working copy build steps so I can type all three repos into it and get a proper clone with everything we need.

This is somewhat upstream-centric and a bit narrow, but I don't think it's totally unreasonable, and most of the underlying stuff is relatively general.

This adds some more typechecking and improves data/type handling for custom fields, too. In particular, it prevents users from entering an invalid/restricted value in a field (for example, you can't "Also Clone" a repository you don't have permission to see).

Test Plan: Restarted build, got a Drydock resource with multiple repositories in it.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9123

Differential Revision: https://secure.phabricator.com/D14183

src/applications/harbormaster/controller/HarbormasterStepEditController.php

@@ -72,9 +72,9 @@ final class HarbormasterStepEditController extends HarbormasterController {
 
     $e_name = true;
     $v_name = $step->getName();
-    $e_description = true;
+    $e_description = null;
     $v_description = $step->getDescription();
-    $e_depends_on = true;
+    $e_depends_on = null;
     $v_depends_on = $step->getDetail('dependsOn', array());
 
     $errors = array();
@@ -82,9 +82,7 @@ final class HarbormasterStepEditController extends HarbormasterController {
     if ($request->isFormPost()) {
       $e_name = null;
       $v_name = $request->getStr('name');
-      $e_description = null;
       $v_description = $request->getStr('description');
-      $e_depends_on = null;
       $v_depends_on = $request->getArr('dependsOn');
 
       $xactions = $field_list->buildFieldTransactionsFromRequest(
@@ -139,6 +137,12 @@ final class HarbormasterStepEditController extends HarbormasterController {
           ->setError($e_name)
           ->setValue($v_name));
 
+    $form->appendChild(id(new AphrontFormDividerControl()));
+
+    $field_list->appendFieldsToForm($form);
+
+    $form->appendChild(id(new AphrontFormDividerControl()));
+
     $form
       ->appendControl(
         id(new AphrontFormTokenizerControl())
@@ -152,8 +156,6 @@ final class HarbormasterStepEditController extends HarbormasterController {
           ->setError($e_depends_on)
           ->setValue($v_depends_on));
 
-    $field_list->appendFieldsToForm($form);
-
     $form
       ->appendChild(
         id(new PhabricatorRemarkupControl())

src/applications/harbormaster/customfield/HarbormasterBuildStepCoreCustomField.php

@@ -72,4 +72,8 @@ final class HarbormasterBuildStepCoreCustomField
     return;
   }
 
+  public function getBuildTargetFieldValue() {
+    return $this->getProxy()->getFieldValue();
+  }
+
 }

src/applications/harbormaster/customfield/HarbormasterBuildStepCustomField.php

@@ -1,4 +1,8 @@
 <?php
 
 abstract class HarbormasterBuildStepCustomField
-  extends PhabricatorCustomField {}
+  extends PhabricatorCustomField {
+
+  abstract public function getBuildTargetFieldValue();
+
+}

src/applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php

@@ -100,6 +100,11 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation
         'type' => 'text',
         'required' => true,
       ),
+      'repositoryPHIDs' => array(
+        'name' => pht('Also Clone'),
+        'type' => 'datasource',
+        'datasource.class' => 'DiffusionRepositoryDatasource',
+      ),
     );
   }
 
@@ -108,22 +113,40 @@ final class HarbormasterLeaseWorkingCopyBuildStepImplementation
     $variables = $build_target->getVariables();
 
     $repository_phid = idx($variables, 'repository.phid');
+    $also_phids = $build_target->getFieldValue('repositoryPHIDs');
 
-    $repository = id(new PhabricatorRepositoryQuery())
+    $all_phids = $also_phids;
+    $all_phids[] = $repository_phid;
+
+    $repositories = id(new PhabricatorRepositoryQuery())
       ->setViewer($viewer)
-      ->withPHIDs(array($repository_phid))
+      ->withPHIDs($all_phids)
-      ->executeOne();
+      ->execute();
-    if (!$repository) {
+    $repositories = mpull($repositories, null, 'getPHID');
+
+    foreach ($all_phids as $phid) {
+      if (empty($repositories[$phid])) {
         throw new PhabricatorWorkerPermanentFailureException(
           pht(
             'Unable to load repository with PHID "%s".',
-          $repository_phid));
+            $phid));
+      }
     }
 
     $commit = idx($variables, 'repository.commit');
 
     $map = array();
 
+    foreach ($also_phids as $also_phid) {
+      $also_repo = $repositories[$also_phid];
+      $map[$also_repo->getCloneName()] = array(
+        'phid' => $also_repo->getPHID(),
+        'branch' => 'master',
+      );
+    }
+
+    $repository = $repositories[$repository_phid];
+
     $directory = $repository->getCloneName();
     $map[$directory] = array(
       'phid' => $repository->getPHID(),

src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php

@@ -263,6 +263,28 @@ final class HarbormasterBuildTarget extends HarbormasterDAO
     return $log;
   }
 
+  public function getFieldValue($key) {
+    $field_list = PhabricatorCustomField::getObjectFields(
+      $this->getBuildStep(),
+      PhabricatorCustomField::ROLE_VIEW);
+
+    $fields = $field_list->getFields();
+    $full_key = "std:harbormaster:core:{$key}";
+
+    $field = idx($fields, $full_key);
+    if (!$field) {
+      throw new Exception(
+        pht(
+          'Unknown build step field "%s"!',
+          $key));
+    }
+
+    $field = clone $field;
+    $field->setValueFromStorage($this->getDetail($key));
+    return $field->getBuildTargetFieldValue();
+  }
+
+
 
 /* -(  Status  )------------------------------------------------------------- */
 

src/applications/harbormaster/worker/HarbormasterTargetWorker.php

@@ -28,6 +28,7 @@ final class HarbormasterTargetWorker extends HarbormasterWorker {
     $target = id(new HarbormasterBuildTargetQuery())
       ->withIDs(array($id))
       ->setViewer($this->getViewer())
+      ->needBuildSteps(true)
       ->executeOne();
 
     if (!$target) {

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php

@@ -152,6 +152,61 @@ abstract class PhabricatorStandardCustomFieldPHIDs
     }
   }
 
+  public function validateApplicationTransactions(
+    PhabricatorApplicationTransactionEditor $editor,
+    $type,
+    array $xactions) {
+
+    $errors = parent::validateApplicationTransactions(
+      $editor,
+      $type,
+      $xactions);
+
+    // If the user is adding PHIDs, make sure the new PHIDs are valid and
+    // visible to the actor. It's OK for a user to edit a field which includes
+    // some invalid or restricted values, but they can't add new ones.
+
+    foreach ($xactions as $xaction) {
+      $old = phutil_json_decode($xaction->getOldValue());
+      $new = phutil_json_decode($xaction->getNewValue());
+
+      $add = array_diff($new, $old);
+
+      if (!$add) {
+        continue;
+      }
+
+      $objects = id(new PhabricatorObjectQuery())
+        ->setViewer($editor->getActor())
+        ->withPHIDs($add)
+        ->execute();
+      $objects = mpull($objects, null, 'getPHID');
+
+      $invalid = array();
+      foreach ($add as $phid) {
+        if (empty($objects[$phid])) {
+          $invalid[] = $phid;
+        }
+      }
+
+      if ($invalid) {
+        $error = new PhabricatorApplicationTransactionValidationError(
+          $type,
+          pht('Invalid'),
+          pht(
+            'Some of the selected PHIDs in field "%s" are invalid or '.
+            'restricted: %s.',
+            $this->getFieldName(),
+            implode(', ', $invalid)),
+          $xaction);
+        $errors[] = $error;
+        $this->setFieldError(pht('Invalid'));
+      }
+    }
+
+    return $errors;
+  }
+
   public function shouldAppearInHerald() {
     return true;
   }

src/infrastructure/daemon/workers/management/PhabricatorWorkerManagementWorkflow.php

@@ -43,8 +43,10 @@ abstract class PhabricatorWorkerManagementWorkflow
     // fake it when doing manual CLI stuff. This makes sure CLI yields have
     // their expires times set properly.
     foreach ($tasks as $task) {
+      if ($task instanceof PhabricatorWorkerActiveTask) {
         $task->setServerTime(PhabricatorTime::getNow());
       }
+    }
 
     return $tasks;
   }