Maciej Szulik added the comment:
Patch updated and most importantly tested with github. Comments inline below:
> I think you'll end up with a user_id in the >1 case and a list (assuming
> filter returns a list) in the <1 case. At the end you take the first element
> of the list, which is
> correct only for the <1 case (unless filters returns a single id, but in that
> case I'm not sure what the [0] at the end is for). I would also suggest to
> user user_ids and
> user_id to distinguish the list from the single id.
> Ezio seems to be right about user_id and getting a list or a name depending
> on which branch is taken based on the line that sets user_id initially. It
> also might be clearer if > you check for `not len(user_id)` or `len(user_id)
> == 0` instead of `len(user_id) < 1`.
Yeah, filter returns list always, I've updated the patch accordingly.
> I would rename set_current_user() to something more explicit, or, if the only
> purpose of this method is to affect the author=self.db.getuid(), perhaps
> remove the method and use
> a module/class-level constant. The same constant could also be used in the
> snippet I pasted above to avoid duplication.
I've changed it to set_roundup_user, not sure what kind of constant you were
talking about.
> Does produces the correct output in the history at the bottom of the issue?
> (i.e. does it show "set messages: +msgXXX" with only the new message, rather
> than the whole list
> of messages?)
Yes, it's 'messages +msgX', as expected.
> I would move the .encode('utf-8') from dispatch() to handle_action() (add it
> to the line "'branch': branch.encode('utf-8'),").
Done
> The commit_id variable in handle_action() is unused.
Removed
> Any reason why you used Template() instead of a simply using format() (or %)?
> You might also be able to get rid of all those encode if you use format().
Laziness, I just copy&pasted what I got from hglookup.py ;) Changed to format.
> github/roundup should be capitalized properly in the docstrings. The
> docstring for handle_action() could be a bit more explicit :)
Done
> And thanks for handling the case when there's no match. I wouldn't be
> surprised if people don't sign the CLA but still get a PR merged that e.g.
> fixes a single line of the
> documentation.
N/p :)
_______________________________________________________
PSF Meta Tracker <metatrac...@psf.upfronthosting.co.za>
<http://psf.upfronthosting.co.za/roundup/meta/issue611>
_______________________________________________________diff --git a/roundup/github.py b/roundup/github.py
--- a/roundup/github.py
+++ b/roundup/github.py
@@ -1,5 +1,3 @@
-from roundup.exceptions import *
-
import hashlib
import hmac
import json
@@ -7,6 +5,8 @@
import os
import logging
+from roundup.exceptions import Unauthorised, MethodNotAllowed, \
+ UnsupportedMediaType, Reject
if hasattr(hmac, 'compare_digest'):
compare_digest = hmac.compare_digest
@@ -14,8 +14,15 @@
def compare_digest(a, b):
return a == b
-url_re = re.compile(r'https://github.com/python/cpython/pull/(?P<number>\d+)')
-issue_id_re = re.compile(r'bpo\s*(\d+)', re.I)
+URL_RE = re.compile(r'https://github.com/python/cpython/pull/(?P<number>\d+)')
+ISSUE_GH_RE = re.compile(r'bpo\s*(\d+)', re.I)
+ISSUE_BPO_RE = re.compile(r'(#|issue|bug)\s*(?P<issue_id>\d+)', re.I)
+
+COMMENT_TEMPLATE = """\
+New changeset {changeset_id} by {author} in branch '{branch}':
+{commit_msg}
+{changeset_url}
+"""
class GitHubHandler:
"""
@@ -59,6 +66,10 @@
data = json.loads(self.form.value)
handler = IssueComment(self.db, data)
handler.dispatch()
+ elif event == 'push':
+ data = json.loads(self.form.value)
+ handler = Push(self.db, data)
+ handler.dispatch()
def validate_webhook_secret(self):
"""
@@ -93,7 +104,7 @@
def get_event(self):
"""
- Extracts github event from header field.
+ Extracts GitHub event from header field.
"""
return self.request.headers.get('X-GitHub-Event', None)
@@ -107,24 +118,27 @@
self.db = db
self.data = data
- def set_current_user(self):
+ def set_roundup_user(self):
"""
- Helper method used for setting current user for roundup, based
- on the information from github about event author.
+ Helper method used for setting the current user for Roundup, based
+ on the information from GitHub about event author.
"""
github_username = self.get_github_username()
- user_id = self.db.user.filter(None, {'github': github_username})
- # TODO set bpobot as userid when none is found
- if len(user_id) == 1:
- # TODO what if multiple bpo users have the same github username?
- username = self.db.user.get(user_id[0], 'username')
- self.db.setCurrentUser(username)
+ user_ids = self.db.user.filter(None, {'github': github_username})
+ if not user_ids:
+ # set bpobot as userid when none is found
+ user_ids = self.db.user.filter(None, {'username': 'python-dev'})
+ if not user_ids:
+ # python-dev does not exists, anonymous will be used instead
+ return
+ username = self.db.user.get(user_ids[0], 'username')
+ self.db.setCurrentUser(username)
def dispatch(self):
"""
- Main method responsible for responding to incoming github event.
+ Main method responsible for responding to incoming GitHub event.
"""
- self.set_current_user()
+ self.set_roundup_user()
action = self.data.get('action', '').encode('utf-8')
issue_ids = self.get_issue_ids()
if not issue_ids:
@@ -220,7 +234,7 @@
raise Reject()
title = pull_request.get('title', '').encode('utf-8')
body = pull_request.get('body', '').encode('utf-8')
- return list(set(issue_id_re.findall(title) +
issue_id_re.findall(body)))
+ return list(set(ISSUE_GH_RE.findall(title) +
ISSUE_GH_RE.findall(body)))
def get_pr_details(self):
"""
@@ -234,7 +248,7 @@
raise Reject()
title = pull_request.get('title', '').encode('utf-8')
status = pull_request.get('state', '').encode('utf-8')
- # github has two states open and closed, information about pull request
+ # GitHub has two states open and closed, information about pull request
# being merged in kept in separate field
if pull_request.get('merged', False):
status = "merged"
@@ -242,13 +256,14 @@
def get_github_username(self):
"""
- Extract github username from a pull request.
+ Extract GitHub username from a pull request.
"""
pull_request = self.data.get('pull_request')
if pull_request is None:
raise Reject()
return pull_request.get('user', {}).get('login', '').encode('utf-8')
+
class IssueComment(Event):
"""
Class responsible for handling issue comment events, but only within the
@@ -274,7 +289,7 @@
raise Reject()
title = issue.get('title', '').encode('utf-8')
body = comment.get('body', '').encode('utf-8')
- return list(set(issue_id_re.findall(title) +
issue_id_re.findall(body)))
+ return list(set(ISSUE_GH_RE.findall(title) +
ISSUE_GH_RE.findall(body)))
def get_pr_details(self):
"""
@@ -284,16 +299,74 @@
if issue is None:
raise Reject()
url = issue.get('pull_request', {}).get('html_url')
- number_match = url_re.search(url)
+ number_match = URL_RE.search(url)
if not number_match:
return (None, None, None)
return number_match.group('number'), None, None
def get_github_username(self):
"""
- Extract github username from a comment.
+ Extract GitHub username from a comment.
"""
issue = self.data.get('issue')
if issue is None:
raise Reject()
return issue.get('user', {}).get('login', '').encode('utf-8')
+
+
+class Push(Event):
+ """
+ Class responsible for handling push events.
+ """
+
+ def dispatch(self):
+ """
+ Main method responsible for responding to incoming GitHub event.
+ """
+ self.set_roundup_user()
+ commits = self.data.get('commits', [])
+ ref = self.data.get('ref', 'refs/heads/master')
+ messages = {}
+ # extract commit messages
+ for commit in commits:
+ messages.update(self.handle_action(commit, ref))
+ if not messages:
+ return
+ # add comments to appropriate issues
+ for issue_id, msg in messages.iteritems():
+ issue_msgs = self.db.issue.get(issue_id, 'messages')
+ newmsg = self.db.msg.create(content=msg, author=self.db.getuid())
+ issue_msgs.append(newmsg)
+ self.db.issue.set(issue_id, messages=issue_msgs)
+ self.db.commit()
+
+ def get_github_username(self):
+ """
+ Extract GitHub username from a push event.
+ """
+ return self.data.get('pusher', []).get('name', '').encode('utf-8')
+
+ def handle_action(self, commit, ref):
+ """
+ This is implementing the same logic as the mercurial hook from here:
+ https://hg.python.org/hooks/file/tip/hgroundup.py
+ """
+ branch = ref.encode('utf-8').split('/')[-1]
+ description = commit.get('message', '').encode('utf-8')
+ matches = ISSUE_BPO_RE.finditer(description)
+ messages = {}
+ for match in matches:
+ # TODO: close issue when close(s|d) is seen
+ data = match.groupdict()
+ # check for duplicated issue numbers in the same commit msg
+ if data['issue_id'] in messages:
+ continue
+ messages[data['issue_id']] = COMMENT_TEMPLATE.format(
+ author=commit.get('committer', {}).get('name',
'').encode('utf-8'),
+ branch=branch,
+ changeset_id=commit.get('id', '').encode('utf-8'),
+ changeset_url=commit.get('url', '').encode('utf-8'),
+ commit_msg=description.splitlines()[0],
+ )
+ return messages
+
diff --git a/test/data/pushevent.txt b/test/data/pushevent.txt
new file mode 100644
--- /dev/null
+++ b/test/data/pushevent.txt
@@ -0,0 +1,11 @@
+POST /python-dev/pull_request HTTP/1.1
+Host: 3ab1787e.ngrok.io
+Accept: */*
+User-Agent: GitHub-Hookshot/98ea3cc
+X-GitHub-Event: push
+X-GitHub-Delivery: 3d4b5180-5c89-11e6-88fd-1aa99d941991
+content-type: application/json
+X-Hub-Signature: sha1=c760213ddccd47bf3bbd1735b70c787bb8d07724
+Content-Length: 5852
+
+{"ref":"refs/heads/test1","before":"6c5d08240c73014f282bc7336c08fe9ec0b557b0","after":"65c3a074262662a2c55109ff9a2456ee7647fcc9","created":false,"deleted":false,"forced":false,"base_ref":null,"compare":"https://github.com/python/cpython/compare/6c5d08240c73...65c3a0742626","commits":[{"id":"65c3a074262662a2c55109ff9a2456ee7647fcc9","tree_id":"2a35adbb6fce03b51dce23b4ed48526a5a74255a","distinct":true,"message":"#1:
fix
tests.","timestamp":"2017-01-19T22:19:07+01:00","url":"https://github.com/python/cpython/commit/65c3a074262662a2c55109ff9a2456ee7647fcc9","author":{"name":"Maciej
Szulik","email":"solt...@gmail.com","username":"soltysh"},"committer":{"name":"Maciej
Szulik","email":"solt...@gmail.com","username":"soltysh"},"added":[],"removed":[],"modified":["README"]}],"head_commit":{"id":"65c3a074262662a2c55109ff9a2456ee7647fcc9","tree_id":"2a35adbb6fce03b51dce23b4ed48526a5a74255a","distinct":true,"message":"Fixes#1","timestamp":"2017-01-19T22:19:07+01:00","url":"https://githu
b.com/python/cpython/commit/65c3a074262662a2c55109ff9a2456ee7647fcc9","author":{"name":"Maciej
Szulik","email":"solt...@gmail.com","username":"soltysh"},"committer":{"name":"Maciej
Szulik","email":"solt...@gmail.com","username":"soltysh"},"added":[],"removed":[],"modified":["README"]},"repository":{"id":76687672,"name":"cpython","full_name":"python/cpython","owner":{"name":"python"},"private":false,"html_url":"https://github.com/python/cpython","description":"Semi-officialread-onlymirroroftheCPythonMercurialrepository","fork":true,"url":"https://github.com/python/cpython","forks_url":"https://api.github.com/repos/python/cpython/forks","keys_url":"https://api.github.com/repos/python/cpython/keys{/key_id}","collaborators_url":"https://api.github.com/repos/python/cpython/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/python/cpython/teams","hooks_url":"https://api.github.com/repos/python/cpython/hooks","issue_events_url":"https://api.github.com/repos/pyt
hon/cpython/issues/events{/number}","events_url":"https://api.github.com/repos/python/cpython/events","assignees_url":"https://api.github.com/repos/python/cpython/assignees{/user}","branches_url":"https://api.github.com/repos/python/cpython/branches{/branch}","tags_url":"https://api.github.com/repos/python/cpython/tags","blobs_url":"https://api.github.com/repos/python/cpython/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/python/cpython/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/python/cpython/git/refs{/sha}","trees_url":"https://api.github.com/repos/python/cpython/git/trees{/sha}","statuses_url":"https://api.github.com/repos/python/cpython/statuses/{sha}","languages_url":"https://api.github.com/repos/python/cpython/languages","stargazers_url":"https://api.github.com/repos/python/cpython/stargazers","contributors_url":"https://api.github.com/repos/python/cpython/contributors","subscribers_url":"https://api.github.com/repos/python/cpython/subs
cribers","subscription_url":"https://api.github.com/repos/python/cpython/subscription","commits_url":"https://api.github.com/repos/python/cpython/commits{/sha}","git_commits_url":"https://api.github.com/repos/python/cpython/git/commits{/sha}","comments_url":"https://api.github.com/repos/python/cpython/comments{/number}","issue_comment_url":"https://api.github.com/repos/python/cpython/issues/comments{/number}","contents_url":"https://api.github.com/repos/python/cpython/contents/{+path}","compare_url":"https://api.github.com/repos/python/cpython/compare/{base}...{head}","merges_url":"https://api.github.com/repos/python/cpython/merges","archive_url":"https://api.github.com/repos/python/cpython/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/python/cpython/downloads","issues_url":"https://api.github.com/repos/python/cpython/issues{/number}","pulls_url":"https://api.github.com/repos/python/cpython/pulls{/number}","milestones_url":"https://api.github.com/repo
s/python/cpython/milestones{/number}","notifications_url":"https://api.github.com/repos/python/cpython/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/python/cpython/labels{/name}","releases_url":"https://api.github.com/repos/python/cpython/releases{/id}","deployments_url":"https://api.github.com/repos/python/cpython/deployments","created_at":1481924711,"updated_at":"2016-12-16T21:45:35Z","pushed_at":1484860759,"git_url":"git://github.com/python/cpython.git","ssh_url":"g...@github.com:python/cpython.git","clone_url":"https://github.com/python/cpython.git","svn_url":"https://github.com/python/cpython","homepage":"","size":269743,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master","stargazers":0,"master_branch":"master"},"pusher":{"name":"sol
tysh","email":"solt...@gmail.com"},"sender":{"login":"soltysh","id":576341,"avatar_url":"https://avatars.githubusercontent.com/u/576341?v=3","gravatar_id":"","url":"https://api.github.com/users/soltysh","html_url":"https://github.com/soltysh","followers_url":"https://api.github.com/users/soltysh/followers","following_url":"https://api.github.com/users/soltysh/following{/other_user}","gists_url":"https://api.github.com/users/soltysh/gists{/gist_id}","starred_url":"https://api.github.com/users/soltysh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/soltysh/subscriptions","organizations_url":"https://api.github.com/users/soltysh/orgs","repos_url":"https://api.github.com/users/soltysh/repos","events_url":"https://api.github.com/users/soltysh/events{/privacy}","received_events_url":"https://api.github.com/users/soltysh/received_events","type":"User","site_admin":false}}
diff --git a/test/test_github.py b/test/test_github.py
--- a/test/test_github.py
+++ b/test/test_github.py
@@ -296,6 +296,18 @@
status = self.db.pull_request.get(prs[0], 'status')
self.assertEqual(status, 'closed')
+ def testPushEventAddsComment(self):
+ dummy_client = self._make_client('pushevent.txt')
+ handler = GitHubHandler(dummy_client)
+ handler.dispatch()
+ msgs = self.db.issue.get('1', 'messages')
+ self.assertEqual(len(msgs), 1)
+ content = self.db.msg.get(msgs[0], 'content')
+ self.assertIn("""New changeset
65c3a074262662a2c55109ff9a2456ee7647fcc9 by Maciej Szulik in branch 'test1':
+#1: fix tests.
+https://github.com/python/cpython/commit/65c3a074262662a2c55109ff9a2456ee7647fcc9
+""",
+ content)
def test_suite():
suite = unittest.TestSuite()
_______________________________________________
Tracker-discuss mailing list
Tracker-discuss@python.org
https://mail.python.org/mailman/listinfo/tracker-discuss
Code of Conduct: https://www.python.org/psf/codeofconduct/
