Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Make github payload parsing for pull requests more consistent
Currently, for a push request the repository field is filled with the
value of ['repository']['url'] but a pull request uses
['repository']['clone_url']. The clone_url has a .git suffix, this
results in a non-regex filter for a git url working with only the push
or the pull request, not both. This schema is only followed on github
enterprise, whereas github.com uses a different domain for pull requests
(api.github.com). ['repository']['html_url'] is preferred instead as it
follows the same format on github & GHE and hence matches everywhere

Fix and also parse and fill repository field for pull requests, update documentation

Sample of the parsed payload for clarification :

Push :

 added change with revision ce9e9d44b2d090541099f540e706c8ae54b89a05 to database
 injected change Change(revision=u'ce9e9d44b2d090541099f540e706c8ae54b89a05', who=u'Anish Bhatt <anish@gatech.edu>', branch=u'master', comments=u'Create TEST', when=1457647806, category=None, project=u'knightswhosaynee/testrepo', repository=u'https://github.com/knightswhosaynee/testrepo', codebase=u'')

Pull :

 added change with revision db35deb31691cd50617342718125ed1aa61303e3 to database
 injected change Change(revision=u'db35deb31691cd50617342718125ed1aa61303e3', who=u'anish-bhatt', branch=u'refs/pull/3/head', comments=u'GitHub Pull Request #3 (1 commit)', when=1457647899, category=u'pull', project=u'', repository=u'https://github.com/knightswhosaynee/testrepo.git', codebase=u'')

Note how the repository field doesn't match up due to suffix and
empty project field in the pull request.
  • Loading branch information
anish committed Mar 17, 2016
1 parent 2d11233 commit 3703f2f
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 3 deletions.
5 changes: 4 additions & 1 deletion master/buildbot/test/unit/test_www_hooks_github.py
Expand Up @@ -37,6 +37,7 @@
"before": "5aef35982fb2d34e9d9d4502f6ede1072793222d",
"repository": {
"url": "http://github.com/defunkt/github",
"html_url": "http://github.com/defunkt/github",
"name": "github",
"full_name": "defunkt/github",
"description": "You're lookin' at it.",
Expand Down Expand Up @@ -84,6 +85,7 @@
"before": "5aef35982fb2d34e9d9d4502f6ede1072793222d",
"repository": {
"url": "http://github.com/defunkt/github",
"html_url": "http://github.com/defunkt/github",
"name": "github",
"full_name": "defunkt/github",
"description": "You're lookin' at it.",
Expand Down Expand Up @@ -244,6 +246,7 @@
"before": "5aef35982fb2d34e9d9d4502f6ede1072793222d",
"repository": {
"url": "http://github.com/defunkt/github",
"html_url": "http://github.com/defunkt/github",
"name": "github",
"full_name": "defunkt/github",
"description": "You're lookin' at it.",
Expand Down Expand Up @@ -459,7 +462,7 @@ def _check_git_with_pull(self, payload):
self.assertEquals(len(self.changeHook.master.addedChanges), 1)
change = self.changeHook.master.addedChanges[0]
self.assertEquals(change["repository"],
"https://github.com/defunkt/github.git")
"https://github.com/defunkt/github")
self.assertEquals(timegm(change["when_timestamp"].utctimetuple()),
1412899790)
self.assertEquals(change["author"],
Expand Down
5 changes: 3 additions & 2 deletions master/buildbot/www/hooks/github.py
Expand Up @@ -99,7 +99,7 @@ def handle_push(self, payload):
user = None
# user = payload['pusher']['name']
repo = payload['repository']['name']
repo_url = payload['repository']['url']
repo_url = payload['repository']['html_url']
# NOTE: what would be a reasonable value for project?
# project = request.args.get('project', [''])[0]
project = payload['repository']['full_name']
Expand Down Expand Up @@ -128,7 +128,8 @@ def handle_pull_request(self, payload):
'when_timestamp': dateparse(payload['pull_request']['created_at']),
'branch': refname,
'revlink': payload['pull_request']['_links']['html']['href'],
'repository': payload['repository']['clone_url'],
'repository': payload['repository']['html_url'],
'project': payload['pull_request']['base']['repo']['full_name'],
'category': 'pull',
# TODO: Get author name based on login id using txgithub module
'author': payload['sender']['login'],
Expand Down
5 changes: 5 additions & 0 deletions master/docs/manual/cfg-wwwhooks.rst
Expand Up @@ -124,6 +124,11 @@ Note that not using ``change_hook_auth`` can expose you to security risks.

Patches are welcome to implement: https://developer.github.com/webhooks/securing/

.. note::

When using a :ref:`ChangeFilter<Change-Filters>` with a GitHub webhook ensure that your filter matches all desired requests as fields such as ``repository`` and ``project`` may differ in different events.


BitBucket hook
++++++++++++++

Expand Down
1 change: 1 addition & 0 deletions master/docs/relnotes/index.rst
Expand Up @@ -29,6 +29,7 @@ Fixes

* Fix loading :class:`~buildbot.ldapuserinfo.LdapUserInfo` plugin and its documentation (:bug:`3371`).
* Fix deprecation warnings seen with docker-py >= 1.4 when passing arguments to ``docker_client.start()``.
* :class:`GitHubEventHandler` now uses the ``['repository']['html_url']`` key in the webhook payload to populate ``repository``, as the previously used ``['url']`` and ``['clone_url']`` keys had a different format between push and pull requests and GitHub and GitHub Enterprise instances.

Deprecations, Removals, and Non-Compatible Changes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down

0 comments on commit 3703f2f

Please sign in to comment.