Thanks, there's an example here of the API:
https://buildkite.com/docs/rest-api/builds#create-a-build

Harbormaster can pass the build target in an environment variable.

Buildkite has a post-command hook script that is called after each command, and could report a failure like:

#!/bin/bash
if [[ "$BUILDKITE_COMMAND_EXIT_STATUS" != "0" ]]; then
  curl "$CONDUIT_API/harbormaster.sendmessage" \
    -d api.token="$PHAB_TOKEN" \
    -d type=fail \
    -d buildTargetPHID="$PHAB_BUILD_TARGET_ID"
fi

The same curl command could be used to send a "pass" result as the final step of the Buildkite pipeline if the others all succeeded.

This was fairly straightforward since the Buildkite API and Harbormaster appear to have very similar models of the world. D17270 adds at least basic support. It uses webhooks, so you don't need to configure any post-commands. There may be some bugs or rough edges, so let us know what you run into if once you have a chance to try it out.

It will deploy to the Phacility cluster during the regular release in about four days, early on Saturday, Feb 4.

Here's where the new step lives:

Here's what configuration looks like:

Notes:

• Buildkite requires that we submit a branch, and does not appear to accept empty values (at least, null was rejected). In the general case, Harbormaster supports building things which may not belong to any branch (for example, commits which are only ancestors of a tag or ref) where we can not possibly provide a legitimate value. As currently written, we always submit "master". We could make more of an effort to guess some reasonable branch name in some cases. I'm not sure if this is important; it only appears to impact pipeline filtering rules.
• When a build is cancelled in Harbormaster, we do not currently cancel it in Buildkite. The Buildkite API supports this but it's presumably not very important and we're missing a bit of infrastructure on our side since this is the first external build system we've integrated which supports canceling builds via API.

Howdy! Keith from Buildkite here. We actually started a Phabricator integration a while ago, but got stuck on the Phabriactor side of things. A ton of our customers use it, so we're super pumped to see it getting some traction.

