From d4a0b1c8709b61b218c4cb408cdad08caf4b016d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 1 Oct 2015 08:13:43 -0700 Subject: [PATCH] Remove names from Drydock resources Summary: Ref T9252. Long ago you sometimes manually created resources, so they had human-enterable names. However, users never make resources manually any more, so this field isn't really useful any more. In particular, it means we write a lot of untranslatable strings like "Working Copy" to the database in the default locale. Instead, do the call at runtime so resource names are translatable. Also clean up a few minor things I hit while kicking the tires here. It's possible we might eventually want to introduce a human-choosable label so you can rename your favorite resources and this would just be a default name. I don't really have much of a use case for that yet, though, and I'm not sure there will ever be one. Test Plan: - Restarted a Harbormaster build, got a clean build. - Released all leases/resources, restarted build, got a clean build with proper resource names. Reviewers: hach-que, chad Reviewed By: hach-que, chad Maniphest Tasks: T9252 Differential Revision: https://secure.phabricator.com/D14213 --- .../autopatches/20151001.drydock.rname.1.sql | 2 ++ ...anacServiceHostBlueprintImplementation.php | 12 ++++++++++- .../DrydockBlueprintImplementation.php | 20 +++++++++++++----- ...dockWorkingCopyBlueprintImplementation.php | 11 +++++++--- .../controller/DrydockLeaseController.php | 2 +- .../controller/DrydockLogController.php | 2 +- .../DrydockResourceViewController.php | 5 ++++- .../drydock/phid/DrydockResourcePHIDType.php | 2 +- .../drydock/storage/DrydockBlueprint.php | 10 +++++++++ .../drydock/storage/DrydockResource.php | 21 +++++++++++++------ .../drydock/view/DrydockLeaseListView.php | 15 +++---------- .../drydock/view/DrydockResourceListView.php | 7 ++++--- .../worker/DrydockLeaseUpdateWorker.php | 8 ++++--- 13 files changed, 80 insertions(+), 37 deletions(-) create mode 100644 resources/sql/autopatches/20151001.drydock.rname.1.sql diff --git a/resources/sql/autopatches/20151001.drydock.rname.1.sql b/resources/sql/autopatches/20151001.drydock.rname.1.sql new file mode 100644 index 0000000000..3dbd66c579 --- /dev/null +++ b/resources/sql/autopatches/20151001.drydock.rname.1.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_drydock.drydock_resource + DROP name; diff --git a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php index 9a71474624..e458020600 100644 --- a/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockAlmanacServiceHostBlueprintImplementation.php @@ -69,8 +69,9 @@ final class DrydockAlmanacServiceHostBlueprintImplementation $binding_phid = $binding->getPHID(); - $resource = $this->newResourceTemplate($blueprint, $device_name) + $resource = $this->newResourceTemplate($blueprint) ->setActivateWhenAllocated(true) + ->setAttribute('almanacDeviceName', $device_name) ->setAttribute('almanacServicePHID', $binding->getServicePHID()) ->setAttribute('almanacBindingPHID', $binding_phid) ->needSlotLock("almanac.host.binding({$binding_phid})"); @@ -95,6 +96,15 @@ final class DrydockAlmanacServiceHostBlueprintImplementation return; } + public function getResourceName( + DrydockBlueprint $blueprint, + DrydockResource $resource) { + $device_name = $resource->getAttribute( + 'almanacDeviceName', + pht('')); + return pht('Host (%s)', $device_name); + } + public function canAcquireLeaseOnResource( DrydockBlueprint $blueprint, DrydockResource $resource, diff --git a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php index b7e39a49a4..5ec2d60ae5 100644 --- a/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockBlueprintImplementation.php @@ -237,6 +237,19 @@ abstract class DrydockBlueprintImplementation extends Phobject { DrydockResource $resource); + /** + * Get a human readable name for a resource. + * + * @param DrydockBlueprint Blueprint which built the resource. + * @param DrydockResource Resource to get the name of. + * @return string Human-readable resource name. + * @task resource + */ + abstract public function getResourceName( + DrydockBlueprint $blueprint, + DrydockResource $resource); + + /* -( Resource Interfaces )------------------------------------------------ */ @@ -260,16 +273,13 @@ abstract class DrydockBlueprintImplementation extends Phobject { return idx(self::getAllBlueprintImplementations(), $class); } - protected function newResourceTemplate( - DrydockBlueprint $blueprint, - $name) { + protected function newResourceTemplate(DrydockBlueprint $blueprint) { $resource = id(new DrydockResource()) ->setBlueprintPHID($blueprint->getPHID()) ->attachBlueprint($blueprint) ->setType($this->getType()) - ->setStatus(DrydockResourceStatus::STATUS_PENDING) - ->setName($name); + ->setStatus(DrydockResourceStatus::STATUS_PENDING); // Pre-allocate the resource PHID. $resource->setPHID($resource->generatePHID()); diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index a61af96ebf..1c6b71a298 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -97,9 +97,7 @@ final class DrydockWorkingCopyBlueprintImplementation DrydockBlueprint $blueprint, DrydockLease $lease) { - $resource = $this->newResourceTemplate( - $blueprint, - pht('Working Copy')); + $resource = $this->newResourceTemplate($blueprint); $resource_phid = $resource->getPHID(); @@ -185,6 +183,13 @@ final class DrydockWorkingCopyBlueprintImplementation } } + public function getResourceName( + DrydockBlueprint $blueprint, + DrydockResource $resource) { + return pht('Working Copy'); + } + + public function activateLease( DrydockBlueprint $blueprint, DrydockResource $resource, diff --git a/src/applications/drydock/controller/DrydockLeaseController.php b/src/applications/drydock/controller/DrydockLeaseController.php index d5a6335454..48082b8728 100644 --- a/src/applications/drydock/controller/DrydockLeaseController.php +++ b/src/applications/drydock/controller/DrydockLeaseController.php @@ -44,7 +44,7 @@ abstract class DrydockLeaseController $this->getApplicationURI('resource/')); $crumbs->addTextCrumb( - $resource->getName(), + $resource->getResourceName(), $this->getApplicationURI("resource/{$id}/")); $crumbs->addTextCrumb( diff --git a/src/applications/drydock/controller/DrydockLogController.php b/src/applications/drydock/controller/DrydockLogController.php index 5ae87e6aad..704cbb53b9 100644 --- a/src/applications/drydock/controller/DrydockLogController.php +++ b/src/applications/drydock/controller/DrydockLogController.php @@ -91,7 +91,7 @@ abstract class DrydockLogController $this->getApplicationURI('resource/')); $crumbs->addTextCrumb( - $resource->getName(), + $resource->getResourceName(), $this->getApplicationURI("resource/{$id}/")); $crumbs->addTextCrumb( diff --git a/src/applications/drydock/controller/DrydockResourceViewController.php b/src/applications/drydock/controller/DrydockResourceViewController.php index f97081e673..4809cf970c 100644 --- a/src/applications/drydock/controller/DrydockResourceViewController.php +++ b/src/applications/drydock/controller/DrydockResourceViewController.php @@ -15,7 +15,10 @@ final class DrydockResourceViewController extends DrydockResourceController { return new Aphront404Response(); } - $title = pht('Resource %s %s', $resource->getID(), $resource->getName()); + $title = pht( + 'Resource %s %s', + $resource->getID(), + $resource->getResourceName()); $header = id(new PHUIHeaderView()) ->setUser($viewer) diff --git a/src/applications/drydock/phid/DrydockResourcePHIDType.php b/src/applications/drydock/phid/DrydockResourcePHIDType.php index 6b266ff169..9eb85e7561 100644 --- a/src/applications/drydock/phid/DrydockResourcePHIDType.php +++ b/src/applications/drydock/phid/DrydockResourcePHIDType.php @@ -33,7 +33,7 @@ final class DrydockResourcePHIDType extends PhabricatorPHIDType { pht( 'Resource %d: %s', $id, - $resource->getName())); + $resource->getResourceName())); $handle->setURI("/drydock/resource/{$id}/"); } diff --git a/src/applications/drydock/storage/DrydockBlueprint.php b/src/applications/drydock/storage/DrydockBlueprint.php index 429e5c2971..a0d440e4ea 100644 --- a/src/applications/drydock/storage/DrydockBlueprint.php +++ b/src/applications/drydock/storage/DrydockBlueprint.php @@ -162,6 +162,16 @@ final class DrydockBlueprint extends DrydockDAO } + /** + * @task resource + */ + public function getResourceName(DrydockResource $resource) { + return $this->getImplementation()->getResourceName( + $this, + $resource); + } + + /* -( Acquiring Leases )--------------------------------------------------- */ diff --git a/src/applications/drydock/storage/DrydockResource.php b/src/applications/drydock/storage/DrydockResource.php index 1d442620ed..7aad1064f4 100644 --- a/src/applications/drydock/storage/DrydockResource.php +++ b/src/applications/drydock/storage/DrydockResource.php @@ -9,7 +9,6 @@ final class DrydockResource extends DrydockDAO protected $status; protected $until; protected $type; - protected $name; protected $attributes = array(); protected $capabilities = array(); protected $ownerPHID; @@ -30,7 +29,6 @@ final class DrydockResource extends DrydockDAO 'capabilities' => self::SERIALIZATION_JSON, ), self::CONFIG_COLUMN_SCHEMA => array( - 'name' => 'text255', 'ownerPHID' => 'phid?', 'status' => 'text32', 'type' => 'text64', @@ -51,6 +49,10 @@ final class DrydockResource extends DrydockDAO return PhabricatorPHID::generateNewPHID(DrydockResourcePHIDType::TYPECONST); } + public function getResourceName() { + return $this->getBlueprint()->getResourceName($this); + } + public function getAttribute($key, $default = null) { return idx($this->attributes, $key, $default); } @@ -118,6 +120,16 @@ final class DrydockResource extends DrydockDAO 'Only new resources may be allocated.')); } + // We expect resources to have a pregenerated PHID, as they should have + // been created by a call to DrydockBlueprint->newResourceTemplate(). + if (!$this->getPHID()) { + throw new Exception( + pht( + 'Trying to allocate a resource with no generated PHID. Use "%s" to '. + 'create new resource templates.', + 'newResourceTemplate()')); + } + $expect_status = DrydockResourceStatus::STATUS_PENDING; $actual_status = $this->getStatus(); if ($actual_status != $expect_status) { @@ -135,12 +147,10 @@ final class DrydockResource extends DrydockDAO $new_status = DrydockResourceStatus::STATUS_PENDING; } - $phid = $this->generatePHID(); - $this->openTransaction(); try { try { - DrydockSlotLock::acquireLocks($phid, $this->slotLocks); + DrydockSlotLock::acquireLocks($this->getPHID(), $this->slotLocks); $this->slotLocks = array(); } catch (DrydockSlotLockException $ex) { $this->logEvent( @@ -152,7 +162,6 @@ final class DrydockResource extends DrydockDAO } $this - ->setPHID($phid) ->setStatus($new_status) ->save(); } catch (Exception $ex) { diff --git a/src/applications/drydock/view/DrydockLeaseListView.php b/src/applications/drydock/view/DrydockLeaseListView.php index d3507546ad..8c161b00ce 100644 --- a/src/applications/drydock/view/DrydockLeaseListView.php +++ b/src/applications/drydock/view/DrydockLeaseListView.php @@ -22,19 +22,10 @@ final class DrydockLeaseListView extends AphrontView { ->setHeader($lease->getLeaseName()) ->setHref('/drydock/lease/'.$lease->getID().'/'); - if ($lease->hasAttachedResource()) { - $resource = $lease->getResource(); - - $resource_href = '/drydock/resource/'.$resource->getID().'/'; - $resource_name = $resource->getName(); - + $resource_phid = $lease->getResourcePHID(); + if ($resource_phid) { $item->addAttribute( - phutil_tag( - 'a', - array( - 'href' => $resource_href, - ), - $resource_name)); + $viewer->renderHandle($resource_phid)); } $status = DrydockLeaseStatus::getNameForStatus($lease->getStatus()); diff --git a/src/applications/drydock/view/DrydockResourceListView.php b/src/applications/drydock/view/DrydockResourceListView.php index 9b7706c38b..739b464bdb 100644 --- a/src/applications/drydock/view/DrydockResourceListView.php +++ b/src/applications/drydock/view/DrydockResourceListView.php @@ -16,11 +16,12 @@ final class DrydockResourceListView extends AphrontView { $view = new PHUIObjectItemListView(); foreach ($resources as $resource) { - $name = pht('Resource %d', $resource->getID()).': '.$resource->getName(); + $id = $resource->getID(); $item = id(new PHUIObjectItemView()) - ->setHref('/drydock/resource/'.$resource->getID().'/') - ->setHeader($name); + ->setHref("/drydock/resource/{$id}/") + ->setObjectName(pht('Resource %d', $id)) + ->setHeader($resource->getResourceName()); $status = DrydockResourceStatus::getNameForStatus($resource->getStatus()); $item->addAttribute($status); diff --git a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php index f5ecc8389c..4ac6a9775c 100644 --- a/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php +++ b/src/applications/drydock/worker/DrydockLeaseUpdateWorker.php @@ -690,7 +690,7 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { ->setStatus(DrydockLeaseStatus::STATUS_BROKEN) ->save(); - $lease->scheduleDestruction(); + $lease->scheduleUpdate(); $lease->logEvent( DrydockLeaseActivationFailureLogType::LOGCONST, @@ -715,9 +715,11 @@ final class DrydockLeaseUpdateWorker extends DrydockWorker { */ private function destroyLease(DrydockLease $lease) { $resource = $lease->getResource(); - $blueprint = $resource->getBlueprint(); - $blueprint->destroyLease($resource, $lease); + if ($resource) { + $blueprint = $resource->getBlueprint(); + $blueprint->destroyLease($resource, $lease); + } DrydockSlotLock::releaseLocks($lease->getPHID());