Page MenuHomePhabricator

[DRAFT] Integrate GitHub pull requests as Differential reviews
AbandonedPublic

Authored by hach-que on Apr 15 2014, 7:29 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 23, 1:06 PM
Unknown Object (File)
Sat, Mar 23, 4:44 AM
Unknown Object (File)
Feb 25 2024, 10:23 PM
Unknown Object (File)
Feb 8 2024, 2:17 AM
Unknown Object (File)
Jan 24 2024, 8:34 AM
Unknown Object (File)
Jan 11 2024, 7:30 PM
Unknown Object (File)
Dec 16 2023, 5:25 AM
Unknown Object (File)
Dec 9 2023, 12:42 PM
Tokens
"Yellow Medal" token, awarded by avivey.

Details

Reviewers
None
Summary

This uses Doorkeeper to import GitHub pull requests as Differential revisions. For this to work, users need to do two things:

  • Create a GitHub service hook to point to https://my-phabricator-install.com/doorkeeper/github-hook/ for the repository they want to monitor, with the payload version "application/vnd.github.v3+form". This can be done by going to "Settings" on the GitHub repository and selecting "Webhooks & Services".
  • Create a bot / service account with the username github. Currently this is hardcoded, but it should really be a configurable option. This is required because anyone can submit a PR on GitHub, including users which haven't connected Phabricator <-> GitHub accounts. I don't know whether we want to auto-create accounts in this case instead of using a service account?
Test Plan

Follow the instructions above, or submit a pull request to https://github.com/hach-que/test and then check https://code.redpointsoftware.com.au/differential/ to see if it appeared.

Also, an example:

Pull Request: https://github.com/hach-que/test/pull/2

Differential Revision: https://code.redpointsoftware.com.au/D269

Diff Detail

Repository
rP Phabricator
Branch
arcpatch-D8775
Lint
Lint Warnings
Unit
No Test Coverage

Event Timeline

hach-que retitled this revision from to [WIP] Integrate GitHub pull requests as Differential reviews.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que edited edge metadata.
  • Got GitHub hook working
  • Bridge now working and syncing PRs
hach-que retitled this revision from [WIP] Integrate GitHub pull requests as Differential reviews to Integrate GitHub pull requests as Differential reviews.Apr 15 2014, 10:04 AM
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.

@epriestley:

So the import logic seems to only work when a logged-in user is performing the current request. e.g. if you look at https://secure.phabricator.com/differential/diff/20824/ you can see I had hooked up a method by which to make GET requests to this logic. When I was logged in and hit a URL like https://code.redpointsoftware.com.au/doorkeeper/github-hook/?type=pull_request&payload=%7B%7D&id=hach-que:test:1, it would import successfully and the revision would update.

However, when GitHub sends it's service request, I still get hach-que:test:1 imported as a response (which means it ran through all of the logic), but the revision hasn't actually been updated.

I'm not sure whether this is because:

  • Doorkeeper's import engine isn't behaving correctly with a public user or
  • Application transactions aren't behaving correctly with a public user (even though they're given a different account)

You probably have a better idea of what's silently not doing it's job here than I do.

src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHub.php
98

I'm not sure what the best option is here, both references and objects seem to accept arbitrary data.

At the moment it doesn't matter too much (the synchronisation is from GitHub into Phabricator), but when we have a worker pushing Phabricator comments back to the GitHub PR it will be important.

201

So here we should map the associated GitHub account name back to a Phabricator account with it connected if possible. There's a few problems, namely:

  • This is a public API, someone could potentially forge a request and act as another user. This is really bad. I believe if we have a configuration option which sets a secret token for this API, with the secret token then also set as a parameter on the GitHub side, then we might be able to mitigate this somewhat (although anyone setting up the web hook could still forge requests).
  • Anyone on GitHub can create pull requests, even users without Phabricator accounts. Either we need to use a service account, or auto-create an account that represents an external account on GitHub.
src/applications/doorkeeper/controller/DoorkeeperGitHubHookController.php
43–47

This is just to make debugging from the GitHub interface easier (it shows the response from the web hook, so this can be used to track down exceptions).

On an additional note, I've noticed that viewing revisions created by this synchronisation are far more slow to load in the web UI than normal revisions. I have no idea why.

  • Got the GitHub service hook responding. tl;dr I derped

Okay, so it's working now when GitHub makes the callback (I derped and got the username and repo around the wrong way when building the pull request ID). Here's an example:

Pull Request: https://github.com/hach-que/test/pull/2

Differential Revision: https://code.redpointsoftware.com.au/D269

@qgil I saw that WMF was looking at this to reduce contributor requirements. If you use this before it's converted to use Nuance, I can't guarantee that things aren't going to break horribly if and when this gets upstreamed. Usage of the (non-existant but planned) Nuance infrastructure solves a lot of otherwise-hard-to-solve problems, so at the very least, all the integration points will change.

@hach-que, thank you for the background. No worries, there are still many steps that need to be completed before thinking about this feature for our users.

hach-que retitled this revision from Integrate GitHub pull requests as Differential reviews to [DRAFT] Integrate GitHub pull requests as Differential reviews.

Abandoning this to get it out of my queue. A proper implementation requires the Nuance infrastructure.