[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: I think we can. -- status: testing -> resolved ___ PSF Meta Tracker ___ _

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Brett C. added the comment: Can this be closed? ___ PSF Meta Tracker ___ ___ Tracker-discus

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Ezio Melotti added the comment: This happened again in http://bugs.python.org/issue29554 -- not sure if someone was playing with the payloads or if something else triggered the duplication. ___ PSF Meta Tracker

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Ezio Melotti added the comment: Note that we check duplication when adding PRs manually to the issue, but afaik not when the PR is added by GitHub through the webhooks. I don't know if there might be cases where separate events might cause a PR to be added more than once (except for manually

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: I'll double check the duplication path and see what might have caused it and work on a fix. ___ PSF Meta Tracker

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Ezio Melotti added the comment: I noticed that it's possible to add the same PR multiple times to the same issue, see e.g. http://bugs.python.org/issue29546 This probably happened because the payload was re-sent manually, and shouldn't otherwise be an issue, so we might not have to fix it (but

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Brett C. added the comment: I've tested this and it seems to be working. ___ PSF Meta Tracker ___ _

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Ezio Melotti added the comment: Done in https://hg.python.org/tracker/roundup/rev/f44e8ff423a4 Thanks everyone for the patches and the reviews! -- assignedto: -> ezio.melotti nosy: +ezio.melotti status: in-progress -> testing ___ PSF

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Brett C. added the comment: LGTM ___ PSF Meta Tracker ___ ___ Tracker-discuss mailing list

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: I tested the latest patch on the my repository and it is working properly. :) ___ PSF Meta Tracker ___ __

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: I'm uploading the current version of the patch, it's still not fully reviewed, I've left a few TODO's here and there for what should be double checked. ___ PSF Meta Tracker

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: LGTM ___ PSF Meta Tracker ___ ___ Tracker-discuss mailing

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: I have updated the patch with all three changes ___ PSF Meta Tracker ___diff --git a/roundup/cgi/client.p

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: Oops. Got it now. I will include the changes. ___ PSF Meta Tracker ___ __

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: > I think, It's because I removed "url_exist" check according to your > previous review. > But, since these requests are triggered by GitHub, there will be only one > request for "opened" action. url_exists was removed due to not being able to link the same pr to

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: > There's still a problem that multiple calls to the same bug will create multiple links to the same PR. > Example: create issue1 and invoke curl -H "X-GitHub-Event: pull_request" -H "content-type: application/json" -H "X-Hub-Signature: sha1=2c74c307193b7276fef7b1776

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: Those are the three remaining issues, all the rest looks good. roundup/pull_request.py: > except Unauthorised, message: > logging.error(message, exc_info=True) > raise Unauthorised(message) > except MethodNotAllowed, message: > logging.error(message

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: new tests: - secret key missing - Github event missing in header - non json body - multiple references in PR body - CREATE_ISSUE is not set and no issue is reference in PR - if github field of user is set, then PR/issue is assigned to that user or else anonymous __

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: > Sorry. I didn't get you. You mean, same tests for PR body, right? Yeah, exactly that. I'd like to see both body and title tests. Sorry I was initially noting down my thoughts and apparently forgot to "normalize" that part of the sentence ;) > I was thinking

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: > * multiple fixes both in title like you have in pullrequestevent3.txt but also in title Sorry. I didn't get you. You mean, same tests for PR body, right? > Is vulnerable to other exceptions, I'm a lazy person and I test your code with following > command: > cur

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: I'm missing following elements in this submission: - tests: * negative cases (you only have unsupported media type and request method) but more is needed, iow. test every path: missing Github event, secret, etc. * issue created by anonymous, user with gi

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: I have updated the patch. There are two main changes :- 1. If multiple issues are referenced, the PR is linked to each one of them. 2. raises Reject exception if any key is missing. (even for environment variable) ___

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: Should I throw Reject exception if 'SECRET_KEY' is not set as an environment variable too? ___ PSF Meta Tracker

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: roundup/pull_request.py: > key = os.environ['SECRET_KEY'] If this env is missing the code throws KeyError, which is not nice. It would be better to check if it exists and fail nicely, informing about the problem. > def testPullRequestEventForTitle(self): >

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: I have added few things to this updated patch: - load JSON while extracting - user who creates GitHub PR will be assigned as PR author on bpo if we find github name on bpo or else "anonymous" - if issue is not referenced in PR title or body and an environment vari

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: Updated the patch ___ PSF Meta Tracker ___diff --git a/roundup/cgi/client.py b/roundup/cgi/client.py inde

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: Anish the ability to create a new issue when you don't find bpoNNN string in title nor in text should be possible to turn on and off. Environment variable might be good for that, for example. ___ PSF Meta Trac

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: > Maybe put the code in with a switch so we can turn it on and off? I'm pretty sure we're going to want it, but not positive. Sorry. I didn't get you. What do you want to turn on and off? ___ PSF Meta Tracker

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

R David Murray added the comment: Maybe put the code in with a switch so we can turn it on and off? I'm pretty sure we're going to want it, but not positive. ___ PSF Meta Tracker __

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: > But as Brett said that someone might forget to add issue number. I'm > currently checking for issue id in PR title, body and issue comments. So, a > reviewer/contributor can add a comment "fixes bpoNN" later on. According to [1] you won't get information when a

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Anish Shah added the comment: Thank you for the review. I will update the patch. @David I can create a new issue if there's no issue number mentioned. But as Brett said that someone might forget to add issue number. I'm currently checking for issue id in PR title, body and issue comments. So,

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: Anish more comments: roundup/pull_request.py: > def __init__(self, client): > self.db = client.db > self.request = client.request > self.form = client.form > self.data = json.loads(self.form.value) > self.env = client.env I'd generally sugge

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

R David Murray added the comment: Yes, I was assuming we'd post to the right account when we can determine it. It was only github accounts with no linkage where we'd need a 'generic' user for posting. And we *will* get PRs from people who won't open bugs.python.org accounts, even if asked to

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Maciej Szulik added the comment: @anish.shah I still don't see my comments from previous review addressed. Additionally, since issue 586 has changed the schema you should update it here as well. @David: > Hmm. Good question. We could use a specific user to create such issues. The other op

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Brett C. added the comment: Another issue is that what happens if there is an issue, but the person simply forgot to include it? Now we have an extra issue that will need to have its superceder set and the issue closed. Eventually we could have a bot that automatically added a comment that eit

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

R David Murray added the comment: Hmm. Good question. We could use a specific user to create such issues. As for noise...we do typo issues now, so if we have an increased volume of them I think that just means we're fixing more typos. I don't really see the problem with that, since committi

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

Berker Peksag added the comment: > And if the PR doesn't reference an issue number, it should create a new issue. Wouldn't that create too much noise for trivial pull requests like typo fixes, documentation tweaks and code style changes? Also how do we handle if the user doesn't have an accoun

[Tracker-discuss] [issue589] Add GitHub PR to b.p.o issues using GitHub webhooks (depends on issue586)

R David Murray added the comment: And if the PR doesn't reference an issue number, it should create a new issue. I think we should also look for issue numbers in the body of the PR as well. The user might put it in the description rather than the title, if they provide a description. ---