Use herald to trigger builds of revisions and commits.
ClosedPublic
Actions

Authored by hach-que on Nov 5 2013, 8:43 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T1049: Implement Harbormaster

Commits

Restricted Diffusion Commit
rP7ffea0463e42: Use herald to trigger builds of revisions and commits.

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()`?

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?

Yep, those are in HEAD now.

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