Skip to content

Commit 7fa0d06

Browse files
author
epriestley
committed
Don't run Herald build rules when Differential revisions are updated automatically
Summary: Ref T2543. After D18731, Herald build rules run more often, but now incorrectly try to run builds when Diffusion closes a revision because a commit landed. Test Plan: Made some mundane updates locally; this is tricky to test comprehensively locally so I'm mostly planning to just push it to `secure`. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18745
1 parent 28cec2f commit 7fa0d06

File tree

2 files changed

+23
-3
lines changed

2 files changed

+23
-3
lines changed

src/applications/differential/editor/DifferentialTransactionEditor.php

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1204,16 +1204,32 @@ protected function buildHeraldAdapter(
12041204
// edited the title or changed subscribers), prevent "Run build plan"
12051205
// and other similar rules from acting yet, since the build results will
12061206
// not (or, at least, should not) change unless the actual source changes.
1207+
// We also don't run Differential builds if the update was caused by
1208+
// discovering a commit, as the expectation is that Diffusion builds take
1209+
// over once things land.
12071210
$has_update = false;
1211+
$has_commit = false;
1212+
12081213
$type_update = DifferentialTransaction::TYPE_UPDATE;
12091214
foreach ($xactions as $xaction) {
1210-
if ($xaction->getTransactionType() == $type_update) {
1215+
if ($xaction->getTransactionType() != $type_update) {
1216+
continue;
1217+
}
1218+
1219+
if ($xaction->getMetadataValue('isCommitUpdate')) {
1220+
$has_commit = true;
1221+
} else {
12111222
$has_update = true;
1212-
break;
12131223
}
1224+
1225+
break;
12141226
}
12151227

1216-
if (!$has_update) {
1228+
if ($has_commit) {
1229+
$adapter->setForbiddenAction(
1230+
HeraldBuildableState::STATECONST,
1231+
DifferentialHeraldStateReasons::REASON_LANDED);
1232+
} else if (!$has_update) {
12171233
$adapter->setForbiddenAction(
12181234
HeraldBuildableState::STATECONST,
12191235
DifferentialHeraldStateReasons::REASON_UNCHANGED);

src/applications/differential/herald/DifferentialHeraldStateReasons.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ final class DifferentialHeraldStateReasons
55

66
const REASON_DRAFT = 'differential.draft';
77
const REASON_UNCHANGED = 'differential.unchanged';
8+
const REASON_LANDED = 'differential.landed';
89

910
public function explainReason($reason) {
1011
$reasons = array(
@@ -14,6 +15,9 @@ public function explainReason($reason) {
1415
self::REASON_UNCHANGED => pht(
1516
'The update which triggered Herald did not update the diff for '.
1617
'this revision, so builds will not run.'),
18+
self::REASON_LANDED => pht(
19+
'The update which triggered Herald was an automatic update in '.
20+
'response to discovering a commit, so builds will not run.'),
1721
);
1822

1923
return idx($reasons, $reason);

0 commit comments

Comments
 (0)