Page MenuHomePhabricator

feed.http-hooks on hosted instance
Closed, ResolvedPublic

Description

We have a bot that watches for "shipit" comments from reviewer+author on github PRs and then deploys that revision to our production site (assuming it merges and passes other checks). We're attempting to reproduce similar behavior with Phabricator. The current idea is to treat closed revisions as having received the dual shipit, and attempt to deploy them. To do this, we're thinking we'd either listen to feed.http-hooks or poll via Conduit.

a) Does that seem reasonable?
b) Since we're on Phacility, feed.http-hooks is locked. Is this something you can change for us?

Event Timeline

venky created this task.Aug 9 2016, 5:10 PM
avivey added a subscriber: avivey.

The "expected" workflow is to have a Harbormaster build do/start the deployment, and have Herald execute that build when a new Commit is landed to the repository... Will that not work for you?

venky added a comment.Aug 9 2016, 5:48 PM

It perhaps does, I'm still trying to figure all of this out. So a harbormaster http request would notify the bot to deploy, and a herald rule would trigger that request. If we wanted to deploy multiple revisions in one push, we'd just buffer the http requests on the deploybot's end.

One property I was hoping to achieve was to avoid having to arc land - having an asynchronous landing + deploying process triggered by the author closing the revision after accept. Similarly, if there were a way for an author to say, "land and deploy this on reviewer accept without bothering me", that'd be convenient too. I don't see a way to have Herald rules trigger on Differential revision closing. Can you suggest an alternative?

Try editing it from the central administration console:

admin.phacility.com(Your Instance)Configurationfeed.http-hooks

avivey added a comment.Aug 9 2016, 5:57 PM

Revisions normally "Close" as a result of arc land / git push - by seeing the relevant commit on the repository. T182 talks about moving the "land" action to the web ui, but I believe that's not available in Phacility.
Are you manually closing the revision as a signal that the revision is "ready to be landed"?

Herald currently doesn't respond to most revision-related events (T10109), so it can't trigger HM.

venky added a comment.Aug 9 2016, 6:03 PM

