Make all build steps support variables

Summary: Ref T1049. This generally simplifies things. The steps which don't support variables generally don't make sense to support varaibles anyway.

Test Plan: Edited some steps.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T1049

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

src/__phutil_library_map__.php

@@ -2537,7 +2537,6 @@ phutil_register_library_map(array(
     'SubscriptionListDialogBuilder' => 'applications/subscriptions/view/SubscriptionListDialogBuilder.php',
     'SubscriptionListStringBuilder' => 'applications/subscriptions/view/SubscriptionListStringBuilder.php',
     'UploadArtifactBuildStepImplementation' => 'applications/harbormaster/step/UploadArtifactBuildStepImplementation.php',
-    'VariableBuildStepImplementation' => 'applications/harbormaster/step/VariableBuildStepImplementation.php',
     'WaitForPreviousBuildStepImplementation' => 'applications/harbormaster/step/WaitForPreviousBuildStepImplementation.php',
   ),
   'function' =>
@@ -2659,7 +2658,7 @@ phutil_register_library_map(array(
     'CelerityResourceGraph' => 'AbstractDirectedGraph',
     'CelerityResourceTransformerTestCase' => 'PhabricatorTestCase',
     'CelerityResourcesOnDisk' => 'CelerityPhysicalResources',
-    'CommandBuildStepImplementation' => 'VariableBuildStepImplementation',
+    'CommandBuildStepImplementation' => 'BuildStepImplementation',
     'ConduitAPIMethod' =>
     array(
       0 => 'Phobject',
@@ -3339,7 +3338,7 @@ phutil_register_library_map(array(
     'HarbormasterCapabilityManagePlans' => 'PhabricatorPolicyCapability',
     'HarbormasterController' => 'PhabricatorController',
     'HarbormasterDAO' => 'PhabricatorLiskDAO',
-    'HarbormasterHTTPRequestBuildStepImplementation' => 'VariableBuildStepImplementation',
+    'HarbormasterHTTPRequestBuildStepImplementation' => 'BuildStepImplementation',
     'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow',
     'HarbormasterManagementWorkflow' => 'PhabricatorManagementWorkflow',
     'HarbormasterObject' => 'HarbormasterDAO',
@@ -5384,7 +5383,7 @@ phutil_register_library_map(array(
     'PonderVoteSaveController' => 'PonderController',
     'ProjectCapabilityCreateProjects' => 'PhabricatorPolicyCapability',
     'ProjectRemarkupRule' => 'PhabricatorRemarkupRuleObject',
-    'PublishFragmentBuildStepImplementation' => 'VariableBuildStepImplementation',
+    'PublishFragmentBuildStepImplementation' => 'BuildStepImplementation',
     'QueryFormattingTestCase' => 'PhabricatorTestCase',
     'ReleephAuthorFieldSpecification' => 'ReleephFieldSpecification',
     'ReleephBranch' =>
@@ -5491,8 +5490,7 @@ phutil_register_library_map(array(
     'SleepBuildStepImplementation' => 'BuildStepImplementation',
     'SlowvoteEmbedView' => 'AphrontView',
     'SlowvoteRemarkupRule' => 'PhabricatorRemarkupRuleObject',
-    'UploadArtifactBuildStepImplementation' => 'VariableBuildStepImplementation',
-    'VariableBuildStepImplementation' => 'BuildStepImplementation',
+    'UploadArtifactBuildStepImplementation' => 'BuildStepImplementation',
     'WaitForPreviousBuildStepImplementation' => 'BuildStepImplementation',
   ),
 ));

src/applications/harbormaster/controller/HarbormasterStepEditController.php

@@ -24,13 +24,7 @@ final class HarbormasterStepEditController
       return new Aphront404Response();
     }
 
-    $plan = id(new HarbormasterBuildPlanQuery())
-      ->setViewer($viewer)
-      ->withPHIDs(array($step->getBuildPlanPHID()))
-      ->executeOne();
-    if (!$plan) {
-      return new Aphront404Response();
-    }
+    $plan = $step->getBuildPlan();
 
     $implementation = $step->getStepImplementation();
     $implementation->validateSettingDefinitions();
@@ -52,9 +46,8 @@ final class HarbormasterStepEditController
         }
       }
 
-      if (count($errors) === 0) {
+      if (!$errors) {
         $step->save();
-
         return id(new AphrontRedirectResponse())
           ->setURI($this->getApplicationURI('plan/'.$plan->getID().'/'));
       }
@@ -63,11 +56,6 @@ final class HarbormasterStepEditController
     $form = id(new AphrontFormView())
       ->setUser($viewer);
 
-    $instructions = $implementation->getSettingRemarkupInstructions();
-    if ($instructions !== null) {
-      $form->appendRemarkupInstructions($instructions);
-    }
-
     // We need to render out all of the fields for the settings that
     // the implementation has.
     foreach ($implementation->getSettingDefinitions() as $name => $opt) {
@@ -121,7 +109,7 @@ final class HarbormasterStepEditController
 
     $form->appendChild(
       id(new AphrontFormSubmitControl())
-        ->setValue(pht('Save Step Configuration'))
+        ->setValue(pht('Save Build Step'))
         ->addCancelButton(
           $this->getApplicationURI('plan/'.$plan->getID().'/')));
 
@@ -137,10 +125,13 @@ final class HarbormasterStepEditController
       $this->getApplicationURI("plan/{$id}/"));
     $crumbs->addTextCrumb(pht('Edit Step'));
 
+    $variables = $this->renderBuildVariablesTable();
+
     return $this->buildApplicationPage(
       array(
         $crumbs,
         $box,
+        $variables,
       ),
       array(
         'title' => $implementation->getName(),
@@ -173,4 +164,31 @@ final class HarbormasterStepEditController
     }
   }
 
+  private function renderBuildVariablesTable() {
+    $viewer = $this->getRequest()->getUser();
+
+    $variables = HarbormasterBuild::getAvailableBuildVariables();
+    ksort($variables);
+
+    $rows = array();
+    $rows[] = pht(
+      'The following variables can be used in most fields. To reference '.
+      'a variable, use `${name}` in a field.');
+    $rows[] = pht('| Variable | Description |');
+    $rows[] = '|---|---|';
+    foreach ($variables as $name => $description) {
+      $rows[] = '| `'.$name.'` | '.$description.' |';
+    }
+    $rows = implode("\n", $rows);
+
+    $form = id(new AphrontFormView())
+      ->setUser($viewer)
+      ->appendRemarkupInstructions($rows);
+
+    return id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('Build Variables'))
+      ->appendChild($form);
+  }
+
+
 }

src/applications/harbormaster/step/BuildStepImplementation.php

@@ -90,13 +90,6 @@ abstract class BuildStepImplementation {
     return array();
   }
 
-  /**
-   * Return relevant setting instructions as Remarkup.
-   */
-  public function getSettingRemarkupInstructions() {
-    return null;
-  }
-
   /**
    * Return the name of artifacts produced by this command.
    *
@@ -160,4 +153,39 @@ abstract class BuildStepImplementation {
     }
     return $artifacts;
   }
+
+  /**
+   * Convert a user-provided string with variables in it, like:
+   *
+   *   ls ${dirname}
+   *
+   * ...into a string with variables merged into it safely:
+   *
+   *   ls 'dir with spaces'
+   *
+   * @param string Name of a `vxsprintf` function, like @{function:vcsprintf}.
+   * @param string User-provided pattern string containing `${variables}`.
+   * @param dict   List of available replacement variables.
+   * @return string String with variables replaced safely into it.
+   */
+  protected function mergeVariables($function, $pattern, array $variables) {
+    $regexp = '/\\$\\{(?P<name>[a-z\\.]+)\\}/';
+
+    $matches = null;
+    preg_match_all($regexp, $pattern, $matches);
+
+    $argv = array();
+    foreach ($matches['name'] as $name) {
+      if (!array_key_exists($name, $variables)) {
+        throw new Exception(pht("No such variable '%s'!", $name));
+      }
+      $argv[] = $variables[$name];
+    }
+
+    $pattern = str_replace('%', '%%', $pattern);
+    $pattern = preg_replace($regexp, '%s', $pattern);
+
+    return call_user_func($function, $pattern, $argv);
+  }
+
 }

src/applications/harbormaster/step/CommandBuildStepImplementation.php

@@ -1,7 +1,7 @@
 <?php
 
 final class CommandBuildStepImplementation
-  extends VariableBuildStepImplementation {
+  extends BuildStepImplementation {
 
   public function getName() {
     return pht('Run Command');

src/applications/harbormaster/step/HarbormasterHTTPRequestBuildStepImplementation.php

@@ -1,7 +1,7 @@
 <?php
 
 final class HarbormasterHTTPRequestBuildStepImplementation
-  extends VariableBuildStepImplementation {
+  extends BuildStepImplementation {
 
   public function getName() {
     return pht('Make HTTP Request');

src/applications/harbormaster/step/PublishFragmentBuildStepImplementation.php

@@ -1,7 +1,7 @@
 <?php
 
 final class PublishFragmentBuildStepImplementation
-  extends VariableBuildStepImplementation {
+  extends BuildStepImplementation {
 
   public function getName() {
     return pht('Publish Fragment');

src/applications/harbormaster/step/UploadArtifactBuildStepImplementation.php

@@ -1,7 +1,7 @@
 <?php
 
 final class UploadArtifactBuildStepImplementation
-  extends VariableBuildStepImplementation {
+  extends BuildStepImplementation {
 
   public function getName() {
     return pht('Upload Artifact');

src/applications/harbormaster/step/VariableBuildStepImplementation.php

@@ -1,52 +0,0 @@
-<?php
-
-abstract class VariableBuildStepImplementation extends BuildStepImplementation {
-
-  /**
-   * Convert a user-provided string with variables in it, like:
-   *
-   *   ls ${dirname}
-   *
-   * ...into a string with variables merged into it safely:
-   *
-   *   ls 'dir with spaces'
-   *
-   * @param string Name of a `vxsprintf` function, like @{function:vcsprintf}.
-   * @param string User-provided pattern string containing `${variables}`.
-   * @param dict   List of available replacement variables.
-   * @return string String with variables replaced safely into it.
-   */
-  protected function mergeVariables($function, $pattern, array $variables) {
-    $regexp = '/\\$\\{(?P<name>[a-z\\.]+)\\}/';
-
-    $matches = null;
-    preg_match_all($regexp, $pattern, $matches);
-
-    $argv = array();
-    foreach ($matches['name'] as $name) {
-      if (!array_key_exists($name, $variables)) {
-        throw new Exception(pht("No such variable '%s'!", $name));
-      }
-      $argv[] = $variables[$name];
-    }
-
-    $pattern = str_replace('%', '%%', $pattern);
-    $pattern = preg_replace($regexp, '%s', $pattern);
-
-    return call_user_func($function, $pattern, $argv);
-  }
-
-  public function getSettingRemarkupInstructions() {
-    $variables = HarbormasterBuild::getAvailableBuildVariables();
-    $text = '';
-    $text .= pht('The following variables are available: ')."\n";
-    $text .= "\n";
-    foreach ($variables as $name => $desc) {
-      $text .= '  - `'.$name.'`: '.$desc."\n";
-    }
-    $text .= "\n";
-    $text .= "Use `\${name}` to merge a variable into a setting.";
-    return $text;
-  }
-
-}