From 71e6386a73b66591bd6a64433fe427dd87c0c79b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Nov 2013 09:58:58 -0800 Subject: [PATCH 1/9] Fix issue with new branch query data getting used incorrectly See --- .../repository/daemon/PhabricatorRepositoryPullLocalDaemon.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php index 50ca2fb064..b7e9e0c9f4 100644 --- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -750,8 +750,7 @@ final class PhabricatorRepositoryPullLocalDaemon $branches = mpull($branches, 'getHeadCommitIdentifier', 'getName'); $got_something = false; - foreach ($branches as $name => $branch) { - $commit = $branch['rev']; + foreach ($branches as $name => $commit) { if ($this->isKnownCommit($repository, $commit)) { continue; } else { From 5cc26f065d3f529e7092b0e49ccd51adbfade12d Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Nov 2013 10:01:44 -0800 Subject: [PATCH 2/9] Expand "local working copy" conditional check in Diffusion Summary: We don't care about any disk resources at all for non-hosted SVN repositories. See . Test Plan: Looked at a non-hosted SVN repo, saw no storage info. Reviewers: btrahan, chad Reviewed By: chad CC: aran Differential Revision: https://secure.phabricator.com/D7506 --- .../DiffusionRepositoryEditMainController.php | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php index 770ee87bde..5ba4423481 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php @@ -726,27 +726,27 @@ final class DiffusionRepositoryEditMainController ->setNote($daemon_instructions)); } - $local_parent = dirname($repository->getLocalPath()); - if (Filesystem::pathExists($local_parent)) { - $view->addItem( - id(new PHUIStatusItemView()) - ->setIcon('accept-green') - ->setTarget(pht('Storage Directory OK')) - ->setNote(phutil_tag('tt', array(), $local_parent))); - } else { - $view->addItem( - id(new PHUIStatusItemView()) - ->setIcon('warning-red') - ->setTarget(pht('No Storage Directory')) - ->setNote( - pht( - 'Storage directory %s does not exist, or is not readable by '. - 'the webserver. Create this directory or make it readable.', - phutil_tag('tt', array(), $local_parent)))); - return $view; - } - if ($repository->usesLocalWorkingCopy()) { + $local_parent = dirname($repository->getLocalPath()); + if (Filesystem::pathExists($local_parent)) { + $view->addItem( + id(new PHUIStatusItemView()) + ->setIcon('accept-green') + ->setTarget(pht('Storage Directory OK')) + ->setNote(phutil_tag('tt', array(), $local_parent))); + } else { + $view->addItem( + id(new PHUIStatusItemView()) + ->setIcon('warning-red') + ->setTarget(pht('No Storage Directory')) + ->setNote( + pht( + 'Storage directory %s does not exist, or is not readable by '. + 'the webserver. Create this directory or make it readable.', + phutil_tag('tt', array(), $local_parent)))); + return $view; + } + $local_path = $repository->getLocalPath(); $message = idx($messages, PhabricatorRepositoryStatusMessage::TYPE_INIT); if ($message) { From ca5400d14bcba4ed067fb12354b64364f224283a Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 5 Nov 2013 12:43:47 -0800 Subject: [PATCH 3/9] Implement basic Harbormaster daemon and start builds. Summary: This implements a basic Harbormaster daemon that takes pending builds and builds them (currently just sleeps 15 seconds before moving to passed state). It also implements an interface to apply a build plan to a buildable, so that users can kick off builds for a buildable. Test Plan: Ran `bin/phd debug PhabricatorHarbormasterBuildDaemon` and used the interface to start some builds by applying a build plan. Observed them move from 'pending' to 'building' to 'passed'. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley CC: Korvin, epriestley, aran Maniphest Tasks: T1049 Differential Revision: https://secure.phabricator.com/D7498 --- src/__phutil_library_map__.php | 8 +- .../PhabricatorApplicationHarbormaster.php | 2 +- .../HarbormasterBuildableApplyController.php | 81 +++++++++++++++++ .../HarbormasterBuildableListController.php | 2 +- .../HarbormasterBuildableViewController.php | 35 ++++++++ .../HarbormasterPlanExecuteController.php | 88 ------------------- .../HarbormasterPlanViewController.php | 8 -- .../query/HarbormasterBuildQuery.php | 13 +++ .../storage/HarbormasterBuildable.php | 6 +- .../storage/build/HarbormasterBuild.php | 32 ++++++- .../worker/HarbormasterBuildWorker.php | 51 +++++++++++ .../worker/HarbormasterRunnerWorker.php | 53 ----------- 12 files changed, 221 insertions(+), 158 deletions(-) create mode 100644 src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php delete mode 100644 src/applications/harbormaster/controller/HarbormasterPlanExecuteController.php create mode 100644 src/applications/harbormaster/worker/HarbormasterBuildWorker.php delete mode 100644 src/applications/harbormaster/worker/HarbormasterRunnerWorker.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9f8ad08066..8882859909 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -656,7 +656,9 @@ phutil_register_library_map(array( 'HarbormasterBuildStepQuery' => 'applications/harbormaster/query/HarbormasterBuildStepQuery.php', 'HarbormasterBuildTarget' => 'applications/harbormaster/storage/build/HarbormasterBuildTarget.php', 'HarbormasterBuildTargetQuery' => 'applications/harbormaster/query/HarbormasterBuildTargetQuery.php', + 'HarbormasterBuildWorker' => 'applications/harbormaster/worker/HarbormasterBuildWorker.php', 'HarbormasterBuildable' => 'applications/harbormaster/storage/HarbormasterBuildable.php', + 'HarbormasterBuildableApplyController' => 'applications/harbormaster/controller/HarbormasterBuildableApplyController.php', 'HarbormasterBuildableArtifactQuery' => 'applications/harbormaster/query/HarbormasterBuildableArtifactQuery.php', 'HarbormasterBuildableEditController' => 'applications/harbormaster/controller/HarbormasterBuildableEditController.php', 'HarbormasterBuildableListController' => 'applications/harbormaster/controller/HarbormasterBuildableListController.php', @@ -675,11 +677,9 @@ phutil_register_library_map(array( 'HarbormasterPHIDTypeBuildable' => 'applications/harbormaster/phid/HarbormasterPHIDTypeBuildable.php', 'HarbormasterPlanController' => 'applications/harbormaster/controller/HarbormasterPlanController.php', 'HarbormasterPlanEditController' => 'applications/harbormaster/controller/HarbormasterPlanEditController.php', - 'HarbormasterPlanExecuteController' => 'applications/harbormaster/controller/HarbormasterPlanExecuteController.php', 'HarbormasterPlanListController' => 'applications/harbormaster/controller/HarbormasterPlanListController.php', 'HarbormasterPlanViewController' => 'applications/harbormaster/controller/HarbormasterPlanViewController.php', 'HarbormasterRemarkupRule' => 'applications/harbormaster/remarkup/HarbormasterRemarkupRule.php', - 'HarbormasterRunnerWorker' => 'applications/harbormaster/worker/HarbormasterRunnerWorker.php', 'HarbormasterScratchTable' => 'applications/harbormaster/storage/HarbormasterScratchTable.php', 'HeraldAction' => 'applications/herald/storage/HeraldAction.php', 'HeraldAdapter' => 'applications/herald/adapter/HeraldAdapter.php', @@ -2868,11 +2868,13 @@ phutil_register_library_map(array( 'HarbormasterBuildStepQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HarbormasterBuildTarget' => 'HarbormasterDAO', 'HarbormasterBuildTargetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildWorker' => 'PhabricatorWorker', 'HarbormasterBuildable' => array( 0 => 'HarbormasterDAO', 1 => 'PhabricatorPolicyInterface', ), + 'HarbormasterBuildableApplyController' => 'HarbormasterController', 'HarbormasterBuildableArtifactQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HarbormasterBuildableEditController' => 'HarbormasterController', 'HarbormasterBuildableListController' => @@ -2895,7 +2897,6 @@ phutil_register_library_map(array( 'HarbormasterPHIDTypeBuildable' => 'PhabricatorPHIDType', 'HarbormasterPlanController' => 'PhabricatorController', 'HarbormasterPlanEditController' => 'HarbormasterPlanController', - 'HarbormasterPlanExecuteController' => 'HarbormasterPlanController', 'HarbormasterPlanListController' => array( 0 => 'HarbormasterPlanController', @@ -2903,7 +2904,6 @@ phutil_register_library_map(array( ), 'HarbormasterPlanViewController' => 'HarbormasterPlanController', 'HarbormasterRemarkupRule' => 'PhabricatorRemarkupRuleObject', - 'HarbormasterRunnerWorker' => 'PhabricatorWorker', 'HarbormasterScratchTable' => 'HarbormasterDAO', 'HeraldAction' => 'HeraldDAO', 'HeraldApplyTranscript' => 'HeraldDAO', diff --git a/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php b/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php index 4e41dc7a02..6bcb87d7b1 100644 --- a/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php +++ b/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php @@ -44,13 +44,13 @@ final class PhabricatorApplicationHarbormaster extends PhabricatorApplication { => 'HarbormasterBuildableListController', 'buildable/' => array( 'edit/(?:(?P\d+)/)?' => 'HarbormasterBuildableEditController', + 'apply/(?:(?P\d+)/)?' => 'HarbormasterBuildableApplyController', ), 'plan/' => array( '(?:query/(?P[^/]+)/)?' => 'HarbormasterPlanListController', 'edit/(?:(?P\d+)/)?' => 'HarbormasterPlanEditController', '(?P\d+)/' => 'HarbormasterPlanViewController', - 'execute/(?P\d+)/' => 'HarbormasterPlanExecuteController', ), ), ); diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php b/src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php new file mode 100644 index 0000000000..215e117953 --- /dev/null +++ b/src/applications/harbormaster/controller/HarbormasterBuildableApplyController.php @@ -0,0 +1,81 @@ +id = $data['id']; + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $id = $this->id; + + $buildable = id(new HarbormasterBuildableQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if ($buildable === null) { + throw new Exception("Buildable not found!"); + } + + $buildable_uri = '/B'.$buildable->getID(); + + if ($request->isDialogFormPost()) { + $plan = id(new HarbormasterBuildPlanQuery()) + ->setViewer($viewer) + ->withIDs(array($request->getInt('build-plan'))) + ->executeOne(); + + $build = HarbormasterBuild::initializeNewBuild($viewer); + $build->setBuildablePHID($buildable->getPHID()); + $build->setBuildPlanPHID($plan->getPHID()); + $build->setBuildStatus(HarbormasterBuild::STATUS_PENDING); + $build->save(); + + PhabricatorWorker::scheduleTask( + 'HarbormasterBuildWorker', + array( + 'buildID' => $build->getID() + )); + + return id(new AphrontRedirectResponse())->setURI($buildable_uri); + } + + $plans = id(new HarbormasterBuildPlanQuery()) + ->setViewer($viewer) + ->execute(); + + $options = array(); + foreach ($plans as $plan) { + $options[$plan->getID()] = $plan->getName(); + } + + // FIXME: I'd really like to use the dialog that "Edit Differential + // Revisions" uses, but that code is quite hard-coded for the particular + // uses, so for now we just give a single dropdown. + + $dialog = new AphrontDialogView(); + $dialog->setTitle(pht('Apply which plan?')) + ->setUser($viewer) + ->addSubmitButton(pht('Apply')) + ->addCancelButton($buildable_uri); + $dialog->appendChild( + phutil_tag( + 'p', + array(), + pht( + 'Select what build plan you want to apply to this buildable:'))); + $dialog->appendChild( + id(new AphrontFormSelectControl()) + ->setUser($viewer) + ->setName('build-plan') + ->setOptions($options)); + return id(new AphrontDialogResponse())->setDialog($dialog); + } + +} diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableListController.php b/src/applications/harbormaster/controller/HarbormasterBuildableListController.php index 4260410474..9bf9fceef0 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableListController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableListController.php @@ -36,7 +36,7 @@ final class HarbormasterBuildableListController $id = $buildable->getID(); $item = id(new PHUIObjectItemView()) - ->setHeader(pht('Build %d', $buildable->getID())); + ->setHeader(pht('Buildable %d', $buildable->getID())); if ($id) { $item->setHref("/B{$id}"); diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php index 7ee92981c9..afe43b1423 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php @@ -37,6 +37,32 @@ final class HarbormasterBuildableViewController $item = id(new PHUIObjectItemView()) ->setObjectName(pht('Build %d', $build->getID())) ->setHeader($build->getName()); + switch ($build->getBuildStatus()) { + case HarbormasterBuild::STATUS_INACTIVE: + $item->setBarColor('grey'); + $item->addAttribute(pht('Inactive')); + break; + case HarbormasterBuild::STATUS_PENDING: + $item->setBarColor('blue'); + $item->addAttribute(pht('Pending')); + break; + case HarbormasterBuild::STATUS_WAITING: + $item->setBarColor('blue'); + $item->addAttribute(pht('Waiting on Resource')); + break; + case HarbormasterBuild::STATUS_BUILDING: + $item->setBarColor('yellow'); + $item->addAttribute(pht('Building')); + break; + case HarbormasterBuild::STATUS_PASSED: + $item->setBarColor('green'); + $item->addAttribute(pht('Passed')); + break; + case HarbormasterBuild::STATUS_FAILED: + $item->setBarColor('red'); + $item->addAttribute(pht('Failed')); + break; + } $build_list->addItem($item); } @@ -80,6 +106,15 @@ final class HarbormasterBuildableViewController ->setObject($buildable) ->setObjectURI("/B{$id}"); + $apply_uri = $this->getApplicationURI('/buildable/apply/'.$id.'/'); + + $list->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Apply Build Plan')) + ->setIcon('edit') + ->setHref($apply_uri) + ->setWorkflow(true)); + return $list; } diff --git a/src/applications/harbormaster/controller/HarbormasterPlanExecuteController.php b/src/applications/harbormaster/controller/HarbormasterPlanExecuteController.php deleted file mode 100644 index c720e0b797..0000000000 --- a/src/applications/harbormaster/controller/HarbormasterPlanExecuteController.php +++ /dev/null @@ -1,88 +0,0 @@ -id = $data['id']; - } - - public function processRequest() { - $request = $this->getRequest(); - $viewer = $request->getUser(); - - $this->requireApplicationCapability( - HarbormasterCapabilityManagePlans::CAPABILITY); - - $id = $this->id; - - $plan = id(new HarbormasterBuildPlanQuery()) - ->setViewer($viewer) - ->withIDs(array($id)) - ->executeOne(); - if (!$plan) { - return new Aphront404Response(); - } - - $cancel_uri = $this->getApplicationURI("plan/{$id}/"); - - $v_buildable = null; - $e_buildable = null; - - $errors = array(); - if ($request->isFormPost()) { - $v_buildable = $request->getStr('buildable'); - - if ($v_buildable) { - $buildable = id(new HarbormasterBuildableQuery()) - ->setViewer($viewer) - ->withIDs(array(trim($v_buildable, 'B'))) - ->executeOne(); - if (!$buildable) { - $e_buildable = pht('Invalid'); - } - } else { - $e_buildable = pht('Required'); - $errors[] = pht('You must provide a buildable.'); - } - - if (!$errors) { - $build_plan = HarbormasterBuild::initializeNewBuild($viewer) - ->setBuildablePHID($buildable->getPHID()) - ->setBuildPlanPHID($plan->getPHID()) - ->save(); - - $buildable_id = $buildable->getID(); - - return id(new AphrontRedirectResponse()) - ->setURI("/B{$buildable_id}"); - } - } - - if ($errors) { - $errors = id(new AphrontErrorView())->setErrors($errors); - } - - $form = id(new PHUIFormLayoutView()) - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('Buildable')) - ->setName('buildable') - ->setValue($v_buildable) - ->setError($e_buildable)); - - $dialog = id(new AphrontDialogView()) - ->setUser($viewer) - ->setTitle(pht('Execute Build Plan')) - ->setWidth(AphrontDialogView::WIDTH_FORM) - ->appendChild($errors) - ->appendChild($form) - ->addSubmitButton(pht('Execute Build Plan')) - ->addCancelButton($cancel_uri); - - return id(new AphrontDialogResponse())->setDialog($dialog); - } - -} diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index cc604e4e9a..58eefbe4ab 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -88,14 +88,6 @@ final class HarbormasterPlanViewController ->setDisabled(!$can_edit) ->setIcon('edit')); - $list->addAction( - id(new PhabricatorActionView()) - ->setName(pht('Manually Execute Plan')) - ->setHref($this->getApplicationURI("plan/execute/{$id}/")) - ->setWorkflow(true) - ->setDisabled(!$can_edit) - ->setIcon('arrow_right')); - return $list; } diff --git a/src/applications/harbormaster/query/HarbormasterBuildQuery.php b/src/applications/harbormaster/query/HarbormasterBuildQuery.php index bd93a08ea6..cc4fe92bcf 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildQuery.php @@ -5,6 +5,7 @@ final class HarbormasterBuildQuery private $ids; private $phids; + private $buildStatuses; private $buildablePHIDs; private $buildPlanPHIDs; @@ -20,6 +21,11 @@ final class HarbormasterBuildQuery return $this; } + public function withBuildStatuses(array $build_statuses) { + $this->buildStatuses = $build_statuses; + return $this; + } + public function withBuildablePHIDs(array $buildable_phids) { $this->buildablePHIDs = $buildable_phids; return $this; @@ -115,6 +121,13 @@ final class HarbormasterBuildQuery $this->phids); } + if ($this->buildStatuses) { + $where[] = qsprintf( + $conn_r, + 'buildStatus in (%Ls)', + $this->buildStatuses); + } + if ($this->buildablePHIDs) { $where[] = qsprintf( $conn_r, diff --git a/src/applications/harbormaster/storage/HarbormasterBuildable.php b/src/applications/harbormaster/storage/HarbormasterBuildable.php index d3100584bd..d3a95edb44 100644 --- a/src/applications/harbormaster/storage/HarbormasterBuildable.php +++ b/src/applications/harbormaster/storage/HarbormasterBuildable.php @@ -12,10 +12,12 @@ final class HarbormasterBuildable extends HarbormasterDAO private $containerObject = self::ATTACHABLE; private $buildableHandle = self::ATTACHABLE; + const STATUS_WHATEVER = 'whatever'; + public static function initializeNewBuildable(PhabricatorUser $actor) { return id(new HarbormasterBuildable()) - ->setBuildStatus('new') // TODO: Define these. - ->setBuildableStatus('active'); // TODO: Define these, too. + ->setBuildStatus(self::STATUS_WHATEVER) + ->setBuildableStatus(self::STATUS_WHATEVER); } public function getConfiguration() { diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 325f30931d..c7d5aa7181 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -10,9 +10,39 @@ final class HarbormasterBuild extends HarbormasterDAO private $buildable = self::ATTACHABLE; private $buildPlan = self::ATTACHABLE; + /** + * Not currently being built. + */ + const STATUS_INACTIVE = 'inactive'; + + /** + * Pending pick up by the Harbormaster daemon. + */ + const STATUS_PENDING = 'pending'; + + /** + * Waiting for a resource to be allocated (not yet relevant). + */ + const STATUS_WAITING = 'waiting'; + + /** + * Current building the buildable. + */ + const STATUS_BUILDING = 'building'; + + /** + * The build has passed. + */ + const STATUS_PASSED = 'passed'; + + /** + * The build has failed. + */ + const STATUS_FAILED = 'failed'; + public static function initializeNewBuild(PhabricatorUser $actor) { return id(new HarbormasterBuild()) - ->setBuildStatus('building'); // TODO: Sort this. + ->setBuildStatus(self::STATUS_INACTIVE); } public function getConfiguration() { diff --git a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php new file mode 100644 index 0000000000..a9d8ed79e2 --- /dev/null +++ b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php @@ -0,0 +1,51 @@ +getTaskData(); + $id = idx($data, 'buildID'); + + // Get a reference to the build. + $build = id(new HarbormasterBuildQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withBuildStatuses(array(HarbormasterBuild::STATUS_PENDING)) + ->withIDs(array($id)) + ->needBuildPlans(true) + ->executeOne(); + if (!$build) { + throw new PhabricatorWorkerPermanentFailureException( + pht('Invalid build ID "%s".', $id)); + } + + try { + $build->setBuildStatus(HarbormasterBuild::STATUS_BUILDING); + $build->save(); + + $buildable = $build->getBuildable(); + $plan = $build->getBuildPlan(); + + // TODO: Do the actual build here. + sleep(15); + + // If we get to here, then the build has passed. + $build->setBuildStatus(HarbormasterBuild::STATUS_PASSED); + $build->save(); + } catch (Exception $e) { + // If any exception is raised, the build is marked as a failure and + // the exception is re-thrown (this ensures we don't leave builds + // in an inconsistent state). + $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED); + $build->save(); + throw $e; + } + } + +} diff --git a/src/applications/harbormaster/worker/HarbormasterRunnerWorker.php b/src/applications/harbormaster/worker/HarbormasterRunnerWorker.php deleted file mode 100644 index 088f072bf2..0000000000 --- a/src/applications/harbormaster/worker/HarbormasterRunnerWorker.php +++ /dev/null @@ -1,53 +0,0 @@ -getTaskData(); - $id = idx($data, 'commitID'); - - $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere( - 'id = %d', - $id); - - if (!$commit) { - throw new PhabricatorWorkerPermanentFailureException( - "Commit '{$id}' does not exist!"); - } - - // TODO: (T603) Policy interaction? - $repository = id(new PhabricatorRepository())->loadOneWhere( - 'id = %d', - $commit->getRepositoryID()); - - if (!$repository) { - throw new PhabricatorWorkerPermanentFailureException( - "Unable to load repository for commit '{$id}'!"); - } - - $lease = id(new DrydockLease()) - ->setResourceType('working-copy') - ->setAttributes( - array( - 'repositoryID' => $repository->getID(), - 'commit' => $commit->getCommitIdentifier(), - )) - ->releaseOnDestruction() - ->waitUntilActive(); - - $cmd = $lease->getInterface('command'); - list($json) = $cmd - ->setWorkingDirectory($lease->getResource()->getAttribute('path')) - ->execx('arc unit --everything --json'); - $lease->release(); - - // TODO: Do something actually useful with this. Requires Harbormaster - // buildout. - echo $json; - } - -} From 5c0edc9351025cabae9f73aa26cb94c581452168 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Tue, 5 Nov 2013 13:00:12 -0800 Subject: [PATCH 4/9] Land Revision button for hosted git repos Summary: ref T182. Simple approach of clone, patch, push. While waiting for drydock, implement a hackish mutex setup for the workspace, which should work ok as long as there's only one committer who is carefull about theses things. Less obvious note: This is taking the both author and commiter's 'primary email' for the commit - which might rub some people wrong. Test Plan: With a hosted repo, created some diffs and landed them. Also clicked button for some error cases, got the right error message. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley CC: hach-que, Korvin, epriestley, aran Maniphest Tasks: T182 Differential Revision: https://secure.phabricator.com/D7486 --- src/__phutil_library_map__.php | 8 ++ .../DifferentialGetWorkingCopy.php | 39 +++++ .../PhabricatorApplicationDifferential.php | 3 + .../DifferentialRevisionLandController.php | 130 +++++++++++++++++ ...erentialLandingActionMenuEventListener.php | 54 +++++++ .../landing/DifferentialLandingStrategy.php | 43 ++++++ .../DifferentialLandingToHostedGit.php | 136 ++++++++++++++++++ 7 files changed, 413 insertions(+) create mode 100644 src/applications/differential/DifferentialGetWorkingCopy.php create mode 100644 src/applications/differential/controller/DifferentialRevisionLandController.php create mode 100644 src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php create mode 100644 src/applications/differential/landing/DifferentialLandingStrategy.php create mode 100644 src/applications/differential/landing/DifferentialLandingToHostedGit.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8882859909..174be1b6cd 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -369,6 +369,7 @@ phutil_register_library_map(array( 'DifferentialFieldValidationException' => 'applications/differential/field/exception/DifferentialFieldValidationException.php', 'DifferentialFreeformFieldSpecification' => 'applications/differential/field/specification/DifferentialFreeformFieldSpecification.php', 'DifferentialFreeformFieldTestCase' => 'applications/differential/field/specification/__tests__/DifferentialFreeformFieldTestCase.php', + 'DifferentialGetWorkingCopy' => 'applications/differential/DifferentialGetWorkingCopy.php', 'DifferentialGitSVNIDFieldSpecification' => 'applications/differential/field/specification/DifferentialGitSVNIDFieldSpecification.php', 'DifferentialHostFieldSpecification' => 'applications/differential/field/specification/DifferentialHostFieldSpecification.php', 'DifferentialHovercardEventListener' => 'applications/differential/event/DifferentialHovercardEventListener.php', @@ -383,6 +384,9 @@ phutil_register_library_map(array( 'DifferentialInlineCommentQuery' => 'applications/differential/query/DifferentialInlineCommentQuery.php', 'DifferentialInlineCommentView' => 'applications/differential/view/DifferentialInlineCommentView.php', 'DifferentialJIRAIssuesFieldSpecification' => 'applications/differential/field/specification/DifferentialJIRAIssuesFieldSpecification.php', + 'DifferentialLandingActionMenuEventListener' => 'applications/differential/landing/DifferentialLandingActionMenuEventListener.php', + 'DifferentialLandingStrategy' => 'applications/differential/landing/DifferentialLandingStrategy.php', + 'DifferentialLandingToHostedGit' => 'applications/differential/landing/DifferentialLandingToHostedGit.php', 'DifferentialLinesFieldSpecification' => 'applications/differential/field/specification/DifferentialLinesFieldSpecification.php', 'DifferentialLintFieldSpecification' => 'applications/differential/field/specification/DifferentialLintFieldSpecification.php', 'DifferentialLintStatus' => 'applications/differential/constants/DifferentialLintStatus.php', @@ -420,6 +424,7 @@ phutil_register_library_map(array( 'DifferentialRevisionEditor' => 'applications/differential/editor/DifferentialRevisionEditor.php', 'DifferentialRevisionIDFieldParserTestCase' => 'applications/differential/field/specification/__tests__/DifferentialRevisionIDFieldParserTestCase.php', 'DifferentialRevisionIDFieldSpecification' => 'applications/differential/field/specification/DifferentialRevisionIDFieldSpecification.php', + 'DifferentialRevisionLandController' => 'applications/differential/controller/DifferentialRevisionLandController.php', 'DifferentialRevisionListController' => 'applications/differential/controller/DifferentialRevisionListController.php', 'DifferentialRevisionListView' => 'applications/differential/view/DifferentialRevisionListView.php', 'DifferentialRevisionMailReceiver' => 'applications/differential/mail/DifferentialRevisionMailReceiver.php', @@ -2586,6 +2591,8 @@ phutil_register_library_map(array( 'DifferentialInlineCommentQuery' => 'PhabricatorOffsetPagedQuery', 'DifferentialInlineCommentView' => 'AphrontView', 'DifferentialJIRAIssuesFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialLandingActionMenuEventListener' => 'PhabricatorEventListener', + 'DifferentialLandingToHostedGit' => 'DifferentialLandingStrategy', 'DifferentialLinesFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialLintFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialLocalCommitsView' => 'AphrontView', @@ -2623,6 +2630,7 @@ phutil_register_library_map(array( 'DifferentialRevisionEditor' => 'PhabricatorEditor', 'DifferentialRevisionIDFieldParserTestCase' => 'PhabricatorTestCase', 'DifferentialRevisionIDFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialRevisionLandController' => 'DifferentialController', 'DifferentialRevisionListController' => array( 0 => 'DifferentialController', diff --git a/src/applications/differential/DifferentialGetWorkingCopy.php b/src/applications/differential/DifferentialGetWorkingCopy.php new file mode 100644 index 0000000000..01641261d2 --- /dev/null +++ b/src/applications/differential/DifferentialGetWorkingCopy.php @@ -0,0 +1,39 @@ +getLocalPath(); + + $path = rtrim($origin_path, '/'); + $path = $path . '__workspace'; + + if (!Filesystem::pathExists($path)) { + $repo->execxLocalCommand( + 'clone -- file://%s %s', + $origin_path, + $path); + } + + $workspace = new ArcanistGitAPI($path); + $workspace->execxLocal('clean -f -d'); + $workspace->execxLocal('checkout master'); + $workspace->execxLocal('fetch'); + $workspace->execxLocal('reset --hard origin/master'); + $workspace->reloadWorkingCopy(); + + return $workspace; + } + +} diff --git a/src/applications/differential/application/PhabricatorApplicationDifferential.php b/src/applications/differential/application/PhabricatorApplicationDifferential.php index d189788347..33234225a2 100644 --- a/src/applications/differential/application/PhabricatorApplicationDifferential.php +++ b/src/applications/differential/application/PhabricatorApplicationDifferential.php @@ -32,6 +32,7 @@ final class PhabricatorApplicationDifferential extends PhabricatorApplication { return array( new DifferentialActionMenuEventListener(), new DifferentialHovercardEventListener(), + new DifferentialLandingActionMenuEventListener(), ); } @@ -48,6 +49,8 @@ final class PhabricatorApplicationDifferential extends PhabricatorApplication { 'changeset/' => 'DifferentialChangesetViewController', 'revision/edit/(?:(?P[1-9]\d*)/)?' => 'DifferentialRevisionEditController', + 'revision/land/(?:(?P[1-9]\d*))/(?P[^/]+)/' + => 'DifferentialRevisionLandController', 'comment/' => array( 'preview/(?P[1-9]\d*)/' => 'DifferentialCommentPreviewController', 'save/' => 'DifferentialCommentSaveController', diff --git a/src/applications/differential/controller/DifferentialRevisionLandController.php b/src/applications/differential/controller/DifferentialRevisionLandController.php new file mode 100644 index 0000000000..003c4900ac --- /dev/null +++ b/src/applications/differential/controller/DifferentialRevisionLandController.php @@ -0,0 +1,130 @@ +revisionID = $data['id']; + $this->strategyClass = $data['strategy']; + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $revision_id = $this->revisionID; + + $revision = id(new DifferentialRevisionQuery()) + ->withIDs(array($revision_id)) + ->setViewer($viewer) + ->executeOne(); + if (!$revision) { + return new Aphront404Response(); + } + + if (is_subclass_of($this->strategyClass, 'DifferentialLandingStrategy')) { + $this->pushStrategy = newv($this->strategyClass, array()); + } else { + throw new Exception( + "Strategy type must be a valid class name and must subclass ". + "DifferentialLandingStrategy. ". + "'{$this->strategyClass}' is not a subclass of ". + "DifferentialLandingStrategy."); + } + + if ($request->isDialogFormPost()) { + try { + $this->attemptLand($revision, $request); + $title = pht("Success!"); + $text = pht("Revision was successfully landed."); + } catch (Exception $ex) { + $title = pht("Failed to land revision"); + $text = 'moo'; + if ($ex instanceof PhutilProxyException) { + $text = hsprintf( + '%s:
%s
', + $ex->getMessage(), + $ex->getPreviousException()->getMessage()); + } else { + $text = hsprintf('
%s
', $ex->getMessage()); + } + $text = id(new AphrontErrorView()) + ->appendChild($text); + } + + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setTitle($title) + ->appendChild(phutil_tag('p', array(), $text)) + ->setSubmitURI('/D'.$revision_id) + ->addSubmitButton(pht('Done')); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + $prompt = hsprintf('%s

%s', + pht( + 'This will squash and rebase revision %s, and push it to '. + 'origin/master.', + $revision_id), + pht('It is an experimental feature and may not work.')); + + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setTitle(pht("Land Revision %s?", $revision_id)) + ->appendChild($prompt) + ->setSubmitURI($request->getRequestURI()) + ->addSubmitButton(pht('Land it!')) + ->addCancelButton('/D'.$revision_id); + + return id(new AphrontDialogResponse())->setDialog($dialog); + } + + private function attemptLand($revision, $request) { + $status = $revision->getStatus(); + if ($status != ArcanistDifferentialRevisionStatus::ACCEPTED) { + throw new Exception("Only Accepted revisions can be landed."); + } + + $repository = $revision->getRepository(); + + if ($repository === null) { + throw new Exception("revision is not attached to a repository."); + } + + $can_push = PhabricatorPolicyFilter::hasCapability( + $request->getUser(), + $repository, + DiffusionCapabilityPush::CAPABILITY); + + if (!$can_push) { + throw new Exception( + pht('You do not have permission to push to this repository.')); + } + + $lock = $this->lockRepository($repository); + + try { + $this->pushStrategy->processLandRequest( + $request, + $revision, + $repository); + } catch (Exception $e) { + $lock->unlock(); + throw $e; + } + + $lock->unlock(); + } + + private function lockRepository($repository) { + $lock_name = __CLASS__.':'.($repository->getCallsign()); + $lock = PhabricatorGlobalLock::newLock($lock_name); + $lock->lock(); + return $lock; + } +} + diff --git a/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php b/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php new file mode 100644 index 0000000000..fdfea16ae0 --- /dev/null +++ b/src/applications/differential/landing/DifferentialLandingActionMenuEventListener.php @@ -0,0 +1,54 @@ +listen(PhabricatorEventType::TYPE_UI_DIDRENDERACTIONS); + } + + public function handleEvent(PhutilEvent $event) { + switch ($event->getType()) { + case PhabricatorEventType::TYPE_UI_DIDRENDERACTIONS: + $this->handleActionsEvent($event); + break; + } + } + + private function handleActionsEvent(PhutilEvent $event) { + $object = $event->getValue('object'); + + $actions = null; + if ($object instanceof DifferentialRevision) { + $actions = $this->renderRevisionAction($event); + } + + $this->addActionMenuItems($event, $actions); + } + + private function renderRevisionAction(PhutilEvent $event) { + if (!$this->canUseApplication($event->getUser())) { + return null; + } + + $revision = $event->getValue('object'); + + $repository = $revision->getRepository(); + if ($repository === null) { + return null; + } + + $strategies = id(new PhutilSymbolLoader()) + ->setAncestorClass('DifferentialLandingStrategy') + ->loadObjects(); + foreach ($strategies as $strategy) { + $actions = $strategy->createMenuItems( + $event->getUser(), + $revision, + $repository); + $this->addActionMenuItems($event, $actions); + } + } + +} + diff --git a/src/applications/differential/landing/DifferentialLandingStrategy.php b/src/applications/differential/landing/DifferentialLandingStrategy.php new file mode 100644 index 0000000000..46a0f74333 --- /dev/null +++ b/src/applications/differential/landing/DifferentialLandingStrategy.php @@ -0,0 +1,43 @@ +getId(); + return id(new PhabricatorActionView()) + ->setRenderAsForm(true) + ->setName($name) + ->setHref("/differential/revision/land/{$revision_id}/{$strategy}/") + ->setDisabled($disabled); + } + + /** + * might break if repository is not Git. + */ + protected function getGitWorkspace(PhabricatorRepository $repository) { + try { + return DifferentialGetWorkingCopy::getCleanGitWorkspace($repository); + } catch (Exception $e) { + throw new PhutilProxyException ( + 'Failed to allocate a workspace', + $e); + } + } +} diff --git a/src/applications/differential/landing/DifferentialLandingToHostedGit.php b/src/applications/differential/landing/DifferentialLandingToHostedGit.php new file mode 100644 index 0000000000..874b0aff2c --- /dev/null +++ b/src/applications/differential/landing/DifferentialLandingToHostedGit.php @@ -0,0 +1,136 @@ +getUser(); + + $workspace = $this->getGitWorkspace($repository); + + try { + $this->commitRevisionToWorkspace( + $revision, + $workspace, + $viewer); + } catch (Exception $e) { + throw new PhutilProxyException( + 'Failed to commit patch', + $e); + } + + try { + $this->pushWorkspaceRepository( + $repository, + $workspace, + $viewer); + } catch (Exception $e) { + throw new PhutilProxyException( + 'Failed to push changes upstream', + $e); + } + } + + public function commitRevisionToWorkspace( + DifferentialRevision $revision, + ArcanistRepositoryAPI $workspace, + PhabricatorUser $user) { + + $diff_id = $revision->loadActiveDiff()->getID(); + + $call = new ConduitCall( + 'differential.getrawdiff', + array( + 'diffID' => $diff_id, + )); + + $call->setUser($user); + $raw_diff = $call->execute(); + + $missing_binary = + "\nindex " + . "0000000000000000000000000000000000000000.." + . "0000000000000000000000000000000000000000\n"; + if (strpos($raw_diff, $missing_binary) !== false) { + throw new Exception("Patch is missing content for a binary file"); + } + + $future = $workspace->execFutureLocal('apply --index -'); + $future->write($raw_diff); + $future->resolvex(); + + $workspace->reloadWorkingCopy(); + + $call = new ConduitCall( + 'differential.getcommitmessage', + array( + 'revision_id' => $revision->getID(), + )); + + $call->setUser($user); + $message = $call->execute(); + + $author = id(new PhabricatorUser())->loadOneWhere( + 'phid = %s', + $revision->getAuthorPHID()); + + $author_string = sprintf( + '%s <%s>', + $author->getRealName(), + $author->loadPrimaryEmailAddress()); + $author_date = $revision->getDateCreated(); + + $workspace->execxLocal( + '-c user.name=%s -c user.email=%s ' . + 'commit --date=%s --author=%s '. + '--message=%s', + // -c will set the 'committer' + $user->getRealName(), + $user->loadPrimaryEmailAddress(), + $author_date, + $author_string, + $message); + } + + + public function pushWorkspaceRepository( + PhabricatorRepository $repository, + ArcanistRepositoryAPI $workspace, + PhabricatorUser $user) { + + $workspace->execxLocal("push origin HEAD:master"); + } + + public function createMenuItems( + PhabricatorUser $viewer, + DifferentialRevision $revision, + PhabricatorRepository $repository) { + + $vcs = $repository->getVersionControlSystem(); + if ($vcs !== PhabricatorRepositoryType::REPOSITORY_TYPE_GIT) { + return; + } + + if (!$repository->isHosted()) { + return; + } + + if (!$repository->isWorkingCopyBare()) { + return; + } + + $can_push = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + DiffusionCapabilityPush::CAPABILITY); + + return $this->createActionView( + $revision, + pht('Land to Hosted Repository'), + !$can_push); + } +} From c514d34b9456a2cd2125f4d44c35c79140cbbd8e Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 5 Nov 2013 13:34:14 -0800 Subject: [PATCH 5/9] Add build step implementation infrastructure and sleep build step. Summary: Depends on D7498. This implements support for a "build step implementation". Build steps have an associated class name (which makes the class in PHP) and a details field, which is serialized JSON (same as PhabricatorRepository). This also implements a SleepBuildStepImplementation which just pauses the build for a specified period of seconds. Test Plan: Inserted a build step with `insert into harbormaster_buildstep (phid, buildPlanPHID, className, details, dateCreated, dateModified) values ('', 'PHID-HMCP-zkh5w6czfbfpk2gxwdeo', 'SleepBuildStepImplementation', '{"seconds":5}', NOW(), NOW());` (adjusting the build plan PHID as appropriate). Started the daemon and applied the build plan to a buildable, and saw the daemon take a 5 second delay after creating `SleepBuildStepImplementation`. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley CC: Korvin, epriestley, aran, chad Maniphest Tasks: T1049 Differential Revision: https://secure.phabricator.com/D7499 --- resources/sql/patches/20131105.buildstep.sql | 11 ++++ src/__phutil_library_map__.php | 9 +++- .../query/HarbormasterBuildStepQuery.php | 38 ++++++++++++++ .../step/BuildStepImplementation.php | 48 +++++++++++++++++ .../step/SleepBuildStepImplementation.php | 24 +++++++++ .../configuration/HarbormasterBuildStep.php | 51 ++++++++++++++++++- .../worker/HarbormasterBuildWorker.php | 22 ++++++-- .../patch/PhabricatorBuiltinPatchList.php | 4 ++ 8 files changed, 201 insertions(+), 6 deletions(-) create mode 100644 resources/sql/patches/20131105.buildstep.sql create mode 100644 src/applications/harbormaster/step/BuildStepImplementation.php create mode 100644 src/applications/harbormaster/step/SleepBuildStepImplementation.php diff --git a/resources/sql/patches/20131105.buildstep.sql b/resources/sql/patches/20131105.buildstep.sql new file mode 100644 index 0000000000..714e9a3088 --- /dev/null +++ b/resources/sql/patches/20131105.buildstep.sql @@ -0,0 +1,11 @@ +CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_buildstep ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARCHAR(64) NOT NULL COLLATE utf8_bin, + buildPlanPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + className VARCHAR(255) NOT NULL COLLATE utf8_bin, + details LONGTEXT CHARACTER SET utf8 COLLATE utf8_bin NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + KEY `key_plan` (buildPlanPHID), + UNIQUE KEY `key_phid` (phid) +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 174be1b6cd..3716d1ba9e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -91,6 +91,7 @@ phutil_register_library_map(array( 'AphrontView' => 'view/AphrontView.php', 'AphrontWebpageResponse' => 'aphront/response/AphrontWebpageResponse.php', 'AuditActionMenuEventListener' => 'applications/audit/events/AuditActionMenuEventListener.php', + 'BuildStepImplementation' => 'applications/harbormaster/step/BuildStepImplementation.php', 'CelerityAPI' => 'infrastructure/celerity/CelerityAPI.php', 'CelerityPhabricatorResourceController' => 'infrastructure/celerity/CelerityPhabricatorResourceController.php', 'CelerityResourceController' => 'infrastructure/celerity/CelerityResourceController.php', @@ -2195,6 +2196,7 @@ 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', + 'SleepBuildStepImplementation' => 'applications/harbormaster/step/SleepBuildStepImplementation.php', 'SlowvoteEmbedView' => 'applications/slowvote/view/SlowvoteEmbedView.php', 'SlowvoteRemarkupRule' => 'applications/slowvote/remarkup/SlowvoteRemarkupRule.php', ), @@ -2872,7 +2874,11 @@ phutil_register_library_map(array( 'HarbormasterBuildPlanTransactionComment' => 'PhabricatorApplicationTransactionComment', 'HarbormasterBuildPlanTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'HarbormasterBuildQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', - 'HarbormasterBuildStep' => 'HarbormasterDAO', + 'HarbormasterBuildStep' => + array( + 0 => 'HarbormasterDAO', + 1 => 'PhabricatorPolicyInterface', + ), 'HarbormasterBuildStepQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HarbormasterBuildTarget' => 'HarbormasterDAO', 'HarbormasterBuildTargetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', @@ -4651,6 +4657,7 @@ phutil_register_library_map(array( 'ReleephStatusFieldSpecification' => 'ReleephFieldSpecification', 'ReleephSummaryFieldSpecification' => 'ReleephFieldSpecification', 'ReleephUserView' => 'AphrontView', + 'SleepBuildStepImplementation' => 'BuildStepImplementation', 'SlowvoteEmbedView' => 'AphrontView', 'SlowvoteRemarkupRule' => 'PhabricatorRemarkupRuleObject', ), diff --git a/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php b/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php index 9a94be6e2b..61190b3285 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php @@ -5,6 +5,7 @@ final class HarbormasterBuildStepQuery private $ids; private $phids; + private $buildPlanPHIDs; public function withIDs(array $ids) { $this->ids = $ids; @@ -16,6 +17,11 @@ final class HarbormasterBuildStepQuery return $this; } + public function withBuildPlanPHIDs(array $phids) { + $this->buildPlanPHIDs = $phids; + return $this; + } + protected function loadPage() { $table = new HarbormasterBuildStep(); $conn_r = $table->establishConnection('r'); @@ -48,11 +54,43 @@ final class HarbormasterBuildStepQuery $this->phids); } + if ($this->buildPlanPHIDs) { + $where[] = qsprintf( + $conn_r, + 'buildPlanPHID in (%Ls)', + $this->buildPlanPHIDs); + } + $where[] = $this->buildPagingClause($conn_r); return $this->formatWhereClause($where); } + protected function willFilterPage(array $page) { + $plans = array(); + + $buildplan_phids = array_filter(mpull($page, 'getBuildPlanPHID')); + if ($buildplan_phids) { + $plans = id(new PhabricatorObjectQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($buildplan_phids) + ->setParentQuery($this) + ->execute(); + $plans = mpull($plans, null, 'getPHID'); + } + + foreach ($page as $key => $build) { + $buildable_phid = $build->getBuildPlanPHID(); + if (empty($plans[$buildable_phid])) { + unset($page[$key]); + continue; + } + $build->attachBuildPlan($plans[$buildable_phid]); + } + + return $page; + } + public function getQueryApplicationClass() { return 'PhabricatorApplicationHarbormaster'; } diff --git a/src/applications/harbormaster/step/BuildStepImplementation.php b/src/applications/harbormaster/step/BuildStepImplementation.php new file mode 100644 index 0000000000..b782496a6f --- /dev/null +++ b/src/applications/harbormaster/step/BuildStepImplementation.php @@ -0,0 +1,48 @@ +settings; + } + + /** + * Loads the settings for this build step implementation from the build step. + */ + public final function loadSettings(HarbormasterBuildStep $build_step) { + $this->settings = array(); + foreach ($this->getSettingDefinitions() as $name => $opt) { + $this->settings[$name] = $build_step->getDetail($name); + } + return $this->settings; + } + + /** + * Return an array of settings for this step implementation. + */ + public function getSettingDefinitions() { + return array(); + } +} diff --git a/src/applications/harbormaster/step/SleepBuildStepImplementation.php b/src/applications/harbormaster/step/SleepBuildStepImplementation.php new file mode 100644 index 0000000000..bf16b22d23 --- /dev/null +++ b/src/applications/harbormaster/step/SleepBuildStepImplementation.php @@ -0,0 +1,24 @@ +getSettings(); + + sleep($settings['seconds']); + } + + public function getSettingDefinitions() { + return array( + 'seconds' => array()); + } + +} diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index 96d3e827d0..42a7e89fa9 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php @@ -1,14 +1,20 @@ true, + self::CONFIG_SERIALIZATION => array( + 'details' => self::SERIALIZATION_JSON, + ) ) + parent::getConfiguration(); } @@ -26,4 +32,47 @@ final class HarbormasterBuildStep extends HarbormasterDAO { return $this->assertAttached($this->buildPlan); } + public function getDetail($key, $default = null) { + return idx($this->details, $key, $default); + } + + public function setDetail($key, $value) { + $this->details[$key] = $value; + return $this; + } + + public function getStepImplementation() { + if ($this->className === null) { + throw new Exception("No implementation set for the given step."); + } + + // TODO: We should look up the class in phutil's system to ensure + // that it actually extends BuildStepImplementation. + $class = $this->className; + $implementation = newv($class, array()); + $implementation->loadSettings($this); + return $implementation; + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + return $this->getBuildPlan()->getPolicy($capability); + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return $this->getBuildPlan()->hasAutomaticCapability($capability, $viewer); + } + + public function describeAutomaticCapability($capability) { + return pht('A build step has the same policies as its build plan.'); + } } diff --git a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php index a9d8ed79e2..347119e348 100644 --- a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php @@ -32,11 +32,25 @@ final class HarbormasterBuildWorker extends PhabricatorWorker { $buildable = $build->getBuildable(); $plan = $build->getBuildPlan(); - // TODO: Do the actual build here. - sleep(15); + $steps = id(new HarbormasterBuildStepQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withBuildPlanPHIDs(array($plan->getPHID())) + ->execute(); - // If we get to here, then the build has passed. - $build->setBuildStatus(HarbormasterBuild::STATUS_PASSED); + // Perform the build. + foreach ($steps as $step) { + $implementation = $step->getStepImplementation(); + $implementation->execute($build); + if ($build->getBuildStatus() !== HarbormasterBuild::STATUS_BUILDING) { + break; + } + } + + // If we get to here, then the build has finished. Set it to passed + // if no build step explicitly set the status. + if ($build->getBuildStatus() === HarbormasterBuild::STATUS_BUILDING) { + $build->setBuildStatus(HarbormasterBuild::STATUS_PASSED); + } $build->save(); } catch (Exception $e) { // If any exception is raised, the build is marked as a failure and diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index f6bb232bd8..04b303273c 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1720,6 +1720,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20131031.vcspassword.sql'), ), + '20131105.buildstep.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20131105.buildstep.sql'), + ), ); } } From e4569e7e7e2e7ba3fea6223396f76fcb3f8638a5 Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Tue, 5 Nov 2013 13:54:03 -0800 Subject: [PATCH 6/9] Implement interface for adding, editing and deleting build steps on plans. Summary: This implements an interface for adding new build steps, editing existing build steps and deleting build steps from build plans. It uses the settings definitions on the build implementation to work out what fields should be displayed on the edit page. Test Plan: See screenshots: {F78529} {F78532} {F78528} {F78531} {F78527} {F78530} Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley CC: Korvin, epriestley, aran Maniphest Tasks: T1049 Differential Revision: https://secure.phabricator.com/D7500 --- src/__phutil_library_map__.php | 6 + .../PhabricatorApplicationHarbormaster.php | 5 + .../HarbormasterBuildableViewController.php | 4 + .../HarbormasterPlanViewController.php | 61 +++++++ .../HarbormasterStepAddController.php | 84 ++++++++++ .../HarbormasterStepDeleteController.php | 52 ++++++ .../HarbormasterStepEditController.php | 156 ++++++++++++++++++ .../query/HarbormasterBuildStepQuery.php | 8 + .../step/BuildStepImplementation.php | 44 ++++- .../step/SleepBuildStepImplementation.php | 25 ++- .../storage/build/HarbormasterBuild.php | 5 + .../configuration/HarbormasterBuildStep.php | 11 +- .../worker/HarbormasterBuildWorker.php | 6 +- 13 files changed, 460 insertions(+), 7 deletions(-) create mode 100644 src/applications/harbormaster/controller/HarbormasterStepAddController.php create mode 100644 src/applications/harbormaster/controller/HarbormasterStepDeleteController.php create mode 100644 src/applications/harbormaster/controller/HarbormasterStepEditController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3716d1ba9e..87463605d9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -687,6 +687,9 @@ phutil_register_library_map(array( 'HarbormasterPlanViewController' => 'applications/harbormaster/controller/HarbormasterPlanViewController.php', 'HarbormasterRemarkupRule' => 'applications/harbormaster/remarkup/HarbormasterRemarkupRule.php', 'HarbormasterScratchTable' => 'applications/harbormaster/storage/HarbormasterScratchTable.php', + 'HarbormasterStepAddController' => 'applications/harbormaster/controller/HarbormasterStepAddController.php', + 'HarbormasterStepDeleteController' => 'applications/harbormaster/controller/HarbormasterStepDeleteController.php', + 'HarbormasterStepEditController' => 'applications/harbormaster/controller/HarbormasterStepEditController.php', 'HeraldAction' => 'applications/herald/storage/HeraldAction.php', 'HeraldAdapter' => 'applications/herald/adapter/HeraldAdapter.php', 'HeraldApplyTranscript' => 'applications/herald/storage/transcript/HeraldApplyTranscript.php', @@ -2919,6 +2922,9 @@ phutil_register_library_map(array( 'HarbormasterPlanViewController' => 'HarbormasterPlanController', 'HarbormasterRemarkupRule' => 'PhabricatorRemarkupRuleObject', 'HarbormasterScratchTable' => 'HarbormasterDAO', + 'HarbormasterStepAddController' => 'HarbormasterController', + 'HarbormasterStepDeleteController' => 'HarbormasterController', + 'HarbormasterStepEditController' => 'HarbormasterController', 'HeraldAction' => 'HeraldDAO', 'HeraldApplyTranscript' => 'HeraldDAO', 'HeraldCapabilityManageGlobalRules' => 'PhabricatorPolicyCapability', diff --git a/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php b/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php index 6bcb87d7b1..bf0d7c21ac 100644 --- a/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php +++ b/src/applications/harbormaster/application/PhabricatorApplicationHarbormaster.php @@ -46,6 +46,11 @@ final class PhabricatorApplicationHarbormaster extends PhabricatorApplication { 'edit/(?:(?P\d+)/)?' => 'HarbormasterBuildableEditController', 'apply/(?:(?P\d+)/)?' => 'HarbormasterBuildableApplyController', ), + 'step/' => array( + 'add/(?:(?P\d+)/)?' => 'HarbormasterStepAddController', + 'edit/(?:(?P\d+)/)?' => 'HarbormasterStepEditController', + 'delete/(?:(?P\d+)/)?' => 'HarbormasterStepDeleteController', + ), 'plan/' => array( '(?:query/(?P[^/]+)/)?' => 'HarbormasterPlanListController', diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php index afe43b1423..a5c9c0b34d 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php @@ -62,6 +62,10 @@ final class HarbormasterBuildableViewController $item->setBarColor('red'); $item->addAttribute(pht('Failed')); break; + case HarbormasterBuild::STATUS_ERROR: + $item->setBarColor('red'); + $item->addAttribute(pht('Unexpected Error')); + break; } $build_list->addItem($item); } diff --git a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php index 58eefbe4ab..9cc6944202 100644 --- a/src/applications/harbormaster/controller/HarbormasterPlanViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterPlanViewController.php @@ -55,10 +55,13 @@ final class HarbormasterPlanViewController id(new PhabricatorCrumbView()) ->setName(pht("Plan %d", $id))); + $step_list = $this->buildStepList($plan); + return $this->buildApplicationPage( array( $crumbs, $box, + $step_list, $xaction_view, ), array( @@ -67,6 +70,56 @@ final class HarbormasterPlanViewController )); } + private function buildStepList(HarbormasterBuildPlan $plan) { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $steps = id(new HarbormasterBuildStepQuery()) + ->setViewer($viewer) + ->withBuildPlanPHIDs(array($plan->getPHID())) + ->execute(); + + $can_edit = $this->hasApplicationCapability( + HarbormasterCapabilityManagePlans::CAPABILITY); + + $i = 1; + $step_list = id(new PHUIObjectItemListView()) + ->setUser($viewer); + foreach ($steps as $step) { + $implementation = $step->getStepImplementation(); + $item = id(new PHUIObjectItemView()) + ->setObjectName("Step ".$i++) + ->setHeader($implementation->getName()); + + if (!$implementation->validateSettings()) { + $item + ->setBarColor('red') + ->addAttribute(pht('This step is not configured correctly.')); + } else { + $item->addAttribute($implementation->getDescription()); + } + + if ($can_edit) { + $edit_uri = $this->getApplicationURI("step/edit/".$step->getID()."/"); + $item + ->setHref($edit_uri) + ->addAction( + id(new PHUIListItemView()) + ->setIcon('delete') + ->addSigil('harbormaster-build-step-delete') + ->setWorkflow(true) + ->setRenderNameAsTooltip(true) + ->setName(pht("Delete")) + ->setHref( + $this->getApplicationURI("step/delete/".$step->getID()."/"))); + } + + $step_list->addItem($item); + } + + return $step_list; + } + private function buildActionList(HarbormasterBuildPlan $plan) { $request = $this->getRequest(); $viewer = $request->getUser(); @@ -88,6 +141,14 @@ final class HarbormasterPlanViewController ->setDisabled(!$can_edit) ->setIcon('edit')); + $list->addAction( + id(new PhabricatorActionView()) + ->setName(pht('Add Build Step')) + ->setHref($this->getApplicationURI("step/add/{$id}/")) + ->setWorkflow($can_edit) + ->setDisabled(!$can_edit) + ->setIcon('new')); + return $list; } diff --git a/src/applications/harbormaster/controller/HarbormasterStepAddController.php b/src/applications/harbormaster/controller/HarbormasterStepAddController.php new file mode 100644 index 0000000000..095f6149ff --- /dev/null +++ b/src/applications/harbormaster/controller/HarbormasterStepAddController.php @@ -0,0 +1,84 @@ +id = $data['id']; + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $this->requireApplicationCapability( + HarbormasterCapabilityManagePlans::CAPABILITY); + + $id = $this->id; + + $plan = id(new HarbormasterBuildPlanQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if ($plan === null) { + throw new Exception("Build plan not found!"); + } + + $implementations = BuildStepImplementation::getImplementations(); + + $cancel_uri = $this->getApplicationURI('plan/'.$plan->getID().'/'); + + if ($request->isDialogFormPost()) { + $class = $request->getStr('step-type'); + if (!in_array($class, $implementations)) { + return $this->createDialog($implementations); + } + + $step = new HarbormasterBuildStep(); + $step->setBuildPlanPHID($plan->getPHID()); + $step->setClassName($class); + $step->setDetails(array()); + $step->save(); + + $edit_uri = $this->getApplicationURI("step/edit/".$step->getID()."/"); + + return id(new AphrontRedirectResponse())->setURI($edit_uri); + } + + return $this->createDialog($implementations, $cancel_uri); + } + + function createDialog(array $implementations, $cancel_uri) { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $control = id(new AphrontFormRadioButtonControl()) + ->setName('step-type'); + + foreach ($implementations as $implementation_name) { + $implementation = new $implementation_name(); + $control + ->addButton( + $implementation_name, + $implementation->getName(), + $implementation->getGenericDescription()); + } + + $dialog = new AphrontDialogView(); + $dialog->setTitle(pht('Add New Step')) + ->setUser($viewer) + ->addSubmitButton(pht('Add Build Step')) + ->addCancelButton($cancel_uri); + $dialog->appendChild( + phutil_tag( + 'p', + array(), + pht( + 'Select what type of build step you want to add: '))); + $dialog->appendChild($control); + return id(new AphrontDialogResponse())->setDialog($dialog); + } + +} diff --git a/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php b/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php new file mode 100644 index 0000000000..a11d565ba9 --- /dev/null +++ b/src/applications/harbormaster/controller/HarbormasterStepDeleteController.php @@ -0,0 +1,52 @@ +id = $data['id']; + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $this->requireApplicationCapability( + HarbormasterCapabilityManagePlans::CAPABILITY); + + $id = $this->id; + + $step = id(new HarbormasterBuildStepQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if ($step === null) { + throw new Exception("Build step not found!"); + } + + $plan_id = $step->getBuildPlan()->getID(); + $done_uri = $this->getApplicationURI('plan/'.$plan_id.'/'); + + if ($request->isDialogFormPost()) { + $step->delete(); + return id(new AphrontRedirectResponse())->setURI($done_uri); + } + + $dialog = new AphrontDialogView(); + $dialog->setTitle(pht('Really Delete Step?')) + ->setUser($viewer) + ->addSubmitButton(pht('Delete Build Step')) + ->addCancelButton($done_uri); + $dialog->appendChild( + phutil_tag( + 'p', + array(), + pht( + 'Are you sure you want to delete this '. + 'step? This can\'t be undone!'))); + return id(new AphrontDialogResponse())->setDialog($dialog); + } + +} diff --git a/src/applications/harbormaster/controller/HarbormasterStepEditController.php b/src/applications/harbormaster/controller/HarbormasterStepEditController.php new file mode 100644 index 0000000000..81d3f0b2a8 --- /dev/null +++ b/src/applications/harbormaster/controller/HarbormasterStepEditController.php @@ -0,0 +1,156 @@ +id = idx($data, 'id'); + } + + public function processRequest() { + $request = $this->getRequest(); + $viewer = $request->getUser(); + + $this->requireApplicationCapability( + HarbormasterCapabilityManagePlans::CAPABILITY); + + $step = id(new HarbormasterBuildStepQuery()) + ->setViewer($viewer) + ->withIDs(array($this->id)) + ->executeOne(); + if (!$step) { + return new Aphront404Response(); + } + + $plan = id(new HarbormasterBuildPlanQuery()) + ->setViewer($viewer) + ->withPHIDs(array($step->getBuildPlanPHID())) + ->executeOne(); + if (!$plan) { + return new Aphront404Response(); + } + + $implementation = $step->getStepImplementation(); + $implementation->validateSettingDefinitions(); + $settings = $implementation->getSettings(); + + $errors = array(); + if ($request->isFormPost()) { + foreach ($implementation->getSettingDefinitions() as $name => $opt) { + $readable_name = $this->getReadableName($name, $opt); + $value = $this->getValueFromRequest($request, $name, $opt['type']); + + // TODO: This won't catch any validation issues unless the field + // is missing completely. How should we check if the user is + // required to enter an integer? + if ($value === null) { + $errors[] = $readable_name.' is not valid.'; + } else { + $step->setDetail($name, $value); + } + } + + if (count($errors) === 0) { + $step->save(); + + return id(new AphrontRedirectResponse()) + ->setURI($this->getApplicationURI('plan/'.$plan->getID().'/')); + } + } + + $form = id(new AphrontFormView()) + ->setUser($viewer); + + // We need to render out all of the fields for the settings that + // the implementation has. + foreach ($implementation->getSettingDefinitions() as $name => $opt) { + if ($request->isFormPost()) { + $value = $this->getValueFromRequest($request, $name, $opt['type']); + } else { + $value = $settings[$name]; + } + + switch ($opt['type']) { + case BuildStepImplementation::SETTING_TYPE_STRING: + case BuildStepImplementation::SETTING_TYPE_INTEGER: + $control = id(new AphrontFormTextControl()) + ->setLabel($this->getReadableName($name, $opt)) + ->setName($name) + ->setValue($value); + break; + case BuildStepImplementation::SETTING_TYPE_BOOLEAN: + $control = id(new AphrontFormCheckboxControl()) + ->setLabel($this->getReadableName($name, $opt)) + ->setName($name) + ->setValue($value); + break; + default: + throw new Exception("Unable to render field with unknown type."); + } + + if (isset($opt['description'])) { + $control->setCaption($opt['description']); + } + + $form->appendChild($control); + } + + $form->appendChild( + id(new AphrontFormSubmitControl()) + ->setValue(pht('Save Step Configuration')) + ->addCancelButton( + $this->getApplicationURI('plan/'.$plan->getID().'/'))); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText('Edit Step: '.$implementation->getName()) + ->setValidationException(null) + ->setForm($form); + + $crumbs = $this->buildApplicationCrumbs(); + $id = $plan->getID(); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName(pht("Plan %d", $id)) + ->setHref($this->getApplicationURI("plan/{$id}/"))); + $crumbs->addCrumb( + id(new PhabricatorCrumbView()) + ->setName(pht('Edit Step'))); + + return $this->buildApplicationPage( + array( + $crumbs, + $box, + ), + array( + 'title' => $implementation->getName(), + 'device' => true, + )); + } + + public function getReadableName($name, $opt) { + $readable_name = $name; + if (isset($opt['name'])) { + $readable_name = $opt['name']; + } + return $readable_name; + } + + public function getValueFromRequest(AphrontRequest $request, $name, $type) { + switch ($type) { + case BuildStepImplementation::SETTING_TYPE_STRING: + return $request->getStr($name); + break; + case BuildStepImplementation::SETTING_TYPE_INTEGER: + return $request->getInt($name); + break; + case BuildStepImplementation::SETTING_TYPE_BOOLEAN: + return $request->getBool($name); + break; + default: + throw new Exception("Unsupported setting type '".$type."'."); + } + } + +} diff --git a/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php b/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php index 61190b3285..3bcc057f16 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php @@ -22,6 +22,14 @@ final class HarbormasterBuildStepQuery return $this; } + public function getPagingColumn() { + return 'id'; + } + + public function getReversePaging() { + return true; + } + protected function loadPage() { $table = new HarbormasterBuildStep(); $conn_r = $table->establishConnection('r'); diff --git a/src/applications/harbormaster/step/BuildStepImplementation.php b/src/applications/harbormaster/step/BuildStepImplementation.php index b782496a6f..6a524f4e39 100644 --- a/src/applications/harbormaster/step/BuildStepImplementation.php +++ b/src/applications/harbormaster/step/BuildStepImplementation.php @@ -4,13 +4,32 @@ abstract class BuildStepImplementation { private $settings; + const SETTING_TYPE_STRING = 'string'; + const SETTING_TYPE_INTEGER = 'integer'; + const SETTING_TYPE_BOOLEAN = 'boolean'; + + public static function getImplementations() { + $symbols = id(new PhutilSymbolLoader()) + ->setAncestorClass("BuildStepImplementation") + ->setConcreteOnly(true) + ->selectAndLoadSymbols(); + return ipull($symbols, 'name'); + } + /** * The name of the implementation. */ abstract public function getName(); /** - * The description of the implementation. + * The generic description of the implementation. + */ + public function getGenericDescription() { + return ''; + } + + /** + * The description of the implementation, based on the current settings. */ public function getDescription() { return ''; @@ -24,21 +43,42 @@ abstract class BuildStepImplementation { /** * Gets the settings for this build step. */ - protected function getSettings() { + public function getSettings() { return $this->settings; } + /** + * Validate the current settings of this build step. + */ + public function validate() { + return true; + } + /** * Loads the settings for this build step implementation from the build step. */ public final function loadSettings(HarbormasterBuildStep $build_step) { $this->settings = array(); + $this->validateSettingDefinitions(); foreach ($this->getSettingDefinitions() as $name => $opt) { $this->settings[$name] = $build_step->getDetail($name); } return $this->settings; } + /** + * Validates that the setting definitions for this implementation are valid. + */ + public final function validateSettingDefinitions() { + foreach ($this->getSettingDefinitions() as $name => $opt) { + if (!isset($opt['type'])) { + throw new Exception( + 'Setting definition \''.$name. + '\' is missing type requirement.'); + } + } + } + /** * Return an array of settings for this step implementation. */ diff --git a/src/applications/harbormaster/step/SleepBuildStepImplementation.php b/src/applications/harbormaster/step/SleepBuildStepImplementation.php index bf16b22d23..e80f559e29 100644 --- a/src/applications/harbormaster/step/SleepBuildStepImplementation.php +++ b/src/applications/harbormaster/step/SleepBuildStepImplementation.php @@ -6,19 +6,40 @@ final class SleepBuildStepImplementation extends BuildStepImplementation { return pht('Sleep'); } - public function getDescription() { + public function getGenericDescription() { return pht('Sleep for a specified number of seconds.'); } + public function getDescription() { + $settings = $this->getSettings(); + + return pht('Sleep for %s seconds.', $settings['seconds']); + } + public function execute(HarbormasterBuild $build) { $settings = $this->getSettings(); sleep($settings['seconds']); } + public function validateSettings() { + $settings = $this->getSettings(); + + if ($settings['seconds'] === null) { + return false; + } + if (!is_int($settings['seconds'])) { + return false; + } + return true; + } + public function getSettingDefinitions() { return array( - 'seconds' => array()); + 'seconds' => array( + 'name' => 'Seconds', + 'description' => 'The number of seconds to sleep for.', + 'type' => BuildStepImplementation::SETTING_TYPE_INTEGER)); } } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index c7d5aa7181..aa8e6c4f53 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -40,6 +40,11 @@ final class HarbormasterBuild extends HarbormasterDAO */ const STATUS_FAILED = 'failed'; + /** + * The build encountered an unexpected error. + */ + const STATUS_ERROR = 'error'; + public static function initializeNewBuild(PhabricatorUser $actor) { return id(new HarbormasterBuild()) ->setBuildStatus(self::STATUS_INACTIVE); diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index 42a7e89fa9..cb9310d2aa 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php @@ -46,9 +46,16 @@ final class HarbormasterBuildStep extends HarbormasterDAO throw new Exception("No implementation set for the given step."); } - // TODO: We should look up the class in phutil's system to ensure - // that it actually extends BuildStepImplementation. + static $implementations = null; + if ($implementations === null) { + $implementations = BuildStepImplementation::getImplementations(); + } + $class = $this->className; + if (!in_array($class, $implementations)) { + throw new Exception( + "Class name '".$class."' does not extend BuildStepImplementation."); + } $implementation = newv($class, array()); $implementation->loadSettings($this); return $implementation; diff --git a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php index 347119e348..0306c18d8f 100644 --- a/src/applications/harbormaster/worker/HarbormasterBuildWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterBuildWorker.php @@ -40,6 +40,10 @@ final class HarbormasterBuildWorker extends PhabricatorWorker { // Perform the build. foreach ($steps as $step) { $implementation = $step->getStepImplementation(); + if (!$implementation->validateSettings()) { + $build->setBuildStatus(HarbormasterBuild::STATUS_ERROR); + break; + } $implementation->execute($build); if ($build->getBuildStatus() !== HarbormasterBuild::STATUS_BUILDING) { break; @@ -56,7 +60,7 @@ final class HarbormasterBuildWorker extends PhabricatorWorker { // If any exception is raised, the build is marked as a failure and // the exception is re-thrown (this ensures we don't leave builds // in an inconsistent state). - $build->setBuildStatus(HarbormasterBuild::STATUS_FAILED); + $build->setBuildStatus(HarbormasterBuild::STATUS_ERROR); $build->save(); throw $e; } From ce48375951bd82bbc5f5ce7cb29cb7f0d4afde55 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Nov 2013 15:24:58 -0800 Subject: [PATCH 7/9] Don't throw when user tries to use an empty password via HTTP auth Summary: Fixes T4064. See discussion there. Test Plan: Tried `git clone http://...` with empty password, got 403. Retried with actual password, got a clone. Reviewers: jamesr, btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4064 Differential Revision: https://secure.phabricator.com/D7508 --- .../diffusion/controller/DiffusionController.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/applications/diffusion/controller/DiffusionController.php b/src/applications/diffusion/controller/DiffusionController.php index 0d03487769..6e9ebb6293 100644 --- a/src/applications/diffusion/controller/DiffusionController.php +++ b/src/applications/diffusion/controller/DiffusionController.php @@ -8,6 +8,8 @@ abstract class DiffusionController extends PhabricatorController { $request = $this->getRequest(); $uri = $request->getRequestURI(); + $user_agent = idx($_SERVER, 'HTTP_USER_AGENT'); + // Check if this is a VCS request, e.g. from "git clone", "hg clone", or // "svn checkout". If it is, we jump off into repository serving code to // process the request. @@ -27,6 +29,8 @@ abstract class DiffusionController extends PhabricatorController { // // ...to get a human-readable error. $vcs = $request->getExists('__vcs__'); + } else if (strncmp($user_agent, "git/", 4) === 0) { + $vcs = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT; } else if ($request->getExists('service')) { $service = $request->getStr('service'); // We get this initially for `info/refs`. @@ -541,6 +545,16 @@ abstract class DiffusionController extends PhabricatorController { return null; } + if (!strlen($username)) { + // No username. + return null; + } + + if (!strlen($password->openEnvelope())) { + // No password. + return null; + } + $user = id(new PhabricatorPeopleQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withUsernames(array($username)) From d5f41ef70ed00d9310732ff0ac4c777a3ba3c8e3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Nov 2013 15:25:12 -0800 Subject: [PATCH 8/9] Return HTTP 500, not HTTP 200, on exception pages Summary: Ref T4064. The response code here isn't normally relevant, but we can hit these via `git clone http://../`, etc., and it's clearly more correct to use HTTP 500. Test Plan: Added a fake `throw new Exception()` and verified I got an HTTP 500 response. Reviewers: jamesr, btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4064 Differential Revision: https://secure.phabricator.com/D7507 --- .../AphrontDefaultApplicationConfiguration.php | 2 ++ src/applications/base/controller/PhabricatorController.php | 7 ++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index c6a55ce773..0b296962f7 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -222,6 +222,7 @@ class AphrontDefaultApplicationConfiguration $response = new AphrontWebpageResponse(); $response->setContent($view->render()); + $response->setHTTPResponseCode(500); return $response; } @@ -268,6 +269,7 @@ class AphrontDefaultApplicationConfiguration $response = new AphrontDialogResponse(); $response->setDialog($dialog); + $response->setHTTPResponseCode(500); return $response; } diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 149f40ee25..5449546b0a 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -249,9 +249,10 @@ abstract class PhabricatorController extends AphrontController { $view->appendChild(hsprintf( '
%s
', $response->buildResponseString())); - $response = new AphrontWebpageResponse(); - $response->setContent($view->render()); - return $response; + $page_response = new AphrontWebpageResponse(); + $page_response->setContent($view->render()); + $page_response->setHTTPResponseCode($response->getHTTPResponseCode()); + return $page_response; } else { $response->getDialog()->setIsStandalone(true); From ebcd60eae55776b0a463720f60679818f23e1ef2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Nov 2013 15:09:08 -0800 Subject: [PATCH 9/9] Fix an issue with non-bare Git repositories and non-master branches Summary: This is a little funky but fixes an issue with Git repos that are non-bare needing "origin/" to resolve branches other than "master". Eventually this should get cleaned up. Test Plan: Reporting user verified this fixed their issue. Auditors: btrahan --- .../diffusion/request/DiffusionGitRequest.php | 8 ++++++-- src/applications/diffusion/request/DiffusionRequest.php | 6 +++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/src/applications/diffusion/request/DiffusionGitRequest.php b/src/applications/diffusion/request/DiffusionGitRequest.php index df3df74c72..2f411c6584 100644 --- a/src/applications/diffusion/request/DiffusionGitRequest.php +++ b/src/applications/diffusion/request/DiffusionGitRequest.php @@ -32,11 +32,15 @@ final class DiffusionGitRequest extends DiffusionRequest { return $this->commit; } + return $this->getResolvableBranchName($this->getBranch()); + } + + protected function getResolvableBranchName($branch) { if ($this->repository->isWorkingCopyBare()) { - return $this->getBranch(); + return $branch; } else { $remote = DiffusionBranchInformation::DEFAULT_GIT_REMOTE; - return $remote.'/'.$this->getBranch(); + return $remote.'/'.$branch; } } diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index 9640df0a2f..928cc15422 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -643,7 +643,7 @@ abstract class DiffusionRequest { } if ($this->getSupportsBranches()) { - $branch = $this->getBranch(); + $branch = $this->getResolvableBranchName($this->getBranch()); } else { $branch = 'HEAD'; } @@ -660,6 +660,10 @@ abstract class DiffusionRequest { return $this->stableCommitName; } + protected function getResolvableBranchName($branch) { + return $branch; + } + private function resolveRefs(array $refs) { if ($this->shouldInitFromConduit()) { return DiffusionQuery::callConduitWithDiffusionRequest(