Page MenuHomePhabricator

Add "Revision has passing builds" Herald rules for commit content (pushes) and commits (discovery)

Authored by epriestley on Apr 23 2019, 6:35 PM.



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.

Diff Detail

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

Event Timeline

epriestley created this revision.Apr 23 2019, 6:35 PM
epriestley requested review of this revision.Apr 23 2019, 6:37 PM
amckinley accepted this revision.May 1 2019, 2:28 PM
This revision is now accepted and ready to land.May 1 2019, 2:28 PM

After some thought, I'm going to adjust this rule slightly into "Revision has ongoing or failed builds", since I think that's probably more consistent with user expectation and less likely to trip people up.

As written, "Revision has passing builds" is sort of misleading, since it really means "Revision exists and does not have ongoing or failed builds", which is a weird implicit negation.

epriestley updated this revision to Diff 48877.May 1 2019, 4:58 PM
  • Flip the rules into "Revision has build warning", which is an attempt to express "Revision has failed or ongoing builds which 'arc land' would raise a warning about" more concisely.