Differential D20470

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

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

Details

Reviewers

Maniphest Tasks

T13276: Fix several audit-adjacent issues, including races when multiple closing commits are discovered at the same time

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 22696
Build 31112: Run Core Tests
Build 31111: arc lint + arc unit

Event Timeline

epriestley created this revision.Apr 23 2019, 6:35 PM

Harbormaster completed remote builds in B22696: Diff 48824.Apr 23 2019, 6:37 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.

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.

Harbormaster completed remote builds in B22750: Diff 48877.May 1 2019, 5:00 PM

Closed by commit rPa15b0063df3d: Add "Revision has passing builds" Herald rules for commit content (pushes) and… (authored by epriestley). · Explain WhyMay 1 2019, 5:02 PM

This revision was automatically updated to reflect the committed changes.

epriestley added an edge: rPa15b0063df3d: Add "Revision has passing builds" Herald rules for commit content (pushes) and….

epriestley mentioned this in T10260: Harbormaster Herald Rules.Feb 19 2021, 4:31 AM

epriestley mentioned this in T13612: Support Herald rules for Harbormaster Builds.