Page MenuHomePhabricator

Allow Harbormaster build plans to run on branch push instead of per-commit
Closed, WontfixPublic

Description

I'm sure there was a task for this somewhere, but searching for related terms didn't help.

Anyway, right now I have a build plan that runs on every commit for a certain repository I have. This build plan not only builds the code, but on success, pushes out the new version to other repositories (which use the tool). I have "Wait for Previous Commits" build step in there to ensure things happen in the right order.

However, this means I can only ever have "master" as a branch in that repository; if I was to push to another branch, it would build and publish the commits on that branch. What I really want to do is build all commits, but only publish the HEAD of master. Unfortunately right now Harbormaster doesn't have any conditional logic for steps (you can only really do conditional logic as part of the Run Command in the command line), and there's no way to tell Harbormaster to run a build plan in response to a branch push (even on the push hooks).

There's also no way of saying in Herald "this commit is a descendant of this specific branch", but that rule doesn't make a lot of sense anyway because that won't get refired when the same commit is pushed to a different branch (because it's already been imported).

This issue actively discourages developers (me in this particular case) from pushing their changes to a branch where people can review them. Because this project is on GitHub, Differential is not a good fit for getting people to review or download experimental or unverified versions of the software.

Event Timeline

hach-que created this task.Nov 21 2015, 1:53 AM
hach-que updated the task description. (Show Details)
hach-que raised the priority of this task from to Needs Triage.
hach-que added a project: Harbormaster.
hach-que added subscribers: hach-que, epriestley.
avivey added a subscriber: avivey.Nov 21 2015, 2:09 AM
epriestley triaged this task as Wishlist priority.Nov 21 2015, 4:00 PM

It sounds like this use case (using Harbormaster to build a GitHub project, and not using Differential) is far afield from the use cases I consider core. There are a large number of existing tools (Travis, CircleCI) which are purpose-built to address this use case.

epriestley moved this task from Backlog to Far Future on the Harbormaster board.Nov 21 2015, 4:21 PM

Neither of those tools let me use my own hardware, which is necessary for my use case. In actuality, GitHub has nothing to do with it, it just happens to be the authoritative repository instead of Phabricator. I still have the same problem regardless of whether or not I make Phabricator the authoritative repository.

What makes it not fit with the per-commit model is what needs to happen on build, and this is because the build contains publishing steps.

Also FYI the way we are working around this issue now is whenever someone wants to push to a non-master branch, they go into herald and disable the herald rule, push to their branch, then turn the rule back on again. When they want to push to master, they amend the commit to update the timestamps (which changes the hash), and then they push to master (which imports as a different commit).

Put simply though, this workflow sucks.

Why can't your publish step just check if the commit is on master (or even if it's HEAD of master) and no-op if it isn't?

Because the publish step isn't run command. The publishing step is actually 8 other "Run Build plan" steps; these in turn have run command, but there's about 10 run command steps (these build plans clone a repository, update the binary artifact in them, then commit and push).

Because Harbormaster doesn't have conditionals or branching, I'd have to prefix every command with that check, which isn't ideal at all.

The way I'm getting around this now is I added a Herald rule "Commit message does not contain [experimental]". Then when pushing to non-master branches we add "[experimental]" to the commit message. When we want to put it on master, we squash and amend to remove the text and then push.

How does this relate to T5879?

Oh that was the task I was looking for before making this one.

clloos added a subscriber: clloos.Dec 14 2015, 11:50 AM

I would also really love this feature. In jenkins, we need to create jobs for new branches (e. g. release branch > release job). Now we are using additional commit hooks, but I would like to integrate this into Phabricator. Is there any technical issue why this is not implemented or is it JUST the time issue?

eadler added a project: Restricted Project.Aug 5 2016, 4:44 PM
urzds added a subscriber: urzds.Jul 12 2017, 11:15 AM
pasik added a subscriber: pasik.Oct 16 2017, 7:42 PM

I would also be interested in this feature; here's our use-case if you possibly find it more persuasive than OP's:

We have several hundred (git) projects hosted in a local Phabricator install using Diffusion and all the other trappings of Phabricator. We use Harbormaster to manage test runs executed on a Jenkins cluster (this, I think, is fairly common). Harbormaster is integrated to automatically run on Differential updates and that works great. Harbormaster is also configured to run on pushes to master (using a Herald rule that looks like the one at the end of this post) so that we actually get coverage of the squashed/merged final SHA. We do not want to run our test suite for every commit in every branch because a lot of people use branches for temporary work that has no expectation of passing tests. Some teams use merge-commit-based workflows instead of squash-based workflows; arcanist supports both so this seems like a legitimate thing to do. However, when a repository that is using a merge-based workflow does a land, every commit in their branch appears in master and get separately sent to Harbormaster, which clogs up Jenkins. This sometimes results in hundreds of superfluous builds at a time. All I'd really like is some way in Herald to be able to invoke harbormaster only for the tip commit (the one pointed to by the ref change) instead of for every new commit that appears, and I think that the subject of this ticket is the way to do that.

As @epriestley points out, we could technically change our Harbormaster Build Plan to use Drydock to lease a working copy and then check out the SHA it was invoked with and then check whether it's actually currently the target of master and then no-op if it isn't, but that's a large amount of infrastructure (and a ton of time for large repositories). We could also modify Jenkins to no-op if the thing it's building isn't currently the tip of master. Neither of these seems quite ideal. We could also just stop using Herald to trigger Harbormaster and drop a bash script into hooks/pre-receive-phabricator.d/ that creates a build.

It looks like implementing this would "just" be a matter of implementing HarbormasterBuildableAdapterInterface on HeraldPreCommitRefAdapter, yes? I mean, I guess technically this would be better-suited to a post-receieve hook than a pre-receive hook, but I don't think there is any support for a post-receieve hook in Diffusion/Herald today.

Our current Herald rule:

Having looked at the code some more, I see when the HeraldPreCommitRefAdapter runs, there is no parsed commit object yet, so there's no way to invoke harbormaster (what would the container PHID be?). This kind of makes sense.

This might be more straightforward for me to implement as an external tool if HeraldPreCommitRefAdapter had a webhook-trigger action; would you be opposed to changing "supportsWebhooks" in HeraldPreCommitAdapter to true? Then I could use the pre-commit adapter to send a webhook saying "SHA xyz is a ref-change on master, if you see a commit for xyz build it otherwise don't" to something, shove that in a database somewhere, and then read it out later.

Unless there are any secret plans in the roadmap to implement native support for this, behavior that is. :-)

tiguchi added a subscriber: tiguchi.
aubort added a subscriber: aubort.Apr 5 2018, 9:09 PM
pasik added a comment.Nov 26 2018, 2:37 PM

@jbrownEP Hey! Was this implemented yet? I'm interested in this feature aswell.. (being able to run hook action once per push, no matter how many commits there are in the push).

pasik added a comment.Thu, Apr 25, 8:24 AM

Gentle ping here again :) Has anyone managed to get a webhook triggered on a phabricator git repo, once-per-push, not once-per-each-commit ?

epriestley closed this task as Wontfix.Thu, Apr 25, 2:42 PM
epriestley claimed this task.

Please use the Discourse forum for this kind of discussion.

We have no customer requests for this, so I don't currently plan to pursue it.