The branch thing that you mentioned @epriestley is a bit of a pain for sure. Most of our processes require a branch. For a while we supported no branch, but it ended up asking more questions than it solved. However as you've seen, you can kinda send any value for "branch" and we'll try and handle it as best we can (but as you saw, the branch filtering rules wouldn't work...)

I'm super keen to see this through, so please let me know if you need anything on our end, and I'll hopefully see what we can do / ship it ASAP! We can implement the API call on our end to send pass/fail if that makes it easier?

We can also give you a complimentary Buildkite account for integration testing if that helps - just shoot me an email keith@buildkite.com and I'll sort it out.

😺 😺 😺

🐈

Thanks for the quick work on this!

I'm using Phacility so I haven't tried this extension itself out yet, but I did run into an issue testing out the Buildkite pipeline manually with the same parameters. There's an existing issue in Buildkite checking out tags provided as the commit parameter: https://github.com/buildkite/agent/issues/338

It works for the first build where the agent clones the repo, but on subsequent fetches it fails to run checkout on the tag name since it isn't added as a local ref. A quick workaround on the Harbormaster side is to send the request as {"branch": "phabricator/diff/1234", "commit": "HEAD"} instead (mentioned in the comments on the other issue).

Ah! Using "phabricator/diff/1234" as the branch name would be pretty cool - we'd _might_ be able to do some special handling of that branch name pattern in our UI to make the whole experience a bit nicer. We could probably also extract the diff number out and have a nice link to the diff on the Phabricator end.

If you go to your organisation settings:

https://buildkite.com/organizations/[slug]/settings

We could add a section there for your Phabricator server so you could configure things like server URL (and some other stuff maybe?) so we can generate links.

Let me poke at things and see how far I can get, there's a lot of future design space here that I want to leave open.

No problems, just lemme know what you need to make it easier and I'll see what I can do :) Also if you have any BK questions feel free to reach out! 🐈

Things seem to work fine as long as we pass the full path to the tag (we currently don't, since I'm reusing some CircleCI code and CircleCI, a lovely an inventive build system with many creative and original ideas, did not work with a full ref).

That is, if we pass:

commit = refs/tags/diff/123
branch = master

...we seem to get the desired result with no additional work.

This leaves branch free to contain the branched-from-branch in the future (which would probably be more useful for filtering) and doesn't back us into a corner when building commits directly. I'll update the adapter to use staging refs instead of CircleCI refs.

Hrrm, I may be incorrect about that and have accidentally forced the tag to fetch in some other way.

I think what I actually did was this:

• Hand Buildkite an invalid refspec, which triggers a full refetch of all branch heads and tags.
• Then do a refs/tags/ build, which had no special power.

So it's not clear that there's a fix from our side. Ideally, we'd like to be able to hand Buildkite an arbitrary commit, branch, tag, or ref and have fetch + build work even if the target is not yet present in the repository.

I'll check in with Sam at Buildkite who wrote most of the tag/git handling gear in the Buildkite Agent. In the meantime, you can override our "checkout" process with a hook: https://buildkite.com/docs/agent/hooks to get it working how'd you prefer it to work. Once you've done that, we can figure out what we need to change on our end to merge in what ever bits you want.

We don't have any examples of checkout hooks, but you can read the built in code path here that does the checkout: https://github.com/buildkite/agent/blob/master/agent/bootstrap.go#L948-L1051

This may be a bad approach, but one possible not-great-but-not-horribly-disruptive change might be this:

If the "commit" begins "refs/", replace this command:

git fetch -v origin refs/adhoc/pasta

...with this one:

git fetch -v origin refs/adhoc/pasta:refs/adhoc/pasta

That is, just repeat the argument, to fetch the remote ref into a local ref of the same name. I believe that will fix things in these cases (explicit build of a named remote ref) without creating a lot of peril around suddenly doing huge fetches of everything in refs/pull from everyone on GitHub or breaking older versions of Git. Maybe?

We can actually make this fetch happen on our side by passing x:x as the commit, but then the git checkout fails.

I guess there's also some amount of overloading here for "branch" and "commit", which is understandable since building tags/refs is rare.

The workaround above, where we pass:

commit = HEAD
branch = refs/adhoc/pasta2

Seems to work for branches, tags, and arbitrary refs.

One downside is that branch filtering won't work if we want to pursue it in the future (e.g., do something different for changes which the author is proposing for "develop", vs changes which the author is proposing for "master").

Another downside is that building an arbitrary commit requires us to find a branch, tag or ref which contains it, but this isn't the end of the world.

Upshot: I'll do the thing that works for now. If we have tension down the road for the "code reviews intended for master" vs "code reviews intended for develop" use case, we can deal with it when it actually arises. It may be completely hypothetical. Users that did care could conceivably lift HARBORMASTER_BUILD_TARGET_PHID out of the environment and use Conduit to figure out where the review was targeted.

When I submit this:

commit = HEAD
branch = refs/tags/phabricator/diff/689

...I get HTTP 422 back with this response body:

{
  "message": "Tags have been disabled for this project"
}

I don't think I disabled tags anywhere? And these parameters build properly from the web UI. I'll double-check what I'm passing but I'm not entirely sure what's going on here.

Check out your pipeline settings, if you "ctrl-f" for the word "tag" - is there an option somewhere?

Here's a more granular transcript which suggests I'm not doing something goofy:

epriestley@orbital ~/dev/phabricator $ curl \
>   -X POST \
>   -H "Authorization: Bearer $BUILDKITE_TOKEN" \
>   "https://api.buildkite.com/v2/organizations/phacility/pipelines/run-git-show/builds" \
>   -d '{
>      "meta_data" : {
>         "buildTargetPHID" : "PHID-HMBT-wcooko2st2scfh7ke2ql"
>      },
>      "branch" : "refs/tags/phabricator/diff/689",
>      "commit" : "HEAD",
>      "env" : {
>         "HARBORMASTER_BUILD_TARGET_PHID" : "PHID-HMBT-wcooko2st2scfh7ke2ql"
>      },
>      "message" : "Harbormaster Build 3369 (\"Run Buildkite\") for B3089"
>   }'

Response:

{
  "message": "Tags have been disabled for this project"
}

Only hit for "tag" is in this paragraph, which has no limits set (unless "empty text field" means "no tags"):

Build also works fine if I submit it unmodified but omit the refs/tags/... prefix.

Oh! I think you found a bug. If you go to "GitHub Settings" on that page, enable "Trigger builds after pushing code", then enable "Build tags", then save. Then switch back to "Disable GitHub triggered builds", then save, it should hopefully start working for you.

Sorry about that! I'll raise an issue internally for that one. Good find!

Using {"branch": "phabricator/diff/689", "commit": "HEAD", "message": "..."} should roughly translate into a git fetch phabricator/diff/689 && git checkout FETCH_HEAD in the Buildkite agent bootstrap script. Git is smart enough to resolve that to the tag. The branch name is really only used in Buildkite for branch filters, on the agent Git does a headless checkout. And a commit value of HEAD means we'll ask Git for the commit sha and back-populate that onto the build. 👍

Another more flexible but more complicated option would be to specific a refspec using $BUILDKITE_REFSPEC. So {"branch": "whatever", "commit": "FETCH_HEAD", message: "...", "env": {"BUILDKITE_REFSPEC": "+refs/tags/blah/thing/whatever:some-local-ref"}} translates into git fetch +refs/tags/blah/thing/whatever:some-local-ref && git checkout FETCH_HEAD. Or use some-local-ref for commit. Or something. (This is also in the bootstrap script linked above.)

