Page MenuHomePhabricator

Use herald to trigger builds of revisions and commits.
ClosedPublic

Authored by hach-que on Nov 5 2013, 8:43 AM.
Tags
None
Referenced Files
F12847087: D7501.diff
Fri, Mar 29, 1:51 AM
Unknown Object (File)
Sun, Mar 17, 11:35 PM
Unknown Object (File)
Sun, Mar 17, 11:35 PM
Unknown Object (File)
Sun, Mar 17, 11:35 PM
Unknown Object (File)
Sun, Mar 17, 11:35 PM
Unknown Object (File)
Sun, Mar 17, 10:01 PM
Unknown Object (File)
Sun, Mar 17, 9:30 PM
Unknown Object (File)
Wed, Mar 13, 6:41 PM

Details

Summary

Depends on D7500.

This seemed like a pretty good idea once I thought of it. Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions. This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).

Test Plan

Ran the usual daemons + the Harbormaster daemon. Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up. Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

There's some discussion of this elsewhere (on the original revision, maybe?) -- my current thinking is that we use rules on BuildPlans for the common cases (and only need to get the basics, pretty much), and then Herald for special cases. In particular, this rule is very common, but fairly awkward to represent in Herald:

Run continuously on HEAD of branch "master" of repository Z.

My guess is that we'll end up with something like the buildplan rules being: "run on every X in Y", "run continuously on X in Y", and maybe, like "run on X in Y every hour", and then everything else (all the weird author/content/etc/etc rules) will be in Herald.

Overall, this looks great to me. I think it may be worthwhile to shove the actual "execute herald to figure out builds" logic into an initial worker, as per earlier discussion, but I'm not really against leaving it in-process for now either. It would be nice to share the pathways somewhere common, though, since I think in the the long run we will want to shift it into the daemons more fully.

src/applications/differential/editor/DifferentialRevisionEditor.php
1120โ€“1135

Can we share more code here, with the Diffusion/commit use case? Depending on where the daemon stuff goes, I expect this to also queue the workers for the new builds.

src/applications/harbormaster/storage/HarbormasterBuildable.php
40

::initializeNewBuildable()?

hach-que updated this revision to Unknown Object (????).Nov 6 2013, 7:28 AM

Unify logic for applying plans to buildables.

Let me get you a patch to add diff PHIDs.

src/applications/differential/editor/DifferentialRevisionEditor.php
278

Oh, one issue here is that the buildable should be the diff, not the revision. Diffs currently don't have PHIDs. This is a straightforward fix, but we should probably hold integration until we can do it properly.

If we make a buildable out of a revision, the different builds may run against different code, because the revision may be updated as builds and build steps run.

src/applications/harbormaster/storage/HarbormasterBuildable.php
41

In the case of a revision, the buildablePHID should be the diff PHID (which does not exist yet) and the containerPHID should be the revision PHID.

In the case of a commit, the buildablePHID should be the commit PHID, and the containerPHID should be the repository PHID.

58

We should do all of this only if Harbormaster is installed.

sorry for raising priority while adding subscribes, was a mistake ;-)

@epriestley I'm assuming that I can modify this to use the diff PHIDs now?

hach-que updated this revision to Unknown Object (????).Nov 8 2013, 11:48 PM

Update to use diff PHIDs.

Two very tiny inlines that I'll tweak in the pull. The rest of this looks great to me.

src/applications/harbormaster/storage/HarbormasterBuildable.php
63

PhabricatorApplication::isClassInstalled() is slightly preferable, although there's no practical difference between the two.

src/applications/phid/PhabricatorPHIDConstants.php
22 โ†—(On Diff #17001)

Oh, these central PHID types are deprecated in favor of the ApplicationPHIDTypeThing classes. I'll just remove this, it isn't used anywhere (and never should be).