Page MenuHomePhabricator

Add behaviors to Build Plans: hold drafts, affect buildables, warn on landing, restartable, runnable
ClosedPublic

Authored by epriestley on Tue, Feb 26, 5:44 PM.

Details

Summary

Depends on D20219. Ref T13258. Ref T11415. Installs sometimes have long-running builds or unimportant builds which they may not want to hold up drafts, affect buildable status, or warn during arc land.

Some builds have side effects (like deployment or merging) and are not idempotent. They can cause problems if restarted.

In other cases, builds are isolated and idempotent and generally safe, and it's okay for marketing interns to restart them.

To address these cases, add "Behaviors" to Build Plans:

  • Hold Drafts: Controls how the build affects revision promotion from "Draft".
  • Warn on Land: Controls the "arc land" warning.
  • Affects Buildable: Controls whether we care about this build when figuring out if a buildable passed or failed overall.
  • Restartable: Controls whether this build may restart or not.
  • Runnable: Allows you to weaken the requirements to run the build if you're confident it's safe to run it on arbitrary old versions of things.
NOTE: This only implements UI, none of these options actually do anything yet.
Test Plan

Mostly poked around the UI. I'll actually implement these behaviors next, and vet them more thoroughly.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Tue, Feb 26, 5:44 PM
Owners added a subscriber: Restricted Owners Package.Tue, Feb 26, 5:44 PM
epriestley edited the summary of this revision. (Show Details)Tue, Feb 26, 5:46 PM
epriestley requested review of this revision.Tue, Feb 26, 5:46 PM
amckinley accepted this revision.Tue, Mar 5, 7:29 PM

Just a bunch of language nitpicks.

src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php
127–129

All three of these are kinda hard to parse with the "do not" combined with "and/but". For all of these, replace "will be sent for review" with "will promote from 'Draft'" if you think that's clearer.

"Revisions will be sent for review only after the build completes successfully."

136–137

"Revisions will be sent for review after the build completes, even if it failed."

144–145

"Revisions will be sent for review immediately, without waiting for the build to complete."

192–193

"The buildable passes after the build completes successfully."

200–201

"The buildable passes after the build completes, even if the build failed."

208–209

"The buildable passes immediately without waiting for the build to complete."

287

"even if builds have failed or are still in progress."

This revision is now accepted and ready to land.Tue, Mar 5, 7:29 PM
epriestley added inline comments.Wed, Mar 6, 1:32 PM
src/applications/harbormaster/plan/HarbormasterBuildPlanBehavior.php
144–145

I worry "immediately" may be unclear because a revision can be waiting on multiple builds, so even if you set this build to "Hold Drafts: Never", you may not get immediate promotions.

To a lesser degree, the other language is trying to focus on this too: these settings give you "less stuff blocking promotion", not necessarily "faster / immediate promotion", since the overall behavior depends on what other builds are running.

That said, I also don't like the "promote from draft" terminology very much either because I don't think we use it elsewhere in the UI very often.

208–209

(As above, no setting here can make the Buildable pass "immediate" if there are other builds running.)

epriestley updated this revision to Diff 48332.Wed, Mar 6, 1:34 PM
  • Wordsmithing.
  • Use "sent for review" over "promoted from draft".
  • Use "wait for the build" over "wait for the build to complete to pass".
  • Try to make "X, and/but Y" structure a little more consistent.
  • Improve clarity of "land" condition.
This revision was automatically updated to reflect the committed changes.