Page MenuHomePhabricator

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

Authored by epriestley on Apr 23 2019, 6:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 12:24 AM
Unknown Object (File)
Tue, Mar 19, 3:25 PM
Unknown Object (File)
Tue, Mar 19, 3:24 PM
Unknown Object (File)
Tue, Mar 19, 3:24 PM
Unknown Object (File)
Fri, Mar 1, 5:17 AM
Unknown Object (File)
Feb 21 2024, 1:33 PM
Unknown Object (File)
Feb 15 2024, 7:08 AM
Unknown Object (File)
Feb 7 2024, 9:20 PM
Subscribers
None

Details

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.

Diff Detail

Repository
rP Phabricator
Branch
audit13
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22750
Build 31193: Run Core Tests
Build 31192: arc lint + arc unit

Event Timeline

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.

  • 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.