Are you manually closing the revision as a signal that the revision is "ready to be landed"?`

Yes, that's what I was aiming to achieve. Is there a better way to do this?

avivey added a comment.Aug 9 2016, 6:15 PM

Not really; We normally just land it when it's ready to land; We generally think that the author should be the one to actually land the code, because they know the details, and use "accepted" as a signal from the reviewers to the author that it's OK.

T4706 talks about making arc land somewhat easier to use, but still be invoked from the CLI.

avivey added a comment.Aug 9 2016, 6:20 PM

Specifically, we feel that the act of landing code (and deployment) should be triggered by a human, because computers are stupid and don't know I have to go out early today and won't be here to fix the new bugs I just committed.

venky added a comment.Aug 9 2016, 6:55 PM

In the common case, I'm not arguing against that principle -- "closing a revision" in my example above is a trigger initiated by a human. I'm just not requiring them to switch branches and type arc land. T182 supports this thinking, it seems.

In the less common case (ie very small fixes), I feel like authors and reviewers can judge whether a change can go out silently without further intervention.

Separate from that, there are other processes that prevent authors from being irresponsible - notifications via other channels that things are landing, on-call engineers. This (asynchronous landing) workflow isn't that novel in my experience, both at my current job and at Facebook.

There's no real way to do this today, since this isn't a workflow that I think we've seen in professional environments.

In the process you describe, how do you deal with this case?

  • Author comments "shipit".
  • Something happens or there's discussion or whatever else. Reviewer later comments "shipit" after some time has passed.
  • There was a miscommunication, and the author is now driving to an offsite meeting or doing a phone screen or on vacation.
  • The change deploys to production and breaks the site.

My concern with this process is that it seems like it will work well up until the first time this happens, and that this will happen more than once way before you get to very many employees. I can't imagine this process working well in a moderately large organization with, say, 20+ engineers, since it seems like reviewers will often be unaware of who is in meetings or whatever else once you get to that scale.

On the flip side, what's the advantage you're seeking in performing automatic deployment on accept? Are you just trying to avoid the direct engineering cost of running arc land?

My gut is that the average cost of arc land (which is predictable and small) is probably much lower than the average cost of changes going into production without the author available, especially as an organization grows. Is the cost of arc land unusually high in your case, or the cost of deploying changes to production without the author present unusually low? I'd generally worry that this process is avoiding a tiny inconvenience but accepting an enormous risk in most professional production environments, and that it only scales up to "everyone is sitting in the same room", and there is actually an additional 'look and see that the author is sitting at their desk' implicit step in the empirical version of the process.

venky added a comment.Aug 9 2016, 6:57 PM

I don't want to distract this too much with the "automatic land on accept without author intervention" case. Is there a way to achieve the less controversial "author-triggered land without going to the CLI" case?

Not in the Phacility cluster today.

"Land Revision" can technically be configured, but it's currently a lot of work, and you have to bring your own host to do the merge on. See discussion in T10246, and T10246#163438 in particular. (That talks about configuring things for Harbormaster builds, but the process is substantially the same to configure "Land Revision".)

But even if you do that, "Land Revision" won't actually show up because it's a prototype and prototypes are disabled in the cluster.

I'd like to pull this out of prototype and provide a more automatic way to set this up in the Phacility cluster eventually, but don't have a timeline on either part.

In the former case, there are a number of unusual but not unreasonable situations we still don't handle gracefully, or don't give the user a very helpful pathway forward to recover from.

In the latter case, there are some technical challenges, and we also can't hand out free build hardware in the general case while remaining solvent. There's little reason for installs to purchase build hardware through us today when they could just bring their own, so doing this work doesn't get us much more than "Land Revision". It may also become unsafe to run land operations on instanced hosts (or in general) after T10329 and/or T5064. I don't yet have a concrete technical plan for how "Land Revision" and running custom code during land stages should interact.

Some possibly helpful notes, since you mention switching branches:

  • You can arc land <branch> instead of git checkout <branch>; arc land; git checkout <original>. Modern arc should switch branches and then put you back where you started when it's done.
  • You can try configuring arc.autostash to automatically stash changes.
  • T4706 will probably happen soon-ish and let you run arc land D123 instead of needing to know the branch name.
  • T3875 may provide some capabilities which help motivate using arc land, by letting it do more work for you.
venky added a comment.Aug 9 2016, 7:41 PM

Thanks for the pointers, I'll try that.

However, it will be a simpler migration path for us to use our current deploying bot (on our own hardware etc), if we could just get a callback upon closing a Differential revision -- which @avivey pointed out was T10109. Is that a dead end?

I don't currently have any plans to change the behavior described in T10109. At least there, I think the behavior was just confusing when testing rules, and not actually bad. My inclination is to try to find a way to make the behavior more obvious instead of changing it to resolve that confusion.

Changing it to run on every edit is trivial, but would result in every comment kicking off new builds in many reasonable setups if not accompanied by other changes.

I don't think the use case here is compelling enough to motivate a large behavioral change, and we discourage using Herald to publish events into remote systems directly (see T5462) anyway.

feed.http-hooks is an awkward fit, but I think is likely your best bet here if you want to implement this beahvior. If you build on this you should note that some changes are likely in the future (see T10363) to improve usability, but that's probably months away.

epriestley closed this task as Resolved.Aug 10 2016, 3:49 PM
epriestley claimed this task.

I think this is resolved as best it can be today, let me know if you still have questions.

venky added a comment.Aug 11 2016, 5:17 PM

@epriestley - I currently don't have the permissions to edit feed.http-hooks -- it claims I don't have permissions to manage the "finxpc.phacility.com" instance, however I'm apparently an admin on it. We (Danny, the person who set up the instance and I) have also been unable to figure out how to grant me any different permissions - could you please give us a nudge in the right direction? Danny also appears to be an admin, same as me, but has permissions to edit that variable.

Check this out if you haven't seen it yet?

https://admin.phacility.com/book/phacility/article/managing/#allowing-other-users-to

Let me know if that doesn't solve things.