From a15b0063df3d67507e5536337c998a063adaf870 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Apr 2019 11:20:29 -0700 Subject: [PATCH] Add "Revision has passing builds" Herald rules for commit content (pushes) and commits (discovery) Summary: Depends on D20469. Ref T13276. See PHI1159. See PHI953. See PHI901. Allow Herald to detect when "arc land" would (or did) warn users about failed or ongoing builds. This respects the "Warn on Landing" build plan behavior. To accomplish this: - When we close a revision, set a "wrong build state" flag if it lands in the wrong build state. - If the revision is closed when we hit Herald, look for the flag. - If not (common for push rules, can happen for commit rules if we race against the revision update worker), hit Harbormaster ourselves and check the current state. Test Plan: - Wrote a "Require Green" rule. - Ran it against various commits with various build states (good, not good). - Fiddled with "Warn on Landing" and saw the effect in rule evaluation. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13276 Differential Revision: https://secure.phabricator.com/D20470 --- src/__phutil_library_map__.php | 4 ++ .../DifferentialDiffExtractionEngine.php | 22 +++++++-- .../storage/DifferentialRevision.php | 1 + ...erentialRevisionWrongBuildsTransaction.php | 4 ++ .../DiffusionCommitWrongBuildsHeraldField.php | 49 +++++++++++++++++++ ...PreCommitContentWrongBuildsHeraldField.php | 49 +++++++++++++++++++ 6 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 src/applications/diffusion/herald/DiffusionCommitWrongBuildsHeraldField.php create mode 100644 src/applications/diffusion/herald/DiffusionPreCommitContentWrongBuildsHeraldField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d09a6ae8ef..e64d6c3ffc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -773,6 +773,7 @@ phutil_register_library_map(array( 'DiffusionCommitTimelineEngine' => 'applications/diffusion/engine/DiffusionCommitTimelineEngine.php', 'DiffusionCommitTransactionType' => 'applications/diffusion/xaction/DiffusionCommitTransactionType.php', 'DiffusionCommitVerifyTransaction' => 'applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php', + 'DiffusionCommitWrongBuildsHeraldField' => 'applications/diffusion/herald/DiffusionCommitWrongBuildsHeraldField.php', 'DiffusionCompareController' => 'applications/diffusion/controller/DiffusionCompareController.php', 'DiffusionConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionConduitAPIMethod.php', 'DiffusionController' => 'applications/diffusion/controller/DiffusionController.php', @@ -904,6 +905,7 @@ phutil_register_library_map(array( 'DiffusionPreCommitContentRevisionHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitContentRevisionHeraldField.php', 'DiffusionPreCommitContentRevisionReviewersHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitContentRevisionReviewersHeraldField.php', 'DiffusionPreCommitContentRevisionSubscribersHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitContentRevisionSubscribersHeraldField.php', + 'DiffusionPreCommitContentWrongBuildsHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitContentWrongBuildsHeraldField.php', 'DiffusionPreCommitRefChangeHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefChangeHeraldField.php', 'DiffusionPreCommitRefHeraldField' => 'applications/diffusion/herald/DiffusionPreCommitRefHeraldField.php', 'DiffusionPreCommitRefHeraldFieldGroup' => 'applications/diffusion/herald/DiffusionPreCommitRefHeraldFieldGroup.php', @@ -6438,6 +6440,7 @@ phutil_register_library_map(array( 'DiffusionCommitTimelineEngine' => 'PhabricatorTimelineEngine', 'DiffusionCommitTransactionType' => 'PhabricatorModularTransactionType', 'DiffusionCommitVerifyTransaction' => 'DiffusionCommitAuditTransaction', + 'DiffusionCommitWrongBuildsHeraldField' => 'DiffusionCommitHeraldField', 'DiffusionCompareController' => 'DiffusionController', 'DiffusionConduitAPIMethod' => 'ConduitAPIMethod', 'DiffusionController' => 'PhabricatorController', @@ -6572,6 +6575,7 @@ phutil_register_library_map(array( 'DiffusionPreCommitContentRevisionHeraldField' => 'DiffusionPreCommitContentHeraldField', 'DiffusionPreCommitContentRevisionReviewersHeraldField' => 'DiffusionPreCommitContentHeraldField', 'DiffusionPreCommitContentRevisionSubscribersHeraldField' => 'DiffusionPreCommitContentHeraldField', + 'DiffusionPreCommitContentWrongBuildsHeraldField' => 'DiffusionPreCommitContentHeraldField', 'DiffusionPreCommitRefChangeHeraldField' => 'DiffusionPreCommitRefHeraldField', 'DiffusionPreCommitRefHeraldField' => 'HeraldField', 'DiffusionPreCommitRefHeraldFieldGroup' => 'HeraldFieldGroup', diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php index feac4cce1c..6ffe333333 100644 --- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -281,7 +281,11 @@ final class DifferentialDiffExtractionEngine extends Phobject { ->setNewValue($revision->getModernRevisionStatus()); } - $concerning_builds = $this->loadConcerningBuilds($revision); + $concerning_builds = self::loadConcerningBuilds( + $this->getViewer(), + $revision, + $strict = false); + if ($concerning_builds) { $build_list = array(); foreach ($concerning_builds as $build) { @@ -328,8 +332,11 @@ final class DifferentialDiffExtractionEngine extends Phobject { $editor->applyTransactions($revision, $xactions); } - private function loadConcerningBuilds(DifferentialRevision $revision) { - $viewer = $this->getViewer(); + public static function loadConcerningBuilds( + PhabricatorUser $viewer, + DifferentialRevision $revision, + $strict) { + $diff = $revision->getActiveDiff(); $buildables = id(new HarbormasterBuildableQuery()) @@ -342,7 +349,6 @@ final class DifferentialDiffExtractionEngine extends Phobject { return array(); } - $land_key = HarbormasterBuildPlanBehavior::BEHAVIOR_LANDWARNING; $behavior = HarbormasterBuildPlanBehavior::getBehavior($land_key); @@ -391,7 +397,13 @@ final class DifferentialDiffExtractionEngine extends Phobject { // cases where the repository is observed and the fetch pipeline // stalls for a while. - continue; + // If we're in strict mode (from a pre-commit content hook), we do + // not ignore these, since we're doing an instantaneous check against + // the current state. + + if (!$strict) { + continue; + } } if ($build->isPassed()) { diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index b1d1c75ab4..5e70b52188 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -62,6 +62,7 @@ final class DifferentialRevision extends DifferentialDAO const PROPERTY_LINES_ADDED = 'lines.added'; const PROPERTY_LINES_REMOVED = 'lines.removed'; const PROPERTY_BUILDABLES = 'buildables'; + const PROPERTY_WRONG_BUILDS = 'wrong.builds'; public static function initializeNewRevision(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) diff --git a/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php b/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php index 260813b75b..8d96b600e8 100644 --- a/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionWrongBuildsTransaction.php @@ -13,6 +13,10 @@ final class DifferentialRevisionWrongBuildsTransaction return $value; } + public function applyInternalEffects($object, $value) { + $object->setProperty(DifferentialRevision::PROPERTY_WRONG_BUILDS, true); + } + public function getIcon() { return 'fa-exclamation'; } diff --git a/src/applications/diffusion/herald/DiffusionCommitWrongBuildsHeraldField.php b/src/applications/diffusion/herald/DiffusionCommitWrongBuildsHeraldField.php new file mode 100644 index 0000000000..ea44f06039 --- /dev/null +++ b/src/applications/diffusion/herald/DiffusionCommitWrongBuildsHeraldField.php @@ -0,0 +1,49 @@ +getAdapter(); + $viewer = $adapter->getViewer(); + + $revision = $adapter->loadDifferentialRevision(); + if (!$revision) { + return false; + } + + if ($revision->isPublished()) { + $wrong_builds = DifferentialRevision::PROPERTY_WRONG_BUILDS; + return !$revision->getProperty($wrong_builds, false); + } + + // Reload the revision to pick up active diffs. + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withPHIDs(array($revision->getPHID())) + ->needActiveDiffs(true) + ->executeOne(); + + $concerning = DifferentialDiffExtractionEngine::loadConcerningBuilds( + $viewer, + $revision, + $strict = false); + + return (bool)$concerning; + } + + protected function getHeraldFieldStandardType() { + return self::STANDARD_BOOL; + } + +} diff --git a/src/applications/diffusion/herald/DiffusionPreCommitContentWrongBuildsHeraldField.php b/src/applications/diffusion/herald/DiffusionPreCommitContentWrongBuildsHeraldField.php new file mode 100644 index 0000000000..99adf16923 --- /dev/null +++ b/src/applications/diffusion/herald/DiffusionPreCommitContentWrongBuildsHeraldField.php @@ -0,0 +1,49 @@ +getAdapter(); + $viewer = $adapter->getViewer(); + + $revision = $adapter->getRevision(); + if (!$revision) { + return false; + } + + if ($revision->isPublished()) { + $wrong_builds = DifferentialRevision::PROPERTY_WRONG_BUILDS; + return !$revision->getProperty($wrong_builds, false); + } + + // Reload the revision to pick up active diffs. + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withPHIDs(array($revision->getPHID())) + ->needActiveDiffs(true) + ->executeOne(); + + $concerning = DifferentialDiffExtractionEngine::loadConcerningBuilds( + $viewer, + $revision, + $strict = true); + + return (bool)$concerning; + } + + protected function getHeraldFieldStandardType() { + return self::STANDARD_BOOL; + } + +}