From 9d0891c7e1ce69b44620ec81c20be3601bf82ff1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 23 Apr 2016 05:15:55 -0700 Subject: [PATCH 01/32] Correct Aphlict documentation for Nginx proxying Summary: Fixes T10857. This documentation did not accurately reflect proper configuration: in the Aphlict config, SSL is inferred from the presence of `ssl.*` configuration. Test Plan: Read documentation. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10857 Differential Revision: https://secure.phabricator.com/D15787 --- src/docs/user/configuration/notifications.diviner | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/docs/user/configuration/notifications.diviner b/src/docs/user/configuration/notifications.diviner index c5cd1f7974..8e70315460 100644 --- a/src/docs/user/configuration/notifications.diviner +++ b/src/docs/user/configuration/notifications.diviner @@ -273,7 +273,7 @@ You do not need to adjust the `"admin"` server. **Aphlict**: Your Aphlict configuration should make these adjustments to the `"client"` server: - - The `protocol` should be `"http"`: `nginx` will send plain HTTP traffic + - Do not specify any `ssl.*` options: `nginx` will send plain HTTP traffic to Aphlict. - Optionally, you can `listen` on `127.0.0.1` instead of `0.0.0.0`, because the server will no longer receive external traffic. From aa9395e38fc9df5c4c3a061e4040e903bf37c383 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 24 Apr 2016 06:33:59 -0700 Subject: [PATCH 02/32] Fix bad variable causing `aphlict` to fail to start with no "logs" config Summary: Fixes T10863. See that task for discussion. Test Plan: - Configured `aphlict` with no "logs". - Started `aphlict`. - Before change: exception. - After change: worked. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10863 Differential Revision: https://secure.phabricator.com/D15788 --- .../aphlict/management/PhabricatorAphlictManagementWorkflow.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php b/src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php index 3f9620d3be..b799d0ef90 100644 --- a/src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php +++ b/src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php @@ -274,7 +274,7 @@ abstract class PhabricatorAphlictManagementWorkflow $pid_path = $this->getPIDPath(); try { - $dir = dirname($path); + $dir = dirname($pid_path); if (!Filesystem::pathExists($dir)) { Filesystem::createDirectory($dir, 0755, true); } From 623ed1f4349bd20401d1bb2f1b698878ed916ec1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Apr 2016 05:36:50 -0700 Subject: [PATCH 03/32] Include directory-ownership note in `sshd` setup instructions Summary: Fixes T9560. We suggest a root-owned location, but users who choose their own location instead can run into trouble. Test Plan: - Changed parent directory to have an non-root owner, verified that `ssh` no longer worked. - Changed parent directory back to a root owner, verified `ssh` worked again. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9560 Differential Revision: https://secure.phabricator.com/D15794 --- src/docs/user/userguide/diffusion_hosting.diviner | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/docs/user/userguide/diffusion_hosting.diviner b/src/docs/user/userguide/diffusion_hosting.diviner index f4baddf5a5..60360c1af8 100644 --- a/src/docs/user/userguide/diffusion_hosting.diviner +++ b/src/docs/user/userguide/diffusion_hosting.diviner @@ -200,10 +200,16 @@ There are three major steps: **Create `phabricator-ssh-hook.sh`**: Copy the template in `phabricator/resources/sshd/phabricator-ssh-hook.sh` to somewhere like `/usr/libexec/phabricator-ssh-hook.sh` and edit it to have the correct -settings. Then make it owned by `root` and restrict editing: +settings. - sudo chown root /path/to/phabricator-ssh-hook.sh - sudo chmod 755 /path/to/phabricator-ssh-hook.sh +Both the script itself **and** the parent directory the script resides in must +be owned by `root`, and the script must have `755` permissions: + +``` +$ sudo chown root /path/to/somewhere/ +$ sudo chown root /path/to/somewhere/phabricator-ssh-hook.sh +$ sudo chmod 755 /path/to/somewhere/phabricator-ssh-hook.sh +``` If you don't do this, `sshd` will refuse to execute the hook. From 1c0980a26a86962fb6442727a3119247fd34110a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Apr 2016 05:02:48 -0700 Subject: [PATCH 04/32] Fix two issues with Remarkup in Pholio Summary: Fixes T10865. - Mock descriptions did not markup. - Image descriptions did not get a proper container `
`. Test Plan: - Created a mock with remarkup in the mock description and in an image description. - Viewed mock detail. - Saw list styles render properly in both mock description and image description. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10865 Differential Revision: https://secure.phabricator.com/D15793 --- .../pholio/controller/PholioMockViewController.php | 5 +++-- .../pholio/view/PholioMockImagesView.php | 12 +++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/applications/pholio/controller/PholioMockViewController.php b/src/applications/pholio/controller/PholioMockViewController.php index 565d2d6cb4..906454c1f3 100644 --- a/src/applications/pholio/controller/PholioMockViewController.php +++ b/src/applications/pholio/controller/PholioMockViewController.php @@ -173,14 +173,15 @@ final class PholioMockViewController extends PholioController { } private function buildDescriptionView(PholioMock $mock) { - $viewer = $this->getViewer(); + $properties = id(new PHUIPropertyListView()) ->setUser($viewer); $description = $mock->getDescription(); if (strlen($description)) { - $properties->addImageContent($description); + $properties->addTextContent( + new PHUIRemarkupView($viewer, $description)); return id(new PHUIObjectBoxView()) ->setHeaderText(pht('Mock Description')) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) diff --git a/src/applications/pholio/view/PholioMockImagesView.php b/src/applications/pholio/view/PholioMockImagesView.php index d50dfb3b3a..6c92f1d21f 100644 --- a/src/applications/pholio/view/PholioMockImagesView.php +++ b/src/applications/pholio/view/PholioMockImagesView.php @@ -92,6 +92,16 @@ final class PholioMockImagesView extends AphrontView { $current_set++; } + $description = $engine->getOutput($image, 'default'); + if (strlen($description)) { + $description = phutil_tag( + 'div', + array( + 'class' => 'phabricator-remarkup', + ), + $description); + } + $history_uri = '/pholio/image/history/'.$image->getID().'/'; $images[] = array( 'id' => $image->getID(), @@ -105,7 +115,7 @@ final class PholioMockImagesView extends AphrontView { 'width' => $x, 'height' => $y, 'title' => $image->getName(), - 'descriptionMarkup' => $engine->getOutput($image, 'default'), + 'descriptionMarkup' => $description, 'isObsolete' => (bool)$image->getIsObsolete(), 'isImage' => $file->isViewableImage(), 'isViewable' => $file->isViewableInBrowser(), From dc75b4bd0602afcd9c75484add4515eb63e0676a Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 24 Apr 2016 09:04:27 -0700 Subject: [PATCH 05/32] Move all cluster locking logic to a separate class Summary: Ref T10860. This doesn't change anything, it just separates all this stuff out of `PhabricatorRepository` since I'm planning to add a bit more state to it and it's already pretty big and fairly separable. Test Plan: Pulled, pushed, browsed Diffusion. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10860 Differential Revision: https://secure.phabricator.com/D15790 --- src/__phutil_library_map__.php | 2 + .../DiffusionQueryCommitsConduitAPIMethod.php | 10 +- .../DiffusionQueryConduitAPIMethod.php | 8 +- .../controller/DiffusionServeController.php | 10 +- .../DiffusionRepositoryClusterEngine.php | 435 ++++++++++++++++++ .../DiffusionGitReceivePackSSHWorkflow.php | 11 +- .../ssh/DiffusionGitUploadPackSSHWorkflow.php | 6 +- .../editor/PhabricatorRepositoryEditor.php | 5 +- .../PhabricatorRepositoryPullEngine.php | 6 +- .../storage/PhabricatorRepository.php | 379 +-------------- 10 files changed, 479 insertions(+), 393 deletions(-) create mode 100644 src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 65eb7f6c20..d20a7b4e14 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -745,6 +745,7 @@ phutil_register_library_map(array( 'DiffusionRenameHistoryQuery' => 'applications/diffusion/query/DiffusionRenameHistoryQuery.php', 'DiffusionRepositoryBasicsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php', 'DiffusionRepositoryByIDRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php', + 'DiffusionRepositoryClusterEngine' => 'applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php', 'DiffusionRepositoryClusterManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryClusterManagementPanel.php', 'DiffusionRepositoryController' => 'applications/diffusion/controller/DiffusionRepositoryController.php', 'DiffusionRepositoryCreateController' => 'applications/diffusion/controller/DiffusionRepositoryCreateController.php', @@ -4953,6 +4954,7 @@ phutil_register_library_map(array( 'DiffusionRenameHistoryQuery' => 'Phobject', 'DiffusionRepositoryBasicsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryByIDRemarkupRule' => 'PhabricatorObjectRemarkupRule', + 'DiffusionRepositoryClusterEngine' => 'Phobject', 'DiffusionRepositoryClusterManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryController' => 'DiffusionController', 'DiffusionRepositoryCreateController' => 'DiffusionRepositoryEditController', diff --git a/src/applications/diffusion/conduit/DiffusionQueryCommitsConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionQueryCommitsConduitAPIMethod.php index bb205aad32..72a03f9579 100644 --- a/src/applications/diffusion/conduit/DiffusionQueryCommitsConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionQueryCommitsConduitAPIMethod.php @@ -29,21 +29,25 @@ final class DiffusionQueryCommitsConduitAPIMethod protected function execute(ConduitAPIRequest $request) { $need_messages = $request->getValue('needMessages'); $bypass_cache = $request->getValue('bypassCache'); + $viewer = $request->getUser(); $query = id(new DiffusionCommitQuery()) - ->setViewer($request->getUser()) + ->setViewer($viewer) ->needCommitData(true); $repository_phid = $request->getValue('repositoryPHID'); if ($repository_phid) { $repository = id(new PhabricatorRepositoryQuery()) - ->setViewer($request->getUser()) + ->setViewer($viewer) ->withPHIDs(array($repository_phid)) ->executeOne(); if ($repository) { $query->withRepository($repository); if ($bypass_cache) { - $repository->synchronizeWorkingCopyBeforeRead(); + id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository) + ->synchronizeWorkingCopyBeforeRead(); } } } diff --git a/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php index 7a7d81f4e0..716824f9f8 100644 --- a/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php @@ -124,10 +124,11 @@ abstract class DiffusionQueryConduitAPIMethod // to prevent infinite recursion. $is_cluster_request = $request->getIsClusterRequest(); + $viewer = $request->getUser(); $repository = $drequest->getRepository(); $client = $repository->newConduitClient( - $request->getUser(), + $viewer, $is_cluster_request); if ($client) { // We're proxying, so just make an intracluster call. @@ -149,7 +150,10 @@ abstract class DiffusionQueryConduitAPIMethod // fetching the most up-to-date data? Synchronization can be slow, and a // lot of web reads are probably fine if they're a few seconds out of // date. - $repository->synchronizeWorkingCopyBeforeRead(); + id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository) + ->synchronizeWorkingCopyBeforeRead(); return $this->getResult($request); } diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php index 32c4936484..f1703655fa 100644 --- a/src/applications/diffusion/controller/DiffusionServeController.php +++ b/src/applications/diffusion/controller/DiffusionServeController.php @@ -540,12 +540,16 @@ final class DiffusionServeController extends DiffusionController { $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $cluster_engine = id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository); + $did_write_lock = false; if ($this->isReadOnlyRequest($repository)) { - $repository->synchronizeWorkingCopyBeforeRead(); + $cluster_engine->synchronizeWorkingCopyBeforeRead(); } else { $did_write_lock = true; - $repository->synchronizeWorkingCopyBeforeWrite($viewer); + $cluster_engine->synchronizeWorkingCopyBeforeWrite(); } $caught = null; @@ -559,7 +563,7 @@ final class DiffusionServeController extends DiffusionController { } if ($did_write_lock) { - $repository->synchronizeWorkingCopyAfterWrite(); + $cluster_engine->synchronizeWorkingCopyAfterWrite(); } unset($unguarded); diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php new file mode 100644 index 0000000000..2ed0a15f18 --- /dev/null +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php @@ -0,0 +1,435 @@ +repository = $repository; + return $this; + } + + public function getRepository() { + return $this->repository; + } + + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + +/* -( Cluster Synchronization )-------------------------------------------- */ + + + /** + * Synchronize repository version information after creating a repository. + * + * This initializes working copy versions for all currently bound devices to + * 0, so that we don't get stuck making an ambiguous choice about which + * devices are leaders when we later synchronize before a read. + * + * @task sync + */ + public function synchronizeWorkingCopyAfterCreation() { + if (!$this->shouldEnableSynchronization()) { + return; + } + + $repository = $this->getRepository(); + $repository_phid = $repository->getPHID(); + + $service = $repository->loadAlmanacService(); + if (!$service) { + throw new Exception(pht('Failed to load repository cluster service.')); + } + + $bindings = $service->getActiveBindings(); + foreach ($bindings as $binding) { + PhabricatorRepositoryWorkingCopyVersion::updateVersion( + $repository_phid, + $binding->getDevicePHID(), + 0); + } + + return $this; + } + + + /** + * @task sync + */ + public function synchronizeWorkingCopyBeforeRead() { + if (!$this->shouldEnableSynchronization()) { + return; + } + + $repository = $this->getRepository(); + $repository_phid = $repository->getPHID(); + + $device = AlmanacKeys::getLiveDevice(); + $device_phid = $device->getPHID(); + + $read_lock = PhabricatorRepositoryWorkingCopyVersion::getReadLock( + $repository_phid, + $device_phid); + + // TODO: Raise a more useful exception if we fail to grab this lock. + $read_lock->lock(phutil_units('2 minutes in seconds')); + + $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions( + $repository_phid); + $versions = mpull($versions, null, 'getDevicePHID'); + + $this_version = idx($versions, $device_phid); + if ($this_version) { + $this_version = (int)$this_version->getRepositoryVersion(); + } else { + $this_version = -1; + } + + if ($versions) { + // This is the normal case, where we have some version information and + // can identify which nodes are leaders. If the current node is not a + // leader, we want to fetch from a leader and then update our version. + + $max_version = (int)max(mpull($versions, 'getRepositoryVersion')); + if ($max_version > $this_version) { + $fetchable = array(); + foreach ($versions as $version) { + if ($version->getRepositoryVersion() == $max_version) { + $fetchable[] = $version->getDevicePHID(); + } + } + + $this->synchronizeWorkingCopyFromDevices($fetchable); + + PhabricatorRepositoryWorkingCopyVersion::updateVersion( + $repository_phid, + $device_phid, + $max_version); + } + + $result_version = $max_version; + } else { + // If no version records exist yet, we need to be careful, because we + // can not tell which nodes are leaders. + + // There might be several nodes with arbitrary existing data, and we have + // no way to tell which one has the "right" data. If we pick wrong, we + // might erase some or all of the data in the repository. + + // Since this is dangeorus, we refuse to guess unless there is only one + // device. If we're the only device in the group, we obviously must be + // a leader. + + $service = $repository->loadAlmanacService(); + if (!$service) { + throw new Exception(pht('Failed to load repository cluster service.')); + } + + $bindings = $service->getActiveBindings(); + $device_map = array(); + foreach ($bindings as $binding) { + $device_map[$binding->getDevicePHID()] = true; + } + + if (count($device_map) > 1) { + throw new Exception( + pht( + 'Repository "%s" exists on more than one device, but no device '. + 'has any repository version information. Phabricator can not '. + 'guess which copy of the existing data is authoritative. Remove '. + 'all but one device from service to mark the remaining device '. + 'as the authority.', + $repository->getDisplayName())); + } + + if (empty($device_map[$device->getPHID()])) { + throw new Exception( + pht( + 'Repository "%s" is being synchronized on device "%s", but '. + 'this device is not bound to the corresponding cluster '. + 'service ("%s").', + $repository->getDisplayName(), + $device->getName(), + $service->getName())); + } + + // The current device is the only device in service, so it must be a + // leader. We can safely have any future nodes which come online read + // from it. + PhabricatorRepositoryWorkingCopyVersion::updateVersion( + $repository_phid, + $device_phid, + 0); + + $result_version = 0; + } + + $read_lock->unlock(); + + return $result_version; + } + + + /** + * @task sync + */ + public function synchronizeWorkingCopyBeforeWrite() { + if (!$this->shouldEnableSynchronization()) { + return; + } + + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + + $repository_phid = $repository->getPHID(); + + $device = AlmanacKeys::getLiveDevice(); + $device_phid = $device->getPHID(); + + $write_lock = PhabricatorRepositoryWorkingCopyVersion::getWriteLock( + $repository_phid); + + // TODO: Raise a more useful exception if we fail to grab this lock. + $write_lock->lock(phutil_units('2 minutes in seconds')); + + $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions( + $repository_phid); + foreach ($versions as $version) { + if (!$version->getIsWriting()) { + continue; + } + + throw new Exception( + pht( + 'An previous write to this repository was interrupted; refusing '. + 'new writes. This issue resolves operator intervention to resolve, '. + 'see "Write Interruptions" in the "Cluster: Repositories" in the '. + 'documentation for instructions.')); + } + + try { + $max_version = $this->synchronizeWorkingCopyBeforeRead(); + } catch (Exception $ex) { + $write_lock->unlock(); + throw $ex; + } + + PhabricatorRepositoryWorkingCopyVersion::willWrite( + $repository_phid, + $device_phid, + array( + 'userPHID' => $viewer->getPHID(), + 'epoch' => PhabricatorTime::getNow(), + 'devicePHID' => $device_phid, + )); + + $this->clusterWriteVersion = $max_version; + $this->clusterWriteLock = $write_lock; + } + + + /** + * @task sync + */ + public function synchronizeWorkingCopyAfterWrite() { + if (!$this->shouldEnableSynchronization()) { + return; + } + + if (!$this->clusterWriteLock) { + throw new Exception( + pht( + 'Trying to synchronize after write, but not holding a write '. + 'lock!')); + } + + $repository = $this->getRepository(); + $repository_phid = $repository->getPHID(); + + $device = AlmanacKeys::getLiveDevice(); + $device_phid = $device->getPHID(); + + // NOTE: This means we're still bumping the version when pushes fail. We + // could select only un-rejected events instead to bump a little less + // often. + + $new_log = id(new PhabricatorRepositoryPushEventQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withRepositoryPHIDs(array($repository_phid)) + ->setLimit(1) + ->executeOne(); + + $old_version = $this->clusterWriteVersion; + if ($new_log) { + $new_version = $new_log->getID(); + } else { + $new_version = $old_version; + } + + PhabricatorRepositoryWorkingCopyVersion::didWrite( + $repository_phid, + $device_phid, + $this->clusterWriteVersion, + $new_log->getID()); + + $this->clusterWriteLock->unlock(); + $this->clusterWriteLock = null; + } + + +/* -( Internals )---------------------------------------------------------- */ + + + /** + * @task internal + */ + private function shouldEnableSynchronization() { + $repository = $this->getRepository(); + + $service_phid = $repository->getAlmanacServicePHID(); + if (!$service_phid) { + return false; + } + + // TODO: For now, this is only supported for Git. + if (!$repository->isGit()) { + return false; + } + + // TODO: It may eventually make sense to try to version and synchronize + // observed repositories (so that daemons don't do reads against out-of + // date hosts), but don't bother for now. + if (!$repository->isHosted()) { + return false; + } + + $device = AlmanacKeys::getLiveDevice(); + if (!$device) { + return false; + } + + return true; + } + + + /** + * @task internal + */ + private function synchronizeWorkingCopyFromDevices(array $device_phids) { + $repository = $this->getRepository(); + + $service = $repository->loadAlmanacService(); + if (!$service) { + throw new Exception(pht('Failed to load repository cluster service.')); + } + + $device_map = array_fuse($device_phids); + $bindings = $service->getActiveBindings(); + + $fetchable = array(); + foreach ($bindings as $binding) { + // We can't fetch from nodes which don't have the newest version. + $device_phid = $binding->getDevicePHID(); + if (empty($device_map[$device_phid])) { + continue; + } + + // TODO: For now, only fetch over SSH. We could support fetching over + // HTTP eventually. + if ($binding->getAlmanacPropertyValue('protocol') != 'ssh') { + continue; + } + + $fetchable[] = $binding; + } + + if (!$fetchable) { + throw new Exception( + pht( + 'Leader lost: no up-to-date nodes in repository cluster are '. + 'fetchable.')); + } + + $caught = null; + foreach ($fetchable as $binding) { + try { + $this->synchronizeWorkingCopyFromBinding($binding); + $caught = null; + break; + } catch (Exception $ex) { + $caught = $ex; + } + } + + if ($caught) { + throw $caught; + } + } + + + /** + * @task internal + */ + private function synchronizeWorkingCopyFromBinding($binding) { + $repository = $this->getRepository(); + + $fetch_uri = $repository->getClusterRepositoryURIFromBinding($binding); + $local_path = $repository->getLocalPath(); + + if ($repository->isGit()) { + if (!Filesystem::pathExists($local_path)) { + $device = AlmanacKeys::getLiveDevice(); + throw new Exception( + pht( + 'Repository "%s" does not have a working copy on this device '. + 'yet, so it can not be synchronized. Wait for the daemons to '. + 'construct one or run `bin/repository update %s` on this host '. + '("%s") to build it explicitly.', + $repository->getDisplayName(), + $repository->getMonogram(), + $device->getName())); + } + + $argv = array( + 'fetch --prune -- %s %s', + $fetch_uri, + '+refs/*:refs/*', + ); + } else { + throw new Exception(pht('Binding sync only supported for git!')); + } + + $future = DiffusionCommandEngine::newCommandEngine($repository) + ->setArgv($argv) + ->setConnectAsDevice(true) + ->setSudoAsDaemon(true) + ->setProtocol($fetch_uri->getProtocol()) + ->newFuture(); + + $future->setCWD($local_path); + + $future->resolvex(); + } + +} diff --git a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php index f5e314f462..ad38480828 100644 --- a/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php @@ -15,19 +15,22 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { protected function executeRepositoryOperations() { $repository = $this->getRepository(); + $viewer = $this->getViewer(); // This is a write, and must have write access. $this->requireWriteAccess(); + $cluster_engine = id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository); + if ($this->shouldProxy()) { $command = $this->getProxyCommand(); $did_synchronize = false; } else { $command = csprintf('git-receive-pack %s', $repository->getLocalPath()); - $did_synchronize = true; - $viewer = $this->getUser(); - $repository->synchronizeWorkingCopyBeforeWrite($viewer); + $cluster_engine->synchronizeWorkingCopyBeforeWrite(); } $caught = null; @@ -40,7 +43,7 @@ final class DiffusionGitReceivePackSSHWorkflow extends DiffusionGitSSHWorkflow { // We've committed the write (or rejected it), so we can release the lock // without waiting for the client to receive the acknowledgement. if ($did_synchronize) { - $repository->synchronizeWorkingCopyAfterWrite(); + $cluster_engine->synchronizeWorkingCopyAfterWrite(); } if ($caught) { diff --git a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php index 1a83a1f30b..eb6a99e004 100644 --- a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php @@ -15,6 +15,7 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { protected function executeRepositoryOperations() { $repository = $this->getRepository(); + $viewer = $this->getUser(); $skip_sync = $this->shouldSkipReadSynchronization(); @@ -23,7 +24,10 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { } else { $command = csprintf('git-upload-pack -- %s', $repository->getLocalPath()); if (!$skip_sync) { - $repository->synchronizeWorkingCopyBeforeRead(); + $cluster_engine = id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository) + ->synchronizeWorkingCopyBeforeRead(); } } $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command); diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 70d904b112..9ef775fe2d 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -684,7 +684,10 @@ final class PhabricatorRepositoryEditor } if ($this->getIsNewObject()) { - $object->synchronizeWorkingCopyAfterCreation(); + id(new DiffusionRepositoryClusterEngine()) + ->setViewer($this->getActor()) + ->setRepository($object) + ->synchronizeWorkingCopyAfterCreation(); } return $xactions; diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index f589f61b9d..621c718577 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -23,6 +23,7 @@ final class PhabricatorRepositoryPullEngine public function pullRepository() { $repository = $this->getRepository(); + $viewer = PhabricatorUser::getOmnipotentUser(); $is_hg = false; $is_git = false; @@ -96,7 +97,10 @@ final class PhabricatorRepositoryPullEngine } if ($repository->isHosted()) { - $repository->synchronizeWorkingCopyBeforeRead(); + id(new DiffusionRepositoryClusterEngine()) + ->setViewer($viewer) + ->setRepository($repository) + ->synchronizeWorkingCopyBeforeRead(); if ($is_git) { $this->installGitHook(); diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 4a217d7e80..1d251cb34b 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -68,9 +68,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO private $projectPHIDs = self::ATTACHABLE; private $uris = self::ATTACHABLE; - private $clusterWriteLock; - private $clusterWriteVersion; - public static function initializeNewRepository(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) @@ -2193,379 +2190,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } -/* -( Cluster Synchronization )-------------------------------------------- */ - - - private function shouldEnableSynchronization() { - $service_phid = $this->getAlmanacServicePHID(); - if (!$service_phid) { - return false; - } - - // TODO: For now, this is only supported for Git. - if (!$this->isGit()) { - return false; - } - - // TODO: It may eventually make sense to try to version and synchronize - // observed repositories (so that daemons don't do reads against out-of - // date hosts), but don't bother for now. - if (!$this->isHosted()) { - return false; - } - - $device = AlmanacKeys::getLiveDevice(); - if (!$device) { - return false; - } - - return true; - } - - - /** - * Synchronize repository version information after creating a repository. - * - * This initializes working copy versions for all currently bound devices to - * 0, so that we don't get stuck making an ambiguous choice about which - * devices are leaders when we later synchronize before a read. - * - * @task sync - */ - public function synchronizeWorkingCopyAfterCreation() { - if (!$this->shouldEnableSynchronization()) { - return; - } - - $repository_phid = $this->getPHID(); - - $service = $this->loadAlmanacService(); - if (!$service) { - throw new Exception(pht('Failed to load repository cluster service.')); - } - - $bindings = $service->getActiveBindings(); - foreach ($bindings as $binding) { - PhabricatorRepositoryWorkingCopyVersion::updateVersion( - $repository_phid, - $binding->getDevicePHID(), - 0); - } - } - - - /** - * @task sync - */ - public function synchronizeWorkingCopyBeforeRead() { - if (!$this->shouldEnableSynchronization()) { - return; - } - - $repository_phid = $this->getPHID(); - - $device = AlmanacKeys::getLiveDevice(); - $device_phid = $device->getPHID(); - - $read_lock = PhabricatorRepositoryWorkingCopyVersion::getReadLock( - $repository_phid, - $device_phid); - - // TODO: Raise a more useful exception if we fail to grab this lock. - $read_lock->lock(phutil_units('2 minutes in seconds')); - - $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions( - $repository_phid); - $versions = mpull($versions, null, 'getDevicePHID'); - - $this_version = idx($versions, $device_phid); - if ($this_version) { - $this_version = (int)$this_version->getRepositoryVersion(); - } else { - $this_version = -1; - } - - if ($versions) { - // This is the normal case, where we have some version information and - // can identify which nodes are leaders. If the current node is not a - // leader, we want to fetch from a leader and then update our version. - - $max_version = (int)max(mpull($versions, 'getRepositoryVersion')); - if ($max_version > $this_version) { - $fetchable = array(); - foreach ($versions as $version) { - if ($version->getRepositoryVersion() == $max_version) { - $fetchable[] = $version->getDevicePHID(); - } - } - - $this->synchronizeWorkingCopyFromDevices($fetchable); - - PhabricatorRepositoryWorkingCopyVersion::updateVersion( - $repository_phid, - $device_phid, - $max_version); - } - - $result_version = $max_version; - } else { - // If no version records exist yet, we need to be careful, because we - // can not tell which nodes are leaders. - - // There might be several nodes with arbitrary existing data, and we have - // no way to tell which one has the "right" data. If we pick wrong, we - // might erase some or all of the data in the repository. - - // Since this is dangeorus, we refuse to guess unless there is only one - // device. If we're the only device in the group, we obviously must be - // a leader. - - $service = $this->loadAlmanacService(); - if (!$service) { - throw new Exception(pht('Failed to load repository cluster service.')); - } - - $bindings = $service->getActiveBindings(); - $device_map = array(); - foreach ($bindings as $binding) { - $device_map[$binding->getDevicePHID()] = true; - } - - if (count($device_map) > 1) { - throw new Exception( - pht( - 'Repository "%s" exists on more than one device, but no device '. - 'has any repository version information. Phabricator can not '. - 'guess which copy of the existing data is authoritative. Remove '. - 'all but one device from service to mark the remaining device '. - 'as the authority.', - $this->getDisplayName())); - } - - if (empty($device_map[$device->getPHID()])) { - throw new Exception( - pht( - 'Repository "%s" is being synchronized on device "%s", but '. - 'this device is not bound to the corresponding cluster '. - 'service ("%s").', - $this->getDisplayName(), - $device->getName(), - $service->getName())); - } - - // The current device is the only device in service, so it must be a - // leader. We can safely have any future nodes which come online read - // from it. - PhabricatorRepositoryWorkingCopyVersion::updateVersion( - $repository_phid, - $device_phid, - 0); - - $result_version = 0; - } - - $read_lock->unlock(); - - return $result_version; - } - - - /** - * @task sync - */ - public function synchronizeWorkingCopyBeforeWrite( - PhabricatorUser $actor) { - if (!$this->shouldEnableSynchronization()) { - return; - } - - $repository_phid = $this->getPHID(); - - $device = AlmanacKeys::getLiveDevice(); - $device_phid = $device->getPHID(); - - $write_lock = PhabricatorRepositoryWorkingCopyVersion::getWriteLock( - $repository_phid); - - // TODO: Raise a more useful exception if we fail to grab this lock. - $write_lock->lock(phutil_units('2 minutes in seconds')); - - $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions( - $repository_phid); - foreach ($versions as $version) { - if (!$version->getIsWriting()) { - continue; - } - - throw new Exception( - pht( - 'An previous write to this repository was interrupted; refusing '. - 'new writes. This issue resolves operator intervention to resolve, '. - 'see "Write Interruptions" in the "Cluster: Repositories" in the '. - 'documentation for instructions.')); - } - - try { - $max_version = $this->synchronizeWorkingCopyBeforeRead(); - } catch (Exception $ex) { - $write_lock->unlock(); - throw $ex; - } - - PhabricatorRepositoryWorkingCopyVersion::willWrite( - $repository_phid, - $device_phid, - array( - 'userPHID' => $actor->getPHID(), - 'epoch' => PhabricatorTime::getNow(), - 'devicePHID' => $device_phid, - )); - - $this->clusterWriteVersion = $max_version; - $this->clusterWriteLock = $write_lock; - } - - - /** - * @task sync - */ - public function synchronizeWorkingCopyAfterWrite() { - if (!$this->shouldEnableSynchronization()) { - return; - } - - if (!$this->clusterWriteLock) { - throw new Exception( - pht( - 'Trying to synchronize after write, but not holding a write '. - 'lock!')); - } - - $repository_phid = $this->getPHID(); - - $device = AlmanacKeys::getLiveDevice(); - $device_phid = $device->getPHID(); - - // NOTE: This means we're still bumping the version when pushes fail. We - // could select only un-rejected events instead to bump a little less - // often. - - $new_log = id(new PhabricatorRepositoryPushEventQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withRepositoryPHIDs(array($repository_phid)) - ->setLimit(1) - ->executeOne(); - - $old_version = $this->clusterWriteVersion; - if ($new_log) { - $new_version = $new_log->getID(); - } else { - $new_version = $old_version; - } - - PhabricatorRepositoryWorkingCopyVersion::didWrite( - $repository_phid, - $device_phid, - $this->clusterWriteVersion, - $new_log->getID()); - - $this->clusterWriteLock->unlock(); - $this->clusterWriteLock = null; - } - - - /** - * @task sync - */ - private function synchronizeWorkingCopyFromDevices(array $device_phids) { - $service = $this->loadAlmanacService(); - if (!$service) { - throw new Exception(pht('Failed to load repository cluster service.')); - } - - $device_map = array_fuse($device_phids); - $bindings = $service->getActiveBindings(); - - $fetchable = array(); - foreach ($bindings as $binding) { - // We can't fetch from nodes which don't have the newest version. - $device_phid = $binding->getDevicePHID(); - if (empty($device_map[$device_phid])) { - continue; - } - - // TODO: For now, only fetch over SSH. We could support fetching over - // HTTP eventually. - if ($binding->getAlmanacPropertyValue('protocol') != 'ssh') { - continue; - } - - $fetchable[] = $binding; - } - - if (!$fetchable) { - throw new Exception( - pht( - 'Leader lost: no up-to-date nodes in repository cluster are '. - 'fetchable.')); - } - - $caught = null; - foreach ($fetchable as $binding) { - try { - $this->synchronizeWorkingCopyFromBinding($binding); - $caught = null; - break; - } catch (Exception $ex) { - $caught = $ex; - } - } - - if ($caught) { - throw $caught; - } - } - - private function synchronizeWorkingCopyFromBinding($binding) { - $fetch_uri = $this->getClusterRepositoryURIFromBinding($binding); - $local_path = $this->getLocalPath(); - - if ($this->isGit()) { - if (!Filesystem::pathExists($local_path)) { - $device = AlmanacKeys::getLiveDevice(); - throw new Exception( - pht( - 'Repository "%s" does not have a working copy on this device '. - 'yet, so it can not be synchronized. Wait for the daemons to '. - 'construct one or run `bin/repository update %s` on this host '. - '("%s") to build it explicitly.', - $this->getDisplayName(), - $this->getMonogram(), - $device->getName())); - } - - $argv = array( - 'fetch --prune -- %s %s', - $fetch_uri, - '+refs/*:refs/*', - ); - } else { - throw new Exception(pht('Binding sync only supported for git!')); - } - - $future = DiffusionCommandEngine::newCommandEngine($this) - ->setArgv($argv) - ->setConnectAsDevice(true) - ->setSudoAsDaemon(true) - ->setProtocol($fetch_uri->getProtocol()) - ->newFuture(); - - $future->setCWD($local_path); - - $future->resolvex(); - } - - private function getClusterRepositoryURIFromBinding( + public function getClusterRepositoryURIFromBinding( AlmanacBinding $binding) { $protocol = $binding->getAlmanacPropertyValue('protocol'); if ($protocol === null) { @@ -2613,8 +2238,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } - - /* -( Symbols )-------------------------------------------------------------*/ public function getSymbolSources() { From d0b5dac36b8703afaaacb7b5f32f89df019591a3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 24 Apr 2016 09:49:18 -0700 Subject: [PATCH 06/32] Make cluster repositories more chatty Summary: Ref T10860. At least in Git over SSH, we can freely echo a bunch of stuff to stderr and Git will print it to the console, so we can tell users what's going on. This should make debugging, etc., easier. We could tone this down a little bit once things are more stable if it's a little too chatty. Test Plan: ``` $ echo D >> record && git commit -am D && git push [master ca5efff] D 1 file changed, 1 insertion(+) # Push received by "local001.phacility.net", forwarding to cluster host. # Waiting up to 120 second(s) for a cluster write lock... # Acquired write lock immediately. # Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"... # Acquired read lock immediately. # Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized. # Ready to receive on cluster host "local001.phacility.net". Counting objects: 3, done. Delta compression using up to 8 threads. Compressing objects: 100% (2/2), done. Writing objects: 100% (3/3), 256 bytes | 0 bytes/s, done. Total 3 (delta 1), reused 0 (delta 0) To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git 8616189..ca5efff master -> master ``` Reviewers: chad Reviewed By: chad Maniphest Tasks: T10860 Differential Revision: https://secure.phabricator.com/D15791 --- src/__phutil_library_map__.php | 6 +- .../DiffusionRepositoryClusterEngine.php | 115 +++++++++++++++++- ...ionRepositoryClusterEngineLogInterface.php | 7 ++ .../DiffusionGitReceivePackSSHWorkflow.php | 18 ++- .../diffusion/ssh/DiffusionGitSSHWorkflow.php | 8 +- .../ssh/DiffusionGitUploadPackSSHWorkflow.php | 16 +++ 6 files changed, 161 insertions(+), 9 deletions(-) create mode 100644 src/applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d20a7b4e14..161ea349bc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -746,6 +746,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryBasicsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php', 'DiffusionRepositoryByIDRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php', 'DiffusionRepositoryClusterEngine' => 'applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php', + 'DiffusionRepositoryClusterEngineLogInterface' => 'applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php', 'DiffusionRepositoryClusterManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryClusterManagementPanel.php', 'DiffusionRepositoryController' => 'applications/diffusion/controller/DiffusionRepositoryController.php', 'DiffusionRepositoryCreateController' => 'applications/diffusion/controller/DiffusionRepositoryCreateController.php', @@ -4854,7 +4855,10 @@ phutil_register_library_map(array( 'DiffusionGitReceivePackSSHWorkflow' => 'DiffusionGitSSHWorkflow', 'DiffusionGitRequest' => 'DiffusionRequest', 'DiffusionGitResponse' => 'AphrontResponse', - 'DiffusionGitSSHWorkflow' => 'DiffusionSSHWorkflow', + 'DiffusionGitSSHWorkflow' => array( + 'DiffusionSSHWorkflow', + 'DiffusionRepositoryClusterEngineLogInterface', + ), 'DiffusionGitUploadPackSSHWorkflow' => 'DiffusionGitSSHWorkflow', 'DiffusionHistoryController' => 'DiffusionController', 'DiffusionHistoryQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php index 2ed0a15f18..f388fc2ad6 100644 --- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php @@ -13,6 +13,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject { private $viewer; private $clusterWriteLock; private $clusterWriteVersion; + private $logger; /* -( Configuring Synchronization )---------------------------------------- */ @@ -36,6 +37,11 @@ final class DiffusionRepositoryClusterEngine extends Phobject { return $this->viewer; } + public function setLog(DiffusionRepositoryClusterEngineLogInterface $log) { + $this->logger = $log; + return $this; + } + /* -( Cluster Synchronization )-------------------------------------------- */ @@ -92,8 +98,36 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $repository_phid, $device_phid); - // TODO: Raise a more useful exception if we fail to grab this lock. - $read_lock->lock(phutil_units('2 minutes in seconds')); + $lock_wait = phutil_units('2 minutes in seconds'); + + $this->logLine( + pht( + 'Waiting up to %s second(s) for a cluster read lock on "%s"...', + new PhutilNumber($lock_wait), + $device->getName())); + + try { + $start = PhabricatorTime::getNow(); + $read_lock->lock($lock_wait); + $waited = (PhabricatorTime::getNow() - $start); + + if ($waited) { + $this->logLine( + pht( + 'Acquired read lock after %s second(s).', + new PhutilNumber($waited))); + } else { + $this->logLine( + pht( + 'Acquired read lock immediately.')); + } + } catch (Exception $ex) { + throw new PhutilProxyException( + pht( + 'Failed to acquire read lock after waiting %s second(s). You '. + 'may be able to retry later.', + new PhutilNumber($lock_wait))); + } $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions( $repository_phid); @@ -126,6 +160,12 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $repository_phid, $device_phid, $max_version); + } else { + $this->logLine( + pht( + 'Device "%s" is already a cluster leader and does not need '. + 'to be synchronized.', + $device->getName())); } $result_version = $max_version; @@ -210,8 +250,35 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $write_lock = PhabricatorRepositoryWorkingCopyVersion::getWriteLock( $repository_phid); - // TODO: Raise a more useful exception if we fail to grab this lock. - $write_lock->lock(phutil_units('2 minutes in seconds')); + $lock_wait = phutil_units('2 minutes in seconds'); + + $this->logLine( + pht( + 'Waiting up to %s second(s) for a cluster write lock...', + new PhutilNumber($lock_wait))); + + try { + $start = PhabricatorTime::getNow(); + $write_lock->lock($lock_wait); + $waited = (PhabricatorTime::getNow() - $start); + + if ($waited) { + $this->logLine( + pht( + 'Acquired write lock after %s second(s).', + new PhutilNumber($waited))); + } else { + $this->logLine( + pht( + 'Acquired write lock immediately.')); + } + } catch (Exception $ex) { + throw new PhutilProxyException( + pht( + 'Failed to acquire write lock after waiting %s second(s). You '. + 'may be able to retry later.', + new PhutilNumber($lock_wait))); + } $versions = PhabricatorRepositoryWorkingCopyVersion::loadVersions( $repository_phid); @@ -393,13 +460,20 @@ final class DiffusionRepositoryClusterEngine extends Phobject { */ private function synchronizeWorkingCopyFromBinding($binding) { $repository = $this->getRepository(); + $device = AlmanacKeys::getLiveDevice(); + + $this->logLine( + pht( + 'Synchronizing this device ("%s") from cluster leader ("%s") before '. + 'read.', + $device->getName(), + $binding->getDevice()->getName())); $fetch_uri = $repository->getClusterRepositoryURIFromBinding($binding); $local_path = $repository->getLocalPath(); if ($repository->isGit()) { if (!Filesystem::pathExists($local_path)) { - $device = AlmanacKeys::getLiveDevice(); throw new Exception( pht( 'Repository "%s" does not have a working copy on this device '. @@ -429,7 +503,36 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $future->setCWD($local_path); - $future->resolvex(); + try { + $future->resolvex(); + } catch (Exception $ex) { + $this->logLine( + pht( + 'Synchronization of "%s" from leader "%s" failed: %s', + $device->getName(), + $binding->getDevice()->getName(), + $ex->getMessage())); + throw $ex; + } } + + /** + * @task internal + */ + private function logLine($message) { + return $this->logText("# {$message}\n"); + } + + + /** + * @task internal + */ + private function logText($message) { + $log = $this->logger; + if ($log) { + $log->writeClusterEngineLogMessage($message); + } + return $this; + } } diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php new file mode 100644 index 0000000000..9b1fe9a506 --- /dev/null +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php @@ -0,0 +1,7 @@ +getRepository(); $viewer = $this->getViewer(); + $device = AlmanacKeys::getLiveDevice(); // This is a write, and must have write access. $this->requireWriteAccess(); $cluster_engine = id(new DiffusionRepositoryClusterEngine()) ->setViewer($viewer) - ->setRepository($repository); + ->setRepository($repository) + ->setLog($this); if ($this->shouldProxy()) { $command = $this->getProxyCommand(); $did_synchronize = false; + + if ($device) { + $this->writeClusterEngineLogMessage( + pht( + "# Push received by \"%s\", forwarding to cluster host.\n", + $device->getName())); + } } else { $command = csprintf('git-receive-pack %s', $repository->getLocalPath()); $did_synchronize = true; $cluster_engine->synchronizeWorkingCopyBeforeWrite(); + + if ($device) { + $this->writeClusterEngineLogMessage( + pht( + "# Ready to receive on cluster host \"%s\".\n", + $device->getName())); + } } $caught = null; diff --git a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php index 4857aa8aa3..79c00231c7 100644 --- a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php @@ -1,12 +1,18 @@ getArgs(); $path = head($args->getArg('dir')); diff --git a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php index eb6a99e004..33b21b2ffe 100644 --- a/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php @@ -16,18 +16,34 @@ final class DiffusionGitUploadPackSSHWorkflow extends DiffusionGitSSHWorkflow { protected function executeRepositoryOperations() { $repository = $this->getRepository(); $viewer = $this->getUser(); + $device = AlmanacKeys::getLiveDevice(); $skip_sync = $this->shouldSkipReadSynchronization(); if ($this->shouldProxy()) { $command = $this->getProxyCommand(); + + if ($device) { + $this->writeClusterEngineLogMessage( + pht( + "# Fetch received by \"%s\", forwarding to cluster host.\n", + $device->getName())); + } } else { $command = csprintf('git-upload-pack -- %s', $repository->getLocalPath()); if (!$skip_sync) { $cluster_engine = id(new DiffusionRepositoryClusterEngine()) ->setViewer($viewer) ->setRepository($repository) + ->setLog($this) ->synchronizeWorkingCopyBeforeRead(); + + if ($device) { + $this->writeClusterEngineLogMessage( + pht( + "# Cleared to fetch on cluster host \"%s\".\n", + $device->getName())); + } } } $command = PhabricatorDaemon::sudoCommandAsDaemonUser($command); From 892a9a1f07d9407b6e0ac4c1e079e779af763ec7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 24 Apr 2016 10:07:35 -0700 Subject: [PATCH 07/32] Make cluster repositories more resistant to freezing Summary: Ref T10860. This allows us to recover if the connection to the database is lost during a push. If we lose the connection to the master database during a push, we would previously freeze the repository. This is very safe, but not very operator-friendly since you have to go manually unfreeze it. We don't need to be quite this aggressive about freezing things. The repository state is still consistent after we've "upgraded" the lock by setting `isWriting = 1`, so we're actually fine even if we lost the global lock. Instead of just freezing the repository immediately, sit there in a loop waiting for the master to come back up for a few minutes. If it recovers, we can release the lock and everything will be OK again. Basically, the changes are: - If we can't release the lock at first, sit in a loop trying really hard to release it for a while. - Add a unique lock identifier so we can be certain we're only releasing //our// lock no matter what else is going on. - Do the version reads on the same connection holding the lock, so we can be sure we haven't lost the lock before we do that read. Test Plan: - Added a `sleep(10)` after accepting the write but before releasing the lock so I could run `mysqld stop` and force this issue to occur. - Pushed like this: ``` $ echo D >> record && git commit -am D && git push [master 707ecc3] D 1 file changed, 1 insertion(+) # Push received by "local001.phacility.net", forwarding to cluster host. # Waiting up to 120 second(s) for a cluster write lock... # Acquired write lock immediately. # Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"... # Acquired read lock immediately. # Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized. # Ready to receive on cluster host "local001.phacility.net". Counting objects: 3, done. Delta compression using up to 8 threads. Compressing objects: 100% (2/2), done. Writing objects: 100% (3/3), 254 bytes | 0 bytes/s, done. Total 3 (delta 1), reused 0 (delta 0) BEGIN SLEEP ``` - Here, I stopped `mysqld` from the CLI in another terminal window. ``` END SLEEP # CRITICAL. Failed to release cluster write lock! # The connection to the master database was lost while receiving the write. # This process will spend 300 more second(s) attempting to recover, then give up. ``` - Here, I started `mysqld` again. ``` # RECOVERED. Link to master database was restored. # Released cluster write lock. To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git 2cbf87c..707ecc3 master -> master ``` Reviewers: chad Reviewed By: chad Maniphest Tasks: T10860 Differential Revision: https://secure.phabricator.com/D15792 --- .../sql/autopatches/20160424.locks.1.sql | 2 + .../DiffusionRepositoryClusterEngine.php | 131 +++++++++++++++--- .../diffusion/ssh/DiffusionGitSSHWorkflow.php | 1 + .../diffusion/ssh/DiffusionSSHWorkflow.php | 15 ++ ...habricatorRepositoryWorkingCopyVersion.php | 33 +++-- .../util/PhabricatorGlobalLock.php | 43 ++++-- 6 files changed, 184 insertions(+), 41 deletions(-) create mode 100644 resources/sql/autopatches/20160424.locks.1.sql diff --git a/resources/sql/autopatches/20160424.locks.1.sql b/resources/sql/autopatches/20160424.locks.1.sql new file mode 100644 index 0000000000..0edea13689 --- /dev/null +++ b/resources/sql/autopatches/20160424.locks.1.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_workingcopyversion + ADD lockOwner VARCHAR(255) COLLATE {$COLLATE_TEXT}; diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php index f388fc2ad6..e3c70fecd9 100644 --- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php @@ -11,9 +11,11 @@ final class DiffusionRepositoryClusterEngine extends Phobject { private $repository; private $viewer; + private $logger; + private $clusterWriteLock; private $clusterWriteVersion; - private $logger; + private $clusterWriteOwner; /* -( Configuring Synchronization )---------------------------------------- */ @@ -247,9 +249,14 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $device = AlmanacKeys::getLiveDevice(); $device_phid = $device->getPHID(); + $table = new PhabricatorRepositoryWorkingCopyVersion(); + $locked_connection = $table->establishConnection('w'); + $write_lock = PhabricatorRepositoryWorkingCopyVersion::getWriteLock( $repository_phid); + $write_lock->useSpecificConnection($locked_connection); + $lock_wait = phutil_units('2 minutes in seconds'); $this->logLine( @@ -290,7 +297,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject { throw new Exception( pht( 'An previous write to this repository was interrupted; refusing '. - 'new writes. This issue resolves operator intervention to resolve, '. + 'new writes. This issue requires operator intervention to resolve, '. 'see "Write Interruptions" in the "Cluster: Repositories" in the '. 'documentation for instructions.')); } @@ -302,14 +309,20 @@ final class DiffusionRepositoryClusterEngine extends Phobject { throw $ex; } + $pid = getmypid(); + $hash = Filesystem::readRandomCharacters(12); + $this->clusterWriteOwner = "{$pid}.{$hash}"; + PhabricatorRepositoryWorkingCopyVersion::willWrite( + $locked_connection, $repository_phid, $device_phid, array( 'userPHID' => $viewer->getPHID(), 'epoch' => PhabricatorTime::getNow(), 'devicePHID' => $device_phid, - )); + ), + $this->clusterWriteOwner); $this->clusterWriteVersion = $max_version; $this->clusterWriteLock = $write_lock; @@ -337,31 +350,105 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $device = AlmanacKeys::getLiveDevice(); $device_phid = $device->getPHID(); - // NOTE: This means we're still bumping the version when pushes fail. We - // could select only un-rejected events instead to bump a little less - // often. + // It is possible that we've lost the global lock while receiving the push. + // For example, the master database may have been restarted between the + // time we acquired the global lock and now, when the push has finished. - $new_log = id(new PhabricatorRepositoryPushEventQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withRepositoryPHIDs(array($repository_phid)) - ->setLimit(1) - ->executeOne(); + // We wrote a durable lock while we were holding the the global lock, + // essentially upgrading our lock. We can still safely release this upgraded + // lock even if we're no longer holding the global lock. - $old_version = $this->clusterWriteVersion; - if ($new_log) { - $new_version = $new_log->getID(); - } else { - $new_version = $old_version; + // If we fail to release the lock, the repository will be frozen until + // an operator can figure out what happened, so we try pretty hard to + // reconnect to the database and release the lock. + + $now = PhabricatorTime::getNow(); + $duration = phutil_units('5 minutes in seconds'); + $try_until = $now + $duration; + + $did_release = false; + $already_failed = false; + while (PhabricatorTime::getNow() <= $try_until) { + try { + // NOTE: This means we're still bumping the version when pushes fail. We + // could select only un-rejected events instead to bump a little less + // often. + + $new_log = id(new PhabricatorRepositoryPushEventQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withRepositoryPHIDs(array($repository_phid)) + ->setLimit(1) + ->executeOne(); + + $old_version = $this->clusterWriteVersion; + if ($new_log) { + $new_version = $new_log->getID(); + } else { + $new_version = $old_version; + } + + PhabricatorRepositoryWorkingCopyVersion::didWrite( + $repository_phid, + $device_phid, + $this->clusterWriteVersion, + $new_log->getID(), + $this->clusterWriteOwner); + $did_release = true; + break; + } catch (AphrontConnectionQueryException $ex) { + $connection_exception = $ex; + } catch (AphrontConnectionLostQueryException $ex) { + $connection_exception = $ex; + } + + if (!$already_failed) { + $already_failed = true; + $this->logLine( + pht('CRITICAL. Failed to release cluster write lock!')); + + $this->logLine( + pht( + 'The connection to the master database was lost while receiving '. + 'the write.')); + + $this->logLine( + pht( + 'This process will spend %s more second(s) attempting to '. + 'recover, then give up.', + new PhutilNumber($duration))); + } + + sleep(1); } - PhabricatorRepositoryWorkingCopyVersion::didWrite( - $repository_phid, - $device_phid, - $this->clusterWriteVersion, - $new_log->getID()); + if ($did_release) { + if ($already_failed) { + $this->logLine( + pht('RECOVERED. Link to master database was restored.')); + } + $this->logLine(pht('Released cluster write lock.')); + } else { + throw new Exception( + pht( + 'Failed to reconnect to master database and release held write '. + 'lock ("%s") on device "%s" for repository "%s" after trying '. + 'for %s seconds(s). This repository will be frozen.', + $this->clusterWriteOwner, + $device->getName(), + $this->getDisplayName(), + new PhutilNumber($duration))); + } + + // We can continue even if we've lost this lock, everything is still + // consistent. + try { + $this->clusterWriteLock->unlock(); + } catch (Exception $ex) { + // Ignore. + } - $this->clusterWriteLock->unlock(); $this->clusterWriteLock = null; + $this->clusterWriteOwner = null; } diff --git a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php index 79c00231c7..9843ca8401 100644 --- a/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionGitSSHWorkflow.php @@ -11,6 +11,7 @@ abstract class DiffusionGitSSHWorkflow public function writeClusterEngineLogMessage($message) { parent::writeError($message); + $this->getErrorChannel()->update(); } protected function identifyRepository() { diff --git a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php index b1694de814..60b929f7c4 100644 --- a/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php +++ b/src/applications/diffusion/ssh/DiffusionSSHWorkflow.php @@ -55,6 +55,21 @@ abstract class DiffusionSSHWorkflow extends PhabricatorSSHWorkflow { return $this; } + protected function getCurrentDeviceName() { + $device = AlmanacKeys::getLiveDevice(); + if ($device) { + return $device->getName(); + } + + return php_uname('n'); + } + + protected function getTargetDeviceName() { + // TODO: This should use the correct device identity. + $uri = new PhutilURI($this->proxyURI); + return $uri->getDomain(); + } + protected function shouldProxy() { return (bool)$this->proxyURI; } diff --git a/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php b/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php index 0feeec759f..51abc70d35 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php +++ b/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php @@ -7,6 +7,7 @@ final class PhabricatorRepositoryWorkingCopyVersion protected $devicePHID; protected $repositoryVersion; protected $isWriting; + protected $lockOwner; protected $writeProperties; protected function getConfiguration() { @@ -16,6 +17,7 @@ final class PhabricatorRepositoryWorkingCopyVersion 'repositoryVersion' => 'uint32', 'isWriting' => 'bool', 'writeProperties' => 'text?', + 'lockOwner' => 'text255?', ), self::CONFIG_KEY_SCHEMA => array( 'key_workingcopy' => array( @@ -69,29 +71,33 @@ final class PhabricatorRepositoryWorkingCopyVersion * by default. */ public static function willWrite( + AphrontDatabaseConnection $locked_connection, $repository_phid, $device_phid, - array $write_properties) { + array $write_properties, + $lock_owner) { + $version = new self(); - $conn_w = $version->establishConnection('w'); $table = $version->getTableName(); queryfx( - $conn_w, + $locked_connection, 'INSERT INTO %T (repositoryPHID, devicePHID, repositoryVersion, isWriting, - writeProperties) + writeProperties, lockOwner) VALUES - (%s, %s, %d, %d, %s) + (%s, %s, %d, %d, %s, %s) ON DUPLICATE KEY UPDATE isWriting = VALUES(isWriting), - writeProperties = VALUES(writeProperties)', + writeProperties = VALUES(writeProperties), + lockOwner = VALUES(lockOwner)', $table, $repository_phid, $device_phid, 0, 1, - phutil_json_encode($write_properties)); + phutil_json_encode($write_properties), + $lock_owner); } @@ -102,7 +108,9 @@ final class PhabricatorRepositoryWorkingCopyVersion $repository_phid, $device_phid, $old_version, - $new_version) { + $new_version, + $lock_owner) { + $version = new self(); $conn_w = $version->establishConnection('w'); $table = $version->getTableName(); @@ -111,17 +119,20 @@ final class PhabricatorRepositoryWorkingCopyVersion $conn_w, 'UPDATE %T SET repositoryVersion = %d, - isWriting = 0 + isWriting = 0, + lockOwner = NULL WHERE repositoryPHID = %s AND devicePHID = %s AND repositoryVersion = %d AND - isWriting = 1', + isWriting = 1 AND + lockOwner = %s', $table, $new_version, $repository_phid, $device_phid, - $old_version); + $old_version, + $lock_owner); } diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php index 394f57d9a9..26e11d1899 100644 --- a/src/infrastructure/util/PhabricatorGlobalLock.php +++ b/src/infrastructure/util/PhabricatorGlobalLock.php @@ -29,6 +29,7 @@ final class PhabricatorGlobalLock extends PhutilLock { private $conn; + private $isExternalConnection = false; private static $pool = array(); @@ -74,6 +75,7 @@ final class PhabricatorGlobalLock extends PhutilLock { */ public function useSpecificConnection(AphrontDatabaseConnection $conn) { $this->conn = $conn; + $this->isExternalConnection = true; return $this; } @@ -109,29 +111,54 @@ final class PhabricatorGlobalLock extends PhutilLock { $max_allowed_timeout = 2147483; queryfx($conn, 'SET wait_timeout = %d', $max_allowed_timeout); + $lock_name = $this->getName(); + $result = queryfx_one( $conn, 'SELECT GET_LOCK(%s, %f)', - $this->getName(), + $lock_name, $wait); $ok = head($result); if (!$ok) { - throw new PhutilLockException($this->getName()); + throw new PhutilLockException($lock_name); } + $conn->rememberLock($lock_name); + $this->conn = $conn; } protected function doUnlock() { - queryfx( - $this->conn, - 'SELECT RELEASE_LOCK(%s)', - $this->getName()); + $lock_name = $this->getName(); + + $conn = $this->conn; + + try { + $result = queryfx_one( + $conn, + 'SELECT RELEASE_LOCK(%s)', + $lock_name); + $conn->forgetLock($lock_name); + } catch (Exception $ex) { + $result = array(null); + } + + $ok = head($result); + if (!$ok) { + // TODO: We could throw here, but then this lock doesn't get marked + // unlocked and we throw again later when exiting. It also doesn't + // particularly matter for any current applications. For now, just + // swallow the error. + } - $this->conn->close(); - self::$pool[] = $this->conn; $this->conn = null; + $this->isExternalConnection = false; + + if (!$this->isExternalConnection) { + $conn->close(); + self::$pool[] = $conn; + } } } From 550a82d438f72477f7efc39b65906c2e7ad09a28 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Apr 2016 12:22:25 -0700 Subject: [PATCH 08/32] Fix two minor formatting issues with `bin/repository move-paths` Summary: This gets over-escaped instead of bolded right now, but I only ever hit it when exporting/importing and never both cleaning it up. Test Plan: Ran `bin/repository move-paths`, saw bolded "Move" instead of ANSI escape sequences. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D15797 --- .../PhabricatorRepositoryManagementMovePathsWorkflow.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php index 99ad42f2ba..f0146847ae 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementMovePathsWorkflow.php @@ -66,7 +66,7 @@ final class PhabricatorRepositoryManagementMovePathsWorkflow } else { $dst = $to.substr($src, strlen($from)); - $row['action'] = phutil_console_format('**%s**', pht('Move')); + $row['action'] = tsprintf('**%s**', pht('Move')); $row['dst'] = $dst; $row['move'] = true; $any_changes = true; @@ -94,7 +94,7 @@ final class PhabricatorRepositoryManagementMovePathsWorkflow ->addColumn( 'dst', array( - 'title' => pht('dst'), + 'title' => pht('Dst'), )) ->setBorders(true); From 2c870bad8688544d356cb603486390250f218d8c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 22 Apr 2016 06:09:31 -0700 Subject: [PATCH 09/32] Document how to register cluster devices with Almanac Summary: Ref T4292. This is a required step in configuring a cluster: document and explain it. Previously `bin/almanac register` could //also// add and trust keys. I've removed this capability since I think it's needless and complicated. If there's some real use for it eventually, we could add a `bin/almanac add-key` or whatever. The workflow is simpler and has better guard rails that point you in the correct direction now. Test Plan: - Read documentation. - Ran `bin/almanac` with various good/bad flags. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4292 Differential Revision: https://secure.phabricator.com/D15795 --- .../AlmanacManagementRegisterWorkflow.php | 119 ++++----- src/docs/user/cluster/cluster_devices.diviner | 242 ++++++++++++++++++ 2 files changed, 298 insertions(+), 63 deletions(-) create mode 100644 src/docs/user/cluster/cluster_devices.diviner diff --git a/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php b/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php index 6df7369ba2..0493068eb4 100644 --- a/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php +++ b/src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php @@ -19,13 +19,6 @@ final class AlmanacManagementRegisterWorkflow 'param' => 'key', 'help' => pht('Path to a private key for the host.'), ), - array( - 'name' => 'allow-key-reuse', - 'help' => pht( - 'Register even if another host is already registered with this '. - 'keypair. This is an advanced featuer which allows a pool of '. - 'devices to share credentials.'), - ), array( 'name' => 'identify-as', 'param' => 'name', @@ -36,13 +29,13 @@ final class AlmanacManagementRegisterWorkflow array( 'name' => 'force', 'help' => pht( - 'Register this host even if keys already exist.'), + 'Register this host even if keys already exist on disk.'), ), )); } public function execute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); + $viewer = $this->getViewer(); $device_name = $args->getArg('device'); if (!strlen($device_name)) { @@ -51,7 +44,7 @@ final class AlmanacManagementRegisterWorkflow } $device = id(new AlmanacDeviceQuery()) - ->setViewer($this->getViewer()) + ->setViewer($viewer) ->withNames(array($device_name)) ->executeOne(); if (!$device) { @@ -59,6 +52,23 @@ final class AlmanacManagementRegisterWorkflow pht('No such device "%s" exists!', $device_name)); } + $identify_as = $args->getArg('identify-as'); + + $raw_device = $device_name; + if (strlen($identify_as)) { + $raw_device = $identify_as; + } + + $identity_device = id(new AlmanacDeviceQuery()) + ->setViewer($viewer) + ->withNames(array($raw_device)) + ->executeOne(); + if (!$identity_device) { + throw new PhutilArgumentUsageException( + pht( + 'No such device "%s" exists!', $raw_device)); + } + $private_key_path = $args->getArg('private-key'); if (!strlen($private_key_path)) { throw new PhutilArgumentUsageException( @@ -67,7 +77,7 @@ final class AlmanacManagementRegisterWorkflow if (!Filesystem::pathExists($private_key_path)) { throw new PhutilArgumentUsageException( - pht('Private key "%s" does not exist!', $private_key_path)); + pht('No private key exists at path "%s"!', $private_key_path)); } $raw_private_key = Filesystem::readFile($private_key_path); @@ -85,8 +95,8 @@ final class AlmanacManagementRegisterWorkflow if ($err) { throw new PhutilArgumentUsageException( pht( - 'Unable to change ownership of a file to daemon user "%s". Run '. - 'this command as %s or root.', + 'Unable to change ownership of an identity file to daemon user '. + '"%s". Run this command as %s or root.', $phd_user, $phd_user)); } @@ -133,43 +143,39 @@ final class AlmanacManagementRegisterWorkflow ->withKeys(array($key_object)) ->executeOne(); - if ($public_key) { - if ($public_key->getObjectPHID() !== $device->getPHID()) { - throw new PhutilArgumentUsageException( - pht( - 'The public key corresponding to the given private key is '. - 'already associated with an object other than the specified '. - 'device. You can not use a single private key to identify '. - 'multiple devices or users.')); - } else if (!$public_key->getIsTrusted()) { - throw new PhutilArgumentUsageException( - pht( - 'The public key corresponding to the given private key is '. - 'already associated with the device, but is not trusted. '. - 'Registering this key would trust the other entities which '. - 'hold it. Use a unique key, or explicitly enable trust for the '. - 'current key.')); - } else if (!$args->getArg('allow-key-reuse')) { - throw new PhutilArgumentUsageException( - pht( - 'The public key corresponding to the given private key is '. - 'already associated with the device. If you do not want to '. - 'use a unique key, use --allow-key-reuse to permit '. - 'reassociation.')); - } - } else { - $public_key = id(new PhabricatorAuthSSHKey()) - ->setObjectPHID($device->getPHID()) - ->attachObject($device) - ->setName($device->getSSHKeyDefaultName()) - ->setKeyType($key_object->getType()) - ->setKeyBody($key_object->getBody()) - ->setKeyComment(pht('Registered')) - ->setIsTrusted(1); + if (!$public_key) { + throw new PhutilArgumentUsageException( + pht( + 'The public key corresponding to the given private key is not '. + 'yet known to Phabricator. Associate the public key with an '. + 'Almanac device in the web interface before registering hosts '. + 'with it.')); } + if ($public_key->getObjectPHID() !== $device->getPHID()) { + $public_phid = $public_key->getObjectPHID(); + $public_handles = $viewer->loadHandles(array($public_phid)); + $public_handle = $public_handles[$public_phid]; - $console->writeOut( + throw new PhutilArgumentUsageException( + pht( + 'The public key corresponding to the given private key is already '. + 'associated with an object ("%s") other than the specified '. + 'device ("%s"). You can not use a single private key to identify '. + 'multiple devices or users.', + $public_handle->getFullName(), + $device->getName())); + } + + if (!$public_key->getIsTrusted()) { + throw new PhutilArgumentUsageException( + pht( + 'The public key corresponding to the given private key is '. + 'properly associated with the device, but is not yet trusted. '. + 'Trust this key before registering devices with it.')); + } + + echo tsprintf( "%s\n", pht('Installing public key...')); @@ -179,18 +185,12 @@ final class AlmanacManagementRegisterWorkflow Filesystem::writeFile($tmp_public, $raw_public_key); execx('mv -f %s %s', $tmp_public, $stored_public_path); - $console->writeOut( + echo tsprintf( "%s\n", pht('Installing private key...')); execx('mv -f %s %s', $tmp_private, $stored_private_path); - $raw_device = $device_name; - $identify_as = $args->getArg('identify-as'); - if (strlen($identify_as)) { - $raw_device = $identify_as; - } - - $console->writeOut( + echo tsprintf( "%s\n", pht('Installing device %s...', $raw_device)); @@ -202,14 +202,7 @@ final class AlmanacManagementRegisterWorkflow Filesystem::writeFile($tmp_device, $raw_device); execx('mv -f %s %s', $tmp_device, $stored_device_path); - if (!$public_key->getID()) { - $console->writeOut( - "%s\n", - pht('Registering device key...')); - $public_key->save(); - } - - $console->writeOut( + echo tsprintf( "** %s ** %s\n", pht('HOST REGISTERED'), pht( diff --git a/src/docs/user/cluster/cluster_devices.diviner b/src/docs/user/cluster/cluster_devices.diviner new file mode 100644 index 0000000000..6fb03cec24 --- /dev/null +++ b/src/docs/user/cluster/cluster_devices.diviner @@ -0,0 +1,242 @@ +@title Cluster: Devices +@group cluster + +Guide to configuring hosts to act as cluster devices. + +Cluster Context +=============== + +This document describes a step in configuring Phabricator to run on +multiple hosts in a cluster configuration. This is an advanced feature. For +more information on clustering, see @{article:Clustering Introduction}. + +In this context, device configuration is mostly relevant to configuring +repository services in a cluster. You can find more details about this in +@{article:Cluster: Repositories}. + + +Overview +======== + +Some cluster services need to be able to authenticate themselves and interact +with other services. For example, two repository hosts holding copies of the +same repository must be able to fetch changes from one another, even if the +repository is private. + +Within a cluster, devices authenticate using SSH keys. Some operations happen +over SSH (using keys in a normal way, as you would when running `ssh` from the +command line), while others happen over HTTP (using SSH keys to sign requests). + +Before hosts can authenticate to one another, you need to configure the +credentials so other devices know the keys can be trusted. Beyond establishing +trust, this configuration will establish //device identity//, so each host +knows which device it is explicitly. + +Today, this is primarily necessary when configuring repository clusters. + + +Using Almanac +============= + +The tool Phabricator uses to manage cluster devices is the **Almanac** +application, and most configuration will occur through the application's web +UI. If you are not familiar with it, see @{article:Almanac User Guide} first. +This document assumes you are familiar with Almanac concepts. + + +What Lies Ahead +=============== + +Here's a brief overview of the steps required to register cluster devices. The +remainder of this document walks through these points in more detail. + + - Create an Almanac device record for each device. + - Generate, add, and trust SSH keys if necessary. + - Install Phabricator on the host. + - Use `bin/almanac register` from the host to register it as a device. + +See below for guidance on each of these steps. + + +Individual vs Shared Keys +========================= + +Before getting started, you should choose how you plan to manage device SSH +keys. Trust and device identity are handled separately, and there are two ways +to set up SSH keys so that devices can authenticate with one another: + + - you can generate a unique SSH key for each device; or + - you can generate one SSH key and share it across multiple devices. + +Using **unique keys** allows the tools to do some more sanity/safety checks and +makes it a bit more difficult to misconfigure things, but you'll have to do +more work managing the actual keys. This may be a better choice if you are +setting up a small cluster (2-3 devices) for the first time. + +Using **shared keys** makes key management easier but safety checks won't be +able to catch a few kinds of mistakes. This may be a better choice if you are +setting up a larger cluster, plan to expand the cluster later, or have +experience with Phabricator clustering. + +Because all cluster keys are all-powerful, there is no material difference +between these methods from a security or trust viewpoint. Unique keys are just +potentially easier to administrate at small scales, while shared keys are +easier at larger scales. + + +Create Almanac Device Records +============================= + +For each host you plan to make part of a Phabricator cluster, go to the +{nav Almanac} application and create a **device** record. For guidance on this +application, see @{article:Almanac User Guide}. + +Add **interfaces** to each device record so Phabricator can tell how to +connect to these hosts. Normally, you'll add one HTTP interface (usually on +port 80) and one SSH interface (often on port 22) to each device: + +For example, if you are building a two-host repository cluster, you may end +up with records that look like these: + + - Device: `repo001.mycompany.net` + - Interface: `123.0.0.1:22` + - Interface: `123.0.0.1:80` + - Device: `repo002.mycopmany.net` + - Interface: `123.0.0.2:22` + - Interface: `123.0.0.2:80` + +Note that these hosts will normally run two `sshd` ports: the standard `sshd` +which you connect to to operate and administrate the host, and the special +Phabricator `sshd` that you connect to to clone and push repositories. + +You should specify the Phabricator `sshd` port, **not** the standard `sshd` +port. + +If you're using **unique** SSH keys for each device, continue to the next step. + +If you're using **shared** SSH keys, create a third device with no interfaces, +like `keywarden.mycompany.net`. This device will just be used as a container to +hold the trusted SSH key and is not a real device. + +NOTE: Do **not** create a **service** record yet. Today, service records become +active immediately once they are created, and you haven't set things up yet. + + +Generate and Trust SSH Keys +=========================== + +Next, you need to generate or upload SSH keys and mark them as trusted. Marking +a key as trusted gives it tremendous power. + +If you're using **unique** SSH keys, upload or generate a key for each +individual device from the device detail screen in the Almanac web UI. Save the +private keys for the next step. + +If you're using a **shared** SSH key, upload or generate a single key for +the keywarden device from the device detail screen in the Almanac web UI. +Save the private key for the next step. + +Regardless of how many keys you generated, take the key IDs from the tables +in the web UI and run this command from the command line for each key, to mark +each key as trusted: + +``` +phabricator/ $ ./bin/almanac trust-key --id +phabricator/ $ ./bin/almanac trust-key --id +... +``` + +The warnings this command emits are serious. The private keys are now trusted, +and allow any user or device possessing them to sign requests that bypass +policy checks without requiring additional credentials. Guard them carefully! + +If you need to revoke trust for a key later, use `untrust-key`: + +``` +phabricator/ $ ./bin/almanac untrust-key --id +``` + +Once the keys are trusted, continue to the next step. + + +Install Phabricator +=================== + +If you haven't already, install Phabricator on each device you plan to enroll +in the cluster. Cluster repository devices must provide services over both HTTP +and SSH, so you need to install and configure both a webserver and a +Phabricator `sshd` on these hosts. + +Generally, you will follow whatever process you otherwise use when installing +Phabricator. + +NOTE: Do not start the daemons on the new devices yet. They won't work properly +until you've finished configuring things. + +Once Phabricator is installed, you can enroll the devices in the cluster by +registering them. + + +Register Devices +================ + +To register a host as an Almanac device, use `bin/almanac register`. + +If you are using **unique** keys, run it like this: + +``` +$ ./bin/almanac register \ + --device \ + --private-key +``` + +For example, you might run this command on `repo001` when using unique keys: + +``` +$ ./bin/almanac register \ + --device repo001.mycompany.net \ + --private-key /path/to/private.key +``` + +If you are using a **shared** key, this will be a little more complicated +because you need to override some checks that are intended to prevent mistakes. +Use the `--identify-as` flag to choose a device identity: + +``` +$ ./bin/almanac register \ + --device \ + --private-key \ + --identify-as +``` + +For example, you might run this command on `repo001` when using a shared key: + +``` +$ ./bin/almanac register + --device keywarden.mycompany.net \ + --private-key /path/to/private-key \ + --identify-as repo001.mycompany.net +``` + +In particular, note that `--device` is always the **trusted** device associated +with the trusted key. The `--identify-as` flag allows several different hosts +to share the same key but still identify as different devices. + +The overall effect of the `bin/almanac` command is to copy identity and key +files into `phabricator/conf/keys/`. You can inspect the results by examining +that directory. The helper script just catches potential mistakes and makes +sure the process is completed correctly. + +Note that a copy of the active private key is stored in the `conf/keys/` +directory permanently. + + +Next Steps +========== + +Now that devices are registered, you can build cluster services from them. +Return to the relevant cluster service documentation to continue: + + - build repository clusters with @{article:Cluster: Repositories}; + - return to @{article:Clustering Introduction}; or + - review the Almanac application with @{article:Almanac User Guide}. From 0630fef9fc3692ad233e9dfebf63ea1e06a48e55 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 05:14:20 -0700 Subject: [PATCH 10/32] Prevent web queries from running for more than 30 seconds Summary: Ref T10849. This enforces a global 30-second per-query time limit for anything not coming from the CLI. If we run into another issue with MySQL hanging in the future, this should prevent it from being nearly as bad as it was. Test Plan: - Set value to 0, verified the UI threw an exception immediately. - Set value back to 30, browsed around a bunch of pages. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10849 Differential Revision: https://secure.phabricator.com/D15799 --- src/infrastructure/storage/lisk/PhabricatorLiskDAO.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php index 610bc06f59..1a51467089 100644 --- a/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php +++ b/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php @@ -75,6 +75,12 @@ abstract class PhabricatorLiskDAO extends LiskDAO { $connection->setReadOnly(true); } + // Unless this is a script running from the CLI, prevent any query from + // running for more than 30 seconds. See T10849 for discussion. + if (php_sapi_name() != 'cli') { + $connection->setQueryTimeout(30); + } + return $connection; } From 8e4a7742ebfaccc48e3194d8bf469a1f23585914 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 05:43:35 -0700 Subject: [PATCH 11/32] Port local storage path to new repository Manage UI Summary: Ref T10748. This merges "Storage" and "Cluster" into a single UI which combines the information of both. Test Plan: {F1246882} Reviewers: chad Reviewed By: chad Subscribers: hach-que Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15800 --- src/__phutil_library_map__.php | 4 +- ...usionRepositoryStorageManagementPanel.php} | 57 ++++++++++++++----- 2 files changed, 44 insertions(+), 17 deletions(-) rename src/applications/diffusion/management/{DiffusionRepositoryClusterManagementPanel.php => DiffusionRepositoryStorageManagementPanel.php} (79%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 161ea349bc..2ee5955733 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -747,7 +747,6 @@ phutil_register_library_map(array( 'DiffusionRepositoryByIDRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php', 'DiffusionRepositoryClusterEngine' => 'applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php', 'DiffusionRepositoryClusterEngineLogInterface' => 'applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php', - 'DiffusionRepositoryClusterManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryClusterManagementPanel.php', 'DiffusionRepositoryController' => 'applications/diffusion/controller/DiffusionRepositoryController.php', 'DiffusionRepositoryCreateController' => 'applications/diffusion/controller/DiffusionRepositoryCreateController.php', 'DiffusionRepositoryDatasource' => 'applications/diffusion/typeahead/DiffusionRepositoryDatasource.php', @@ -781,6 +780,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php', 'DiffusionRepositorySearchConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionRepositorySearchConduitAPIMethod.php', 'DiffusionRepositoryStatusManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php', + 'DiffusionRepositoryStorageManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php', 'DiffusionRepositorySymbolsController' => 'applications/diffusion/controller/DiffusionRepositorySymbolsController.php', 'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php', 'DiffusionRepositoryTestAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php', @@ -4959,7 +4959,6 @@ phutil_register_library_map(array( 'DiffusionRepositoryBasicsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryByIDRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'DiffusionRepositoryClusterEngine' => 'Phobject', - 'DiffusionRepositoryClusterManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryController' => 'DiffusionController', 'DiffusionRepositoryCreateController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryDatasource' => 'PhabricatorTypeaheadDatasource', @@ -4993,6 +4992,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'DiffusionRepositorySearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'DiffusionRepositoryStatusManagementPanel' => 'DiffusionRepositoryManagementPanel', + 'DiffusionRepositoryStorageManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositorySymbolsController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryTag' => 'Phobject', 'DiffusionRepositoryTestAutomationController' => 'DiffusionRepositoryEditController', diff --git a/src/applications/diffusion/management/DiffusionRepositoryClusterManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php similarity index 79% rename from src/applications/diffusion/management/DiffusionRepositoryClusterManagementPanel.php rename to src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php index 22ecb3db3f..54c446b9fe 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryClusterManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php @@ -1,12 +1,12 @@ buildStorageStatusPanel(), + $this->buildClusterStatusPanel(), + ); + } + + private function buildStorageStatusPanel() { + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setViewer($viewer); + + if ($repository->usesLocalWorkingCopy()) { + $storage_path = $repository->getHumanReadableDetail('local-path'); + } else { + $storage_path = phutil_tag('em', array(), pht('No Local Working Copy')); + } + + $service_phid = $repository->getAlmanacServicePHID(); + if ($service_phid) { + $storage_service = $viewer->renderHandle($service_phid); + } else { + $storage_service = phutil_tag('em', array(), pht('Local')); + } + + $view->addProperty(pht('Storage Path'), $storage_path); + $view->addProperty(pht('Storage Cluster'), $storage_service); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Storage')); + + return id(new PHUIObjectBoxView()) + ->setHeader($header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->addPropertyList($view); + } + + private function buildClusterStatusPanel() { $repository = $this->getRepository(); $viewer = $this->getViewer(); @@ -175,18 +214,6 @@ final class DiffusionRepositoryClusterManagementPanel ->setTag('a') ->setText(pht('Documentation'))); - if ($service) { - $header->setSubheader( - pht( - 'This repository is hosted on %s.', - phutil_tag( - 'a', - array( - 'href' => $service->getURI(), - ), - $service->getName()))); - } - return id(new PHUIObjectBoxView()) ->setHeader($header) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) From 8606fb588f8e85f07c4e8a5a6e3d24d2fc45c77f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 05:54:51 -0700 Subject: [PATCH 12/32] Port "Staging Area" repository section to new management UI Summary: Ref T10748. Brings this over and adds EditEngine support for it. Test Plan: Viewed and edited staging area information. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15801 --- src/__phutil_library_map__.php | 2 + .../editor/DiffusionRepositoryEditEngine.php | 10 ++++ ...fusionRepositoryStagingManagementPanel.php | 55 +++++++++++++++++++ 3 files changed, 67 insertions(+) create mode 100644 src/applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2ee5955733..0bcd80eeb2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -779,6 +779,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryRef' => 'applications/diffusion/data/DiffusionRepositoryRef.php', 'DiffusionRepositoryRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryRemarkupRule.php', 'DiffusionRepositorySearchConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionRepositorySearchConduitAPIMethod.php', + 'DiffusionRepositoryStagingManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php', 'DiffusionRepositoryStatusManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php', 'DiffusionRepositoryStorageManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php', 'DiffusionRepositorySymbolsController' => 'applications/diffusion/controller/DiffusionRepositorySymbolsController.php', @@ -4991,6 +4992,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryRef' => 'Phobject', 'DiffusionRepositoryRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'DiffusionRepositorySearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', + 'DiffusionRepositoryStagingManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryStatusManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryStorageManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositorySymbolsController' => 'DiffusionRepositoryEditController', diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 602a3059d6..0a33d87712 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -152,6 +152,16 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Set the default branch name.')) ->setConduitTypeDescription(pht('New default branch name.')) ->setValue($object->getDetail('default-branch')), + id(new PhabricatorTextEditField()) + ->setKey('stagingAreaURI') + ->setLabel(pht('Staging Area URI')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_STAGING_URI) + ->setIsCopyable(true) + ->setDescription(pht('Staging area URI.')) + ->setConduitDescription(pht('Set the staging area URI.')) + ->setConduitTypeDescription(pht('New staging area URI.')) + ->setValue($object->getStagingURI()), id(new PhabricatorPolicyEditField()) ->setKey('policy.push') ->setLabel(pht('Push Policy')) diff --git a/src/applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php new file mode 100644 index 0000000000..bff0420193 --- /dev/null +++ b/src/applications/diffusion/management/DiffusionRepositoryStagingManagementPanel.php @@ -0,0 +1,55 @@ +getRepository(); + $viewer = $this->getViewer(); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + PhabricatorPolicyCapability::CAN_EDIT); + + $staging_uri = $repository->getPathURI('edit/staging/'); + + return array( + id(new PhabricatorActionView()) + ->setIcon('fa-pencil') + ->setName(pht('Edit Staging')) + ->setHref($staging_uri) + ->setDisabled(!$can_edit) + ->setWorkflow(!$can_edit), + ); + } + + public function buildManagementPanelContent() { + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setViewer($viewer) + ->setActionList($this->newActions()); + + $staging_uri = $repository->getStagingURI(); + if (!$staging_uri) { + $staging_uri = phutil_tag('em', array(), pht('No Staging Area')); + } + + $view->addProperty(pht('Staging Area URI'), $staging_uri); + + return $this->newBox(pht('Staging Area'), $view); + } + +} From 3fda965288cc64101594e051287aa618640b5efd Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 07:33:07 -0700 Subject: [PATCH 13/32] When multiple web hosts are in service, don't require setup warnings to be dismissed on each one Summary: Fixes T10876. Currently, we can end up with a setup warning banner sticking on each web device, since the state is stored in local cache. Instead: - When we actually run the setup checks, save the current state in the database. - Before we show a cached banner, make sure the database still says the checks are a problem. This could lead to some inconsistencies if setup checks legitimately pass on some hosts but not on others. For example, if you have `git` installed on one machine but not on another, we may raise a setup warning ("No Git Binary!") about it on one host only. For now, assume users have their operational environments in some sort of reasonable shape and can install the same stuff everywhere. In the future, we could split the issues into "global" and "per-host" issues if we run into problems with this. Test Plan: This is somewhat tricky to test locally since you really need multiple webservers to test it properly, but I: - Created some setup issues, saw banner. - Ignored/cleared them, saw banner go away. - Verified database cache writes were occurring properly. Then I sort of faked it like this: - Created a setup issue. - Manually set the database cache value to `[]` ("no issues"). - Reloaded page. - No more banner. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10876 Differential Revision: https://secure.phabricator.com/D15802 --- .../config/check/PhabricatorSetupCheck.php | 44 ++++++++++++++++++- .../PhabricatorConfigIssueListController.php | 3 +- .../PhabricatorConfigIssueViewController.php | 3 +- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/applications/config/check/PhabricatorSetupCheck.php b/src/applications/config/check/PhabricatorSetupCheck.php index 4c6c0b581c..a657c52799 100644 --- a/src/applications/config/check/PhabricatorSetupCheck.php +++ b/src/applications/config/check/PhabricatorSetupCheck.php @@ -50,9 +50,35 @@ abstract class PhabricatorSetupCheck extends Phobject { return $cache->getKey('phabricator.setup.issue-keys'); } - final public static function setOpenSetupIssueKeys(array $keys) { + final public static function setOpenSetupIssueKeys( + array $keys, + $update_database) { $cache = PhabricatorCaches::getSetupCache(); $cache->setKey('phabricator.setup.issue-keys', $keys); + + if ($update_database) { + $db_cache = new PhabricatorKeyValueDatabaseCache(); + try { + $json = phutil_json_encode($keys); + $db_cache->setKey('phabricator.setup.issue-keys', $json); + } catch (Exception $ex) { + // Ignore any write failures, since they likely just indicate that we + // have a database-related setup issue that needs to be resolved. + } + } + } + + final public static function getOpenSetupIssueKeysFromDatabase() { + $db_cache = new PhabricatorKeyValueDatabaseCache(); + try { + $value = $db_cache->getKey('phabricator.setup.issue-keys'); + if (!strlen($value)) { + return null; + } + return phutil_json_decode($value); + } catch (Exception $ex) { + return null; + } } final public static function getUnignoredIssueKeys(array $all_issues) { @@ -97,7 +123,21 @@ abstract class PhabricatorSetupCheck extends Phobject { ->setView($view); } } - self::setOpenSetupIssueKeys(self::getUnignoredIssueKeys($issues)); + $issue_keys = self::getUnignoredIssueKeys($issues); + self::setOpenSetupIssueKeys($issue_keys, $update_database = true); + } else if ($issue_keys) { + // If Phabricator is configured in a cluster with multiple web devices, + // we can end up with setup issues cached on every device. This can cause + // a warning banner to show on every device so that each one needs to + // be dismissed individually, which is pretty annoying. See T10876. + + // To avoid this, check if the issues we found have already been cleared + // in the database. If they have, we'll just wipe out our own cache and + // move on. + $issue_keys = self::getOpenSetupIssueKeysFromDatabase(); + if ($issue_keys !== null) { + self::setOpenSetupIssueKeys($issue_keys, $update_database = false); + } } // Try to repair configuration unless we have a clean bill of health on it. diff --git a/src/applications/config/controller/PhabricatorConfigIssueListController.php b/src/applications/config/controller/PhabricatorConfigIssueListController.php index 02bec4e082..c3dc7f577c 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueListController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueListController.php @@ -11,7 +11,8 @@ final class PhabricatorConfigIssueListController $issues = PhabricatorSetupCheck::runAllChecks(); PhabricatorSetupCheck::setOpenSetupIssueKeys( - PhabricatorSetupCheck::getUnignoredIssueKeys($issues)); + PhabricatorSetupCheck::getUnignoredIssueKeys($issues), + $update_database = true); $important = $this->buildIssueList( $issues, PhabricatorSetupCheck::GROUP_IMPORTANT); diff --git a/src/applications/config/controller/PhabricatorConfigIssueViewController.php b/src/applications/config/controller/PhabricatorConfigIssueViewController.php index b1d1c299a5..36fb77ce74 100644 --- a/src/applications/config/controller/PhabricatorConfigIssueViewController.php +++ b/src/applications/config/controller/PhabricatorConfigIssueViewController.php @@ -9,7 +9,8 @@ final class PhabricatorConfigIssueViewController $issues = PhabricatorSetupCheck::runAllChecks(); PhabricatorSetupCheck::setOpenSetupIssueKeys( - PhabricatorSetupCheck::getUnignoredIssueKeys($issues)); + PhabricatorSetupCheck::getUnignoredIssueKeys($issues), + $update_database = true); if (empty($issues[$issue_key])) { $content = id(new PHUIInfoView()) From 8d9bc401e4562b2f8cb6844b7dad34ee5d75c18b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Apr 2016 08:16:04 -0700 Subject: [PATCH 14/32] Improve Diffusion hosting setup instructions somewhat? Summary: Ref T10866. Fixes T10386. This attempts to make it a little more plausible to follow these directions: - Use simpler language in general. - Remove language suggesting that HTTP requires no additional configuration. - Suggest using a load balancer or an ugly port number instead of swapping SSH to a different port. - Be more granular about `sudo` setup. - Organize better? Test Plan: Read documentation. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10386, T10866 Differential Revision: https://secure.phabricator.com/D15796 --- .../sshd/sshd_config.phabricator.example | 2 +- .../user/userguide/diffusion_hosting.diviner | 374 ++++++++++++------ 2 files changed, 250 insertions(+), 126 deletions(-) diff --git a/resources/sshd/sshd_config.phabricator.example b/resources/sshd/sshd_config.phabricator.example index 64b7fdc641..2f2a581223 100644 --- a/resources/sshd/sshd_config.phabricator.example +++ b/resources/sshd/sshd_config.phabricator.example @@ -10,7 +10,7 @@ AllowUsers vcs-user # You may need to tweak these options, but mostly they just turn off everything # dangerous. -Port 22 +Port 2222 Protocol 2 PermitRootLogin no AllowAgentForwarding no diff --git a/src/docs/user/userguide/diffusion_hosting.diviner b/src/docs/user/userguide/diffusion_hosting.diviner index 60360c1af8..f576e4fe7a 100644 --- a/src/docs/user/userguide/diffusion_hosting.diviner +++ b/src/docs/user/userguide/diffusion_hosting.diviner @@ -3,13 +3,15 @@ Guide to configuring Phabricator repository hosting. -= Overview = +Overview +======== Phabricator can host repositories and provide authenticated read and write access to them over HTTP and SSH. This document describes how to configure repository hosting. -= Understanding Supported Protocols = +Understanding Supported Protocols +================================= Phabricator supports hosting over these protocols: @@ -35,99 +37,165 @@ performant, but HTTP is easier to set up and supports anonymous access. | Performance | Better | Okay | | Setup | Hard | Easy | -Each repository can be configured individually, and you can use either protocol, -or both, or a mixture across different repositories. +Each repository can be configured individually, and you can use either +protocol, or both, or a mixture across different repositories. SSH is recommended unless you need anonymous access, or are not able to configure it for technical reasons. -= Configuring System User Accounts = -Phabricator uses as many as three user accounts. This section will guide you -through creating and configuring them. These are system user accounts on the -machine Phabricator runs on, not Phabricator user accounts. +Creating System User Accounts +============================= -The system accounts are: +Phabricator uses two system user accounts, plus a third account if you +configure SSH access. This section will guide you through creating and +configuring them. These are system user accounts on the machine Phabricator +runs on, not Phabricator user accounts. - - The user the daemons run as. We'll call this `daemon-user`. For more - information on the daemons, see @{article:Managing Daemons with phd}. This +The system accounts Phabricator uses are: + + - The user the webserver runs as. We'll call this `www-user`. + - The user the daemons run as. We'll call this `daemon-user`. This user is the only user which will interact with the repositories directly. - Other accounts will `sudo` to this account in order to perform VCS + Other accounts will `sudo` to this account in order to perform repository operations. - - The user the webserver runs as. We'll call this `www-user`. If you do not - plan to make repositories available over HTTP, you do not need to perform - any special configuration for this user. - - The user that users will connect over SSH as. We'll call this `vcs-user`. + - The user that humans will connect over SSH as. We'll call this `vcs-user`. If you do not plan to make repositories available over SSH, you do not need - to perform any special configuration for this user. + to create or configure this user. -To configure these users: +To create these users: + - Create a `www-user` if one does not already exist. In most cases, this + user will already exist and you just need to identify which user it is. Run + your webserver as this user. - Create a `daemon-user` if one does not already exist (you can call this user - whatever you want, or use an existing account). When you start the daemons, - start them using this user. - - Create a `www-user` if one does not already exist. Run your webserver as - this user. In most cases, this user will already exist. - - Create a `vcs-user` if one does not already exist. Common names for this - user are `git` or `hg`. When users clone repositories, they will use a URI - like `vcs-user@phabricator.yourcompany.com`. + whatever you want, or use an existing account). Below, you'll configure + the daemons to start as this user. + - Create a `vcs-user` if one does not already exist and you plan to set up + SSH. When users clone repositories, they will use a URI like + `vcs-user@phabricator.yourcompany.com`, so common names for this user are + `git` or `hg`. -Now, allow the `vcs-user` and `www-user` to `sudo` as the `daemon-user`. Add -this to `/etc/sudoers`, using `visudo` or `sudoedit`. +Continue below to configure these accounts. -If you plan to use SSH: - vcs-user ALL=(daemon-user) SETENV: NOPASSWD: /path/to/bin/git-upload-pack, /path/to/bin/git-receive-pack, /path/to/bin/hg, /path/to/bin/svnserve +Configuring Phabricator +======================= -If you plan to use HTTP: +Now that you have created or identified these accounts, update the Phabricator +configuration to specify them. - www-user ALL=(daemon-user) SETENV: NOPASSWD: /usr/bin/git-http-backend, /usr/bin/hg +First, set `phd.user` to the `daemon-user`: -Replace `vcs-user`, `www-user` and `daemon-user` with the right usernames for -your configuration. Make sure all the paths point to the real locations of the -binaries on your system. You can omit any binaries associated with VCSes you do -not use. +``` +phabricator/ $ ./bin/config set phd.user daemon-user +``` -Adding these commands to `sudoers` will allow the daemon and webserver users to -write to repositories as the daemon user. +Restart the daemons to make sure this configuration works properly. They should +start as the correct user automatically. -Before saving and closing `/etc/sudoers`, look for this line: +If you're using a `vcs-user` for SSH, you should also configure that: + +``` +phabricator/ $ ./bin/config set diffusion.ssh-user vcs-user +``` + +Next, you'll set up `sudo` permissions so these users can interact with one +another. + + +Configuring Sudo +================ + +The `www-user` and `vcs-user` need to be able to `sudo` as the `daemon-user` +so they can interact with repositories. + +To grant them access, edit the `sudo` system configuration. On many systems, +you will do this by modifying the `/etc/sudoers` file using `visudo` or +`sudoedit`. In some cases, you may add a new file to `/etc/sudoers.d` instead. + +To give a user account `sudo` access to run a list of binaries, add a line like +this to the configuration file (this example would grant `vcs-user` permission +to run `ls` as `daemon-user`): + +``` +vcs-user ALL=(daemon-user) SETENV: NOPASSWD: /path/to/bin/ls +``` + +The `www-user` needs to be able to run these binaries as the `daemon-user`: + + - `git` (if using Git) + - `git-http-backend` (if using Git) + - `hg` (if using Mercurial) + - `ssh` (if configuring clusters) + +If you plan to use SSH, the `vcs-user` needs to be able to run these binaries +as the `daemon-user`: + + - `git` (if using Git) + - `git-upload-pack` (if using Git) + - `git-receive-pack` (if using Git) + - `hg` (if using Mercurial) + - `svnserve` (if using Subversion) + - `ssh` (if configuring clusters) + +Identify the full paths to all of these binaries on your system and add the +appropriate permissions to the `sudo` configuration. + +Normally, you'll add two lines that look something like this: + +``` +www-user ALL=(daemon-user) SETENV: NOPASSWD: /path/to/x, /path/to/y, ... +vcs-user ALL=(daemon-user) SETENV: NOPASSWD: /path/to/x, /path/to/y, ... +``` + +This is just a template. In the real configuration file, you need to: + + - Replace `www-user`, `dameon-user` and `vcs-user` with the correct + usernames for your system. + - List every binary that these users need access to, as described above. + - Make sure each binary path is the full path to the correct binary location + on your system. + +Before continuing, look for this line in your `sudo` configuration: Defaults requiretty If it's present, comment it out by putting a `#` at the beginning of the line. With this option enabled, VCS SSH sessions won't be able to use `sudo`. + +Additional SSH User Configuration +================================= + If you're planning to use SSH, you should also edit `/etc/passwd` and `/etc/shadow` to make sure the `vcs-user` account is set up correctly. - - Open `/etc/shadow` and find the line for the `vcs-user` account. - - The second field (which is the password field) must not be set to - `!!`. This value will prevent login. If it is set to `!!`, edit it - and set it to `NP` ("no password") instead. - - Open `/etc/passwd` and find the line for the `vcs-user` account. - - The last field (which is the login shell) must be set to a real shell. - If it is set to something like `/bin/false`, then `sshd` will not be able - to execute commands. Instead, you should set it to a real shell, like - `/bin/sh`. +**`/etc/shadow`**: Open `/etc/shadow` and find the line for the `vcs-user` +account. -Finally, once you've configured `/etc/sudoers`, `/etc/shadow` and `/etc/passwd`, -set `phd.user` to the `daemon-user`: +The second field (which is the password field) must not be set to `!!`. This +value will prevent login. If it is set to `!!`, edit it and set it to `NP` ("no +password") instead. - phabricator/ $ ./bin/config set phd.user daemon-user +**`/etc/passwd`**: Open `/etc/passwd` and find the line for the `vcs-user` +account. -If you're using a `vcs-user`, you should also configure that here: +The last field (which is the login shell) must be set to a real shell. If it is +set to something like `/bin/false`, then `sshd` will not be able to execute +commands. Instead, you should set it to a real shell, like `/bin/sh`. - phabricator/ $ ./bin/config set diffusion.ssh-user vcs-user -= Configuring HTTP = +Configuring HTTP +================ -If you plan to use authenticated HTTP, you need to set -`diffusion.allow-http-auth` in Config. If you don't plan to use HTTP, or plan to -use only anonymous HTTP, you can leave this setting disabled. +If you plan to serve repositories over authenticated HTTP, you need to set +`diffusion.allow-http-auth` in Config. If you don't plan to serve repositories +over HTTP (or plan to use only anonymous HTTP) you can leave this setting +disabled. -If you plan to use authenticated HTTP, you'll also need to configure a VCS -password in {nav Settings > VCS Password}. +If you plan to use authenticated HTTP, you (and all other users) also need to +configure a VCS password for your account in {nav Settings > VCS Password}. Your VCS password must be a different password than your main Phabricator password because VCS passwords are very easy to accidentally disclose. They are @@ -136,60 +204,58 @@ and present in command output and logs. We strongly encourage you to use SSH instead of HTTP to authenticate access to repositories. Otherwise, if you've configured system accounts above, you're all set. No -additional server configuration is required to make HTTP work. +additional server configuration is required to make HTTP work. You should now +be able to fetch and push repositories over HTTP. See "Cloning a Repository" +below for more details. -= Configuring SSH = +If you're having trouble, see "Troubleshooting HTTP" below. -SSH access requires some additional setup. Here's an overview of how setup -works: - - You'll move the normal `sshd` daemon to another port, like `222`. When - connecting to the machine to administrate it, you'll use this alternate - port to get a normal login shell. - - You'll run a highly restricted `sshd` on port 22, with a special locked-down - configuration that uses Phabricator to authorize users and execute commands. - - The `sshd` on port 22 **MUST** be 6.2 or newer, because Phabricator relies - on the `AuthorizedKeysCommand` option. +Configuring SSH +=============== -Here's a walkthrough of how to perform this configuration in detail: +SSH access requires some additional setup. You will configure and run a second, +restricted copy of `sshd` on the machine, on a different port from the standard +`sshd`. This special copy of `sshd` will serve repository requests and provide +other Phabricator SSH services. -**Move Normal SSHD**: Be careful when editing the configuration for `sshd`. If -you get it wrong, you may lock yourself out of the machine. Restarting `sshd` -generally will not interrupt existing connections, but you should exercise -caution. Two strategies you can use to mitigate this risk are: smoke-test -configuration by starting a second `sshd`; and use a `screen` session which -automatically repairs configuration unless stopped. +NOTE: The Phabricator `sshd` service **MUST** be 6.2 or newer, because +Phabricator relies on the `AuthorizedKeysCommand` option. -To smoke-test a configuration, just start another `sshd` using the `-f` flag: +**Choose a Port**: These instructions will configure the alternate `sshd` on +port `2222`. This is easy to configure, but if you run the service on this port +users will clone and push to URIs like `ssh://git@host.com:2222/`, which is +a little ugly. - sudo /path/to/sshd -f /path/to/config_file.edited +The easiest way to fix this is to put a load balancer in front of the host and +have it forward TCP traffic on port `22` to port `2222`. Then users can clone +from `ssh://git@host.com/` without an explicit port number and you don't need +to do anything else. -You can then connect and make sure the edited config file is valid before -replacing your primary configuration file. +Alternatively, you can move the administrative `sshd` to a new port, then run +Phabricator `sshd` on port 22. This is complicated and risky. See "Moving the +sshd Port" below for help. -To automatically repair configuration, start a `screen` session with a command -like this in it: +Finally, you can just run on port `2222` and accept the explicit port in the +URIs. This is the simplest approach, and you can start here and clean things +up later. - sleep 60 ; mv sshd_config.good sshd_config ; /etc/init.d/sshd restart +If you plan to connect to a port other than `22`, you should set this port +as `diffusion.ssh-port` in your Phabricator config: -The specific command may vary for your system, but the general idea is to have -the machine automatically restore configuration after some period of time if -you don't stop it. If you lock yourself out, this will fix things automatically. +``` +$ ./bin/config set diffusion.ssh-port 2222 +``` -Now that you're ready to edit your configuration, open up your `sshd` config -(often `/etc/ssh/sshd_config`) and change the `Port` setting to some other port, -like `222` (you can choose any port other than 22). +This port is not special, and you are free to choose a different port, provided +you make the appropriate configuration adjustment below. - Port 222 +**Configure and Start Phabricator SSHD**: Now, you'll configure and start a +copy of `sshd` which will serve Phabricator services, including repositories, +over SSH. -Very carefully, restart `sshd`. Verify that you can connect on the new port: - - ssh -p 222 ... - -**Configure and Start Phabricator SSHD**: Now, configure and start a second -`sshd` instance which will run on port `22`. This instance will use a special -locked-down configuration that uses Phabricator to handle authentication and -command execution. +This instance will use a special locked-down configuration that uses +Phabricator to handle authentication and command execution. There are three major steps: @@ -221,34 +287,38 @@ Open the file and edit the `AuthorizedKeysCommand`, `AuthorizedKeysCommandUser`, and `AllowUsers` settings to be correct for your system. +This configuration file also specifies the `Port` the service should run on. +If you intend to run on a non-default port, adjust it now. + **Start SSHD**: Now, start the Phabricator `sshd`: sudo /path/to/sshd -f /path/to/sshd_config.phabricator -If you did everything correctly, you should be able to run this: +If you did everything correctly, you should be able to run this command: - echo {} | ssh vcs-user@phabricator.yourcompany.com conduit conduit.ping +``` +$ echo {} | ssh vcs-user@phabricator.yourcompany.com conduit conduit.ping +``` ...and get a response like this: - {"result":"orbital","error_code":null,"error_info":null} +```lang=json +{"result":"phabricator.yourcompany.com","error_code":null,"error_info":null} +``` -(If you get an authentication error, make sure you added your public key in -**Settings > SSH Public Keys**.) If you're having trouble, check the +If you get an authentication error, make sure you added your public key in +{nav Settings > SSH Public Keys}. If you're having trouble, check the troubleshooting section below. -= Authentication Over HTTP = +Authentication Over SSH +======================= -To authenticate over HTTP, users should configure a **VCS Password** in the -**Settings** screen. This panel is available only if `diffusion.allow-http-auth` -is enabled. +To authenticate over SSH, users should add their public keys under +{nav Settings > SSH Public Keys}. -= Authentication Over SSH = -To authenticate over SSH, users should add **SSH Public Keys** in the -**Settings** screen. - -= Cloning a Repository = +Cloning a Repository +==================== If you've already set up a hosted repository, you can try cloning it now. To do this, browse to the repository's main screen in Diffusion. You should see @@ -259,13 +329,15 @@ To clone the repository, just run the appropriate command. If you don't see the commands or running them doesn't work, see below for tips on troubleshooting. -= Troubleshooting HTTP = + +Troubleshooting HTTP +==================== Some general tips for troubleshooting problems with HTTP: - Make sure `diffusion.allow-http-auth` is enabled in your Phabricator config. - - Make sure HTTP serving is enabled for the repository you're trying to clone. - You can find this in {nav Edit Repository > Hosting}. + - Make sure HTTP serving is enabled for the repository you're trying to + clone. You can find this in {nav Edit Repository > Hosting}. - Make sure you've configured a VCS password. This is separate from your main account password. You can configure this in {nav Settings > VCS Password}. - Make sure the main repository screen in Diffusion shows a clone/checkout @@ -287,7 +359,8 @@ with the HTTP response is likely to be useful: In many cases, this can give you more information about what's wrong. -= Troubleshooting SSH = +Troubleshooting SSH +=================== Some general tips for troubleshooting problems with SSH: @@ -311,11 +384,11 @@ Some general tips for troubleshooting problems with SSH: - Check your `phabricator-ssh-hook.sh` file for proper settings. - Check your `sshd_config.phabricator` file for proper settings. -To troubleshoot SSH setup: connect to the server with `ssh`, without running -a command. You may need to use the `-T` flag. You should see a message like -this one: +To troubleshoot SSH setup: connect to the server with `ssh`, without running a +command. You may need to use the `-T` flag, and will need to use `-p` if you +are running on a nonstandard port. You should see a message like this one: - $ ssh -T dweller@secure.phabricator.com + $ ssh -T -p 2222 vcs-user@phabricator.yourcompany.com phabricator-ssh-exec: Welcome to Phabricator. You are logged in as alincoln. @@ -338,8 +411,8 @@ settings: - You're connecting as the `vcs-user`. - The `vcs-user` has `NP` in `/etc/shadow`. - The `vcs-user` has `/bin/sh` or some other valid shell in `/etc/passwd`. - - Your SSH key is correct, and you've added it to Phabricator in the Settings - panel. + - Your SSH private key is correct, and you've added the corresponding + public key to Phabricator in the Settings panel. If you can get this far, but can't execute VCS commands like `git clone`, there is probably an issue with your `sudoers` configuration. Check: @@ -357,7 +430,7 @@ It may also be helpful to run `sshd` in debug mode: $ /path/to/sshd -d -d -d -f /path/to/sshd_config.phabricator This will run it in the foreground and emit a large amount of debugging -information. +information when you connect to it. Finally, you can usually test that `sudoers` is configured correctly by doing something like this: @@ -369,7 +442,9 @@ That will try to run the binary via `sudo` in a manner similar to the way that Phabricator will run it. This can give you better error messages about issues with `sudoers` configuration. -= Miscellaneous Troubleshooting = + +Miscellaneous Troubleshooting +============================= - If you're getting an error about `svnlook` not being found, add the path where `svnlook` is located to the Phabricator configuration @@ -377,6 +452,54 @@ with `sudoers` configuration. is caused by SVN wiping the environment (including PATH) when invoking commit hooks. + +Moving the sshd Port +==================== + +If you want to move the standard (administrative) `sshd` to a different port to +make Phabricator repository URIs cleaner, this section has some tips. + +This is optional, and it is normally easier to do this by putting a load +balancer in front of Phabricator and having it accept TCP traffic on port 22 +and forward it to some other port. + +When moving `sshd`, be careful when editing the configuration. If you get it +wrong, you may lock yourself out of the machine. Restarting `sshd` generally +will not interrupt existing connections, but you should exercise caution. Two +strategies you can use to mitigate this risk are: smoke-test configuration by +starting a second `sshd`; and use a `screen` session which automatically +repairs configuration unless stopped. + +To smoke-test a configuration, just start another `sshd` using the `-f` flag: + + sudo /path/to/sshd -f /path/to/config_file.edited + +You can then connect and make sure the edited config file is valid before +replacing your primary configuration file. + +To automatically repair configuration, start a `screen` session with a command +like this in it: + + sleep 60 ; mv sshd_config.good sshd_config ; /etc/init.d/sshd restart + +The specific command may vary for your system, but the general idea is to have +the machine automatically restore configuration after some period of time if +you don't stop it. If you lock yourself out, this can fix things automatically. + +Now that you're ready to edit your configuration, open up your `sshd` config +(often `/etc/ssh/sshd_config`) and change the `Port` setting to some other port, +like `222` (you can choose any port other than 22). + + Port 222 + +Very carefully, restart `sshd`. Verify that you can connect on the new port: + + ssh -p 222 ... + +Now you can move the Phabricator `sshd` to port 22, then adjust the value +for `diffusion.ssh-port` in your Phabricator configuration. + + No Direct Pushes ================ @@ -412,7 +535,8 @@ document provides instructions for configuring. Its absence indicates that the request did not pass through Phabricator. -= Next Steps = +Next Steps +========== Once hosted repositories are set up: From dc3a13c5e83492257f72666d77bf2c4ca7b6f5b7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Apr 2016 07:45:10 -0700 Subject: [PATCH 15/32] Add `bin/repository clusterize` and document setup and migration for clusters Summary: Ref T4292. This provides at least some sort of hint about how to set up cluster repositories. Test Plan: - Read documentation. - Ran `bin/repository clusterize` to add + remove clusters. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4292 Differential Revision: https://secure.phabricator.com/D15798 --- src/__phutil_library_map__.php | 2 + ...RepositoryManagementClusterizeWorkflow.php | 117 ++++++++++++ src/docs/user/cluster/cluster_devices.diviner | 11 +- .../user/cluster/cluster_repositories.diviner | 180 ++++++++++++++++-- 4 files changed, 286 insertions(+), 24 deletions(-) create mode 100644 src/applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0bcd80eeb2..cb7520e3a1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3174,6 +3174,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryGraphCache' => 'applications/repository/graphcache/PhabricatorRepositoryGraphCache.php', 'PhabricatorRepositoryGraphStream' => 'applications/repository/daemon/PhabricatorRepositoryGraphStream.php', 'PhabricatorRepositoryManagementCacheWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementCacheWorkflow.php', + 'PhabricatorRepositoryManagementClusterizeWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php', 'PhabricatorRepositoryManagementDiscoverWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php', 'PhabricatorRepositoryManagementEditWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementEditWorkflow.php', 'PhabricatorRepositoryManagementImportingWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementImportingWorkflow.php', @@ -7832,6 +7833,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryGraphCache' => 'Phobject', 'PhabricatorRepositoryGraphStream' => 'Phobject', 'PhabricatorRepositoryManagementCacheWorkflow' => 'PhabricatorRepositoryManagementWorkflow', + 'PhabricatorRepositoryManagementClusterizeWorkflow' => 'PhabricatorRepositoryManagementWorkflow', 'PhabricatorRepositoryManagementDiscoverWorkflow' => 'PhabricatorRepositoryManagementWorkflow', 'PhabricatorRepositoryManagementEditWorkflow' => 'PhabricatorRepositoryManagementWorkflow', 'PhabricatorRepositoryManagementImportingWorkflow' => 'PhabricatorRepositoryManagementWorkflow', diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php new file mode 100644 index 0000000000..7424b84eae --- /dev/null +++ b/src/applications/repository/management/PhabricatorRepositoryManagementClusterizeWorkflow.php @@ -0,0 +1,117 @@ +setName('clusterize') + ->setExamples('**clusterize** [options] __repository__ ...') + ->setSynopsis( + pht('Convert existing repositories into cluster repositories.')) + ->setArguments( + array( + array( + 'name' => 'service', + 'param' => 'service', + 'help' => pht( + 'Cluster repository service in Almanac to move repositories '. + 'into.'), + ), + array( + 'name' => 'remove-service', + 'help' => pht('Take repositories out of a cluster.'), + ), + array( + 'name' => 'repositories', + 'wildcard' => true, + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $repositories = $this->loadRepositories($args, 'repositories'); + if (!$repositories) { + throw new PhutilArgumentUsageException( + pht('Specify one or more repositories to clusterize.')); + } + + $service_name = $args->getArg('service'); + $remove_service = $args->getArg('remove-service'); + + if ($remove_service && $service_name) { + throw new PhutilArgumentUsageException( + pht('Specify --service or --remove-service, but not both.')); + } + + if (!$service_name && !$remove_service) { + throw new PhutilArgumentUsageException( + pht('Specify --service or --remove-service.')); + } + + if ($remove_service) { + $service = null; + } else { + $service = id(new AlmanacServiceQuery()) + ->setViewer($viewer) + ->withNames(array($service_name)) + ->withServiceTypes( + array( + AlmanacClusterRepositoryServiceType::SERVICETYPE, + )) + ->executeOne(); + if (!$service) { + throw new PhutilArgumentUsageException( + pht( + 'No repository service "%s" exists.', + $service_name)); + } + } + + + if ($service) { + $service_phid = $service->getPHID(); + } else { + $service_phid = null; + } + + $content_source = $this->newContentSource(); + $diffusion_phid = id(new PhabricatorDiffusionApplication())->getPHID(); + + foreach ($repositories as $repository) { + $xactions = array(); + + $xactions[] = id(new PhabricatorRepositoryTransaction()) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_SERVICE) + ->setNewValue($service_phid); + + id(new PhabricatorRepositoryEditor()) + ->setActor($viewer) + ->setActingAsPHID($diffusion_phid) + ->setContentSource($content_source) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->applyTransactions($repository, $xactions); + + if ($service) { + echo tsprintf( + "%s\n", + pht( + 'Moved repository "%s" to cluster service "%s".', + $repository->getDisplayName(), + $service->getName())); + } else { + echo tsprintf( + "%s\n", + pht( + 'Removed repository "%s" from cluster service.', + $repository->getDisplayName())); + } + } + + return 0; + } + +} diff --git a/src/docs/user/cluster/cluster_devices.diviner b/src/docs/user/cluster/cluster_devices.diviner index 6fb03cec24..197620e461 100644 --- a/src/docs/user/cluster/cluster_devices.diviner +++ b/src/docs/user/cluster/cluster_devices.diviner @@ -93,16 +93,16 @@ application, see @{article:Almanac User Guide}. Add **interfaces** to each device record so Phabricator can tell how to connect to these hosts. Normally, you'll add one HTTP interface (usually on -port 80) and one SSH interface (often on port 22) to each device: +port 80) and one SSH interface (by default, on port 2222) to each device: For example, if you are building a two-host repository cluster, you may end up with records that look like these: - Device: `repo001.mycompany.net` - - Interface: `123.0.0.1:22` + - Interface: `123.0.0.1:2222` - Interface: `123.0.0.1:80` - Device: `repo002.mycopmany.net` - - Interface: `123.0.0.2:22` + - Interface: `123.0.0.2:2222` - Interface: `123.0.0.2:80` Note that these hosts will normally run two `sshd` ports: the standard `sshd` @@ -230,6 +230,11 @@ sure the process is completed correctly. Note that a copy of the active private key is stored in the `conf/keys/` directory permanently. +When converting a host into a cluster host, you may need to revisit +@{article:Diffusion User Guide: Repository Hosting} and double check the `sudo` +permission for the host. In particular, cluster hosts need to be able to run +`ssh` via `sudo` so they can read the device private key. + Next Steps ========== diff --git a/src/docs/user/cluster/cluster_repositories.diviner b/src/docs/user/cluster/cluster_repositories.diviner index bb6019ee4d..35508b0813 100644 --- a/src/docs/user/cluster/cluster_repositories.diviner +++ b/src/docs/user/cluster/cluster_repositories.diviner @@ -9,9 +9,9 @@ Overview WARNING: This feature is a very early prototype; the features this document describes are mostly speculative fantasy. -If you use Git or Mercurial, you can deploy Phabricator with multiple -repository hosts, configured so that each host is readable and writable. The -advantages of doing this are: +If you use Git, you can deploy Phabricator with multiple repository hosts, +configured so that each host is readable and writable. The advantages of doing +this are: - you can completely survive the loss of repository hosts; - reads and writes can scale across multiple machines; and @@ -22,24 +22,6 @@ This configuration is complex, and many installs do not need to pursue it. This configuration is not currently supported with Subversion or Mercurial. -Repository Hosts -================ - -Repository hosts must run a complete, fully configured copy of Phabricator, -including a webserver. They must also run a properly configured `sshd`. - -Generally, these hosts will run the same set of services and configuration that -web hosts run. If you prefer, you can overlay these services and put web and -repository services on the same hosts. See @{article:Clustering Introduction} -for some guidance on overlaying services. - -When a user requests information about a repository that can only be satisfied -by examining a repository working copy, the webserver receiving the request -will make an HTTP service call to a repository server which hosts the -repository to retrieve the data it needs. It will use the result of this query -to respond to the user. - - How Reads and Writes Work ========================= @@ -95,6 +77,162 @@ Other mitigations are possible, but securing a network against the NSA and similar agents of other rogue nations is beyond the scope of this document. +Repository Hosts +================ + +Repository hosts must run a complete, fully configured copy of Phabricator, +including a webserver. They must also run a properly configured `sshd`. + +If you are converting existing hosts into cluster hosts, you may need to +revisit @{article:Diffusion User Guide: Repository Hosting} and make sure +the system user accounts have all the necessary `sudo` permissions. In +particular, cluster devices need `sudo` access to `ssh` so they can read +device keys. + +Generally, these hosts will run the same set of services and configuration that +web hosts run. If you prefer, you can overlay these services and put web and +repository services on the same hosts. See @{article:Clustering Introduction} +for some guidance on overlaying services. + +When a user requests information about a repository that can only be satisfied +by examining a repository working copy, the webserver receiving the request +will make an HTTP service call to a repository server which hosts the +repository to retrieve the data it needs. It will use the result of this query +to respond to the user. + + +Setting up a Cluster Services +============================= + +To set up clustering, first register the devices that you want to use as part +of the cluster with Almanac. For details, see @{article:Cluster: Devices}. + +NOTE: Once you create a service, new repositories will immediately allocate +on it. You may want to disable repository creation during initial setup. + +Once the hosts are registered as devices, you can create a new service in +Almanac: + + - First, register at least one device according to the device clustering + instructions. + - Create a new service of type **Phabricator Cluster: Repository** in + Almanac. + - Bind this service to all the interfaces on the device or devices. + - For each binding, add a `protocol` key with one of these values: + `ssh`, `http`, `https`. + +For example, a service might look like this: + + - Service: `repos001.mycompany.net` + - Binding: `repo001.mycompany.net:80`, `protocol=http` + - Binding: `repo001.mycompany.net:2222`, `protocol=ssh` + +The service itself has a `closed` property. You can set this to `true` to +disable new repository allocations on this service (for example, if it is +reaching capacity). + + +Migrating to Clustered Services +=============================== + +To convert existing repositories on an install into cluster repositories, you +will generally perform these steps: + + - Register the existing host as a cluster device. + - Configure a single host repository service using //only// that host. + +This puts you in a transitional state where repositories on the host can work +as either on-host repositories or cluster repositories. You can move forward +from here slowly and make sure services still work, with a quick path back to +safety if you run into trouble. + +To move forward, migrate one repository to the service and make sure things +work correctly. If you run into issues, you can back out by migrating the +repository off the service. + +To migrate a repository onto a cluster service, use this command: + +``` +$ ./bin/repository clusterize --service +``` + +To migrate a repository back off a service, use this command: + +``` +$ ./bin/repoistory clusterize --remove-service +``` + +This command only changes how Phabricator connects to the repository; it does +not move any data or make any complex structural changes. + +When Phabricator needs information about a non-clustered repository, it just +runs a command like `git log` directly on disk. When Phabricator needs +information about a clustered repository, it instead makes a service call to +another server, asking that server to run `git log` instead. + +In a single-host cluster the server will make this service call to itself, so +nothing will really change. But this //is// an effective test for most +possible configuration mistakes. + +If your canary repository works well, you can migrate the rest of your +repositories when ready (you can use `bin/repository list` to quickly get a +list of all repository monograms). + +Once all repositories are migrated, you've reached a stable state and can +remain here as long as you want. This state is sufficient to convert daemons, +SSH, and web services into clustered versions and spread them across multiple +machines if those goals are more interesting. + +Obviously, your single-device "cluster" will not be able to survive the loss of +the single repository host, but you can take as long as you want to expand the +cluster and add redundancy. + +After creating a service, you do not need to `clusterize` new repositories: +they will automatically allocate onto an open service. + +When you're ready to expand the cluster, continue below. + + +Expanding a Cluster +=================== + +To expand an existing cluster, follow these general steps: + + - Register new devices in Almanac. + - Add bindings to the new devices to the repository service, also in Almanac. + - Start the daemons on the new devices. + +For instructions on configuring and registering devices, see +@{article:Cluster: Devices}. + +As soon as you add active bindings to a service, Phabricator will begin +synchronizing repositories and sending traffic to the new device. You do not +need to copy any repository data to the device: Phabricator will automatically +synchronize it. + +If you have a large amount of repository data, you may want to help this +process along by copying the repository directory from an existing cluster +device before bringing the new host online. This is optional, but can reduce +the amount of time required to fully synchronize the cluster. + +You do not need to synchronize the most up-to-date data or stop writes during +this process. For example, loading the most recent backup snapshot onto the new +device will substantially reduce the amount of data that needs to be +synchronized. + + +Contracting a Cluster +===================== + +To reduce the size of an existing cluster, follow these general steps: + + - Disable the bindings from the service to the dead device in Almanac. + +If you are removing a device because it failed abruptly (or removing several +devices at once) it is possible that some repositories will have lost all their +leaders. See "Loss of Leaders" below to understand and resolve this. + + Monitoring Services =================== From 467c4e84e5ac6982a58ef1e870c4a8c9177171f4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 11:19:03 -0700 Subject: [PATCH 16/32] Add an edge table to the `search` database Summary: Fixes T10778. This is a result of T10262: when we save a form configuration and adjust the policy, we try to scramble attached file secrets. There aren't going to be any attached files, but there's also no edge table, so we fail. We could skip this code, but we'll likely need an edge table here sooner or later so it's probably simpler in the long run to just add an empty one. Test Plan: - Ran `bin/storage upgrade`, got a clean bill of health. - Saved a form configuration after making a policy edit, no more `edge` exception. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10778 Differential Revision: https://secure.phabricator.com/D15803 --- .../sql/autopatches/20160426.searchedge.sql | 16 ++++++++++++++++ src/__phutil_library_map__.php | 2 ++ .../storage/PhabricatorSearchSchemaSpec.php | 10 ++++++++++ 3 files changed, 28 insertions(+) create mode 100644 resources/sql/autopatches/20160426.searchedge.sql create mode 100644 src/applications/search/storage/PhabricatorSearchSchemaSpec.php diff --git a/resources/sql/autopatches/20160426.searchedge.sql b/resources/sql/autopatches/20160426.searchedge.sql new file mode 100644 index 0000000000..630f9759ae --- /dev/null +++ b/resources/sql/autopatches/20160426.searchedge.sql @@ -0,0 +1,16 @@ +CREATE TABLE {$NAMESPACE}_search.edge ( + src VARBINARY(64) NOT NULL, + type INT UNSIGNED NOT NULL, + dst VARBINARY(64) NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + seq INT UNSIGNED NOT NULL, + dataID INT UNSIGNED, + PRIMARY KEY (src, type, dst), + KEY `src` (src, type, dateCreated, seq), + UNIQUE KEY `key_dst` (dst, type, src) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; + +CREATE TABLE {$NAMESPACE}_search.edgedata ( + id INT UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT, + data LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT} +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index cb7520e3a1..953e5a4eb8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3306,6 +3306,7 @@ phutil_register_library_map(array( 'PhabricatorSearchPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorSearchPreferencesSettingsPanel.php', 'PhabricatorSearchRelationship' => 'applications/search/constants/PhabricatorSearchRelationship.php', 'PhabricatorSearchResultView' => 'applications/search/view/PhabricatorSearchResultView.php', + 'PhabricatorSearchSchemaSpec' => 'applications/search/storage/PhabricatorSearchSchemaSpec.php', 'PhabricatorSearchSelectController' => 'applications/search/controller/PhabricatorSearchSelectController.php', 'PhabricatorSearchSelectField' => 'applications/search/field/PhabricatorSearchSelectField.php', 'PhabricatorSearchStringListField' => 'applications/search/field/PhabricatorSearchStringListField.php', @@ -7982,6 +7983,7 @@ phutil_register_library_map(array( 'PhabricatorSearchPreferencesSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorSearchRelationship' => 'Phobject', 'PhabricatorSearchResultView' => 'AphrontView', + 'PhabricatorSearchSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorSearchSelectController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchSelectField' => 'PhabricatorSearchField', 'PhabricatorSearchStringListField' => 'PhabricatorSearchField', diff --git a/src/applications/search/storage/PhabricatorSearchSchemaSpec.php b/src/applications/search/storage/PhabricatorSearchSchemaSpec.php new file mode 100644 index 0000000000..4e9e739016 --- /dev/null +++ b/src/applications/search/storage/PhabricatorSearchSchemaSpec.php @@ -0,0 +1,10 @@ +buildEdgeSchemata(new PhabricatorProfilePanelConfiguration()); + } + +} From 58b55c2fa6d4c7f600cee469a78b59ddb26e1a41 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 16:27:31 -0700 Subject: [PATCH 17/32] Probably improve behavior around duplicate notifications Summary: We're sometimes getting duplicate notifications right now. I think this is because multiple windows are racing and becoming leaders. Clean this up a little: - Fix the `timeoout` typo. - Only try to usurp once. - Use different usurp and expire delays, so we don't fire them at the exact same time. Not sure if this'll work or not but it should theoretically be a little cleaner. Test Plan: - Quit Safari, reopened Safari, still saw a fast reconnect to the notification server (this is the goal of usurping). - Did normal notification stuff like opening a chat in two windows, got notifications. - Hard to reproduce the race for sure, but this at least fixes the outright `timeoout` bug. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D15806 --- resources/celerity/map.php | 12 +++++----- webroot/rsrc/externals/javelin/lib/Leader.js | 25 +++++++++++++------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index b0731d69eb..d72545dc93 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -8,7 +8,7 @@ return array( 'names' => array( 'core.pkg.css' => '04a95108', - 'core.pkg.js' => '37344f3c', + 'core.pkg.js' => '6972d365', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '7ba78475', 'differential.pkg.js' => 'd0cd0df6', @@ -232,7 +232,7 @@ return array( 'rsrc/externals/javelin/lib/DOM.js' => '805b806a', 'rsrc/externals/javelin/lib/History.js' => 'd4505101', 'rsrc/externals/javelin/lib/JSON.js' => '69adf288', - 'rsrc/externals/javelin/lib/Leader.js' => 'b4ba945c', + 'rsrc/externals/javelin/lib/Leader.js' => 'fea0eb47', 'rsrc/externals/javelin/lib/Mask.js' => '8a41885b', 'rsrc/externals/javelin/lib/Quicksand.js' => '6b8ef10b', 'rsrc/externals/javelin/lib/Request.js' => '94b750d2', @@ -704,7 +704,7 @@ return array( 'javelin-history' => 'd4505101', 'javelin-install' => '05270951', 'javelin-json' => '69adf288', - 'javelin-leader' => 'b4ba945c', + 'javelin-leader' => 'fea0eb47', 'javelin-magical-init' => '3010e992', 'javelin-mask' => '8a41885b', 'javelin-quicksand' => '6b8ef10b', @@ -1772,9 +1772,6 @@ return array( 'javelin-typeahead-preloaded-source', 'javelin-util', ), - 'b4ba945c' => array( - 'javelin-install', - ), 'b59e1e96' => array( 'javelin-behavior', 'javelin-stratcom', @@ -2176,6 +2173,9 @@ return array( 'javelin-view-visitor', 'javelin-util', ), + 'fea0eb47' => array( + 'javelin-install', + ), ), 'packages' => array( 'core.pkg.css' => array( diff --git a/webroot/rsrc/externals/javelin/lib/Leader.js b/webroot/rsrc/externals/javelin/lib/Leader.js index e0b71e2e48..80ba7fcc44 100644 --- a/webroot/rsrc/externals/javelin/lib/Leader.js +++ b/webroot/rsrc/externals/javelin/lib/Leader.js @@ -118,6 +118,10 @@ JX.install('Leader', { // Read the current leadership lease. var lease = self._read(); + // Stagger these delays so that they are unlikely to race one another. + var expire_delay = 50; + var usurp_delay = 75; + // If the lease is good, we're all set. var now = +new Date(); if (lease.until > now) { @@ -135,15 +139,17 @@ JX.install('Leader', { } else { // Set a callback to try to become the leader shortly after the - // current lease expires. This lets us recover from cases where the - // leader goes missing quickly. - if (self._timeoout) { - window.clearTimeout(self._timeout); - self._timeout = null; + // current lease expires. This lets us quickly recover from cases + // where the leader goes missing. + + // In particular, this can happen in Safari if you close windows or + // quit the browser instead of browsing away: the "pagehide" event + // does not fire when the leader is simply destroyed, so it does not + // evict itself from the throne of power. + if (!self._timeout) { + var usurp_at = (lease.until - now) + usurp_delay; + self._timeout = window.setTimeout(self._usurp, usurp_at); } - self._timeout = window.setTimeout( - self._usurp, - (lease.until - now) + 50); follower_callback(); } @@ -174,7 +180,7 @@ JX.install('Leader', { window.setTimeout( JX.bind(null, self._callIf, leader_callback, follower_callback), - 50); + expire_delay); }, @@ -306,6 +312,7 @@ JX.install('Leader', { _usurp: function() { var self = JX.Leader; self.call(JX.bag); + self._timeout = null; }, From 57a76d8a70a124c76025dcdf4427e624138687fb Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 15:54:01 -0700 Subject: [PATCH 18/32] Port "Automation" panel to new Repository Manage UI Summary: Ref T10748. Ports this UI and exposes it on the EditEngine. Test Plan: - Edited via EditEngine. - Viewed new manage UI. Reviewers: chad Reviewed By: chad Subscribers: hach-que Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15804 --- src/__phutil_library_map__.php | 2 + .../editor/DiffusionRepositoryEditEngine.php | 11 +++ ...ionRepositoryAutomationManagementPanel.php | 69 +++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 src/applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 953e5a4eb8..337d4406a9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -743,6 +743,7 @@ phutil_register_library_map(array( 'DiffusionRefTableController' => 'applications/diffusion/controller/DiffusionRefTableController.php', 'DiffusionRefsQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php', 'DiffusionRenameHistoryQuery' => 'applications/diffusion/query/DiffusionRenameHistoryQuery.php', + 'DiffusionRepositoryAutomationManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php', 'DiffusionRepositoryBasicsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php', 'DiffusionRepositoryByIDRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php', 'DiffusionRepositoryClusterEngine' => 'applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php', @@ -4959,6 +4960,7 @@ phutil_register_library_map(array( 'DiffusionRefTableController' => 'DiffusionController', 'DiffusionRefsQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionRenameHistoryQuery' => 'Phobject', + 'DiffusionRepositoryAutomationManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryBasicsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryByIDRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'DiffusionRepositoryClusterEngine' => 'Phobject', diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 0a33d87712..dc3db7d0d5 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -162,6 +162,17 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Set the staging area URI.')) ->setConduitTypeDescription(pht('New staging area URI.')) ->setValue($object->getStagingURI()), + id(new PhabricatorDatasourceEditField()) + ->setKey('automationBlueprintPHIDs') + ->setLabel(pht('Use Blueprints')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS) + ->setIsCopyable(true) + ->setDatasource(new DrydockBlueprintDatasource()) + ->setDescription(pht('Automation blueprints.')) + ->setConduitDescription(pht('Change automation blueprints.')) + ->setConduitTypeDescription(pht('New blueprint PHIDs.')) + ->setValue($object->getAutomationBlueprintPHIDs()), id(new PhabricatorPolicyEditField()) ->setKey('policy.push') ->setLabel(pht('Push Policy')) diff --git a/src/applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php new file mode 100644 index 0000000000..beed039c4f --- /dev/null +++ b/src/applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php @@ -0,0 +1,69 @@ +getRepository(); + $viewer = $this->getViewer(); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + PhabricatorPolicyCapability::CAN_EDIT); + + $can_test = $can_edit && $repository->canPerformAutomation(); + + $automation_uri = $repository->getPathURI('edit/automation/'); + $test_uri = $repository->getPathURI('edit/testautomation/'); + + return array( + id(new PhabricatorActionView()) + ->setIcon('fa-pencil') + ->setName(pht('Edit Automation')) + ->setHref($automation_uri) + ->setDisabled(!$can_edit) + ->setWorkflow(!$can_edit), + id(new PhabricatorActionView()) + ->setIcon('fa-gamepad') + ->setName(pht('Test Configuration')) + ->setWorkflow(true) + ->setDisabled(!$can_test) + ->setHref($test_uri), + ); + } + + public function buildManagementPanelContent() { + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setViewer($viewer) + ->setActionList($this->newActions()); + + $blueprint_phids = $repository->getAutomationBlueprintPHIDs(); + if (!$blueprint_phids) { + $blueprint_view = phutil_tag('em', array(), pht('Not Configured')); + } else { + $blueprint_view = id(new DrydockObjectAuthorizationView()) + ->setUser($viewer) + ->setObjectPHID($repository->getPHID()) + ->setBlueprintPHIDs($blueprint_phids); + } + + $view->addProperty(pht('Automation'), $blueprint_view); + + return $this->newBox(pht('Automation'), $view); + } + +} From 63bbe6b129f02f65e90754b70f5d11dfc919f9fa Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 Apr 2016 16:02:57 -0700 Subject: [PATCH 19/32] Port "Allow Dangerous Changes" to new Manage UI Summary: Ref T10748. Brings this forward in the UI and EditEngine. Test Plan: - Edited via Conduit. - Viewed via Manage UI. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15805 --- src/__phutil_library_map__.php | 4 ++- .../ConduitBoolParameterType.php | 2 +- .../editor/DiffusionRepositoryEditEngine.php | 10 ++++++ ...ffusionRepositoryBasicsManagementPanel.php | 32 +++++++++++++++++++ .../editfield/PhabricatorBoolEditField.php | 23 +++++++++++++ 5 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 src/applications/transactions/editfield/PhabricatorBoolEditField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 337d4406a9..becb311d7b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1934,6 +1934,7 @@ phutil_register_library_map(array( 'PhabricatorBoardLayoutEngine' => 'applications/project/engine/PhabricatorBoardLayoutEngine.php', 'PhabricatorBoardRenderingEngine' => 'applications/project/engine/PhabricatorBoardRenderingEngine.php', 'PhabricatorBoardResponseEngine' => 'applications/project/engine/PhabricatorBoardResponseEngine.php', + 'PhabricatorBoolEditField' => 'applications/transactions/editfield/PhabricatorBoolEditField.php', 'PhabricatorBot' => 'infrastructure/daemon/bot/PhabricatorBot.php', 'PhabricatorBotChannel' => 'infrastructure/daemon/bot/target/PhabricatorBotChannel.php', 'PhabricatorBotDebugLogHandler' => 'infrastructure/daemon/bot/handler/PhabricatorBotDebugLogHandler.php', @@ -4426,7 +4427,7 @@ phutil_register_library_map(array( 'ConduitAPIRequest' => 'Phobject', 'ConduitAPIResponse' => 'Phobject', 'ConduitApplicationNotInstalledException' => 'ConduitMethodNotFoundException', - 'ConduitBoolParameterType' => 'ConduitListParameterType', + 'ConduitBoolParameterType' => 'ConduitParameterType', 'ConduitCall' => 'Phobject', 'ConduitCallTestCase' => 'PhabricatorTestCase', 'ConduitColumnsParameterType' => 'ConduitParameterType', @@ -6363,6 +6364,7 @@ phutil_register_library_map(array( 'PhabricatorBoardLayoutEngine' => 'Phobject', 'PhabricatorBoardRenderingEngine' => 'Phobject', 'PhabricatorBoardResponseEngine' => 'Phobject', + 'PhabricatorBoolEditField' => 'PhabricatorEditField', 'PhabricatorBot' => 'PhabricatorDaemon', 'PhabricatorBotChannel' => 'PhabricatorBotTarget', 'PhabricatorBotDebugLogHandler' => 'PhabricatorBotHandler', diff --git a/src/applications/conduit/parametertype/ConduitBoolParameterType.php b/src/applications/conduit/parametertype/ConduitBoolParameterType.php index e15925827e..fe1564350f 100644 --- a/src/applications/conduit/parametertype/ConduitBoolParameterType.php +++ b/src/applications/conduit/parametertype/ConduitBoolParameterType.php @@ -1,7 +1,7 @@ setConduitDescription(pht('Change the default text encoding.')) ->setConduitTypeDescription(pht('New text encoding.')) ->setValue($object->getDetail('encoding')), + id(new PhabricatorBoolEditField()) + ->setKey('allowDangerousChanges') + ->setLabel(pht('Allow Dangerous Changes')) + ->setIsCopyable(true) + ->setIsConduitOnly(true) + ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_DANGEROUS) + ->setDescription(pht('Permit dangerous changes to be made.')) + ->setConduitDescription(pht('Allow or prevent dangerous changes.')) + ->setConduitTypeDescription(pht('New protection setting.')) + ->setValue($object->shouldAllowDangerousChanges()), id(new PhabricatorSelectEditField()) ->setKey('status') ->setLabel(pht('Status')) diff --git a/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php index d8614923ce..7ddb38d34c 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php @@ -26,6 +26,7 @@ final class DiffusionRepositoryBasicsManagementPanel $activate_uri = $repository->getPathURI('edit/activate/'); $delete_uri = $repository->getPathURI('edit/delete/'); $encoding_uri = $repository->getPathURI('edit/encoding/'); + $dangerous_uri = $repository->getPathURI('edit/dangerous/'); if ($repository->isTracked()) { $activate_icon = 'fa-pause'; @@ -35,6 +36,17 @@ final class DiffusionRepositoryBasicsManagementPanel $activate_label = pht('Activate Repository'); } + $should_dangerous = $repository->shouldAllowDangerousChanges(); + if ($should_dangerous) { + $dangerous_icon = 'fa-shield'; + $dangerous_name = pht('Prevent Dangerous Changes'); + $can_dangerous = $can_edit; + } else { + $dangerous_icon = 'fa-bullseye'; + $dangerous_name = pht('Allow Dangerous Changes'); + $can_dangerous = ($can_edit && $repository->canAllowDangerousChanges()); + } + return array( id(new PhabricatorActionView()) ->setIcon('fa-pencil') @@ -48,6 +60,12 @@ final class DiffusionRepositoryBasicsManagementPanel ->setHref($encoding_uri) ->setDisabled(!$can_edit) ->setWorkflow(!$can_edit), + id(new PhabricatorActionView()) + ->setIcon($dangerous_icon) + ->setName($dangerous_name) + ->setHref($dangerous_uri) + ->setDisabled(!$can_dangerous) + ->setWorkflow(true), id(new PhabricatorActionView()) ->setHref($activate_uri) ->setIcon($activate_icon) @@ -110,6 +128,20 @@ final class DiffusionRepositoryBasicsManagementPanel } $view->addProperty(pht('Encoding'), $encoding); + $can_dangerous = $repository->canAllowDangerousChanges(); + if (!$can_dangerous) { + $dangerous = phutil_tag('em', array(), pht('Not Preventable')); + } else { + $should_dangerous = $repository->shouldAllowDangerousChanges(); + if ($should_dangerous) { + $dangerous = pht('Allowed'); + } else { + $dangerous = pht('Not Allowed'); + } + } + + $view->addProperty(pht('Dangerous Changes'), $dangerous); + return $view; } diff --git a/src/applications/transactions/editfield/PhabricatorBoolEditField.php b/src/applications/transactions/editfield/PhabricatorBoolEditField.php new file mode 100644 index 0000000000..1383826b2f --- /dev/null +++ b/src/applications/transactions/editfield/PhabricatorBoolEditField.php @@ -0,0 +1,23 @@ +setOptions( + array( + '0' => pht('False'), + '1' => pht('True'), + )); + } + + protected function newHTTPParameterType() { + return new AphrontBoolHTTPParameterType(); + } + + protected function newConduitParameterType() { + return new ConduitBoolParameterType(); + } + +} From fbc4967154ba1f647cb31a0745a1d52518749955 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 27 Apr 2016 06:42:32 -0700 Subject: [PATCH 20/32] Improve cache behaviors for font files and other nonstandard resource types Summary: Ref T10843. There are actually two separate notions of cacheability here: - Is this cacheable by the browser (e.g., should we emit "Expires: long in the future")? - Is this cacheable locally (e.g., should we stick it in APC, or just read it off disk every time)? These got a little mixed up by D15775, so we aren't currently emitting proper "Expires" headers on font files and a few other resource types. Straighten this out so that we "Expires" these unusual resources correctly. Test Plan: Verified that `.woff` files get a proper "Expires" header now, not just CSS/JS. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10843 Differential Revision: https://secure.phabricator.com/D15807 --- .../celerity/controller/CelerityResourceController.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/applications/celerity/controller/CelerityResourceController.php b/src/applications/celerity/controller/CelerityResourceController.php index debb37c1ca..c1ea652084 100644 --- a/src/applications/celerity/controller/CelerityResourceController.php +++ b/src/applications/celerity/controller/CelerityResourceController.php @@ -50,8 +50,8 @@ abstract class CelerityResourceController extends PhabricatorController { // is not, refuse to cache this resource. This avoids poisoning caches // and CDNs if we're getting a request for a new resource to an old node // shortly after a push. - $is_cacheable = ($hash === $expect_hash) && - $this->isCacheableResourceType($type); + $is_cacheable = ($hash === $expect_hash); + $is_locally_cacheable = $this->isLocallyCacheableResourceType($type); if (AphrontRequest::getHTTPHeader('If-Modified-Since') && $is_cacheable) { // Return a "304 Not Modified". We don't care about the value of this // field since we never change what resource is served by a given URI. @@ -60,7 +60,7 @@ abstract class CelerityResourceController extends PhabricatorController { $cache = null; $data = null; - if ($is_cacheable && !$dev_mode) { + if ($is_cacheable && $is_locally_cacheable && !$dev_mode) { $cache = PhabricatorCaches::getImmutableCache(); $request_path = $this->getRequest()->getPath(); @@ -168,7 +168,7 @@ abstract class CelerityResourceController extends PhabricatorController { * @param string Resource type. * @return bool True to enable caching. */ - private function isCacheableResourceType($type) { + private function isLocallyCacheableResourceType($type) { $types = array( 'js' => true, 'css' => true, From 8f81930b5d544f64b667cc91576b7c8324b4385e Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 27 Apr 2016 10:32:43 -0700 Subject: [PATCH 21/32] Port Repository "Symbols" to Manage/Panel UI Summary: Ref T10748. Port this, add EditEngine support, add some type validation to the transaction. Test Plan: - Edited via EditEngine. - Edited via Conduit. - Viewed via Management UI. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15808 --- src/__phutil_library_map__.php | 2 + .../editor/DiffusionRepositoryEditEngine.php | 24 +++++++ ...fusionRepositoryHistoryManagementPanel.php | 2 +- ...fusionRepositorySymbolsManagementPanel.php | 64 +++++++++++++++++++ .../editor/PhabricatorRepositoryEditor.php | 37 +++++++++++ 5 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 src/applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index becb311d7b..09730900c4 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -784,6 +784,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryStatusManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php', 'DiffusionRepositoryStorageManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryStorageManagementPanel.php', 'DiffusionRepositorySymbolsController' => 'applications/diffusion/controller/DiffusionRepositorySymbolsController.php', + 'DiffusionRepositorySymbolsManagementPanel' => 'applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php', 'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php', 'DiffusionRepositoryTestAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php', 'DiffusionRepositoryURIsIndexEngineExtension' => 'applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php', @@ -5001,6 +5002,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryStatusManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryStorageManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositorySymbolsController' => 'DiffusionRepositoryEditController', + 'DiffusionRepositorySymbolsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryTag' => 'Phobject', 'DiffusionRepositoryTestAutomationController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryURIsIndexEngineExtension' => 'PhabricatorIndexEngineExtension', diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 37af501bf6..b31f6a2469 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -183,6 +183,30 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Change automation blueprints.')) ->setConduitTypeDescription(pht('New blueprint PHIDs.')) ->setValue($object->getAutomationBlueprintPHIDs()), + id(new PhabricatorStringListEditField()) + ->setKey('symbolLanguages') + ->setLabel(pht('Languages')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE) + ->setIsCopyable(true) + ->setDescription( + pht('Languages which define symbols in this repository.')) + ->setConduitDescription( + pht('Change symbol languages for this repository.')) + ->setConduitTypeDescription( + pht('New symbol langauges.')) + ->setValue($object->getSymbolLanguages()), + id(new PhabricatorDatasourceEditField()) + ->setKey('symbolRepositoryPHIDs') + ->setLabel(pht('Uses Symbols From')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES) + ->setIsCopyable(true) + ->setDatasource(new DiffusionRepositoryDatasource()) + ->setDescription(pht('Repositories to link symbols from.')) + ->setConduitDescription(pht('Change symbol source repositories.')) + ->setConduitTypeDescription(pht('New symbol repositories.')) + ->setValue($object->getSymbolSources()), id(new PhabricatorPolicyEditField()) ->setKey('policy.push') ->setLabel(pht('Push Policy')) diff --git a/src/applications/diffusion/management/DiffusionRepositoryHistoryManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryHistoryManagementPanel.php index c98d3d76ca..e574030bfc 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryHistoryManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryHistoryManagementPanel.php @@ -10,7 +10,7 @@ final class DiffusionRepositoryHistoryManagementPanel } public function getManagementPanelOrder() { - return 900; + return 2000; } public function buildManagementPanelContent() { diff --git a/src/applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php new file mode 100644 index 0000000000..4e934c67af --- /dev/null +++ b/src/applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php @@ -0,0 +1,64 @@ +getRepository(); + $viewer = $this->getViewer(); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + PhabricatorPolicyCapability::CAN_EDIT); + + $symbols_uri = $repository->getPathURI('edit/symbols/'); + + return array( + id(new PhabricatorActionView()) + ->setIcon('fa-pencil') + ->setName(pht('Edit Symbols')) + ->setHref($symbols_uri) + ->setDisabled(!$can_edit) + ->setWorkflow(!$can_edit), + ); + } + + public function buildManagementPanelContent() { + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setViewer($viewer) + ->setActionList($this->newActions()); + + $languages = $repository->getSymbolLanguages(); + if ($languages) { + $languages = implode(', ', $languages); + } else { + $languages = phutil_tag('em', array(), pht('Any')); + } + $view->addProperty(pht('Languages'), $languages); + + $sources = $repository->getSymbolSources(); + if ($sources) { + $sources = $viewer->renderHandleList($sources); + } else { + $sources = phutil_tag('em', array(), pht('This Repository Only')); + } + $view->addProperty(pht('Uses Symbols From'), $sources); + + return $this->newBox(pht('Symbols'), $view); + } + +} diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 9ef775fe2d..02f5d2bd19 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -636,6 +636,43 @@ final class PhabricatorRepositoryEditor } } break; + + case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES: + foreach ($xactions as $xaction) { + $old = $object->getSymbolSources(); + $new = $xaction->getNewValue(); + + // If the viewer is adding new repositories, make sure they are + // valid and visible. + $add = array_diff($new, $old); + if (!$add) { + continue; + } + + $repositories = id(new PhabricatorRepositoryQuery()) + ->setViewer($this->getActor()) + ->withPHIDs($add) + ->execute(); + $repositories = mpull($repositories, null, 'getPHID'); + + foreach ($add as $phid) { + if (isset($repositories[$phid])) { + continue; + } + + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Repository ("%s") does not exist, or you do not have '. + 'permission to see it.', + $phid), + $xaction); + break; + } + } + break; + } return $errors; From 4c66a92f9246d90767e57abf976c75423b951f36 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 27 Apr 2016 10:51:13 -0700 Subject: [PATCH 22/32] Port Repository "Branches" to new UI Summary: Ref T10748. Makes a "Branches" panel, enables these transactions in the EditEngine. Test Plan: - Edited via EditEngine + Conduit. - Viewed via manage UI. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15809 --- src/__phutil_library_map__.php | 2 + .../editor/DiffusionRepositoryEditEngine.php | 28 ++++++++ ...usionRepositoryBranchesManagementPanel.php | 68 +++++++++++++++++++ .../PhabricatorTextAreaEditField.php | 29 +++++++- 4 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 09730900c4..a603bce0d9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -745,6 +745,7 @@ phutil_register_library_map(array( 'DiffusionRenameHistoryQuery' => 'applications/diffusion/query/DiffusionRenameHistoryQuery.php', 'DiffusionRepositoryAutomationManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php', 'DiffusionRepositoryBasicsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php', + 'DiffusionRepositoryBranchesManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php', 'DiffusionRepositoryByIDRemarkupRule' => 'applications/diffusion/remarkup/DiffusionRepositoryByIDRemarkupRule.php', 'DiffusionRepositoryClusterEngine' => 'applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php', 'DiffusionRepositoryClusterEngineLogInterface' => 'applications/diffusion/protocol/DiffusionRepositoryClusterEngineLogInterface.php', @@ -4964,6 +4965,7 @@ phutil_register_library_map(array( 'DiffusionRenameHistoryQuery' => 'Phobject', 'DiffusionRepositoryAutomationManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryBasicsManagementPanel' => 'DiffusionRepositoryManagementPanel', + 'DiffusionRepositoryBranchesManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryByIDRemarkupRule' => 'PhabricatorObjectRemarkupRule', 'DiffusionRepositoryClusterEngine' => 'Phobject', 'DiffusionRepositoryController' => 'DiffusionController', diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index b31f6a2469..4fe1d2fe77 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -75,6 +75,12 @@ final class DiffusionRepositoryEditEngine ->setObject($object) ->execute(); + $track_value = $object->getDetail('branch-filter', array()); + $track_value = array_keys($track_value); + + $autoclose_value = $object->getDetail('close-commits-filter', array()); + $autoclose_value = array_keys($autoclose_value); + return array( id(new PhabricatorSelectEditField()) ->setKey('vcs') @@ -162,6 +168,28 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Set the default branch name.')) ->setConduitTypeDescription(pht('New default branch name.')) ->setValue($object->getDetail('default-branch')), + id(new PhabricatorTextAreaEditField()) + ->setIsStringList(true) + ->setKey('trackOnly') + ->setLabel(pht('Track Only')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_TRACK_ONLY) + ->setIsCopyable(true) + ->setDescription(pht('Track only these branches.')) + ->setConduitDescription(pht('Set the tracked branches.')) + ->setConduitTypeDescription(pht('New tracked branchs.')) + ->setValue($track_value), + id(new PhabricatorTextAreaEditField()) + ->setIsStringList(true) + ->setKey('autocloseOnly') + ->setLabel(pht('Autoclose Only')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE_ONLY) + ->setIsCopyable(true) + ->setDescription(pht('Autoclose commits on only these branches.')) + ->setConduitDescription(pht('Set the autoclose branches.')) + ->setConduitTypeDescription(pht('New default tracked branchs.')) + ->setValue($autoclose_value), id(new PhabricatorTextEditField()) ->setKey('stagingAreaURI') ->setLabel(pht('Staging Area URI')) diff --git a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php new file mode 100644 index 0000000000..54184cd8ba --- /dev/null +++ b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php @@ -0,0 +1,68 @@ +getRepository(); + $viewer = $this->getViewer(); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + PhabricatorPolicyCapability::CAN_EDIT); + + $branches_uri = $repository->getPathURI('edit/branches/'); + + return array( + id(new PhabricatorActionView()) + ->setIcon('fa-pencil') + ->setName(pht('Edit Branches')) + ->setHref($branches_uri) + ->setDisabled(!$can_edit) + ->setWorkflow(!$can_edit), + ); + } + + public function buildManagementPanelContent() { + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setViewer($viewer) + ->setActionList($this->newActions()); + + $default_branch = nonempty( + $repository->getHumanReadableDetail('default-branch'), + phutil_tag('em', array(), $repository->getDefaultBranch())); + $view->addProperty(pht('Default Branch'), $default_branch); + + $track_only = nonempty( + $repository->getHumanReadableDetail('branch-filter', array()), + phutil_tag('em', array(), pht('Track All Branches'))); + $view->addProperty(pht('Track Only'), $track_only); + + $autoclose_only = nonempty( + $repository->getHumanReadableDetail('close-commits-filter', array()), + phutil_tag('em', array(), pht('Autoclose On All Branches'))); + + if ($repository->getDetail('disable-autoclose')) { + $autoclose_only = phutil_tag('em', array(), pht('Disabled')); + } + + $view->addProperty(pht('Autoclose Only'), $autoclose_only); + + return $this->newBox(pht('Branches'), $view); + } + +} diff --git a/src/applications/transactions/editfield/PhabricatorTextAreaEditField.php b/src/applications/transactions/editfield/PhabricatorTextAreaEditField.php index 2c7b2daa98..4c07b31e54 100644 --- a/src/applications/transactions/editfield/PhabricatorTextAreaEditField.php +++ b/src/applications/transactions/editfield/PhabricatorTextAreaEditField.php @@ -5,6 +5,7 @@ final class PhabricatorTextAreaEditField private $monospaced; private $height; + private $isStringList; public function setMonospaced($monospaced) { $this->monospaced = $monospaced; @@ -24,6 +25,15 @@ final class PhabricatorTextAreaEditField return $this->height; } + public function setIsStringList($is_string_list) { + $this->isStringList = $is_string_list; + return $this; + } + + public function getIsStringList() { + return $this->isStringList; + } + protected function newControl() { $control = new AphrontFormTextAreaControl(); @@ -39,8 +49,25 @@ final class PhabricatorTextAreaEditField return $control; } + protected function getValueForControl() { + $value = $this->getValue(); + return implode("\n", $value); + } + protected function newConduitParameterType() { - return new ConduitStringParameterType(); + if ($this->getIsStringList()) { + return new ConduitStringListParameterType(); + } else { + return new ConduitStringParameterType(); + } + } + + protected function newHTTPParameterType() { + if ($this->getIsStringList()) { + return new AphrontStringListHTTPParameterType(); + } else { + return new AphrontStringHTTPParameterType(); + } } } From 311de580d6e349f0bfa517fd41dad25d391f3af8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 27 Apr 2016 15:40:35 -0700 Subject: [PATCH 23/32] Port "Actions" to new Repository UI Summary: Ref T10748. This brings the "Actions" items (publish/notify + autoclose enabled) into the new UI. Test Plan: - Edited this stuff via EditEngine and Conduit. - Viewed via new Manage UI. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15811 --- src/__phutil_library_map__.php | 2 + .../editor/DiffusionRepositoryEditEngine.php | 29 +++++++++ ...fusionRepositoryActionsManagementPanel.php | 60 +++++++++++++++++++ .../editor/PhabricatorRepositoryEditor.php | 2 +- .../editengine/PhabricatorEditEngine.php | 5 +- .../editfield/PhabricatorBoolEditField.php | 29 +++++++-- 6 files changed, 119 insertions(+), 8 deletions(-) create mode 100644 src/applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a603bce0d9..de558ee890 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -743,6 +743,7 @@ phutil_register_library_map(array( 'DiffusionRefTableController' => 'applications/diffusion/controller/DiffusionRefTableController.php', 'DiffusionRefsQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionRefsQueryConduitAPIMethod.php', 'DiffusionRenameHistoryQuery' => 'applications/diffusion/query/DiffusionRenameHistoryQuery.php', + 'DiffusionRepositoryActionsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php', 'DiffusionRepositoryAutomationManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryAutomationManagementPanel.php', 'DiffusionRepositoryBasicsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryBasicsManagementPanel.php', 'DiffusionRepositoryBranchesManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php', @@ -4963,6 +4964,7 @@ phutil_register_library_map(array( 'DiffusionRefTableController' => 'DiffusionController', 'DiffusionRefsQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionRenameHistoryQuery' => 'Phobject', + 'DiffusionRepositoryActionsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryAutomationManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryBasicsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryBranchesManagementPanel' => 'DiffusionRepositoryManagementPanel', diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 4fe1d2fe77..d4d1bac841 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -143,6 +143,9 @@ final class DiffusionRepositoryEditEngine ->setLabel(pht('Allow Dangerous Changes')) ->setIsCopyable(true) ->setIsConduitOnly(true) + ->setOptions( + pht('Prevent Dangerous Changes'), + pht('Allow Dangerous Changes')) ->setTransactionType(PhabricatorRepositoryTransaction::TYPE_DANGEROUS) ->setDescription(pht('Permit dangerous changes to be made.')) ->setConduitDescription(pht('Allow or prevent dangerous changes.')) @@ -235,6 +238,32 @@ final class DiffusionRepositoryEditEngine ->setConduitDescription(pht('Change symbol source repositories.')) ->setConduitTypeDescription(pht('New symbol repositories.')) ->setValue($object->getSymbolSources()), + id(new PhabricatorBoolEditField()) + ->setKey('publish') + ->setLabel(pht('Publish/Notify')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_NOTIFY) + ->setIsCopyable(true) + ->setOptions( + pht('Disable Notifications, Feed, and Herald'), + pht('Enable Notifications, Feed, and Herald')) + ->setDescription(pht('Configure how changes are published.')) + ->setConduitDescription(pht('Change publishing options.')) + ->setConduitTypeDescription(pht('New notification setting.')) + ->setValue(!$object->getDetail('herald-disabled')), + id(new PhabricatorBoolEditField()) + ->setKey('autoclose') + ->setLabel(pht('Autoclose')) + ->setTransactionType( + PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE) + ->setIsCopyable(true) + ->setOptions( + pht('Disable Autoclose'), + pht('Enable Autoclose')) + ->setDescription(pht('Stop or resume autoclosing in this repository.')) + ->setConduitDescription(pht('Change autoclose setting.')) + ->setConduitTypeDescription(pht('New autoclose setting.')) + ->setValue(!$object->getDetail('disable-autoclose')), id(new PhabricatorPolicyEditField()) ->setKey('policy.push') ->setLabel(pht('Push Policy')) diff --git a/src/applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php new file mode 100644 index 0000000000..4b9fa5b528 --- /dev/null +++ b/src/applications/diffusion/management/DiffusionRepositoryActionsManagementPanel.php @@ -0,0 +1,60 @@ +getRepository(); + $viewer = $this->getViewer(); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + PhabricatorPolicyCapability::CAN_EDIT); + + $actions_uri = $repository->getPathURI('edit/actions/'); + + return array( + id(new PhabricatorActionView()) + ->setIcon('fa-pencil') + ->setName(pht('Edit Actions')) + ->setHref($actions_uri) + ->setDisabled(!$can_edit) + ->setWorkflow(!$can_edit), + ); + } + + public function buildManagementPanelContent() { + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setViewer($viewer) + ->setActionList($this->newActions()); + + $notify = $repository->getDetail('herald-disabled') + ? pht('Off') + : pht('On'); + $notify = phutil_tag('em', array(), $notify); + $view->addProperty(pht('Publish/Notify'), $notify); + + $autoclose = $repository->getDetail('disable-autoclose') + ? pht('Off') + : pht('On'); + $autoclose = phutil_tag('em', array(), $autoclose); + $view->addProperty(pht('Autoclose'), $autoclose); + + return $this->newBox(pht('Branches'), $view); + } + +} diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 02f5d2bd19..3dc842d94f 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -342,7 +342,7 @@ final class PhabricatorRepositoryEditor $errors = parent::validateTransaction($object, $type, $xactions); switch ($type) { - case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE: + case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE_ONLY: case PhabricatorRepositoryTransaction::TYPE_TRACK_ONLY: foreach ($xactions as $xaction) { foreach ($xaction->getNewValue() as $pattern) { diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 185ac13a5a..7c9c0fbed1 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1808,8 +1808,9 @@ abstract class PhabricatorEditEngine } catch (Exception $ex) { throw new PhutilProxyException( pht( - 'Exception when processing transaction of type "%s".', - $xaction['type']), + 'Exception when processing transaction of type "%s": %s', + $xaction['type'], + $ex->getMessage()), $ex); } diff --git a/src/applications/transactions/editfield/PhabricatorBoolEditField.php b/src/applications/transactions/editfield/PhabricatorBoolEditField.php index 1383826b2f..11eabf2bad 100644 --- a/src/applications/transactions/editfield/PhabricatorBoolEditField.php +++ b/src/applications/transactions/editfield/PhabricatorBoolEditField.php @@ -3,13 +3,32 @@ final class PhabricatorBoolEditField extends PhabricatorEditField { + private $options; + + public function setOptions($off_label, $on_label) { + $this->options = array( + '0' => $off_label, + '1' => $on_label, + ); + return $this; + } + + public function getOptions() { + return $this->options; + } + protected function newControl() { + $options = $this->getOptions(); + + if (!$options) { + $options = array( + '0' => pht('False'), + '1' => pht('True'), + ); + } + return id(new AphrontFormSelectControl()) - ->setOptions( - array( - '0' => pht('False'), - '1' => pht('True'), - )); + ->setOptions($options); } protected function newHTTPParameterType() { From 0459e95242573b0c0594a0c5d20c3b18c250657c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 27 Apr 2016 16:01:29 -0700 Subject: [PATCH 24/32] Give users a modal VCS choice when creating a new repository Summary: Ref T10748. Allow the new EditEngine workflow to create repositories by giving the user a modal repository type choice upfront. (The rest of this flow is still confusing/weird, though.) Test Plan: - Created a new repository. {F1249626} Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15813 --- .../DiffusionRepositoryEditproController.php | 63 ++++++++++++++++++- .../editor/DiffusionRepositoryEditEngine.php | 20 +++++- ...ffusionRepositoryStatusManagementPanel.php | 38 +++++++---- ...DiffusionRepositoryURIsManagementPanel.php | 2 +- .../constants/PhabricatorRepositoryType.php | 40 +++++++++--- 5 files changed, 139 insertions(+), 24 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryEditproController.php b/src/applications/diffusion/controller/DiffusionRepositoryEditproController.php index 1a8e1a8668..79febc2cb7 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryEditproController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryEditproController.php @@ -4,9 +4,66 @@ final class DiffusionRepositoryEditproController extends DiffusionRepositoryEditController { public function handleRequest(AphrontRequest $request) { - return id(new DiffusionRepositoryEditEngine()) - ->setController($this) - ->buildResponse(); + $engine = id(new DiffusionRepositoryEditEngine()) + ->setController($this); + + $id = $request->getURIData('id'); + if (!$id) { + $this->requireApplicationCapability( + DiffusionCreateRepositoriesCapability::CAPABILITY); + + $vcs = $request->getStr('vcs'); + $vcs_types = PhabricatorRepositoryType::getRepositoryTypeMap(); + if (empty($vcs_types[$vcs])) { + return $this->buildVCSTypeResponse(); + } + + $engine + ->addContextParameter('vcs', $vcs) + ->setVersionControlSystem($vcs); + } + + return $engine->buildResponse(); + } + + private function buildVCSTypeResponse() { + $vcs_types = PhabricatorRepositoryType::getRepositoryTypeMap(); + + $request = $this->getRequest(); + $viewer = $this->getViewer(); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb(pht('Create Repository')); + $crumbs->setBorder(true); + + $title = pht('Choose Repository Type'); + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Create Repository')) + ->setHeaderIcon('fa-plus-square'); + + $layout = id(new AphrontMultiColumnView()) + ->setFluidLayout(true); + + $create_uri = $request->getRequestURI(); + + foreach ($vcs_types as $vcs_key => $vcs_type) { + $action = id(new PHUIActionPanelView()) + ->setIcon(idx($vcs_type, 'icon')) + ->setHeader(idx($vcs_type, 'create.header')) + ->setHref($create_uri->alter('vcs', $vcs_key)) + ->setSubheader(idx($vcs_type, 'create.subheader')); + + $layout->addColumn($action); + } + + $view = id(new PHUITwoColumnView()) + ->setHeader($header) + ->setFooter($layout); + + return $this->newPage() + ->setTitle($title) + ->setCrumbs($crumbs) + ->appendChild($view); } } diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index d4d1bac841..1ebbabe2ed 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -5,6 +5,17 @@ final class DiffusionRepositoryEditEngine const ENGINECONST = 'diffusion.repository'; + private $versionControlSystem; + + public function setVersionControlSystem($version_control_system) { + $this->versionControlSystem = $version_control_system; + return $this; + } + + public function getVersionControlSystem() { + return $this->versionControlSystem; + } + public function isEngineConfigurable() { return false; } @@ -27,7 +38,14 @@ final class DiffusionRepositoryEditEngine protected function newEditableObject() { $viewer = $this->getViewer(); - return PhabricatorRepository::initializeNewRepository($viewer); + $repository = PhabricatorRepository::initializeNewRepository($viewer); + + $vcs = $this->getVersionControlSystem(); + if ($vcs) { + $repository->setVersionControlSystem($vcs); + } + + return $repository; } protected function newObjectQuery() { diff --git a/src/applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php index db31e0d71e..1666749980 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryStatusManagementPanel.php @@ -46,8 +46,12 @@ final class DiffusionRepositoryStatusManagementPanel pht('Update Frequency'), $this->buildRepositoryUpdateInterval($repository)); + $messages = id(new PhabricatorRepositoryStatusMessage()) + ->loadAllWhere('repositoryID = %d', $repository->getID()); + $messages = mpull($messages, null, 'getStatusType'); - list($status, $raw_error) = $this->buildRepositoryStatus($repository); + $status = $this->buildRepositoryStatus($repository, $messages); + $raw_error = $this->buildRepositoryRawError($repository, $messages); $view->addProperty(pht('Status'), $status); if ($raw_error) { @@ -80,17 +84,14 @@ final class DiffusionRepositoryStatusManagementPanel } private function buildRepositoryStatus( - PhabricatorRepository $repository) { + PhabricatorRepository $repository, + array $messages) { $viewer = $this->getViewer(); $is_cluster = $repository->getAlmanacServicePHID(); $view = new PHUIStatusListView(); - $messages = id(new PhabricatorRepositoryStatusMessage()) - ->loadAllWhere('repositoryID = %d', $repository->getID()); - $messages = mpull($messages, null, 'getStatusType'); - if ($repository->isTracked()) { $view->addItem( id(new PHUIStatusItemView()) @@ -361,8 +362,6 @@ final class DiffusionRepositoryStatusManagementPanel } } - $raw_error = null; - $message = idx($messages, PhabricatorRepositoryStatusMessage::TYPE_FETCH); if ($message) { switch ($message->getStatusCode()) { @@ -377,8 +376,6 @@ final class DiffusionRepositoryStatusManagementPanel 'access the repository.'); } - $raw_error = $message; - $view->addItem( id(new PHUIStatusItemView()) ->setIcon(PHUIStatusItemView::ICON_WARNING, 'red') @@ -432,11 +429,30 @@ final class DiffusionRepositoryStatusManagementPanel ->setNote(pht('This repository will be updated soon!'))); } + return $view; + } + + private function buildRepositoryRawError( + PhabricatorRepository $repository, + array $messages) { + $viewer = $this->getViewer(); + $can_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $repository, PhabricatorPolicyCapability::CAN_EDIT); + $raw_error = null; + + $message = idx($messages, PhabricatorRepositoryStatusMessage::TYPE_FETCH); + if ($message) { + switch ($message->getStatusCode()) { + case PhabricatorRepositoryStatusMessage::CODE_ERROR: + $raw_error = $message->getParameter('message'); + break; + } + } + if ($raw_error !== null) { if (!$can_edit) { $raw_message = pht( @@ -450,7 +466,7 @@ final class DiffusionRepositoryStatusManagementPanel $raw_message = null; } - return array($view, $raw_message); + return $raw_message; } diff --git a/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php index 3535c64bbc..2eb1991f07 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php @@ -106,7 +106,7 @@ final class DiffusionRepositoryURIsManagementPanel )); $doc_href = PhabricatorEnv::getDoclink( - 'Diffusion User Guide: Repository URIs'); + 'Diffusion User Guide: URIs'); $header = id(new PHUIHeaderView()) ->setHeader(pht('Repository URIs')) diff --git a/src/applications/repository/constants/PhabricatorRepositoryType.php b/src/applications/repository/constants/PhabricatorRepositoryType.php index 4861641411..998bff0d2e 100644 --- a/src/applications/repository/constants/PhabricatorRepositoryType.php +++ b/src/applications/repository/constants/PhabricatorRepositoryType.php @@ -7,17 +7,41 @@ final class PhabricatorRepositoryType extends Phobject { const REPOSITORY_TYPE_MERCURIAL = 'hg'; public static function getAllRepositoryTypes() { - $map = array( - self::REPOSITORY_TYPE_GIT => pht('Git'), - self::REPOSITORY_TYPE_MERCURIAL => pht('Mercurial'), - self::REPOSITORY_TYPE_SVN => pht('Subversion'), - ); - return $map; + $map = self::getRepositoryTypeMap(); + return ipull($map, 'name'); } public static function getNameForRepositoryType($type) { - $map = self::getAllRepositoryTypes(); - return idx($map, $type, pht('Unknown')); + $spec = self::getRepositoryTypeSpec($type); + return idx($spec, 'name', pht('Unknown ("%s")', $type)); + } + + public static function getRepositoryTypeSpec($type) { + $map = self::getRepositoryTypeMap(); + return idx($map, $type, array()); + } + + public static function getRepositoryTypeMap() { + return array( + self::REPOSITORY_TYPE_GIT => array( + 'name' => pht('Git'), + 'icon' => 'fa-git', + 'create.header' => pht('Create Git Repository'), + 'create.subheader' => pht('Create a new Git repository.'), + ), + self::REPOSITORY_TYPE_MERCURIAL => array( + 'name' => pht('Mercurial'), + 'icon' => 'fa-code-fork', + 'create.header' => pht('Create Mercurial Repository'), + 'create.subheader' => pht('Create a new Mercurial repository.'), + ), + self::REPOSITORY_TYPE_SVN => array( + 'name' => pht('Subversion'), + 'icon' => 'fa-database', + 'create.header' => pht('Create Subversion Repository'), + 'create.subheader' => pht('Create a new Subversion repository.'), + ), + ); } } From 616c9ae88777dac5f5e12c50d490db44221179ad Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Apr 2016 08:33:44 -0700 Subject: [PATCH 25/32] Rough sketch of new repository URI editing Summary: Ref T10748. Ref T10366. This adds a new EditEngine, EditController, Editor, Query, and Transaction for RepositoryURIs. None of these really do anything helpful yet, and these URIs are still unused in the actual application. Test Plan: {F1249794} Reviewers: chad Reviewed By: chad Maniphest Tasks: T10366, T10748 Differential Revision: https://secure.phabricator.com/D15815 --- .../20160428.repo.1.urixaction.sql | 19 ++++ src/__phutil_library_map__.php | 19 +++- .../PhabricatorDiffusionApplication.php | 2 + .../DiffusionRepositoryURIEditController.php | 21 ++++ .../editor/DiffusionURIEditEngine.php | 93 +++++++++++++++ .../diffusion/editor/DiffusionURIEditor.php | 99 ++++++++++++++++ ...DiffusionRepositoryURIsManagementPanel.php | 8 +- .../diffusion/request/DiffusionRequest.php | 3 +- .../phid/PhabricatorRepositoryURIPHIDType.php | 40 +++++++ .../query/PhabricatorRepositoryQuery.php | 20 ++++ .../query/PhabricatorRepositoryURIQuery.php | 106 ++++++++++++++++++ .../storage/PhabricatorRepositoryURI.php | 80 ++++++++++++- .../PhabricatorRepositoryURITransaction.php | 20 ++++ 13 files changed, 524 insertions(+), 6 deletions(-) create mode 100644 resources/sql/autopatches/20160428.repo.1.urixaction.sql create mode 100644 src/applications/diffusion/controller/DiffusionRepositoryURIEditController.php create mode 100644 src/applications/diffusion/editor/DiffusionURIEditEngine.php create mode 100644 src/applications/diffusion/editor/DiffusionURIEditor.php create mode 100644 src/applications/repository/phid/PhabricatorRepositoryURIPHIDType.php create mode 100644 src/applications/repository/query/PhabricatorRepositoryURIQuery.php create mode 100644 src/applications/repository/storage/PhabricatorRepositoryURITransaction.php diff --git a/resources/sql/autopatches/20160428.repo.1.urixaction.sql b/resources/sql/autopatches/20160428.repo.1.urixaction.sql new file mode 100644 index 0000000000..54d50dfeb7 --- /dev/null +++ b/resources/sql/autopatches/20160428.repo.1.urixaction.sql @@ -0,0 +1,19 @@ +CREATE TABLE {$NAMESPACE}_repository.repository_uritransaction ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARBINARY(64) NOT NULL, + authorPHID VARBINARY(64) NOT NULL, + objectPHID VARBINARY(64) NOT NULL, + viewPolicy VARBINARY(64) NOT NULL, + editPolicy VARBINARY(64) NOT NULL, + commentPHID VARBINARY(64) DEFAULT NULL, + commentVersion INT UNSIGNED NOT NULL, + transactionType VARCHAR(32) COLLATE {$COLLATE_TEXT} NOT NULL, + oldValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + newValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + contentSource LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + metadata LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_phid` (`phid`), + KEY `key_object` (`objectPHID`) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index de558ee890..1384501c51 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -789,6 +789,7 @@ phutil_register_library_map(array( 'DiffusionRepositorySymbolsManagementPanel' => 'applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php', 'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php', 'DiffusionRepositoryTestAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php', + 'DiffusionRepositoryURIEditController' => 'applications/diffusion/controller/DiffusionRepositoryURIEditController.php', 'DiffusionRepositoryURIsIndexEngineExtension' => 'applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php', 'DiffusionRepositoryURIsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php', 'DiffusionRequest' => 'applications/diffusion/request/DiffusionRequest.php', @@ -814,6 +815,8 @@ phutil_register_library_map(array( 'DiffusionTagListController' => 'applications/diffusion/controller/DiffusionTagListController.php', 'DiffusionTagListView' => 'applications/diffusion/view/DiffusionTagListView.php', 'DiffusionTagsQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionTagsQueryConduitAPIMethod.php', + 'DiffusionURIEditEngine' => 'applications/diffusion/editor/DiffusionURIEditEngine.php', + 'DiffusionURIEditor' => 'applications/diffusion/editor/DiffusionURIEditor.php', 'DiffusionURITestCase' => 'applications/diffusion/request/__tests__/DiffusionURITestCase.php', 'DiffusionUpdateCoverageConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionUpdateCoverageConduitAPIMethod.php', 'DiffusionView' => 'applications/diffusion/view/DiffusionView.php', @@ -3237,7 +3240,10 @@ phutil_register_library_map(array( 'PhabricatorRepositoryURIIndex' => 'applications/repository/storage/PhabricatorRepositoryURIIndex.php', 'PhabricatorRepositoryURINormalizer' => 'applications/repository/data/PhabricatorRepositoryURINormalizer.php', 'PhabricatorRepositoryURINormalizerTestCase' => 'applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php', + 'PhabricatorRepositoryURIPHIDType' => 'applications/repository/phid/PhabricatorRepositoryURIPHIDType.php', + 'PhabricatorRepositoryURIQuery' => 'applications/repository/query/PhabricatorRepositoryURIQuery.php', 'PhabricatorRepositoryURITestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php', + 'PhabricatorRepositoryURITransaction' => 'applications/repository/storage/PhabricatorRepositoryURITransaction.php', 'PhabricatorRepositoryVCSPassword' => 'applications/repository/storage/PhabricatorRepositoryVCSPassword.php', 'PhabricatorRepositoryVersion' => 'applications/repository/constants/PhabricatorRepositoryVersion.php', 'PhabricatorRepositoryWorkingCopyVersion' => 'applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php', @@ -5009,6 +5015,7 @@ phutil_register_library_map(array( 'DiffusionRepositorySymbolsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryTag' => 'Phobject', 'DiffusionRepositoryTestAutomationController' => 'DiffusionRepositoryEditController', + 'DiffusionRepositoryURIEditController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryURIsIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'DiffusionRepositoryURIsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRequest' => 'Phobject', @@ -5034,6 +5041,8 @@ phutil_register_library_map(array( 'DiffusionTagListController' => 'DiffusionController', 'DiffusionTagListView' => 'DiffusionView', 'DiffusionTagsQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', + 'DiffusionURIEditEngine' => 'PhabricatorEditEngine', + 'DiffusionURIEditor' => 'PhabricatorApplicationTransactionEditor', 'DiffusionURITestCase' => 'PhutilTestCase', 'DiffusionUpdateCoverageConduitAPIMethod' => 'DiffusionConduitAPIMethod', 'DiffusionView' => 'AphrontView', @@ -7913,11 +7922,19 @@ phutil_register_library_map(array( 'PhabricatorRepositoryTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorRepositoryTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorRepositoryType' => 'Phobject', - 'PhabricatorRepositoryURI' => 'PhabricatorRepositoryDAO', + 'PhabricatorRepositoryURI' => array( + 'PhabricatorRepositoryDAO', + 'PhabricatorApplicationTransactionInterface', + 'PhabricatorPolicyInterface', + 'PhabricatorExtendedPolicyInterface', + ), 'PhabricatorRepositoryURIIndex' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryURINormalizer' => 'Phobject', 'PhabricatorRepositoryURINormalizerTestCase' => 'PhabricatorTestCase', + 'PhabricatorRepositoryURIPHIDType' => 'PhabricatorPHIDType', + 'PhabricatorRepositoryURIQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorRepositoryURITestCase' => 'PhabricatorTestCase', + 'PhabricatorRepositoryURITransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorRepositoryVCSPassword' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryVersion' => 'Phobject', 'PhabricatorRepositoryWorkingCopyVersion' => 'PhabricatorRepositoryDAO', diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index 775bdc203e..41d95a8cb7 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -91,6 +91,8 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { => 'DiffusionCommitEditController', 'manage/(?:(?P[^/]+)/)?' => 'DiffusionRepositoryManageController', + $this->getEditRoutePattern('uri/edit/') + => 'DiffusionRepositoryURIEditController', 'edit/' => array( '' => 'DiffusionRepositoryEditMainController', 'basic/' => 'DiffusionRepositoryEditBasicController', diff --git a/src/applications/diffusion/controller/DiffusionRepositoryURIEditController.php b/src/applications/diffusion/controller/DiffusionRepositoryURIEditController.php new file mode 100644 index 0000000000..f52edd1f35 --- /dev/null +++ b/src/applications/diffusion/controller/DiffusionRepositoryURIEditController.php @@ -0,0 +1,21 @@ +loadDiffusionContextForEdit(); + if ($response) { + return $response; + } + + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + return id(new DiffusionURIEditEngine()) + ->setController($this) + ->setRepository($repository) + ->buildResponse(); + } + +} diff --git a/src/applications/diffusion/editor/DiffusionURIEditEngine.php b/src/applications/diffusion/editor/DiffusionURIEditEngine.php new file mode 100644 index 0000000000..7e867ecbfa --- /dev/null +++ b/src/applications/diffusion/editor/DiffusionURIEditEngine.php @@ -0,0 +1,93 @@ +repository = $repository; + return $this; + } + + public function getRepository() { + return $this->repository; + } + + public function isEngineConfigurable() { + return false; + } + + public function getEngineName() { + return pht('Repository URIs'); + } + + public function getSummaryHeader() { + return pht('Edit Repository URI'); + } + + public function getSummaryText() { + return pht('Creates and edits repository URIs.'); + } + + public function getEngineApplicationClass() { + return 'PhabricatorDiffusionApplication'; + } + + protected function newEditableObject() { + $repository = $this->getRepository(); + return PhabricatorRepositoryURI::initializeNewURI($repository); + } + + protected function newObjectQuery() { + return new PhabricatorRepositoryURIQuery(); + } + + protected function getObjectCreateTitleText($object) { + return pht('Create Repository URI'); + } + + protected function getObjectCreateButtonText($object) { + return pht('Create Repository URI'); + } + + protected function getObjectEditTitleText($object) { + return pht('Edit Repository URI: %s', $object->getDisplayURI()); + } + + protected function getObjectEditShortText($object) { + return $object->getDisplayURI(); + } + + protected function getObjectCreateShortText() { + return pht('Create Repository URI'); + } + + protected function getObjectName() { + return pht('Repository URI'); + } + + protected function getObjectViewURI($object) { + $repository = $this->getRepository(); + return $repository->getPathURI('manage/uris/'); + } + + protected function buildCustomEditFields($object) { + $viewer = $this->getViewer(); + + return array( + id(new PhabricatorTextEditField()) + ->setKey('uri') + ->setLabel(pht('URI')) + ->setIsRequired(true) + ->setTransactionType(PhabricatorRepositoryURITransaction::TYPE_URI) + ->setDescription(pht('The repository URI.')) + ->setConduitDescription(pht('Change the repository URI.')) + ->setConduitTypeDescription(pht('New repository URI.')) + ->setValue($object->getURI()), + ); + } + +} diff --git a/src/applications/diffusion/editor/DiffusionURIEditor.php b/src/applications/diffusion/editor/DiffusionURIEditor.php new file mode 100644 index 0000000000..9cc20853bf --- /dev/null +++ b/src/applications/diffusion/editor/DiffusionURIEditor.php @@ -0,0 +1,99 @@ +getTransactionType()) { + case PhabricatorRepositoryURITransaction::TYPE_URI: + return $object->getURI(); + } + + return parent::getCustomTransactionOldValue($object, $xaction); + } + + protected function getCustomTransactionNewValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorRepositoryURITransaction::TYPE_URI: + return $xaction->getNewValue(); + } + + return parent::getCustomTransactionNewValue($object, $xaction); + } + + protected function applyCustomInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorRepositoryURITransaction::TYPE_URI: + $object->setURI($xaction->getNewValue()); + break; + } + } + + protected function applyCustomExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorRepositoryURITransaction::TYPE_URI: + return; + } + + return parent::applyCustomExternalTransaction($object, $xaction); + } + + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = parent::validateTransaction($object, $type, $xactions); + + switch ($type) { + case PhabricatorRepositoryURITransaction::TYPE_URI: + $missing = $this->validateIsEmptyTextField( + $object->getURI(), + $xactions); + + if ($missing) { + $error = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht('Repository URI is required.'), + nonempty(last($xactions), null)); + + $error->setIsMissingFieldError(true); + $errors[] = $error; + break; + } + break; + } + + return $errors; + } + +} diff --git a/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php index 2eb1991f07..105892c8f1 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php @@ -16,8 +16,6 @@ final class DiffusionRepositoryURIsManagementPanel public function buildManagementPanelContent() { $repository = $this->getRepository(); $viewer = $this->getViewer(); - - $repository->attachURIs(array()); $uris = $repository->getURIs(); Javelin::initBehavior('phabricator-tooltips'); @@ -25,6 +23,12 @@ final class DiffusionRepositoryURIsManagementPanel foreach ($uris as $uri) { $uri_name = $uri->getDisplayURI(); + $uri_name = phutil_tag( + 'a', + array( + 'href' => $repository->getPathURI('uri/edit/'.$uri->getID().'/'), + ), + $uri_name); if ($uri->getIsDisabled()) { $status_icon = 'fa-times grey'; diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index f86170f129..417052b594 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -142,7 +142,8 @@ abstract class DiffusionRequest extends Phobject { $query = id(new PhabricatorRepositoryQuery()) ->setViewer($viewer) - ->withIdentifiers(array($identifier)); + ->withIdentifiers(array($identifier)) + ->needURIs(true); if ($need_edit) { $query->requireCapabilities( diff --git a/src/applications/repository/phid/PhabricatorRepositoryURIPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryURIPHIDType.php new file mode 100644 index 0000000000..658b942418 --- /dev/null +++ b/src/applications/repository/phid/PhabricatorRepositoryURIPHIDType.php @@ -0,0 +1,40 @@ +withPHIDs($phids); + } + + public function loadHandles( + PhabricatorHandleQuery $query, + array $handles, + array $objects) { + + foreach ($handles as $phid => $handle) { + $uri = $objects[$phid]; + + $handle->setName( + pht('URI %d %s', $uri->getID(), $uri->getDisplayURI())); + } + } + +} diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index cbb2b1b3e9..78693d54d6 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -34,6 +34,7 @@ final class PhabricatorRepositoryQuery private $needMostRecentCommits; private $needCommitCounts; private $needProjectPHIDs; + private $needURIs; public function withIDs(array $ids) { $this->ids = $ids; @@ -148,6 +149,11 @@ final class PhabricatorRepositoryQuery return $this; } + public function needURIs($need_uris) { + $this->needURIs = $need_uris; + return $this; + } + public function getBuiltinOrders() { return array( 'committed' => array( @@ -348,6 +354,20 @@ final class PhabricatorRepositoryQuery } } + $viewer = $this->getViewer(); + + if ($this->needURIs) { + $uris = id(new PhabricatorRepositoryURIQuery()) + ->setViewer($viewer) + ->withRepositories($repositories) + ->execute(); + $uri_groups = mgroup($uris, 'getRepositoryPHID'); + foreach ($repositories as $repository) { + $repository_uris = idx($uri_groups, $repository->getPHID(), array()); + $repository->attachURIs($repository_uris); + } + } + return $repositories; } diff --git a/src/applications/repository/query/PhabricatorRepositoryURIQuery.php b/src/applications/repository/query/PhabricatorRepositoryURIQuery.php new file mode 100644 index 0000000000..330fe769f5 --- /dev/null +++ b/src/applications/repository/query/PhabricatorRepositoryURIQuery.php @@ -0,0 +1,106 @@ +ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function withRepositoryPHIDs(array $phids) { + $this->repositoryPHIDs = $phids; + return $this; + } + + public function withRepositories(array $repositories) { + $repositories = mpull($repositories, null, 'getPHID'); + $this->withRepositoryPHIDs(array_keys($repositories)); + $this->repositories = $repositories; + return $this; + } + + public function withObjectHashes(array $hashes) { + $this->objectHashes = $hashes; + return $this; + } + + public function newResultObject() { + return new PhabricatorRepositoryURI(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->phids !== null) { + $where[] = qsprintf( + $conn, + 'phid IN (%Ls)', + $this->phids); + } + + if ($this->repositoryPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'repositoryPHID IN (%Ls)', + $this->repositoryPHIDs); + } + + return $where; + } + + protected function willFilterPage(array $uris) { + $repositories = $this->repositories; + + $repository_phids = mpull($uris, 'getRepositoryPHID'); + $repository_phids = array_fuse($repository_phids); + $repository_phids = array_diff_key($repository_phids, $repositories); + + if ($repository_phids) { + $more_repositories = id(new PhabricatorRepositoryQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($repository_phids) + ->execute(); + $repositories += mpull($more_repositories, null, 'getPHID'); + } + + foreach ($uris as $key => $uri) { + $repository_phid = $uri->getRepositoryPHID(); + $repository = idx($repositories, $repository_phid); + if (!$repository) { + $this->didRejectResult($uri); + unset($uris[$key]); + continue; + } + $uri->attachRepository($repository); + } + + return $uris; + } + + public function getQueryApplicationClass() { + return 'PhabricatorDiffusionApplication'; + } + +} diff --git a/src/applications/repository/storage/PhabricatorRepositoryURI.php b/src/applications/repository/storage/PhabricatorRepositoryURI.php index c2872c56ac..524a09b43f 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryURI.php +++ b/src/applications/repository/storage/PhabricatorRepositoryURI.php @@ -1,7 +1,11 @@ setIsDisabled(0); } + public function generatePHID() { + return PhabricatorPHID::generateNewPHID( + PhabricatorRepositoryURIPHIDType::TYPECONST); + } + public function attachRepository(PhabricatorRepository $repository) { $this->repository = $repository; return $this; @@ -176,7 +185,7 @@ final class PhabricatorRepositoryURI } } - return self::IO_IGNORE; + return self::IO_NONE; } @@ -298,4 +307,71 @@ final class PhabricatorRepositoryURI } } + +/* -( PhabricatorApplicationTransactionInterface )------------------------- */ + + + public function getApplicationTransactionEditor() { + return new DiffusionURIEditor(); + } + + public function getApplicationTransactionObject() { + return $this; + } + + public function getApplicationTransactionTemplate() { + return new PhabricatorRepositoryURITransaction(); + } + + public function willRenderTimeline( + PhabricatorApplicationTransactionView $timeline, + AphrontRequest $request) { + return $timeline; + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + ); + } + + public function getPolicy($capability) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + case PhabricatorPolicyCapability::CAN_EDIT: + return PhabricatorPolicies::getMostOpenPolicy(); + } + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return false; + } + + public function describeAutomaticCapability($capability) { + return null; + } + + +/* -( PhabricatorExtendedPolicyInterface )--------------------------------- */ + + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { + $extended = array(); + + switch ($capability) { + case PhabricatorPolicyCapability::CAN_EDIT: + // To edit a repository URI, you must be able to edit the + // corresponding repository. + $extended[] = array($this->getRepository(), $capability); + break; + } + + return $extended; + } + } diff --git a/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php b/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php new file mode 100644 index 0000000000..d8a404bd15 --- /dev/null +++ b/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php @@ -0,0 +1,20 @@ + Date: Thu, 28 Apr 2016 10:40:35 -0700 Subject: [PATCH 26/32] Add repository URI view pages and IO/Display edit logic Summary: Ref T10748. - New View page for repository URIs. - Make display and I/O behavior (observe, mirror, read, read/write) editable. - Add a bunch of checks to prevent you from completely screwing up a repository by making it writable from a bunch of differnet sources. Test Plan: {F1249866} {F1249867} Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15816 --- src/__phutil_library_map__.php | 4 + .../PhabricatorDiffusionApplication.php | 7 +- .../DiffusionRepositoryURIViewController.php | 162 ++++++++++++++++++ .../editor/DiffusionURIEditEngine.php | 25 ++- .../diffusion/editor/DiffusionURIEditor.php | 144 ++++++++++++++++ ...DiffusionRepositoryURIsManagementPanel.php | 52 ++---- ...abricatorRepositoryURITransactionQuery.php | 10 ++ .../storage/PhabricatorRepositoryURI.php | 147 ++++++++++++++++ .../PhabricatorRepositoryURITransaction.php | 41 ++++- 9 files changed, 549 insertions(+), 43 deletions(-) create mode 100644 src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php create mode 100644 src/applications/repository/query/PhabricatorRepositoryURITransactionQuery.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1384501c51..41c2d8cbc0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -790,6 +790,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php', 'DiffusionRepositoryTestAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php', 'DiffusionRepositoryURIEditController' => 'applications/diffusion/controller/DiffusionRepositoryURIEditController.php', + 'DiffusionRepositoryURIViewController' => 'applications/diffusion/controller/DiffusionRepositoryURIViewController.php', 'DiffusionRepositoryURIsIndexEngineExtension' => 'applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php', 'DiffusionRepositoryURIsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php', 'DiffusionRequest' => 'applications/diffusion/request/DiffusionRequest.php', @@ -3244,6 +3245,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryURIQuery' => 'applications/repository/query/PhabricatorRepositoryURIQuery.php', 'PhabricatorRepositoryURITestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php', 'PhabricatorRepositoryURITransaction' => 'applications/repository/storage/PhabricatorRepositoryURITransaction.php', + 'PhabricatorRepositoryURITransactionQuery' => 'applications/repository/query/PhabricatorRepositoryURITransactionQuery.php', 'PhabricatorRepositoryVCSPassword' => 'applications/repository/storage/PhabricatorRepositoryVCSPassword.php', 'PhabricatorRepositoryVersion' => 'applications/repository/constants/PhabricatorRepositoryVersion.php', 'PhabricatorRepositoryWorkingCopyVersion' => 'applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php', @@ -5016,6 +5018,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryTag' => 'Phobject', 'DiffusionRepositoryTestAutomationController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryURIEditController' => 'DiffusionRepositoryEditController', + 'DiffusionRepositoryURIViewController' => 'DiffusionController', 'DiffusionRepositoryURIsIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'DiffusionRepositoryURIsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRequest' => 'Phobject', @@ -7935,6 +7938,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryURIQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorRepositoryURITestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryURITransaction' => 'PhabricatorApplicationTransaction', + 'PhabricatorRepositoryURITransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorRepositoryVCSPassword' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryVersion' => 'Phobject', 'PhabricatorRepositoryWorkingCopyVersion' => 'PhabricatorRepositoryDAO', diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index 41d95a8cb7..f910c090b7 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -91,8 +91,11 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { => 'DiffusionCommitEditController', 'manage/(?:(?P[^/]+)/)?' => 'DiffusionRepositoryManageController', - $this->getEditRoutePattern('uri/edit/') - => 'DiffusionRepositoryURIEditController', + 'uri/' => array( + 'view/(?P[0-9]\d*)/' => 'DiffusionRepositoryURIViewController', + $this->getEditRoutePattern('edit/') + => 'DiffusionRepositoryURIEditController', + ), 'edit/' => array( '' => 'DiffusionRepositoryEditMainController', 'basic/' => 'DiffusionRepositoryEditBasicController', diff --git a/src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php b/src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php new file mode 100644 index 0000000000..b7da37ebe2 --- /dev/null +++ b/src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php @@ -0,0 +1,162 @@ +loadDiffusionContext(); + if ($response) { + return $response; + } + + $viewer = $this->getViewer(); + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + $id = $request->getURIData('id'); + + $uri = id(new PhabricatorRepositoryURIQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->withRepositories(array($repository)) + ->executeOne(); + if (!$uri) { + return new Aphront404Response(); + } + + $title = array( + pht('URI'), + $repository->getDisplayName(), + ); + + $crumbs = $this->buildApplicationCrumbs(); + $crumbs->addTextCrumb( + $repository->getDisplayName(), + $repository->getURI()); + $crumbs->addTextCrumb( + pht('Manage'), + $repository->getPathURI('manage/')); + + $panel_label = id(new DiffusionRepositoryURIsManagementPanel()) + ->getManagementPanelLabel(); + $panel_uri = $repository->getPathURI('manage/uris/'); + $crumbs->addTextCrumb($panel_label, $panel_uri); + + $crumbs->addTextCrumb(pht('URI %d', $uri->getID())); + + $header_text = pht( + '%s: URI %d', + $repository->getDisplayName(), + $uri->getID()); + + $header = id(new PHUIHeaderView()) + ->setHeader($header_text) + ->setHeaderIcon('fa-pencil'); + if ($uri->getIsDisabled()) { + $header->setStatus('fa-ban', 'dark', pht('Disabled')); + } else { + $header->setStatus('fa-check', 'bluegrey', pht('Active')); + } + + $curtain = $this->buildCurtain($uri); + $details = $this->buildPropertySection($uri); + + $timeline = $this->buildTransactionTimeline( + $uri, + new PhabricatorRepositoryURITransactionQuery()); + $timeline->setShouldTerminate(true); + + $view = id(new PHUITwoColumnView()) + ->setHeader($header) + ->setMainColumn( + array( + $details, + $timeline, + )) + ->setCurtain($curtain); + + return $this->newPage() + ->setTitle($title) + ->setCrumbs($crumbs) + ->appendChild($view); + } + + private function buildCurtain(PhabricatorRepositoryURI $uri) { + $viewer = $this->getViewer(); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $uri, + PhabricatorPolicyCapability::CAN_EDIT); + + $edit_uri = $uri->getEditURI(); + + $curtain = $this->newCurtainView($uri); + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setIcon('fa-pencil') + ->setName(pht('Edit URI')) + ->setHref($edit_uri) + ->setWorkflow(!$can_edit) + ->setDisabled(!$can_edit)); + + return $curtain; + } + + private function buildPropertySection(PhabricatorRepositoryURI $uri) { + $viewer = $this->getViewer(); + + $properties = id(new PHUIPropertyListView()) + ->setUser($viewer); + + $properties->addProperty(pht('URI'), $uri->getDisplayURI()); + $properties->addProperty(pht('Credential'), 'TODO'); + + + $io_type = $uri->getEffectiveIOType(); + $io_map = PhabricatorRepositoryURI::getIOTypeMap(); + $io_spec = idx($io_map, $io_type, array()); + + $io_icon = idx($io_spec, 'icon'); + $io_color = idx($io_spec, 'color'); + $io_label = idx($io_spec, 'label', $io_type); + $io_note = idx($io_spec, 'note'); + + $io_item = id(new PHUIStatusItemView()) + ->setIcon($io_icon, $io_color) + ->setTarget(phutil_tag('strong', array(), $io_label)) + ->setNote($io_note); + + $io_view = id(new PHUIStatusListView()) + ->addItem($io_item); + + $properties->addProperty(pht('I/O'), $io_view); + + + $display_type = $uri->getEffectiveDisplayType(); + $display_map = PhabricatorRepositoryURI::getDisplayTypeMap(); + $display_spec = idx($display_map, $display_type, array()); + + $display_icon = idx($display_spec, 'icon'); + $display_color = idx($display_spec, 'color'); + $display_label = idx($display_spec, 'label', $display_type); + $display_note = idx($display_spec, 'note'); + + $display_item = id(new PHUIStatusItemView()) + ->setIcon($display_icon, $display_color) + ->setTarget(phutil_tag('strong', array(), $display_label)) + ->setNote($display_note); + + $display_view = id(new PHUIStatusListView()) + ->addItem($display_item); + + $properties->addProperty(pht('Display'), $display_view); + + + return id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Details')) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->appendChild($properties); + } + +} diff --git a/src/applications/diffusion/editor/DiffusionURIEditEngine.php b/src/applications/diffusion/editor/DiffusionURIEditEngine.php index 7e867ecbfa..7d4196ac66 100644 --- a/src/applications/diffusion/editor/DiffusionURIEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionURIEditEngine.php @@ -54,11 +54,11 @@ final class DiffusionURIEditEngine } protected function getObjectEditTitleText($object) { - return pht('Edit Repository URI: %s', $object->getDisplayURI()); + return pht('Edit Repository URI %d', $object->getID()); } protected function getObjectEditShortText($object) { - return $object->getDisplayURI(); + return pht('URI %d', $object->getID()); } protected function getObjectCreateShortText() { @@ -70,8 +70,7 @@ final class DiffusionURIEditEngine } protected function getObjectViewURI($object) { - $repository = $this->getRepository(); - return $repository->getPathURI('manage/uris/'); + return $object->getViewURI(); } protected function buildCustomEditFields($object) { @@ -87,6 +86,24 @@ final class DiffusionURIEditEngine ->setConduitDescription(pht('Change the repository URI.')) ->setConduitTypeDescription(pht('New repository URI.')) ->setValue($object->getURI()), + id(new PhabricatorSelectEditField()) + ->setKey('io') + ->setLabel(pht('I/O Type')) + ->setTransactionType(PhabricatorRepositoryURITransaction::TYPE_IO) + ->setDescription(pht('URI I/O behavior.')) + ->setConduitDescription(pht('Adjust I/O behavior.')) + ->setConduitTypeDescription(pht('New I/O behavior.')) + ->setValue($object->getIOType()) + ->setOptions($object->getAvailableIOTypeOptions()), + id(new PhabricatorSelectEditField()) + ->setKey('display') + ->setLabel(pht('Display Type')) + ->setTransactionType(PhabricatorRepositoryURITransaction::TYPE_DISPLAY) + ->setDescription(pht('URI display behavior.')) + ->setConduitDescription(pht('Change display behavior.')) + ->setConduitTypeDescription(pht('New display behavior.')) + ->setValue($object->getDisplayType()) + ->setOptions($object->getAvailableDisplayTypeOptions()), ); } diff --git a/src/applications/diffusion/editor/DiffusionURIEditor.php b/src/applications/diffusion/editor/DiffusionURIEditor.php index 9cc20853bf..0dfdd4e704 100644 --- a/src/applications/diffusion/editor/DiffusionURIEditor.php +++ b/src/applications/diffusion/editor/DiffusionURIEditor.php @@ -15,6 +15,8 @@ final class DiffusionURIEditor $types = parent::getTransactionTypes(); $types[] = PhabricatorRepositoryURITransaction::TYPE_URI; + $types[] = PhabricatorRepositoryURITransaction::TYPE_IO; + $types[] = PhabricatorRepositoryURITransaction::TYPE_DISPLAY; return $types; } @@ -26,6 +28,10 @@ final class DiffusionURIEditor switch ($xaction->getTransactionType()) { case PhabricatorRepositoryURITransaction::TYPE_URI: return $object->getURI(); + case PhabricatorRepositoryURITransaction::TYPE_IO: + return $object->getIOType(); + case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: + return $object->getDisplayType(); } return parent::getCustomTransactionOldValue($object, $xaction); @@ -37,6 +43,8 @@ final class DiffusionURIEditor switch ($xaction->getTransactionType()) { case PhabricatorRepositoryURITransaction::TYPE_URI: + case PhabricatorRepositoryURITransaction::TYPE_IO: + case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: return $xaction->getNewValue(); } @@ -51,6 +59,12 @@ final class DiffusionURIEditor case PhabricatorRepositoryURITransaction::TYPE_URI: $object->setURI($xaction->getNewValue()); break; + case PhabricatorRepositoryURITransaction::TYPE_IO: + $object->setIOType($xaction->getNewValue()); + break; + case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: + $object->setDisplayType($xaction->getNewValue()); + break; } } @@ -60,6 +74,8 @@ final class DiffusionURIEditor switch ($xaction->getTransactionType()) { case PhabricatorRepositoryURITransaction::TYPE_URI: + case PhabricatorRepositoryURITransaction::TYPE_IO: + case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: return; } @@ -91,6 +107,134 @@ final class DiffusionURIEditor break; } break; + case PhabricatorRepositoryURITransaction::TYPE_IO: + $available = $object->getAvailableIOTypeOptions(); + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if (empty($available[$new])) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Value "%s" is not a valid display setting for this URI. '. + 'Available types for this URI are: %s.', + implode(', ', array_keys($available))), + $xaction); + continue; + } + + // If we are setting this URI to use "Observe", we must have no + // other "Observe" URIs and must also have no "Read/Write" URIs. + + // If we are setting this URI to "Read/Write", we must have no + // other "Observe" URIs. It's OK to have other "Read/Write" URIs. + + $no_observers = false; + $no_readwrite = false; + switch ($new) { + case PhabricatorRepositoryURI::IO_OBSERVE: + $no_readwrite = true; + $no_observers = true; + break; + case PhabricatorRepositoryURI::IO_READWRITE: + $no_observers = true; + break; + } + + if ($no_observers || $no_readwrite) { + $repository = id(new PhabricatorRepositoryQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs(array($object->getRepositoryPHID())) + ->needURIs(true) + ->executeOne(); + $uris = $repository->getURIs(); + + $observe_conflict = null; + $readwrite_conflict = null; + foreach ($uris as $uri) { + // If this is the URI being edited, it can not conflict with + // itself. + if ($uri->getID() == $object->getID()) { + continue; + } + + $io_type = $uri->getIoType(); + + if ($io_type == PhabricatorRepositoryURI::IO_READWRITE) { + if ($no_readwrite) { + $readwite_conflict = $uri; + break; + } + } + + if ($io_type == PhabricatorRepositoryURI::IO_OBSERVE) { + if ($no_observers) { + $observe_conflict = $uri; + break; + } + } + } + + if ($observe_conflict) { + if ($new == PhabricatorRepositoryURI::IO_OBSERVE) { + $message = pht( + 'You can not set this URI to use Observe IO because '. + 'another URI for this repository is already configured '. + 'in Observe IO mode. A repository can not observe two '. + 'different remotes simultaneously. Turn off IO for the '. + 'other URI first.'); + } else { + $message = pht( + 'You can not set this URI to use Read/Write IO because '. + 'another URI for this repository is already configured '. + 'in Observe IO mode. An observed repository can not be '. + 'made writable. Turn off IO for the other URI first.'); + } + + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + $message, + $xaction); + continue; + } + + if ($readwrite_conflict) { + $message = pht( + 'You can not set this URI to use Observe IO because '. + 'another URI for this repository is already configured '. + 'in Read/Write IO mode. A repository can not simultaneously '. + 'be writable and observe a remote. Turn off IO for the '. + 'other URI first.'); + + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + $message, + $xaction); + continue; + } + } + } + + break; + case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: + $available = $object->getAvailableDisplayTypeOptions(); + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if (empty($available[$new])) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Value "%s" is not a valid display setting for this URI. '. + 'Available types for this URI are: %s.', + implode(', ', array_keys($available)))); + } + } + break; } return $errors; diff --git a/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php index 105892c8f1..113914b8d4 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php @@ -26,7 +26,7 @@ final class DiffusionRepositoryURIsManagementPanel $uri_name = phutil_tag( 'a', array( - 'href' => $repository->getPathURI('uri/edit/'.$uri->getID().'/'), + 'href' => $uri->getViewURI(), ), $uri_name); @@ -38,48 +38,30 @@ final class DiffusionRepositoryURIsManagementPanel $uri_status = id(new PHUIIconView())->setIcon($status_icon); - switch ($uri->getEffectiveIOType()) { - case PhabricatorRepositoryURI::IO_OBSERVE: - $io_icon = 'fa-download green'; - $io_label = pht('Observe'); - break; - case PhabricatorRepositoryURI::IO_MIRROR: - $io_icon = 'fa-upload green'; - $io_label = pht('Mirror'); - break; - case PhabricatorRepositoryURI::IO_NONE: - $io_icon = 'fa-times grey'; - $io_label = pht('No I/O'); - break; - case PhabricatorRepositoryURI::IO_READ: - $io_icon = 'fa-folder blue'; - $io_label = pht('Read Only'); - break; - case PhabricatorRepositoryURI::IO_READWRITE: - $io_icon = 'fa-folder-open blue'; - $io_label = pht('Read/Write'); - break; - } + $io_type = $uri->getEffectiveIOType(); + $io_map = PhabricatorRepositoryURI::getIOTypeMap(); + $io_spec = idx($io_map, $io_type, array()); + + $io_icon = idx($io_spec, 'icon'); + $io_color = idx($io_spec, 'color'); + $io_label = idx($io_spec, 'label', $io_type); $uri_io = array( - id(new PHUIIconView())->setIcon($io_icon), + id(new PHUIIconView())->setIcon("{$io_icon} {$io_color}"), ' ', $io_label, ); - switch ($uri->getEffectiveDisplayType()) { - case PhabricatorRepositoryURI::DISPLAY_NEVER: - $display_icon = 'fa-eye-slash grey'; - $display_label = pht('Hidden'); - break; - case PhabricatorRepositoryURI::DISPLAY_ALWAYS: - $display_icon = 'fa-eye green'; - $display_label = pht('Visible'); - break; - } + $display_type = $uri->getEffectiveDisplayType(); + $display_map = PhabricatorRepositoryURI::getDisplayTypeMap(); + $display_spec = idx($display_map, $display_type, array()); + + $display_icon = idx($display_spec, 'icon'); + $display_color = idx($display_spec, 'color'); + $display_label = idx($display_spec, 'label', $display_type); $uri_display = array( - id(new PHUIIconView())->setIcon($display_icon), + id(new PHUIIconView())->setIcon("{$display_icon} {$display_color}"), ' ', $display_label, ); diff --git a/src/applications/repository/query/PhabricatorRepositoryURITransactionQuery.php b/src/applications/repository/query/PhabricatorRepositoryURITransactionQuery.php new file mode 100644 index 0000000000..28ae9d9d62 --- /dev/null +++ b/src/applications/repository/query/PhabricatorRepositoryURITransactionQuery.php @@ -0,0 +1,10 @@ +getBuiltinProtocol(), $this->getBuiltinIdentifier(), ); + return implode('.', $parts); } @@ -108,6 +109,10 @@ final class PhabricatorRepositoryURI return $display; } + return $this->getDefaultDisplayType(); + } + + public function getDefaultDisplayType() { switch ($this->getEffectiveIOType()) { case self::IO_MIRROR: case self::IO_OBSERVE: @@ -156,6 +161,8 @@ final class PhabricatorRepositoryURI return self::DISPLAY_ALWAYS; } + + return self::DISPLAY_NEVER; } @@ -166,6 +173,10 @@ final class PhabricatorRepositoryURI return $io; } + return $this->getDefaultIOType(); + } + + public function getDefaultIOType() { if ($this->isBuiltin()) { $repository = $this->getRepository(); $other_uris = $repository->getURIs(); @@ -307,6 +318,142 @@ final class PhabricatorRepositoryURI } } + public function getViewURI() { + $id = $this->getID(); + return $this->getRepository()->getPathURI("uri/view/{$id}/"); + } + + public function getEditURI() { + $id = $this->getID(); + return $this->getRepository()->getPathURI("uri/edit/{$id}/"); + } + + public function getAvailableIOTypeOptions() { + $options = array( + self::IO_DEFAULT, + self::IO_NONE, + ); + + if ($this->isBuiltin()) { + $options[] = self::IO_READ; + $options[] = self::IO_WRITE; + } else { + $options[] = self::IO_OBSERVE; + $options[] = self::IO_MIRROR; + } + + $map = array(); + $io_map = self::getIOTypeMap(); + foreach ($options as $option) { + $spec = idx($io_map, $option, array()); + + $label = idx($spec, 'label', $option); + $short = idx($spec, 'short'); + + $name = pht('%s: %s', $label, $short); + $map[$option] = $name; + } + + return $map; + } + + public function getAvailableDisplayTypeOptions() { + $options = array( + self::DISPLAY_DEFAULT, + self::DISPLAY_ALWAYS, + self::DISPLAY_NEVER, + ); + + $map = array(); + $display_map = self::getDisplayTypeMap(); + foreach ($options as $option) { + $spec = idx($display_map, $option, array()); + + $label = idx($spec, 'label', $option); + $short = idx($spec, 'short'); + + $name = pht('%s: %s', $label, $short); + $map[$option] = $name; + } + + return $map; + } + + public static function getIOTypeMap() { + return array( + self::IO_DEFAULT => array( + 'label' => pht('Default'), + 'short' => pht('Use default behavior.'), + ), + self::IO_OBSERVE => array( + 'icon' => 'fa-download', + 'color' => 'green', + 'label' => pht('Observe'), + 'note' => pht( + 'Phabricator will observe changes to this URI and copy them.'), + 'short' => pht('Copy from a remote.'), + ), + self::IO_MIRROR => array( + 'icon' => 'fa-upload', + 'color' => 'green', + 'label' => pht('Mirror'), + 'note' => pht( + 'Phabricator will push a copy of any changes to this URI.'), + 'short' => pht('Push a copy to a remote.'), + ), + self::IO_NONE => array( + 'icon' => 'fa-times', + 'color' => 'grey', + 'label' => pht('No I/O'), + 'note' => pht( + 'Phabricator will not push or pull any changes to this URI.'), + 'short' => pht('Do not perform any I/O.'), + ), + self::IO_READ => array( + 'icon' => 'fa-folder', + 'color' => 'blue', + 'label' => pht('Read Only'), + 'note' => pht( + 'Phabricator will serve a read-only copy of the repository from '. + 'this URI.'), + 'short' => pht('Serve repository in read-only mode.'), + ), + self::IO_READWRITE => array( + 'icon' => 'fa-folder-open', + 'color' => 'blue', + 'label' => pht('Read/Write'), + 'note' => pht( + 'Phabricator will serve a read/write copy of the repository from '. + 'this URI.'), + 'short' => pht('Serve repository in read/write mode.'), + ), + ); + } + + public static function getDisplayTypeMap() { + return array( + self::DISPLAY_DEFAULT => array( + 'label' => pht('Default'), + 'short' => pht('Use default behavior.'), + ), + self::DISPLAY_ALWAYS => array( + 'icon' => 'fa-eye', + 'color' => 'green', + 'label' => pht('Visible'), + 'note' => pht('This URI will be shown to users as a clone URI.'), + 'short' => pht('Show as a clone URI.'), + ), + self::DISPLAY_NEVER => array( + 'icon' => 'fa-eye-slash', + 'color' => 'grey', + 'label' => pht('Hidden'), + 'note' => pht( + 'This URI will be hidden from users.'), + 'short' => pht('Do not show as a clone URI.'), + ), + ); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php b/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php index d8a404bd15..e27a91c20b 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php +++ b/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php @@ -4,6 +4,8 @@ final class PhabricatorRepositoryURITransaction extends PhabricatorApplicationTransaction { const TYPE_URI = 'diffusion.uri.uri'; + const TYPE_IO = 'diffusion.uri.io'; + const TYPE_DISPLAY = 'diffusion.uri.display'; public function getApplicationName() { return 'repository'; @@ -13,8 +15,43 @@ final class PhabricatorRepositoryURITransaction return PhabricatorRepositoryURIPHIDType::TYPECONST; } - public function getApplicationTransactionCommentObject() { - return null; + public function getTitle() { + $author_phid = $this->getAuthorPHID(); + + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + switch ($this->getTransactionType()) { + case self::TYPE_URI: + return pht( + '%s changed this URI from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old, + $new); + case self::TYPE_IO: + $map = PhabricatorRepositoryURI::getIOTypeMap(); + $old_label = idx(idx($map, $old, array()), 'label', $old); + $new_label = idx(idx($map, $new, array()), 'label', $new); + + return pht( + '%s changed the display type for this URI from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old_label, + $new_label); + case self::TYPE_DISPLAY: + $map = PhabricatorRepositoryURI::getDisplayTypeMap(); + $old_label = idx(idx($map, $old, array()), 'label', $old); + $new_label = idx(idx($map, $new, array()), 'label', $new); + + return pht( + '%s changed the display type for this URI from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old_label, + $new_label); + + } + + return parent::getTitle(); } } From 128995f1acd38f6a6f49684229a17f1b0dc9efcc Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 28 Apr 2016 13:34:09 -0700 Subject: [PATCH 27/32] Document all the hypothetical URI features we plan to support soon Summary: Ref T10748. Ref T10366. This documents how everything is planned to work shortly. Test Plan: Read documentation. Reviewers: chad Reviewed By: chad Subscribers: eadler, scode Maniphest Tasks: T10366, T10748 Differential Revision: https://secure.phabricator.com/D15817 --- .../storage/PhabricatorRepositoryURI.php | 57 ++--- .../user/userguide/diffusion_uris.diviner | 232 +++++++++++++++++- 2 files changed, 255 insertions(+), 34 deletions(-) diff --git a/src/applications/repository/storage/PhabricatorRepositoryURI.php b/src/applications/repository/storage/PhabricatorRepositoryURI.php index 9aeb0c6868..364aad0a29 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryURI.php +++ b/src/applications/repository/storage/PhabricatorRepositoryURI.php @@ -105,7 +105,7 @@ final class PhabricatorRepositoryURI public function getEffectiveDisplayType() { $display = $this->getDisplayType(); - if ($display != self::IO_DEFAULT) { + if ($display != self::DISPLAY_DEFAULT) { return $display; } @@ -116,47 +116,40 @@ final class PhabricatorRepositoryURI switch ($this->getEffectiveIOType()) { case self::IO_MIRROR: case self::IO_OBSERVE: - return self::DISPLAY_NEVER; case self::IO_NONE: - if ($this->isBuiltin()) { - return self::DISPLAY_NEVER; - } else { - return self::DISPLAY_ALWAYS; - } + return self::DISPLAY_NEVER; case self::IO_READ: case self::IO_READWRITE: // By default, only show the "best" version of the builtin URI, not the // other redundant versions. - if ($this->isBuiltin()) { - $repository = $this->getRepository(); - $other_uris = $repository->getURIs(); + $repository = $this->getRepository(); + $other_uris = $repository->getURIs(); - $identifier_value = array( - self::BUILTIN_IDENTIFIER_CALLSIGN => 3, - self::BUILTIN_IDENTIFIER_SHORTNAME => 2, - self::BUILTIN_IDENTIFIER_ID => 1, - ); + $identifier_value = array( + self::BUILTIN_IDENTIFIER_CALLSIGN => 3, + self::BUILTIN_IDENTIFIER_SHORTNAME => 2, + self::BUILTIN_IDENTIFIER_ID => 1, + ); - $have_identifiers = array(); - foreach ($other_uris as $other_uri) { - if ($other_uri->getIsDisabled()) { - continue; - } - - $identifier = $other_uri->getBuiltinIdentifier(); - if (!$identifier) { - continue; - } - - $have_identifiers[$identifier] = $identifier_value[$identifier]; + $have_identifiers = array(); + foreach ($other_uris as $other_uri) { + if ($other_uri->getIsDisabled()) { + continue; } - $best_identifier = max($have_identifiers); - $this_identifier = $identifier_value[$this->getBuiltinIdentifier()]; - - if ($this_identifier < $best_identifier) { - return self::DISPLAY_NEVER; + $identifier = $other_uri->getBuiltinIdentifier(); + if (!$identifier) { + continue; } + + $have_identifiers[$identifier] = $identifier_value[$identifier]; + } + + $best_identifier = max($have_identifiers); + $this_identifier = $identifier_value[$this->getBuiltinIdentifier()]; + + if ($this_identifier < $best_identifier) { + return self::DISPLAY_NEVER; } return self::DISPLAY_ALWAYS; diff --git a/src/docs/user/userguide/diffusion_uris.diviner b/src/docs/user/userguide/diffusion_uris.diviner index 3ff122e85e..b560f4ab3f 100644 --- a/src/docs/user/userguide/diffusion_uris.diviner +++ b/src/docs/user/userguide/diffusion_uris.diviner @@ -9,8 +9,8 @@ Overview WARNING: This document describes a feature which is still under development, and is not necessarily accurate or complete. -Phabricator can host, observe, mirror, and proxy repositories. For example, -here are some supported use cases: +Phabricator can create, host, observe, mirror, proxy, and import repositories. +For example, you can: **Host Repositories**: Phabricator can host repositories locally. Phabricator maintains the writable master version of the repository, and you can push and @@ -45,3 +45,231 @@ configuring URIs and marking them to be fetched from, mirrored to, clonable, and so on. By configuring all the URIs that a repository should interact with and expose to users, you configure the read, write, and mirroring behavior of the repository. + +The remainder of this document walks through this configuration in greater +detail. + + +Host a Repository +================= + +You can create new repositories that Phabricator will host, like you would +create repositories on services like GitHub or Bitbucket. Phabricator will +serve a read-write copy of the repository and you can clone it from Phabricator +and push changes to Phabricator. + +If you haven't already, you may need to configure Phabricator for hosting +before you can create your first hosted repository. For a detailed guide, +see @{article:Diffusion User Guide: Repository Hosting}. + +This is the default mode for new repositories. To host a repository: + + - Create a new repository. + - Activate it. + +Phabricator will create an empty repository and allow you to fetch from it and +push to it. + + +Observe a Repository +==================== + +If you have an existing repository hosted on another service (like GitHub, +Bitbucket, or a private server) that you want to work with in Phabricator, +you can configure Phabricator to observe it. + +When observing a repository, Phabricator will keep track of changes in the +remote repository and allow you to browse and interact with the repository from +the web UI in Diffusion and other applications, but you can continue hosting it +elsewhere. + +To observe a repository: + + - Create a new repository, but don't activate it yet. + - Add the remote URI you want to observe as a repository URI. + - Set the **I/O Type** for the URI to **Observe**. + - If necessary, configure a credential. + - Activate the repository. + +Phabricator will perform an initial import of the repository, creating a local +read-only copy. Once this process completes, it will continue keeping track of +changes in the remote, fetching them, and reflecting them in the UI. + + +Mirror a Repository +=================== + +NOTE: Mirroring is not supported in Subversion. + +You can create a read-only mirror of an existing repository. Phabricator will +push all changes made to the repository to the mirror. + +For example, if you have a repository hosted in Phabricator that you want to +mirror to GitHub, you can configure Phabricator to automatically maintain the +mirror. This is how the upstream repositories are set up. + +You can mirror any repository, even if Phabricator is only observing it and not +hosting it directly. + +To begin mirroring a repository: + + - Create a hosted or observed repository by following the relevant + instructions above. + - Add the remote URI you want to mirror to as a repository URI. + - Set the **I/O Type** for the URI to **Mirror**. + - If necessary, configure a credential. + +To stop mirroring: + + - Disable the mirror URI; or + - Change the **I/O Type** for the URI to **None**. + + +Import a Repository +=================== + +If you have an existing repository that you want to move so it is hosted on +Phabricator, there are three ways to do it: + +**Push Everything**: //(Git, Mercurial)// Create a new empty hosted repository +according to the instructions above. Once the empty repository initializes, +push your entire existing repository to it. + +**Observe First**: //(Git, Mercurial)// Observe the existing repository first, +according to the instructions above. Once Phabricator's copy of the repository +is fully synchronized, change the **I/O Type** for the **Observe** URI to +**None** to stop fetching changes from the remote. + +By default, this will automatically make Phabricator's copy of the repository +writable, and you can begin pushing to it. If you've adjusted URI +configuration away from the defaults, you may need to set at least one URI +to **Read/Write** mode so you can push to it. + +**Copy on Disk**: //(Git, Mercurial, Subversion)// Create a new empty hosted +repository according to the instructions above, but do not activate it yet. + +Using the **Storage** tab, find the location of the repository's working copy +on disk, and place a working copy of the repository you wish to import there. + +For Git and Mercurial, use a bare working copy for best results. + +This is the only way to import a Subversion repository because only the master +copy of the repository has history. + +Once you've put a working copy in the right place on disk, activate the +repository. + + +Customizing Displayed Clone URIs +================================ + +If you have an unusual configuration and want the UI to offers users specific +clone URIs other than the URIs that Phabricator serves or interacts with, you +can add those URIs with the **I/O Type** set to **None** and then set their +**Display Type** to **Always**. + +Likewise, you can set the **Display Type** of any URIs you do //not// want +to be visible to **Never**. + +This allows you to precisely configure which clone URIs are shown to users for +a repository. + + +Reference: I/O Types +==================== + +This section details the available **I/O Type** options for URIs. + +Each repository has some **builtin** URIs. These are URIs hosted by Phabricator +itself. The modes available for each URI depend primarily on whether it is a +builtin URI or not. + +**Default**: This setting has Phabricator guess the correct option for the +URI. + +For **builtin** URIs, the default behavior is //Read/Write// if the repository +is hosted, and //Read-Only// if the repository is observed. + +For custom URIs, the default type is //None// because we can not automatically +guess if you want to ignore, observe, or mirror a URI and //None// is the +safest default. + +**Observe**: Phabricator will observe this repository and regularly fetch any +changes made to it to a local read-only copy. + +You can not observe builtin URIs because reading a repository from itself +does not make sense. + +You can not add a URI in Observe mode if an existing builtin URI is in +//Read/Write// mode, because this would mean the repository had two different +authorities: the observed remote copy and the hosted local copy. Take the +other URI out of //Read/Write// mode first. + +**Mirror**: Phabricator will push any changes made to this repository to the +remote URI, keeping a read-only mirror hosted at that URI up to date. + +This works for both observed and hosted repositories. + +This option is not available for builtin URIs because it does not make sense +to mirror a repository to itself. + +It is possible to mirror a repository to another repository that is also +hosted by Phabricator by adding that other repository's URI, although this is +silly and probably very rarely of any use. + +**None**: Phabricator will not fetch changes from or push changes to this URI. +For builtin URIs, it will not let users fetch changes from or push changes to +this URI. + +You can use this mode to turn off an Observe URI after an import, stop a Mirror +URI from updating, or to add URIs that you're only using to customize which +clone URIs are displayed to the user but don't want Phabricator to interact +with directly. + +**Read Only**: Phabricator will serve the repository from this URI in read-only +mode. Users will be able to fetch from it but will not be able to push to it. + +Because Phabricator must be able to serve the repository from URIs configured +in this mode, this option is only available for builtin URIs. + +**Read/Write**: Phabricator will serve the repository from this URI in +read/write mode. Users will be able to fetch from it and push to it. + +URIs can not be set into this mode if another URI is set to //Observe// mode, +because that would mean the repository had two different authorities: the +observed remote copy and the hosted local copy. Take the other URI out of +//Observe// mode first. + +Because Phabricator must be able to serve the repository from URIs configured +in this mode, this option is only available for builtin URIs. + + +Reference: Display Types +======================== + +This section details the available **Display Type** options for URIs. + +**Default**: Phabricator will guess the correct option for the URI. It +guesses based on the configured **I/O Type** and whether the URI is +**builtin** or not. + +For //Observe//, //Mirror// and //None// URIs, the default is //Never//. + +For builtin URIs in //Read Only// or //Read/Write// mode, the most +human-readable URI defaults to //Always// and the others default to //Never//. + +**Always**: This URI will be shown to users as a clone/checkout URI. You can +add URIs in this mode to cutomize exactly what users are shown. + +**Never**: This URI will not be shown to users. You can hide less-preferred +URIs to guide users to the URIs they should be using to interact with the +repository. + + +Next Steps +========== + +Continue by: + + - configuring Phabricator to host repositories with + @{article:Diffusion User Guide: Repository Hosting}. From da599386f6452875c4c283c7c19573eb3d32ac82 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Apr 2016 13:10:37 -0700 Subject: [PATCH 28/32] Add `diffusion.uri.edit` for creating and editing repository URIs Summary: Ref T10748. Brings the rest of the transactions to EditEngine, supports creating via API. Test Plan: - Created a URI via API. - Created a URI via web. - Tried to apply sneaky transactions, got rejected with good error messages. <_< >_> Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15821 --- src/__phutil_library_map__.php | 2 + .../DiffusionURIEditConduitAPIMethod.php | 20 ++++ .../editor/DiffusionURIEditEngine.php | 48 +++++++- .../diffusion/editor/DiffusionURIEditor.php | 113 +++++++++++++++++- .../storage/PhabricatorRepository.php | 4 +- .../storage/PhabricatorRepositoryURI.php | 4 +- .../PhabricatorRepositoryURITransaction.php | 3 + 7 files changed, 188 insertions(+), 6 deletions(-) create mode 100644 src/applications/diffusion/conduit/DiffusionURIEditConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 41c2d8cbc0..bcb7c6374a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -816,6 +816,7 @@ phutil_register_library_map(array( 'DiffusionTagListController' => 'applications/diffusion/controller/DiffusionTagListController.php', 'DiffusionTagListView' => 'applications/diffusion/view/DiffusionTagListView.php', 'DiffusionTagsQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionTagsQueryConduitAPIMethod.php', + 'DiffusionURIEditConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionURIEditConduitAPIMethod.php', 'DiffusionURIEditEngine' => 'applications/diffusion/editor/DiffusionURIEditEngine.php', 'DiffusionURIEditor' => 'applications/diffusion/editor/DiffusionURIEditor.php', 'DiffusionURITestCase' => 'applications/diffusion/request/__tests__/DiffusionURITestCase.php', @@ -5044,6 +5045,7 @@ phutil_register_library_map(array( 'DiffusionTagListController' => 'DiffusionController', 'DiffusionTagListView' => 'DiffusionView', 'DiffusionTagsQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', + 'DiffusionURIEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'DiffusionURIEditEngine' => 'PhabricatorEditEngine', 'DiffusionURIEditor' => 'PhabricatorApplicationTransactionEditor', 'DiffusionURITestCase' => 'PhutilTestCase', diff --git a/src/applications/diffusion/conduit/DiffusionURIEditConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionURIEditConduitAPIMethod.php new file mode 100644 index 0000000000..dace4da838 --- /dev/null +++ b/src/applications/diffusion/conduit/DiffusionURIEditConduitAPIMethod.php @@ -0,0 +1,20 @@ +getRepository(); - return PhabricatorRepositoryURI::initializeNewURI($repository); + if ($repository) { + $uri->setRepositoryPHID($repository->getPHID()); + $uri->attachRepository($repository); + } + + return $uri; } protected function newObjectQuery() { @@ -77,6 +84,23 @@ final class DiffusionURIEditEngine $viewer = $this->getViewer(); return array( + id(new PhabricatorHandlesEditField()) + ->setKey('repository') + ->setAliases(array('repositoryPHID')) + ->setLabel(pht('Repository')) + ->setIsRequired(true) + ->setIsConduitOnly(true) + ->setTransactionType( + PhabricatorRepositoryURITransaction::TYPE_REPOSITORY) + ->setDescription(pht('The repository this URI is associated with.')) + ->setConduitDescription( + pht( + 'Create a URI in a given repository. This transaction type '. + 'must be present when creating a new URI and must not be '. + 'present when editing an existing URI.')) + ->setConduitTypeDescription( + pht('Repository PHID to create a new URI for.')) + ->setSingleValue($object->getRepositoryPHID()), id(new PhabricatorTextEditField()) ->setKey('uri') ->setLabel(pht('URI')) @@ -104,6 +128,28 @@ final class DiffusionURIEditEngine ->setConduitTypeDescription(pht('New display behavior.')) ->setValue($object->getDisplayType()) ->setOptions($object->getAvailableDisplayTypeOptions()), + id(new PhabricatorHandlesEditField()) + ->setKey('credential') + ->setAliases(array('credentialPHID')) + ->setLabel(pht('Credential')) + ->setIsConduitOnly(true) + ->setTransactionType( + PhabricatorRepositoryURITransaction::TYPE_CREDENTIAL) + ->setDescription( + pht('The credential to use when interacting with this URI.')) + ->setConduitDescription(pht('Change the credential for this URI.')) + ->setConduitTypeDescription(pht('New credential PHID, or null.')) + ->setSingleValue($object->getCredentialPHID()), + id(new PhabricatorBoolEditField()) + ->setKey('disable') + ->setLabel(pht('Disabled')) + ->setIsConduitOnly(true) + ->setTransactionType(PhabricatorRepositoryURITransaction::TYPE_DISABLE) + ->setDescription(pht('Active status of the URI.')) + ->setConduitDescription(pht('Disable or activate the URI.')) + ->setConduitTypeDescription(pht('True to disable the URI.')) + ->setOptions(pht('Enable'), pht('Disable')) + ->setValue($object->getIsDisabled()), ); } diff --git a/src/applications/diffusion/editor/DiffusionURIEditor.php b/src/applications/diffusion/editor/DiffusionURIEditor.php index 0dfdd4e704..d124ff4b4c 100644 --- a/src/applications/diffusion/editor/DiffusionURIEditor.php +++ b/src/applications/diffusion/editor/DiffusionURIEditor.php @@ -14,9 +14,12 @@ final class DiffusionURIEditor public function getTransactionTypes() { $types = parent::getTransactionTypes(); + $types[] = PhabricatorRepositoryURITransaction::TYPE_REPOSITORY; $types[] = PhabricatorRepositoryURITransaction::TYPE_URI; $types[] = PhabricatorRepositoryURITransaction::TYPE_IO; $types[] = PhabricatorRepositoryURITransaction::TYPE_DISPLAY; + $types[] = PhabricatorRepositoryURITransaction::TYPE_CREDENTIAL; + $types[] = PhabricatorRepositoryURITransaction::TYPE_DISABLE; return $types; } @@ -32,6 +35,12 @@ final class DiffusionURIEditor return $object->getIOType(); case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: return $object->getDisplayType(); + case PhabricatorRepositoryURITransaction::TYPE_REPOSITORY: + return $object->getRepositoryPHID(); + case PhabricatorRepositoryURITransaction::TYPE_CREDENTIAL: + return $object->getCredentialPHID(); + case PhabricatorRepositoryURITransaction::TYPE_DISABLE: + return (int)$object->getIsDisabled(); } return parent::getCustomTransactionOldValue($object, $xaction); @@ -45,7 +54,11 @@ final class DiffusionURIEditor case PhabricatorRepositoryURITransaction::TYPE_URI: case PhabricatorRepositoryURITransaction::TYPE_IO: case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: + case PhabricatorRepositoryURITransaction::TYPE_REPOSITORY: + case PhabricatorRepositoryURITransaction::TYPE_CREDENTIAL: return $xaction->getNewValue(); + case PhabricatorRepositoryURITransaction::TYPE_DISABLE: + return (int)$xaction->getNewValue(); } return parent::getCustomTransactionNewValue($object, $xaction); @@ -65,6 +78,15 @@ final class DiffusionURIEditor case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: $object->setDisplayType($xaction->getNewValue()); break; + case PhabricatorRepositoryURITransaction::TYPE_REPOSITORY: + $object->setRepositoryPHID($xaction->getNewValue()); + break; + case PhabricatorRepositoryURITransaction::TYPE_CREDENTIAL: + $object->setCredentialPHID($xaction->getNewValue()); + break; + case PhabricatorRepositoryURITransaction::TYPE_DISABLE: + $object->setIsDisabled($xaction->getNewValue()); + break; } } @@ -76,6 +98,9 @@ final class DiffusionURIEditor case PhabricatorRepositoryURITransaction::TYPE_URI: case PhabricatorRepositoryURITransaction::TYPE_IO: case PhabricatorRepositoryURITransaction::TYPE_DISPLAY: + case PhabricatorRepositoryURITransaction::TYPE_REPOSITORY: + case PhabricatorRepositoryURITransaction::TYPE_CREDENTIAL: + case PhabricatorRepositoryURITransaction::TYPE_DISABLE: return; } @@ -90,6 +115,92 @@ final class DiffusionURIEditor $errors = parent::validateTransaction($object, $type, $xactions); switch ($type) { + case PhabricatorRepositoryURITransaction::TYPE_REPOSITORY: + $missing = $this->validateIsEmptyTextField( + $object->getRepositoryPHID(), + $xactions); + if ($missing) { + // NOTE: This isn't being marked as a missing field error because + // it's a fundamental, required property of the URI. + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Required'), + pht( + 'When creating a repository URI, you must specify which '. + 'repository the URI will belong to.'), + nonempty(last($xactions), null)); + break; + } + + $viewer = $this->getActor(); + + foreach ($xactions as $xaction) { + $repository_phid = $xaction->getNewValue(); + + // If this isn't changing anything, let it through as-is. + if ($repository_phid == $object->getRepositoryPHID()) { + continue; + } + + if (!$this->getIsNewObject()) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'The repository a URI is associated with is immutable, and '. + 'can not be changed after the URI is created.'), + $xaction); + continue; + } + + $repository = id(new PhabricatorRepositoryQuery()) + ->setViewer($viewer) + ->withPHIDs(array($repository_phid)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$repository) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'To create a URI for a repository ("%s"), it must exist and '. + 'you must have permission to edit it.', + $repository_phid), + $xaction); + continue; + } + } + break; + case PhabricatorRepositoryURITransaction::TYPE_CREDENTIAL: + $viewer = $this->getActor(); + foreach ($xactions as $xaction) { + $credential_phid = $xaction->getNewValue(); + + if ($credential_phid == $object->getCredentialPHID()) { + continue; + } + + $credential = id(new PassphraseCredentialQuery()) + ->setViewer($viewer) + ->withPHIDs(array($credential_phid)) + ->executeOne(); + if (!$credential) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'You can only associate a credential ("%s") with a repository '. + 'URI if it exists and you have permission to see it.', + $credential_phid), + $xaction); + continue; + } + } + break; case PhabricatorRepositoryURITransaction::TYPE_URI: $missing = $this->validateIsEmptyTextField( $object->getURI(), @@ -99,7 +210,7 @@ final class DiffusionURIEditor $error = new PhabricatorApplicationTransactionValidationError( $type, pht('Required'), - pht('Repository URI is required.'), + pht('A repository URI must have a nonempty URI.'), nonempty(last($xactions), null)); $error->setIsMissingFieldError(true); diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 1d251cb34b..94584fd382 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -2179,7 +2179,9 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $uris = array(); foreach ($protocol_map as $protocol => $proto_supported) { foreach ($identifier_map as $identifier => $id_supported) { - $uris[] = PhabricatorRepositoryURI::initializeNewURI($this) + $uris[] = PhabricatorRepositoryURI::initializeNewURI() + ->setRepositoryPHID($this->getPHID()) + ->attachRepository($this) ->setBuiltinProtocol($protocol) ->setBuiltinIdentifier($identifier) ->setIsDisabled(!$proto_supported || !$id_supported); diff --git a/src/applications/repository/storage/PhabricatorRepositoryURI.php b/src/applications/repository/storage/PhabricatorRepositoryURI.php index 364aad0a29..5d1ed6d358 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryURI.php +++ b/src/applications/repository/storage/PhabricatorRepositoryURI.php @@ -62,10 +62,8 @@ final class PhabricatorRepositoryURI ) + parent::getConfiguration(); } - public static function initializeNewURI(PhabricatorRepository $repository) { + public static function initializeNewURI() { return id(new self()) - ->attachRepository($repository) - ->setRepositoryPHID($repository->getPHID()) ->setIoType(self::IO_DEFAULT) ->setDisplayType(self::DISPLAY_DEFAULT) ->setIsDisabled(0); diff --git a/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php b/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php index e27a91c20b..f3aee8dce0 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php +++ b/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php @@ -3,9 +3,12 @@ final class PhabricatorRepositoryURITransaction extends PhabricatorApplicationTransaction { + const TYPE_REPOSITORY = 'diffusion.uri.repository'; const TYPE_URI = 'diffusion.uri.uri'; const TYPE_IO = 'diffusion.uri.io'; const TYPE_DISPLAY = 'diffusion.uri.display'; + const TYPE_CREDENTIAL = 'diffusion.uri.credential'; + const TYPE_DISABLE = 'diffusion.uri.disable'; public function getApplicationName() { return 'repository'; From c314a3672f70200d0409bd726fe83316af5965a3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Apr 2016 14:10:04 -0700 Subject: [PATCH 29/32] Allow callers to query information about repository URIs from diffusion.repository.search Summary: Ref T10748. Adds a "uris" attachment with URI information. Test Plan: Queried URI information via Conduit, saw reasonable looking information. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15822 --- src/__phutil_library_map__.php | 3 + ...onRepositoryURIsSearchEngineAttachment.php | 34 ++++++++ .../storage/PhabricatorRepository.php | 10 ++- .../storage/PhabricatorRepositoryURI.php | 86 ++++++++++++++++++- 4 files changed, 131 insertions(+), 2 deletions(-) create mode 100644 src/applications/diffusion/engineextension/DiffusionRepositoryURIsSearchEngineAttachment.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bcb7c6374a..996d873ca9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -793,6 +793,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryURIViewController' => 'applications/diffusion/controller/DiffusionRepositoryURIViewController.php', 'DiffusionRepositoryURIsIndexEngineExtension' => 'applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php', 'DiffusionRepositoryURIsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php', + 'DiffusionRepositoryURIsSearchEngineAttachment' => 'applications/diffusion/engineextension/DiffusionRepositoryURIsSearchEngineAttachment.php', 'DiffusionRequest' => 'applications/diffusion/request/DiffusionRequest.php', 'DiffusionResolveRefsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionResolveRefsConduitAPIMethod.php', 'DiffusionResolveUserQuery' => 'applications/diffusion/query/DiffusionResolveUserQuery.php', @@ -5022,6 +5023,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryURIViewController' => 'DiffusionController', 'DiffusionRepositoryURIsIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'DiffusionRepositoryURIsManagementPanel' => 'DiffusionRepositoryManagementPanel', + 'DiffusionRepositoryURIsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'DiffusionRequest' => 'Phobject', 'DiffusionResolveRefsConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionResolveUserQuery' => 'Phobject', @@ -7932,6 +7934,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionInterface', 'PhabricatorPolicyInterface', 'PhabricatorExtendedPolicyInterface', + 'PhabricatorConduitResultInterface', ), 'PhabricatorRepositoryURIIndex' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryURINormalizer' => 'Phobject', diff --git a/src/applications/diffusion/engineextension/DiffusionRepositoryURIsSearchEngineAttachment.php b/src/applications/diffusion/engineextension/DiffusionRepositoryURIsSearchEngineAttachment.php new file mode 100644 index 0000000000..1eab49d77a --- /dev/null +++ b/src/applications/diffusion/engineextension/DiffusionRepositoryURIsSearchEngineAttachment.php @@ -0,0 +1,34 @@ +needURIs(true); + } + + public function getAttachmentForObject($object, $data, $spec) { + $uris = array(); + foreach ($object->getURIs() as $uri) { + $uris[] = array( + 'id' => $uri->getID(), + 'type' => phid_get_type($uri->getPHID()), + 'phid' => $uri->getPHID(), + 'fields' => $uri->getFieldValuesForConduit(), + ); + } + + return array( + 'uris' => $uris, + ); + } + +} diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 94584fd382..11614b3649 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -2454,6 +2454,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO ->setKey('shortName') ->setType('string') ->setDescription(pht('Unique short name, if the repository has one.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('status') + ->setType('string') + ->setDescription(pht('Active or inactive status.')), ); } @@ -2463,11 +2467,15 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO 'vcs' => $this->getVersionControlSystem(), 'callsign' => $this->getCallsign(), 'shortName' => $this->getRepositorySlug(), + 'status' => $this->getStatus(), ); } public function getConduitSearchAttachments() { - return array(); + return array( + id(new DiffusionRepositoryURIsSearchEngineAttachment()) + ->setAttachmentKey('uris'), + ); } } diff --git a/src/applications/repository/storage/PhabricatorRepositoryURI.php b/src/applications/repository/storage/PhabricatorRepositoryURI.php index 5d1ed6d358..7e7741e372 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryURI.php +++ b/src/applications/repository/storage/PhabricatorRepositoryURI.php @@ -5,7 +5,8 @@ final class PhabricatorRepositoryURI implements PhabricatorApplicationTransactionInterface, PhabricatorPolicyInterface, - PhabricatorExtendedPolicyInterface { + PhabricatorExtendedPolicyInterface, + PhabricatorConduitResultInterface { protected $repositoryPHID; protected $uri; @@ -512,4 +513,87 @@ final class PhabricatorRepositoryURI return $extended; } + +/* -( PhabricatorConduitResultInterface )---------------------------------- */ + + + public function getFieldSpecificationsForConduit() { + return array( + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('repositoryPHID') + ->setType('phid') + ->setDescription(pht('The associated repository PHID.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('uri') + ->setType('map') + ->setDescription(pht('The raw and effective URI.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('io') + ->setType('map') + ->setDescription( + pht('The raw, default, and effective I/O Type settings.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('display') + ->setType('map') + ->setDescription( + pht('The raw, default, and effective Display Type settings.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('credentialPHID') + ->setType('phid?') + ->setDescription( + pht('The associated credential PHID, if one exists.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('disabled') + ->setType('bool') + ->setDescription(pht('True if the URI is disabled.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('builtin') + ->setType('map') + ->setDescription( + pht('Information about builtin URIs.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('dateCreated') + ->setType('int') + ->setDescription( + pht('Epoch timestamp when the object was created.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('dateModified') + ->setType('int') + ->setDescription( + pht('Epoch timestamp when the object was last updated.')), + ); + } + + public function getFieldValuesForConduit() { + return array( + 'repositoryPHID' => $this->getRepositoryPHID(), + 'uri' => array( + 'raw' => $this->getURI(), + 'effective' => (string)$this->getDisplayURI(), + ), + 'io' => array( + 'raw' => $this->getIOType(), + 'default' => $this->getDefaultIOType(), + 'effective' => $this->getEffectiveIOType(), + ), + 'display' => array( + 'raw' => $this->getDisplayType(), + 'default' => $this->getDefaultDisplayType(), + 'effective' => $this->getEffectiveDisplayType(), + ), + 'credentialPHID' => $this->getCredentialPHID(), + 'disabled' => (bool)$this->getIsDisabled(), + 'builtin' => array( + 'protocol' => $this->getBuiltinProtocol(), + 'identifier' => $this->getBuiltinIdentifier(), + ), + 'dateCreated' => $this->getDateCreated(), + 'dateModified' => $this->getDateModified(), + ); + } + + public function getConduitSearchAttachments() { + return array(); + } + } From 06abeb20105f315891604736d9def9aea599b83b Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Apr 2016 16:37:45 -0700 Subject: [PATCH 30/32] Allow tall two-column/curtain headers to be triple-clicked in Firefox Summary: Fixes T10905. In Firefox, triple clicking the new headers doesn't select the entire line, so you can't easily copy/paste an entire task title or revision name. It works fine in Safari/Chrome. This seems to fix that without breaking anything. Test Plan: - Viewed headers in Safari, Firefox, Chrome. - Triple-clicked headers in Safari, Firefox, Chrome. - Viewed tablet/device layouts in Safari, Firefox, Chrome. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10905 Differential Revision: https://secure.phabricator.com/D15823 --- resources/celerity/map.php | 6 +++--- src/view/phui/PHUIHeaderView.php | 9 +++++++++ webroot/rsrc/css/phui/phui-header-view.css | 4 ++++ 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index d72545dc93..0ec094c82b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => '04a95108', + 'core.pkg.css' => 'ede6bf7a', 'core.pkg.js' => '6972d365', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '7ba78475', @@ -136,7 +136,7 @@ return array( 'rsrc/css/phui/phui-form-view.css' => '6a51768e', 'rsrc/css/phui/phui-form.css' => 'aac1d51d', 'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f', - 'rsrc/css/phui/phui-header-view.css' => '230254d3', + 'rsrc/css/phui/phui-header-view.css' => '9961e480', 'rsrc/css/phui/phui-hovercard.css' => 'de1a2119', 'rsrc/css/phui/phui-icon-set-selector.css' => '1ab67aad', 'rsrc/css/phui/phui-icon.css' => '3f33ab57', @@ -829,7 +829,7 @@ return array( 'phui-form-css' => 'aac1d51d', 'phui-form-view-css' => '6a51768e', 'phui-head-thing-view-css' => 'fd311e5f', - 'phui-header-view-css' => '230254d3', + 'phui-header-view-css' => '9961e480', 'phui-hovercard' => '1bd28176', 'phui-hovercard-view-css' => 'de1a2119', 'phui-icon-set-selector-css' => '1ab67aad', diff --git a/src/view/phui/PHUIHeaderView.php b/src/view/phui/PHUIHeaderView.php index 8111944c4d..85f55b3ef7 100644 --- a/src/view/phui/PHUIHeaderView.php +++ b/src/view/phui/PHUIHeaderView.php @@ -316,6 +316,15 @@ final class PHUIHeaderView extends AphrontTagView { $header_content); } + // This wrapper element allows titles to be triple-clicked to select them + // in Firefox. See T10905 for discussion. + $header_content = phutil_tag( + 'span', + array( + 'class' => 'phui-header-content', + ), + $header_content); + $left[] = phutil_tag( 'span', array( diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css index 2a1731eb61..6cbf3b891a 100644 --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -80,6 +80,10 @@ body .phui-header-shell.phui-bleed-header color: {$lightbluetext}; } +.phui-header-content { + display: inline-block; +} + .phui-object-box .phui-header-tall .phui-header-header, .phui-document-view .phui-header-tall .phui-header-header { font-size: 18px; From e8dca5a4f9be51398091c82fddce5165b6853a74 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Apr 2016 16:52:27 -0700 Subject: [PATCH 31/32] Alternate fix for Firefox triple click selection Summary: Fixes T10905. This reverts D15823, which didn't work well for tasks with very long titles (the title would break as a block element). This is slightly more magic but works with long titles. Test Plan: Did everything from D15823, but also with long titles. Triple-click, wrapping, and mobile/device worked in Safari, Firefox and Chrome. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10905 Differential Revision: https://secure.phabricator.com/D15824 --- resources/celerity/map.php | 6 +++--- src/view/phui/PHUIHeaderView.php | 9 --------- webroot/rsrc/css/phui/phui-header-view.css | 6 +++--- 3 files changed, 6 insertions(+), 15 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0ec094c82b..cd0efd5db5 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -7,7 +7,7 @@ */ return array( 'names' => array( - 'core.pkg.css' => 'ede6bf7a', + 'core.pkg.css' => 'b729f9f5', 'core.pkg.js' => '6972d365', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '7ba78475', @@ -136,7 +136,7 @@ return array( 'rsrc/css/phui/phui-form-view.css' => '6a51768e', 'rsrc/css/phui/phui-form.css' => 'aac1d51d', 'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f', - 'rsrc/css/phui/phui-header-view.css' => '9961e480', + 'rsrc/css/phui/phui-header-view.css' => '4c7dd8f5', 'rsrc/css/phui/phui-hovercard.css' => 'de1a2119', 'rsrc/css/phui/phui-icon-set-selector.css' => '1ab67aad', 'rsrc/css/phui/phui-icon.css' => '3f33ab57', @@ -829,7 +829,7 @@ return array( 'phui-form-css' => 'aac1d51d', 'phui-form-view-css' => '6a51768e', 'phui-head-thing-view-css' => 'fd311e5f', - 'phui-header-view-css' => '9961e480', + 'phui-header-view-css' => '4c7dd8f5', 'phui-hovercard' => '1bd28176', 'phui-hovercard-view-css' => 'de1a2119', 'phui-icon-set-selector-css' => '1ab67aad', diff --git a/src/view/phui/PHUIHeaderView.php b/src/view/phui/PHUIHeaderView.php index 85f55b3ef7..8111944c4d 100644 --- a/src/view/phui/PHUIHeaderView.php +++ b/src/view/phui/PHUIHeaderView.php @@ -316,15 +316,6 @@ final class PHUIHeaderView extends AphrontTagView { $header_content); } - // This wrapper element allows titles to be triple-clicked to select them - // in Firefox. See T10905 for discussion. - $header_content = phutil_tag( - 'span', - array( - 'class' => 'phui-header-content', - ), - $header_content); - $left[] = phutil_tag( 'span', array( diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css index 6cbf3b891a..2336ec3696 100644 --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -78,10 +78,10 @@ body .phui-header-shell.phui-bleed-header .phui-header-header .phui-header-icon { margin-right: 8px; color: {$lightbluetext}; -} -.phui-header-content { - display: inline-block; + /* This allows the header text to be triple-clicked to select it in Firefox, + see T10905 for discussion. */ + display: inline; } .phui-object-box .phui-header-tall .phui-header-header, From 0ba3939ce344fe55ab1cb9226bfd1b7dbc91d278 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Apr 2016 16:51:05 -0700 Subject: [PATCH 32/32] Flesh out more web UI actions for new URI interface Summary: Ref T10748. - Allow users to add new URIs by clicking a button instead of knowing a secret URI. - Validate that URIs are actually valid URIs. - Add enable/disable action and strings. Test Plan: - Created a new URI. - Tried to create a nonsense URI, created a good URI. - Enabled/disabled a URI. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10748 Differential Revision: https://secure.phabricator.com/D15825 --- src/__phutil_library_map__.php | 4 +- .../PhabricatorDiffusionApplication.php | 2 + ...iffusionRepositoryURIDisableController.php | 72 +++++++++++++++++++ .../DiffusionRepositoryURIEditController.php | 2 +- .../DiffusionRepositoryURIViewController.php | 22 +++++- .../diffusion/editor/DiffusionURIEditor.php | 19 +++++ ...DiffusionRepositoryURIsManagementPanel.php | 10 ++- .../storage/PhabricatorRepository.php | 4 +- .../PhabricatorRepositoryURITransaction.php | 10 +++ 9 files changed, 138 insertions(+), 7 deletions(-) create mode 100644 src/applications/diffusion/controller/DiffusionRepositoryURIDisableController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 996d873ca9..b0ffd4eef5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -789,6 +789,7 @@ phutil_register_library_map(array( 'DiffusionRepositorySymbolsManagementPanel' => 'applications/diffusion/management/DiffusionRepositorySymbolsManagementPanel.php', 'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php', 'DiffusionRepositoryTestAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php', + 'DiffusionRepositoryURIDisableController' => 'applications/diffusion/controller/DiffusionRepositoryURIDisableController.php', 'DiffusionRepositoryURIEditController' => 'applications/diffusion/controller/DiffusionRepositoryURIEditController.php', 'DiffusionRepositoryURIViewController' => 'applications/diffusion/controller/DiffusionRepositoryURIViewController.php', 'DiffusionRepositoryURIsIndexEngineExtension' => 'applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php', @@ -5019,7 +5020,8 @@ phutil_register_library_map(array( 'DiffusionRepositorySymbolsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryTag' => 'Phobject', 'DiffusionRepositoryTestAutomationController' => 'DiffusionRepositoryEditController', - 'DiffusionRepositoryURIEditController' => 'DiffusionRepositoryEditController', + 'DiffusionRepositoryURIDisableController' => 'DiffusionController', + 'DiffusionRepositoryURIEditController' => 'DiffusionController', 'DiffusionRepositoryURIViewController' => 'DiffusionController', 'DiffusionRepositoryURIsIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'DiffusionRepositoryURIsManagementPanel' => 'DiffusionRepositoryManagementPanel', diff --git a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php index f910c090b7..ec27124d05 100644 --- a/src/applications/diffusion/application/PhabricatorDiffusionApplication.php +++ b/src/applications/diffusion/application/PhabricatorDiffusionApplication.php @@ -93,6 +93,8 @@ final class PhabricatorDiffusionApplication extends PhabricatorApplication { => 'DiffusionRepositoryManageController', 'uri/' => array( 'view/(?P[0-9]\d*)/' => 'DiffusionRepositoryURIViewController', + 'disable/(?P[0-9]\d*)/' + => 'DiffusionRepositoryURIDisableController', $this->getEditRoutePattern('edit/') => 'DiffusionRepositoryURIEditController', ), diff --git a/src/applications/diffusion/controller/DiffusionRepositoryURIDisableController.php b/src/applications/diffusion/controller/DiffusionRepositoryURIDisableController.php new file mode 100644 index 0000000000..b846b3f4e2 --- /dev/null +++ b/src/applications/diffusion/controller/DiffusionRepositoryURIDisableController.php @@ -0,0 +1,72 @@ +loadDiffusionContextForEdit(); + if ($response) { + return $response; + } + + $viewer = $this->getViewer(); + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + $id = $request->getURIData('id'); + $uri = id(new PhabricatorRepositoryURIQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->withRepositories(array($repository)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$uri) { + return new Aphront404Response(); + } + + $is_disabled = $uri->getIsDisabled(); + $view_uri = $uri->getViewURI(); + + if ($request->isFormPost()) { + $xactions = array(); + + $xactions[] = id(new PhabricatorRepositoryURITransaction()) + ->setTransactionType(PhabricatorRepositoryURITransaction::TYPE_DISABLE) + ->setNewValue(!$is_disabled); + + $editor = id(new DiffusionURIEditor()) + ->setActor($viewer) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->setContentSourceFromRequest($request) + ->applyTransactions($uri, $xactions); + + return id(new AphrontRedirectResponse())->setURI($view_uri); + } + + if ($is_disabled) { + $title = pht('Enable URI'); + $body = pht( + 'Enable this URI? Any configured behaviors will begin working '. + 'again.'); + $button = pht('Enable URI'); + } else { + $title = pht('Disable URI'); + $body = pht( + 'Disable this URI? It will no longer be observed, fetched, mirrored, '. + 'served or shown to users.'); + $button = pht('Disable URI'); + } + + return $this->newDialog() + ->setTitle($title) + ->appendParagraph($body) + ->addCancelButton($view_uri) + ->addSubmitButton($button); + } + +} diff --git a/src/applications/diffusion/controller/DiffusionRepositoryURIEditController.php b/src/applications/diffusion/controller/DiffusionRepositoryURIEditController.php index f52edd1f35..4a1dc56d4a 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryURIEditController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryURIEditController.php @@ -1,7 +1,7 @@ loadDiffusionContextForEdit(); diff --git a/src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php b/src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php index b7da37ebe2..8cd143c5a1 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryURIViewController.php @@ -82,16 +82,18 @@ final class DiffusionRepositoryURIViewController private function buildCurtain(PhabricatorRepositoryURI $uri) { $viewer = $this->getViewer(); + $id = $uri->getID(); $can_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $uri, PhabricatorPolicyCapability::CAN_EDIT); - $edit_uri = $uri->getEditURI(); $curtain = $this->newCurtainView($uri); + $edit_uri = $uri->getEditURI(); + $curtain->addAction( id(new PhabricatorActionView()) ->setIcon('fa-pencil') @@ -100,6 +102,24 @@ final class DiffusionRepositoryURIViewController ->setWorkflow(!$can_edit) ->setDisabled(!$can_edit)); + if ($uri->getIsDisabled()) { + $disable_name = pht('Enable URI'); + $disable_icon = 'fa-check'; + } else { + $disable_name = pht('Disable URI'); + $disable_icon = 'fa-ban'; + } + + $disable_uri = $uri->getRepository()->getPathURI("uri/disable/{$id}/"); + + $curtain->addAction( + id(new PhabricatorActionView()) + ->setIcon($disable_icon) + ->setName($disable_name) + ->setHref($disable_uri) + ->setWorkflow(true) + ->setDisabled(!$can_edit)); + return $curtain; } diff --git a/src/applications/diffusion/editor/DiffusionURIEditor.php b/src/applications/diffusion/editor/DiffusionURIEditor.php index d124ff4b4c..3d2164ad0b 100644 --- a/src/applications/diffusion/editor/DiffusionURIEditor.php +++ b/src/applications/diffusion/editor/DiffusionURIEditor.php @@ -217,6 +217,25 @@ final class DiffusionURIEditor $errors[] = $error; break; } + + foreach ($xactions as $xaction) { + $new_uri = $xaction->getNewValue(); + if ($new_uri == $object->getURI()) { + continue; + } + + try { + PhabricatorRepository::assertValidRemoteURI($new_uri); + } catch (Exception $ex) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + $ex->getMessage(), + $xaction); + continue; + } + } + break; case PhabricatorRepositoryURITransaction::TYPE_IO: $available = $object->getAvailableIOTypeOptions(); diff --git a/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php index 113914b8d4..634535fef4 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryURIsManagementPanel.php @@ -91,11 +91,17 @@ final class DiffusionRepositoryURIsManagementPanel null, )); - $doc_href = PhabricatorEnv::getDoclink( - 'Diffusion User Guide: URIs'); + $doc_href = PhabricatorEnv::getDoclink('Diffusion User Guide: URIs'); + $add_href = $repository->getPathURI('uri/edit/'); $header = id(new PHUIHeaderView()) ->setHeader(pht('Repository URIs')) + ->addActionLink( + id(new PHUIButtonView()) + ->setIcon('fa-plus') + ->setHref($add_href) + ->setTag('a') + ->setText(pht('Add New URI'))) ->addActionLink( id(new PHUIButtonView()) ->setIcon('fa-book') diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 11614b3649..be77852be8 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1829,8 +1829,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO throw new Exception( pht( - "The URI protocol is unrecognized. It should begin ". - "'%s', '%s', '%s', '%s', '%s', '%s', or be in the form '%s'.", + 'The URI protocol is unrecognized. It should begin with '. + '"%s", "%s", "%s", "%s", "%s", "%s", or be in the form "%s".', 'ssh://', 'http://', 'https://', diff --git a/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php b/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php index f3aee8dce0..034aa48d7c 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php +++ b/src/applications/repository/storage/PhabricatorRepositoryURITransaction.php @@ -51,6 +51,16 @@ final class PhabricatorRepositoryURITransaction $this->renderHandleLink($author_phid), $old_label, $new_label); + case self::TYPE_DISABLE: + if ($new) { + return pht( + '%s disabled this URI.', + $this->renderHandleLink($author_phid)); + } else { + return pht( + '%s enabled this URI.', + $this->renderHandleLink($author_phid)); + } }