• After D17282, we do branch=phabricator/diff/689, commit=HEAD when building revisions.
• After D17282, we do branch=<one arbitrary containing branch>, commit=<commit hash> when building commits.
• The "GitHub Settings" workaround above resolved the "tags disabled" issue, as suggested.

Future work:

• We currently refuse to build commits which are not ancestors of any branch head with Buildkite. This is a limitation on our side: there's just no easy way for us to run git for-each-ref --contains or similar to find a tag or ref at the moment. I suspect it will be a long time before anyone hits this in practice and the error message should be reasonably clear.
• We currently pass phabricator/diff/689, not refs/tags/phabricator/diff/689, since this works around the settings interaction and saves us from writing instructions around making sure the GitHub tag build setting is adjusted correctly. I suspect no real user will ever encounter a situation where this name is ambiguous (i.e., users do not normally push branches with names like phabricator/diff/123), but we could add the prefix any time after the bug is cleaned up.
• Since we pass an arbitrary containing branch when building commits, branch filtering may not be as useful as it could be: we may pass epriestley-tmp-bugfix3 when you would prefer we pass master, even though both branches contain the commit. A small improvement we could consider here is to prefer to choose the repository default branch, then any tracked branch, then any branch. I'm unsure if this will matter much in practice. I would resist adding any kind of formal tagging to Phabricator repositories for this use case, although we could have an optional field like "When passing a branch to Buildkite, prefer these branches in order: master, develop, release, stable" on the build step.
• Since we pass phabricator/diff/698 as the branch when building revisions, branch filtering won't be useful for those builds. This may not matter. In theory, it looks like we could pass the "branched from master" branch and pass BUILDKITE_REFSPEC in the env instead. Per above, build scripts can also lift HARBORMATER_BUILD_TARGET_PHID out of env and then use Conduit to pull down related data from Phabricator. Until T3462 / T10607 / etc our own tracking of "branched from" branches isn't terribly great anyway so I'm not sure we could consistently pass a convincing value even if this field was purely a descriptive label on the Buildkite side.
• In the Buildkite UI, builds are grouped by branch and sorted alphabetically (I think?). If users rely on this UI, and particularly if they have branches which are alphabetically after "phabricator/" (perhaps "release"), they may find the web interface heavily gummed up by the many unique builds we'll submit (one tag per revision). I'm not sure what the best remedy is for this, or if it will be a problem in practice.
• Internally, CircleCI and Buildkite integrations rely on having objects implement bespoke CircleCI and Buildkite interfaces. At some point in the future, I'd like to move away from this. We don't have an established pattern for clean modularity here because the problem space can extend in two dimensions: third-party code can add new types of build systems, but can also add new types of buildable objects. There isn't a terribly clean way for a build system added by one third party to handle a buildable added by another. This does not create material problems today, but a future version of Phabricator which supports 20 external build systems should not require first-party objects to implement 20 interfaces. Offhand, the best approach I see here is to try to implement a more formal intermediate representation, make all buildables emit it, and then let all build systems try to consume it. This probably isn't perfect but maybe it's close enough.

This sounds great, thanks. We only plan to use it for testing Diffs at this time, so the other branch limitations aren't a concern right now.

How frequently do updates like this get deployed to Phacility?

Usually every Saturday we cut stable, unless we're writing a lot of bugs that particular week.

This is now available in the Phacility cluster. It appeared to work correctly on a test instance:

Building commits directly also seems to work.

Let us know if you run into trouble or issues.

Any thoughts on deeper integration with Phabricator? Namely:

• Reporting back which tests passed/failed
• Reporting back code coverage (I guess right now it's part of reporting tests via the API, but I'd be happy even if it's merged under 1 dummy test case)

This thread actually brought buildkite to my attention, and it looks like a nice service. I'd probably write my own script to parse the xml file + post it to phabricator, but ideally we get some official support for it...

If you configure your project so that arc unit runs your tests and then make your Buildkite build run arc unit --everything --target $HARBORMASTER_TARGET_PHID --conduit-token ..., you should get full reporting support. Then arc unit on its own will also work, arc diff will get the same integration, etc.

Our business case for integrations like this is almost entirely about lowering the barrier to adoption ("we can use our existing build system today...") to get installs onto Phabricator/Phacility, then lure them to Harbormaster/Drydock instead in order to sink our claws deeper into their process ("...if we switch to Drydock, we get all this extra stuff for free"). So we're much more likely to spend time improving Harbormaster/Drydock than improving third-party integrations once an integration is no longer a barrier to adoption, since having a really great Harbormaster UX is much more valuable to us than having a really great integration with a third-party system that has a really great build UX.

Today, the Harbormaster/Drydock UX and setup experience are several steps behind systems like Buildkite and CircleCI, but I think most of these issues are skin deep. You can do bring-your-own-hardware builds (like Buildkite) with Drydock today (we do here, and they're supported in the Phacility cluster too -- T10246).