From a817aa6c71716d0603d1ae2ab3a1a59d7b77c87f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Apr 2018 09:18:36 -0700 Subject: [PATCH] Add an "Abort Older Builds" build step to Harbormaster Summary: Ref T13124. See PHI531. When a revision is updated, builds against the older diff tend to stop being relevant. Add an option to abort outstanding older builds automatically. At least for now, I'm adding this as a build step instead of some kind of special checkbox. An alternate implementation would be some kind of "Edit Options" action on plans with a checkbox like `[X] When this build starts, abort older builds.` I think adding it as a build step is a bit simpler, and likely to lead to greater consistency and flexibility down the road, make it easier to add options, etc., and since we don't really have any other current use cases for "a bunch of checkboxes". This might change eventually if we add a bunch of checkboxes for some other reason. The actual step activates //before// the build queues, so it doesn't need to wait in queue before it can actually act. T13088 discusses some plans here if this sticks. Test Plan: - Created a "Sleep for 120 seconds" build plan and triggered it with Herald. - Added an "Abort Older Builds" step. - Updated a revision several times in a row, saw older builds abort. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13124 Differential Revision: https://secure.phabricator.com/D19376 --- src/__phutil_library_map__.php | 4 + .../constants/HarbormasterBuildStatus.php | 13 ++ .../HarbormasterBuildActionController.php | 13 +- .../query/HarbormasterBuildStepQuery.php | 39 ++--- ...bortOlderBuildsBuildStepImplementation.php | 135 ++++++++++++++++++ .../HarbormasterBuildStepImplementation.php | 9 ++ .../HarbormasterControlBuildStepGroup.php | 20 +++ .../storage/HarbormasterBuildable.php | 9 ++ .../storage/build/HarbormasterBuild.php | 29 ++++ .../configuration/HarbormasterBuildStep.php | 13 ++ 10 files changed, 248 insertions(+), 36 deletions(-) create mode 100644 src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php create mode 100644 src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index da5674cbbd..9eda923457 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1263,6 +1263,7 @@ phutil_register_library_map(array( 'FundInitiativeTransactionType' => 'applications/fund/xaction/FundInitiativeTransactionType.php', 'FundInitiativeViewController' => 'applications/fund/controller/FundInitiativeViewController.php', 'FundSchemaSpec' => 'applications/fund/storage/FundSchemaSpec.php', + 'HarbormasterAbortOlderBuildsBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php', 'HarbormasterArcLintBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php', 'HarbormasterArcUnitBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcUnitBuildStepImplementation.php', 'HarbormasterArtifact' => 'applications/harbormaster/artifact/HarbormasterArtifact.php', @@ -1358,6 +1359,7 @@ phutil_register_library_map(array( 'HarbormasterCircleCIBuildableInterface' => 'applications/harbormaster/interface/HarbormasterCircleCIBuildableInterface.php', 'HarbormasterCircleCIHookController' => 'applications/harbormaster/controller/HarbormasterCircleCIHookController.php', 'HarbormasterConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterConduitAPIMethod.php', + 'HarbormasterControlBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php', 'HarbormasterController' => 'applications/harbormaster/controller/HarbormasterController.php', 'HarbormasterCreateArtifactConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterCreateArtifactConduitAPIMethod.php', 'HarbormasterCreatePlansCapability' => 'applications/harbormaster/capability/HarbormasterCreatePlansCapability.php', @@ -6636,6 +6638,7 @@ phutil_register_library_map(array( 'FundInitiativeTransactionType' => 'PhabricatorModularTransactionType', 'FundInitiativeViewController' => 'FundController', 'FundSchemaSpec' => 'PhabricatorConfigSchemaSpec', + 'HarbormasterAbortOlderBuildsBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterArcLintBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterArcUnitBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterArtifact' => 'Phobject', @@ -6771,6 +6774,7 @@ phutil_register_library_map(array( 'HarbormasterCircleCIBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterCircleCIHookController' => 'HarbormasterController', 'HarbormasterConduitAPIMethod' => 'ConduitAPIMethod', + 'HarbormasterControlBuildStepGroup' => 'HarbormasterBuildStepGroup', 'HarbormasterController' => 'PhabricatorController', 'HarbormasterCreateArtifactConduitAPIMethod' => 'HarbormasterConduitAPIMethod', 'HarbormasterCreatePlansCapability' => 'PhabricatorPolicyCapability', diff --git a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php index dc63b5fd95..7437e48f6c 100644 --- a/src/applications/harbormaster/constants/HarbormasterBuildStatus.php +++ b/src/applications/harbormaster/constants/HarbormasterBuildStatus.php @@ -98,6 +98,19 @@ final class HarbormasterBuildStatus extends Phobject { ); } + public static function getIncompleteStatusConstants() { + $map = self::getBuildStatusSpecMap(); + + $constants = array(); + foreach ($map as $constant => $spec) { + if (!$spec['isComplete']) { + $constants[] = $constant; + } + } + + return $constants; + } + public static function getCompletedStatusConstants() { return array( self::STATUS_PASSED, diff --git a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php index ddab677faf..843ffd4702 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildActionController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildActionController.php @@ -51,18 +51,7 @@ final class HarbormasterBuildActionController } if ($request->isDialogFormPost() && $can_issue) { - $editor = id(new HarbormasterBuildTransactionEditor()) - ->setActor($viewer) - ->setContentSourceFromRequest($request) - ->setContinueOnNoEffect(true) - ->setContinueOnMissingFields(true); - - $xaction = id(new HarbormasterBuildTransaction()) - ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND) - ->setNewValue($action); - - $editor->applyTransactions($build, array($xaction)); - + $build->sendMessage($viewer, $action); return id(new AphrontRedirectResponse())->setURI($return_uri); } diff --git a/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php b/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php index c14185116c..992dd4fad1 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildStepQuery.php @@ -22,48 +22,39 @@ final class HarbormasterBuildStepQuery return $this; } - protected function loadPage() { - $table = new HarbormasterBuildStep(); - $conn_r = $table->establishConnection('r'); - - $data = queryfx_all( - $conn_r, - 'SELECT * FROM %T %Q %Q %Q', - $table->getTableName(), - $this->buildWhereClause($conn_r), - $this->buildOrderClause($conn_r), - $this->buildLimitClause($conn_r)); - - return $table->loadAllFromArray($data); + public function newResultObject() { + return new HarbormasterBuildStep(); } - protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { - $where = array(); + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } - if ($this->ids) { + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'id IN (%Ld)', $this->ids); } - if ($this->phids) { + if ($this->phids !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'phid in (%Ls)', $this->phids); } - if ($this->buildPlanPHIDs) { + if ($this->buildPlanPHIDs !== null) { $where[] = qsprintf( - $conn_r, + $conn, 'buildPlanPHID in (%Ls)', $this->buildPlanPHIDs); } - $where[] = $this->buildPagingClause($conn_r); - - return $this->formatWhereClause($where); + return $where; } protected function willFilterPage(array $page) { diff --git a/src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php new file mode 100644 index 0000000000..d6c2a46734 --- /dev/null +++ b/src/applications/harbormaster/step/HarbormasterAbortOlderBuildsBuildStepImplementation.php @@ -0,0 +1,135 @@ +getIsManualBuildable()) { + // Don't abort anything if this is a manual buildable. + return; + } + + $object_phid = $buildable->getBuildablePHID(); + if (phid_get_type($object_phid) !== DifferentialDiffPHIDType::TYPECONST) { + // If this buildable isn't building a diff, bail out. For example, we + // might be building a commit. In this case, this step has no effect. + return; + } + + $diff = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withPHIDs(array($object_phid)) + ->executeOne(); + if (!$diff) { + return; + } + + $revision_id = $diff->getRevisionID(); + + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withIDs(array($revision_id)) + ->executeOne(); + if (!$revision) { + return; + } + + $active_phid = $revision->getActiveDiffPHID(); + if ($active_phid !== $object_phid) { + // If we aren't building the active diff, bail out. + return; + } + + $diffs = id(new DifferentialDiffQuery()) + ->setViewer($viewer) + ->withRevisionIDs(array($revision_id)) + ->execute(); + $abort_diff_phids = array(); + foreach ($diffs as $diff) { + if ($diff->getPHID() !== $active_phid) { + $abort_diff_phids[] = $diff->getPHID(); + } + } + + if (!$abort_diff_phids) { + return; + } + + // We're fetching buildables even if they have "passed" or "failed" + // because they may still have ongoing builds. At the time of writing + // only "failed" buildables may still be ongoing, but it seems likely that + // "passed" buildables may be ongoing in the future. + + $abort_buildables = id(new HarbormasterBuildableQuery()) + ->setViewer($viewer) + ->withBuildablePHIDs($abort_diff_phids) + ->withManualBuildables(false) + ->execute(); + if (!$abort_buildables) { + return; + } + + $statuses = HarbormasterBuildStatus::getIncompleteStatusConstants(); + + $abort_builds = id(new HarbormasterBuildQuery()) + ->setViewer($viewer) + ->withBuildablePHIDs(mpull($abort_buildables, 'getPHID')) + ->withBuildPlanPHIDs(array($plan->getPHID())) + ->withBuildStatuses($statuses) + ->execute(); + if (!$abort_builds) { + return; + } + + foreach ($abort_builds as $abort_build) { + $abort_build->sendMessage( + $viewer, + HarbormasterBuildCommand::COMMAND_ABORT); + } + } + + public function execute( + HarbormasterBuild $build, + HarbormasterBuildTarget $build_target) { + return; + } + +} diff --git a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php index 47af992630..ccf97451b7 100644 --- a/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php +++ b/src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php @@ -308,6 +308,15 @@ abstract class HarbormasterBuildStepImplementation extends Phobject { 'enabled in configuration.')); } + public function willStartBuild( + PhabricatorUser $viewer, + HarbormasterBuildable $buildable, + HarbormasterBuild $build, + HarbormasterBuildPlan $plan, + HarbormasterBuildStep $step) { + return; + } + /* -( Automatic Targets )-------------------------------------------------- */ diff --git a/src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php b/src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php new file mode 100644 index 0000000000..f49fd0b127 --- /dev/null +++ b/src/applications/harbormaster/stepgroup/HarbormasterControlBuildStepGroup.php @@ -0,0 +1,20 @@ +save(); + $steps = id(new HarbormasterBuildStepQuery()) + ->setViewer($viewer) + ->withBuildPlanPHIDs(array($plan->getPHID())) + ->execute(); + + foreach ($steps as $step) { + $step->willStartBuild($viewer, $this, $build, $plan); + } + PhabricatorWorker::scheduleTask( 'HarbormasterBuildWorker', array( diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index ae0f6b13f3..6acbac6468 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -356,6 +356,35 @@ final class HarbormasterBuild extends HarbormasterDAO } } + public function sendMessage(PhabricatorUser $viewer, $command) { + // TODO: This should not be an editor transaction, but there are plans to + // merge BuildCommand into BuildMessage which should moot this. As this + // exists today, it can race against BuildEngine. + + // This is a bogus content source, but this whole flow should be obsolete + // soon. + $content_source = PhabricatorContentSource::newForSource( + PhabricatorConsoleContentSource::SOURCECONST); + + $editor = id(new HarbormasterBuildTransactionEditor()) + ->setActor($viewer) + ->setContentSource($content_source) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true); + + $viewer_phid = $viewer->getPHID(); + if (!$viewer_phid) { + $acting_phid = id(new PhabricatorHarbormasterApplication())->getPHID(); + $editor->setActingAsPHID($acting_phid); + } + + $xaction = id(new HarbormasterBuildTransaction()) + ->setTransactionType(HarbormasterBuildTransaction::TYPE_COMMAND) + ->setNewValue($command); + + $editor->applyTransactions($this, array($xaction)); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php index b29958882c..54c069e97d 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildStep.php @@ -100,6 +100,19 @@ final class HarbormasterBuildStep extends HarbormasterDAO return ($this->getStepAutoKey() !== null); } + public function willStartBuild( + PhabricatorUser $viewer, + HarbormasterBuildable $buildable, + HarbormasterBuild $build, + HarbormasterBuildPlan $plan) { + return $this->getStepImplementation()->willStartBuild( + $viewer, + $buildable, + $build, + $plan, + $this); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */