From 5c02113bf9eb31a9413e43e602f9dafb547ebe97 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Thu, 5 Dec 2013 14:06:22 +1100 Subject: [PATCH] Migrate "Run Command" to use Drydock hosts Summary: This migrates the "Run Remote Command" build step over to use Drydock hosts and Harbormaster artifacts. Test Plan: Created a build plan with a "Lease Host" step and a "Run Command" step. Configured the "Run Command" step to use the artifact from the "Lease Host" step. Saw the results: {F87377} {F87378} Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley CC: Korvin, epriestley, aran Maniphest Tasks: T1049, T4111 Differential Revision: https://secure.phabricator.com/D7707 --- src/__phutil_library_map__.php | 4 +- .../PhabricatorHarbormasterConfigOptions.php | 19 +--- .../HarbormasterBuildViewController.php | 31 ++++++- .../HarbormasterStepEditController.php | 18 ++++ .../query/HarbormasterBuildArtifactQuery.php | 7 +- .../step/BuildStepImplementation.php | 14 +-- ...php => CommandBuildStepImplementation.php} | 92 ++++++++----------- .../step/LeaseHostBuildStepImplementation.php | 15 ++- .../step/VariableBuildStepImplementation.php | 3 +- .../storage/build/HarbormasterBuild.php | 4 +- .../build/HarbormasterBuildArtifact.php | 48 ++++++++-- 11 files changed, 157 insertions(+), 98 deletions(-) rename src/applications/harbormaster/step/{RemoteCommandBuildStepImplementation.php => CommandBuildStepImplementation.php} (54%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7578262670..c16b54c9c2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -102,6 +102,7 @@ phutil_register_library_map(array( 'CelerityResourceTransformerTestCase' => 'infrastructure/celerity/__tests__/CelerityResourceTransformerTestCase.php', 'CeleritySpriteGenerator' => 'infrastructure/celerity/CeleritySpriteGenerator.php', 'CelerityStaticResourceResponse' => 'infrastructure/celerity/CelerityStaticResourceResponse.php', + 'CommandBuildStepImplementation' => 'applications/harbormaster/step/CommandBuildStepImplementation.php', 'ConduitAPIMethod' => 'applications/conduit/method/ConduitAPIMethod.php', 'ConduitAPIRequest' => 'applications/conduit/protocol/ConduitAPIRequest.php', 'ConduitAPIResponse' => 'applications/conduit/protocol/ConduitAPIResponse.php', @@ -2322,7 +2323,6 @@ phutil_register_library_map(array( 'ReleephStatusFieldSpecification' => 'applications/releeph/field/specification/ReleephStatusFieldSpecification.php', 'ReleephSummaryFieldSpecification' => 'applications/releeph/field/specification/ReleephSummaryFieldSpecification.php', 'ReleephUserView' => 'applications/releeph/view/user/ReleephUserView.php', - 'RemoteCommandBuildStepImplementation' => 'applications/harbormaster/step/RemoteCommandBuildStepImplementation.php', 'ShellLogView' => 'applications/harbormaster/view/ShellLogView.php', 'SleepBuildStepImplementation' => 'applications/harbormaster/step/SleepBuildStepImplementation.php', 'SlowvoteEmbedView' => 'applications/slowvote/view/SlowvoteEmbedView.php', @@ -2442,6 +2442,7 @@ phutil_register_library_map(array( 'CelerityResourceController' => 'PhabricatorController', 'CelerityResourceGraph' => 'AbstractDirectedGraph', 'CelerityResourceTransformerTestCase' => 'PhabricatorTestCase', + 'CommandBuildStepImplementation' => 'VariableBuildStepImplementation', 'ConduitAPIMethod' => array( 0 => 'Phobject', @@ -4950,7 +4951,6 @@ phutil_register_library_map(array( 'ReleephStatusFieldSpecification' => 'ReleephFieldSpecification', 'ReleephSummaryFieldSpecification' => 'ReleephFieldSpecification', 'ReleephUserView' => 'AphrontView', - 'RemoteCommandBuildStepImplementation' => 'VariableBuildStepImplementation', 'ShellLogView' => 'AphrontView', 'SleepBuildStepImplementation' => 'BuildStepImplementation', 'SlowvoteEmbedView' => 'AphrontView', diff --git a/src/applications/harbormaster/config/PhabricatorHarbormasterConfigOptions.php b/src/applications/harbormaster/config/PhabricatorHarbormasterConfigOptions.php index fdfa27152e..ee1729d6ad 100644 --- a/src/applications/harbormaster/config/PhabricatorHarbormasterConfigOptions.php +++ b/src/applications/harbormaster/config/PhabricatorHarbormasterConfigOptions.php @@ -12,24 +12,7 @@ final class PhabricatorHarbormasterConfigOptions } public function getOptions() { - return array( - $this->newOption( - 'harbormaster.temporary.hosts.whitelist', - 'list', - array()) - ->setSummary('Temporary configuration value.') - ->setLocked(true) - ->setDescription( - pht( - "This specifies a whitelist of remote hosts that the \"Run ". - "Remote Command\" may connect to. This is a temporary ". - "configuration option as Drydock is not yet available.". - "\n\n". - "**This configuration option will be removed in the future and ". - "your build configuration will no longer work when Drydock ". - "replaces this option. There is ABSOLUTELY NO SUPPORT for ". - "using this functionality!**")) - ); + return array(); } } diff --git a/src/applications/harbormaster/controller/HarbormasterBuildViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildViewController.php index 07d2b6ffbc..2a946dad72 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildViewController.php @@ -73,10 +73,10 @@ final class HarbormasterBuildViewController ->setHeader($header) ->addPropertyList($properties); + $targets[] = $this->buildArtifacts($build_target); $targets[] = $this->buildLog($build, $build_target); } - return $this->buildApplicationPage( array( $crumbs, @@ -89,6 +89,35 @@ final class HarbormasterBuildViewController )); } + private function buildArtifacts(HarbormasterBuildTarget $build_target) { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $artifacts = id(new HarbormasterBuildArtifactQuery()) + ->setViewer($viewer) + ->withBuildTargetPHIDs(array($build_target->getPHID())) + ->execute(); + + if (count($artifacts) === 0) { + return null; + } + + $list = new PHUIObjectItemListView(); + + foreach ($artifacts as $artifact) { + $list->addItem($artifact->getObjectItemView($viewer)); + } + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Build Artifacts')) + ->setUser($viewer); + + $box = id(new PHUIObjectBoxView()) + ->setHeader($header); + + return array($box, $list); + } + private function buildLog( HarbormasterBuild $build, HarbormasterBuildTarget $build_target) { diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php index db98d7e858..476ae5d54f 100644 --- a/src/applications/harbormaster/controller/HarbormasterStepEditController.php +++ b/src/applications/harbormaster/controller/HarbormasterStepEditController.php @@ -91,6 +91,23 @@ final class HarbormasterStepEditController ->setName($name) ->setValue($value); break; + case BuildStepImplementation::SETTING_TYPE_ARTIFACT: + $filter = $opt['artifact_type']; + $available_artifacts = + BuildStepImplementation::getAvailableArtifacts( + $plan, + $step, + $filter); + $options = array(); + foreach ($available_artifacts as $key => $type) { + $options[$key] = $key; + } + $control = id(new AphrontFormSelectControl()) + ->setLabel($this->getReadableName($name, $opt)) + ->setName($name) + ->setValue($value) + ->setOptions($options); + break; default: throw new Exception("Unable to render field with unknown type."); } @@ -145,6 +162,7 @@ final class HarbormasterStepEditController public function getValueFromRequest(AphrontRequest $request, $name, $type) { switch ($type) { case BuildStepImplementation::SETTING_TYPE_STRING: + case BuildStepImplementation::SETTING_TYPE_ARTIFACT: return $request->getStr($name); break; case BuildStepImplementation::SETTING_TYPE_INTEGER: diff --git a/src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php b/src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php index e3decf26ae..44a5e7dcda 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildArtifactQuery.php @@ -7,6 +7,7 @@ final class HarbormasterBuildArtifactQuery private $buildTargetPHIDs; private $artifactTypes; private $artifactKeys; + private $keyBuildPHID; public function withIDs(array $ids) { $this->ids = $ids; @@ -23,7 +24,8 @@ final class HarbormasterBuildArtifactQuery return $this; } - public function withArtifactKeys(array $artifact_keys) { + public function withArtifactKeys($build_phid, array $artifact_keys) { + $this->keyBuildPHID = $build_phid; $this->artifactKeys = $artifact_keys; return $this; } @@ -95,7 +97,8 @@ final class HarbormasterBuildArtifactQuery if ($this->artifactKeys) { $indexes = array(); foreach ($this->artifactKeys as $key) { - $indexes[] = PhabricatorHash::digestForIndex($key); + $indexes[] = PhabricatorHash::digestForIndex( + $this->keyBuildPHID.$key); } $where[] = qsprintf( diff --git a/src/applications/harbormaster/step/BuildStepImplementation.php b/src/applications/harbormaster/step/BuildStepImplementation.php index 26f10cd6ae..3c3346cbe8 100644 --- a/src/applications/harbormaster/step/BuildStepImplementation.php +++ b/src/applications/harbormaster/step/BuildStepImplementation.php @@ -7,6 +7,7 @@ abstract class BuildStepImplementation { const SETTING_TYPE_STRING = 'string'; const SETTING_TYPE_INTEGER = 'integer'; const SETTING_TYPE_BOOLEAN = 'boolean'; + const SETTING_TYPE_ARTIFACT = 'artifact'; public static function getImplementations() { $symbols = id(new PhutilSymbolLoader()) @@ -117,23 +118,18 @@ abstract class BuildStepImplementation { * Returns a list of all artifacts made available by previous build steps. */ public static function getAvailableArtifacts( + HarbormasterBuildPlan $build_plan, HarbormasterBuildStep $current_build_step, $artifact_type) { - $build_plan_phid = $current_build_step->getBuildPlanPHID(); - - $build_plan = id(new HarbormasterBuildPlanQuery()) - ->withPHIDs(array($build_plan_phid)) - ->executeOne(); - $build_steps = $build_plan->loadOrderedBuildSteps(); $previous_implementations = array(); - for ($i = 0; $i < count($build_steps); $i++) { - if ($build_steps[$i]->getPHID() === $current_build_step->getPHID()) { + foreach ($build_steps as $build_step) { + if ($build_step->getPHID() === $current_build_step->getPHID()) { break; } - $previous_implementations[$i] = $build_steps[$i]->getStepImplementation(); + $previous_implementations[] = $build_step->getStepImplementation(); } $artifact_arrays = mpull($previous_implementations, 'getArtifactMappings'); diff --git a/src/applications/harbormaster/step/RemoteCommandBuildStepImplementation.php b/src/applications/harbormaster/step/CommandBuildStepImplementation.php similarity index 54% rename from src/applications/harbormaster/step/RemoteCommandBuildStepImplementation.php rename to src/applications/harbormaster/step/CommandBuildStepImplementation.php index 3829cea406..0366944d1b 100644 --- a/src/applications/harbormaster/step/RemoteCommandBuildStepImplementation.php +++ b/src/applications/harbormaster/step/CommandBuildStepImplementation.php @@ -1,14 +1,14 @@ setViewer(PhabricatorUser::getOmnipotentUser()) + ->withArtifactKeys( + $build->getPHID(), + array($settings['hostartifact'])) + ->executeOne(); + if ($artifact === null) { + throw new Exception("Associated Drydock host artifact not found!"); } + $data = $artifact->getArtifactData(); + + // FIXME: Is there a better way of doing this? + $lease = id(new DrydockLease())->load( + $data['drydock-lease']); + if ($lease === null) { + throw new Exception("Associated Drydock lease not found!"); + } + $resource = id(new DrydockResource())->load( + $lease->getResourceID()); + if ($resource === null) { + throw new Exception("Associated Drydock resource not found!"); + } + $lease->attachResource($resource); + + $interface = $lease->getInterface('command'); + + $future = $interface->getExecFuture('%C', $command); + $log_stdout = $build->createLog($build_target, "remote", "stdout"); $log_stderr = $build->createLog($build_target, "remote", "stderr"); @@ -98,25 +111,12 @@ final class RemoteCommandBuildStepImplementation if ($settings['command'] === null || !is_string($settings['command'])) { return false; } - if ($settings['sshhost'] === null || !is_string($settings['sshhost'])) { - return false; - } - if ($settings['sshuser'] === null || !is_string($settings['sshuser'])) { - return false; - } - if ($settings['sshkey'] === null || !is_string($settings['sshkey'])) { - return false; - } - if ($settings['sshport'] === null || !is_int($settings['sshport']) || - $settings['sshport'] <= 0 || $settings['sshport'] >= 65536) { + if ($settings['hostartifact'] === null || + !is_string($settings['hostartifact'])) { return false; } - $whitelist = PhabricatorEnv::getEnvConfig( - 'harbormaster.temporary.hosts.whitelist'); - if (!in_array($settings['sshhost'], $whitelist)) { - return false; - } + // TODO: Check if the host artifact is provided by previous build steps. return true; } @@ -127,25 +127,13 @@ final class RemoteCommandBuildStepImplementation 'name' => 'Command', 'description' => 'The command to execute on the remote machine.', 'type' => BuildStepImplementation::SETTING_TYPE_STRING), - 'sshhost' => array( - 'name' => 'SSH Host', - 'description' => 'The SSH host that the command will be run on.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING), - 'sshport' => array( - 'name' => 'SSH Port', - 'description' => 'The SSH port to connect to.', - 'type' => BuildStepImplementation::SETTING_TYPE_INTEGER, - 'default' => 22), // TODO: 'default' doesn't do anything yet.. - 'sshuser' => array( - 'name' => 'SSH Username', - 'description' => 'The SSH username to use.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING), - 'sshkey' => array( - 'name' => 'SSH Identity File', + 'hostartifact' => array( + 'name' => 'Host Artifact', 'description' => - 'The path to the SSH identity file (private key) '. - 'on the local web server.', - 'type' => BuildStepImplementation::SETTING_TYPE_STRING)); + 'The host artifact that determines what machine the command '. + 'will run on.', + 'type' => BuildStepImplementation::SETTING_TYPE_ARTIFACT, + 'artifact_type' => HarbormasterBuildArtifact::TYPE_HOST)); } } diff --git a/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php b/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php index 5f34d66809..925fcf7612 100644 --- a/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php +++ b/src/applications/harbormaster/step/LeaseHostBuildStepImplementation.php @@ -15,8 +15,10 @@ final class LeaseHostBuildStepImplementation $settings = $this->getSettings(); return pht( - 'Obtain a lease on a Drydock host whose platform is \'%s\'.', - $settings['platform']); + 'Obtain a lease on a Drydock host whose platform is \'%s\' and store '. + 'the resulting lease in a host artifact called \'%s\'.', + $settings['platform'], + $settings['name']); } public function execute( @@ -41,12 +43,19 @@ final class LeaseHostBuildStepImplementation $artifact = $build->createArtifact( $build_target, $settings['name'], - 'host'); + HarbormasterBuildArtifact::TYPE_HOST); $artifact->setArtifactData(array( 'drydock-lease' => $lease->getID())); $artifact->save(); } + public function getArtifactMappings() { + $settings = $this->getSettings(); + + return array( + $settings['name'] => 'host'); + } + public function validateSettings() { $settings = $this->getSettings(); diff --git a/src/applications/harbormaster/step/VariableBuildStepImplementation.php b/src/applications/harbormaster/step/VariableBuildStepImplementation.php index b41471c231..fd114ada29 100644 --- a/src/applications/harbormaster/step/VariableBuildStepImplementation.php +++ b/src/applications/harbormaster/step/VariableBuildStepImplementation.php @@ -37,10 +37,11 @@ abstract class VariableBuildStepImplementation extends BuildStepImplementation { } public function getSettingRemarkupInstructions() { + $variables = HarbormasterBuild::getAvailableBuildVariables(); $text = ''; $text .= pht('The following variables are available: ')."\n"; $text .= "\n"; - foreach ($this->getAvailableVariables() as $name => $desc) { + foreach ($variables as $name => $desc) { $text .= ' - `'.$name.'`: '.$desc."\n"; } $text .= "\n"; diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index c626447b1e..529464b297 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -113,7 +113,7 @@ final class HarbormasterBuild extends HarbormasterDAO $artifact = HarbormasterBuildArtifact::initializeNewBuildArtifact($build_target); - $artifact->setArtifactKey($artifact_key); + $artifact->setArtifactKey($this->getPHID(), $artifact_key); $artifact->setArtifactType($artifact_type); $artifact->save(); return $artifact; @@ -172,7 +172,7 @@ final class HarbormasterBuild extends HarbormasterDAO return $results; } - public function getAvailableBuildVariables() { + public static function getAvailableBuildVariables() { return array( 'buildable.diff' => pht('The differential diff ID, if applicable.'), diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php index 88fc7bc18a..4db4a00b51 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php @@ -9,6 +9,11 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO protected $artifactKey; protected $artifactData = array(); + private $buildTarget = self::ATTACHABLE; + + const TYPE_FILE = 'file'; + const TYPE_HOST = 'host'; + public static function initializeNewBuildArtifact( HarbormasterBuildTarget $build_target) { return id(new HarbormasterBuildArtifact()) @@ -23,22 +28,49 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO ) + parent::getConfiguration(); } - public function attachBuildable(HarbormasterBuildable $buildable) { - $this->buildable = $buildable; + public function attachBuildTarget(HarbormasterBuildTarget $build_target) { + $this->buildTarget = $build_target; return $this; } - public function getBuildable() { - return $this->assertAttached($this->buildable); + public function getBuildTarget() { + return $this->assertAttached($this->buildTarget); } - public function setArtifactKey($key) { + public function setArtifactKey($build_phid, $key) { $this->artifactIndex = - PhabricatorHash::digestForIndex($this->buildTargetPHID.$key); + PhabricatorHash::digestForIndex($build_phid.$key); $this->artifactKey = $key; return $this; } + public function getObjectItemView(PhabricatorUser $viewer) { + $data = $this->getArtifactData(); + switch ($this->getArtifactType()) { + case self::TYPE_FILE: + $handle = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs($data) + ->executeOne(); + + return id(new PHUIObjectItemView()) + ->setObjectName(pht('File')) + ->setHeader($handle->getFullName()) + ->setHref($handle->getURI()); + case self::TYPE_HOST: + $leases = id(new DrydockLeaseQuery()) + ->withIDs(array($data["drydock-lease"])) + ->execute(); + $lease = $leases[$data["drydock-lease"]]; + + return id(new PHUIObjectItemView()) + ->setObjectName(pht('Drydock Lease')) + ->setHeader($lease->getID()) + ->setHref('/drydock/lease/'.$lease->getID()); + default: + return null; + } + } /* -( PhabricatorPolicyInterface )----------------------------------------- */ @@ -50,11 +82,11 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO } public function getPolicy($capability) { - return $this->getBuildable()->getPolicy($capability); + return $this->getBuildTarget()->getPolicy($capability); } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { - return $this->getBuildable()->hasAutomaticCapability( + return $this->getBuildTarget()->hasAutomaticCapability( $capability, $viewer); }