Simply how Differential drafts ignore Harbormaster autobuilds

Summary:
Ref T2543. When a revision is created, we check if any builds are waiting/failed, and submit it for review immediately if we aren't waiting for anything.

In doing this, we ignore builds with only autotargets, since these are client-side and failures from local `arc lint` / `arc unit` should not count (the user has already chosen to ignore/skip them).

The way we do this has some issues:

- Herald may have started builds, but they may still be PENDING and not have any targets yet. In this case, we'll see "no non-autotargets" and ignore the build, which is wrong.
- We have to load targets but don't really care about them, which is more work than we really need to do.
- And it's kind of complex, too.

Instead, just let `BuildQuery` filter out "autobuilds" (builds generated from autoplans) with a JOIN.

Test Plan: Ran `arc diff` with builds configured, got a clean "Draft" state instead of an incorrect promotion directly to "Needs Review".

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18721

epriestley 8 years ago 63e6b255 672247ef

+51 -29

2 changed files

expand all

src

applications

differential

storage

DifferentialRevision.php

harbormaster

query

HarbormasterBuildQuery.php

+2 -23

src/applications/differential/storage/DifferentialRevision.php

··· 721 721 return array(); 722 722 } 723 723 724 - $builds = id(new HarbormasterBuildQuery()) 724 + return id(new HarbormasterBuildQuery()) 725 725 ->setViewer($viewer) 726 726 ->withBuildablePHIDs(mpull($buildables, 'getPHID')) 727 + ->withAutobuilds(false) 727 728 ->withBuildStatuses( 728 729 array( 729 730 HarbormasterBuildStatus::STATUS_INACTIVE, ··· 735 736 HarbormasterBuildStatus::STATUS_PAUSED, 736 737 HarbormasterBuildStatus::STATUS_DEADLOCKED, 737 738 )) 738 - ->needBuildTargets(true) 739 739 ->execute(); 740 - if (!$builds) { 741 - return array(); 742 - } 743 - 744 - $active = array(); 745 - foreach ($builds as $key => $build) { 746 - foreach ($build->getBuildTargets() as $target) { 747 - if ($target->isAutotarget()) { 748 - // Ignore autotargets when looking for active of failed builds. If 749 - // local tests fail and you continue anyway, you don't need to 750 - // double-confirm them. 751 - continue; 752 - } 753 - 754 - // This build has at least one real target that's doing something. 755 - $active[$key] = $build; 756 - break; 757 - } 758 - } 759 - 760 - return $active; 761 740 } 762 741 763 742

+49 -6

src/applications/harbormaster/query/HarbormasterBuildQuery.php

··· 10 10 private $buildPlanPHIDs; 11 11 private $initiatorPHIDs; 12 12 private $needBuildTargets; 13 + private $autobuilds; 13 14 14 15 public function withIDs(array $ids) { 15 16 $this->ids = $ids; ··· 38 39 39 40 public function withInitiatorPHIDs(array $initiator_phids) { 40 41 $this->initiatorPHIDs = $initiator_phids; 42 + return $this; 43 + } 44 + 45 + public function withAutobuilds($with_autobuilds) { 46 + $this->autobuilds = $with_autobuilds; 41 47 return $this; 42 48 } 43 49 ··· 141 147 if ($this->ids !== null) { 142 148 $where[] = qsprintf( 143 149 $conn, 144 - 'id IN (%Ld)', 150 + 'b.id IN (%Ld)', 145 151 $this->ids); 146 152 } 147 153 148 154 if ($this->phids !== null) { 149 155 $where[] = qsprintf( 150 156 $conn, 151 - 'phid in (%Ls)', 157 + 'b.phid in (%Ls)', 152 158 $this->phids); 153 159 } 154 160 155 161 if ($this->buildStatuses !== null) { 156 162 $where[] = qsprintf( 157 163 $conn, 158 - 'buildStatus in (%Ls)', 164 + 'b.buildStatus in (%Ls)', 159 165 $this->buildStatuses); 160 166 } 161 167 162 168 if ($this->buildablePHIDs !== null) { 163 169 $where[] = qsprintf( 164 170 $conn, 165 - 'buildablePHID IN (%Ls)', 171 + 'b.buildablePHID IN (%Ls)', 166 172 $this->buildablePHIDs); 167 173 } 168 174 169 175 if ($this->buildPlanPHIDs !== null) { 170 176 $where[] = qsprintf( 171 177 $conn, 172 - 'buildPlanPHID IN (%Ls)', 178 + 'b.buildPlanPHID IN (%Ls)', 173 179 $this->buildPlanPHIDs); 174 180 } 175 181 176 182 if ($this->initiatorPHIDs !== null) { 177 183 $where[] = qsprintf( 178 184 $conn, 179 - 'initiatorPHID IN (%Ls)', 185 + 'b.initiatorPHID IN (%Ls)', 180 186 $this->initiatorPHIDs); 181 187 } 182 188 189 + if ($this->autobuilds !== null) { 190 + if ($this->autobuilds) { 191 + $where[] = qsprintf( 192 + $conn, 193 + 'p.planAutoKey IS NOT NULL'); 194 + } else { 195 + $where[] = qsprintf( 196 + $conn, 197 + 'p.planAutoKey IS NULL'); 198 + } 199 + } 200 + 183 201 return $where; 184 202 } 185 203 204 + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { 205 + $joins = parent::buildJoinClauseParts($conn); 206 + 207 + if ($this->shouldJoinPlanTable()) { 208 + $joins[] = qsprintf( 209 + $conn, 210 + 'JOIN %T p ON b.buildPlanPHID = p.phid', 211 + id(new HarbormasterBuildPlan())->getTableName()); 212 + } 213 + 214 + return $joins; 215 + } 216 + 217 + private function shouldJoinPlanTable() { 218 + if ($this->autobuilds !== null) { 219 + return true; 220 + } 221 + 222 + return false; 223 + } 224 + 186 225 public function getQueryApplicationClass() { 187 226 return 'PhabricatorHarbormasterApplication'; 227 + } 228 + 229 + protected function getPrimaryTableAlias() { 230 + return 'b'; 188 231 } 189 232 190 233 }

Configure Feed

Configure Feed