Use github api for osg-bot PR interaction (SOFTWARE-3564) #444

edquist · 2019-02-13T05:17:24Z

Since we have @osg-bot now; the webhook app can use an access token for the github api to make comments directly on PRs, rather than emailing a select list.

Also, @osg-bot can hit the big green Merge button directly with the github api, rather than the webhook app needing to generate and push a merge commit manually with an ssh key.

This is currently functional on topology-itb, though i'm marking this PR WIP, as there was already some feedback about things that can be improved (#436), and i'm sure i'll want to clean up this branch before merging it.

edquist · 2019-02-14T04:44:51Z

For @matyasselmeci and @brianhlin,

New osg-bot PR comments:

non-dt pr: #450 (no osg-bot message)
dt, automerge checks pass, but out-of-date pr: #449
dt, not all automerge checks pass: #451
dt, automerge checks pass but ci fail: #452
dt, automerge approved & merge success: #457
dt, automerge approved, unexpected merge failure: #458

... and, do we wanna add the help@ email always, or only on failures?

(SOFTWARE-3564)

we only want to allow 'merge', which is the default, not 'squash' or 'rebase'. (SOFTWARE-3564)

(SOFTWARE-3564)

sendit! sendat comment! (SOFTWARE-3564)

(SOFTWARE-3564)

...required rearranging status_hook a bit... (SOFTWARE-3564)

... if this all works, we'll strip out the stuff to generate the merge commit, and maybe the mailx stuff too (SOFTWARE-3564)

(SOFTWARE-3564)

which probably belong in the github module... but one step at a time (SOFTWARE-3564)

(SOFTWARE-3564)

note we also dropped 👍 for mat (SOFTWARE-3564)

(SOFTWARE-3564)

if gh_api_user/token aren't configured, this will just be an empty call (SOFTWARE-3564)

put validate_request_signature next to validate_webhook_signature (SOFTWARE-3564)

(SOFTWARE-3564)

status sender is the ci job owner, which is always derek (SOFTWARE-3564)

(SOFTWARE-3564)

edquist · 2019-02-21T18:04:08Z

The force-push just now was a rebase to tidy up git history a bit, no content changes.

This is currently working on topology-itb and will only make at most a single PR comment, rather than the multiple seen in previous iterations.

src/webapp/webhook_status_messages.py

(SOFTWARE-3564) Co-Authored-By: edquist <edquist@users.noreply.github.com>

(SOFTWARE-3564)

src/webapp/webhook_status_messages.py

brianhlin · 2019-02-22T23:01:47Z

src/webapp/webhook_status_messages.py

+    "Unexpected merge failure!\n"
+    "\n"
+    "{fail_message}\n"
+    "\n"


We should express that OSG staff will investigate this issue

brianhlin · 2019-02-22T23:02:41Z

src/webhook_app.py

 import glob
 import hmac
 import logging
 import os
 import re
 import subprocess
-from subprocess import PIPE
+from   subprocess import PIPE


Errant whitespace change

Suggested change

from subprocess import PIPE

from subprocess import PIPE

src/webhook_app.py

(SOFTWARE-3564) Co-Authored-By: edquist <edquist@users.noreply.github.com>

(SOFTWARE-3564)

edquist · 2019-02-25T21:50:53Z

@matyasselmeci, @brianhlin, lmk if you have anything further for this

matyasselmeci · 2019-02-27T01:27:57Z

src/webapp/github.py

+    def github_api_call(self, method, url, data):
+        if data is not None:
+            data = json.dumps(data).encode()
+        req = urllib.request.Request(url, data)


Does doing req = urllib.request.Request(url, data, method=method) not work? Then you wouldn't have to use a lambda below.

How about that.

In python2, urllib2.Request (which python3 moved to urllib.request.Request), the __init__ method did not take a method parameter, and it was somewhat annoyingly required that req.get_method be set to a function that would return the method to be used. Thus the lambda.

I didn't realize they added a method parameter. Apparently this was added in Python 3.3.

So, while this github_api_call goes about things in a way that is also python2 compatible, yeah it looks cleaner to just pass in a method parameter to Request, since we only care about python3.6+.

matyasselmeci · 2019-02-27T01:43:17Z

Feel free to merge once you've brought the branch up to date.

Apparently in python3.3+, Request takes a method parameter, rather than needing to provide a get_method function. And since we only care about python3.6+, we might as well just do that. Suggested by Mat. (SOFTWARE-3564)

edquist added the WIP label Feb 13, 2019

edquist added 28 commits February 20, 2019 18:08

introduce WEBHOOK_GH_API_TOKEN

21abcfa

(SOFTWARE-3564)

publish_issue_comment skeleton

23cd991

(SOFTWARE-3564)

cleanup unused

f3f6be0

(SOFTWARE-3564)

introduce WEBHOOK_GH_API_USER

40a4b20

(SOFTWARE-3564)

github_api_call, flesh out publish_issue_comment

a469f8a

(SOFTWARE-3564)

publish_pr_review

ea33750

(SOFTWARE-3564)

break out github api stuff to separate module

d73280b

(SOFTWARE-3564)

commit_id is a key name

23a0b39

(SOFTWARE-3564)

github api: hit_merge_button

dc38f81

(SOFTWARE-3564)

comment github api response statuses

e5719dd

(SOFTWARE-3564)

encode to bytes

e271be5

(SOFTWARE-3564)

don't need add_gh_preview_header currently

135f22b

(SOFTWARE-3564)

avoid double-slash

c795c91

(SOFTWARE-3564)

data should be json bytes

79f7a52

(SOFTWARE-3564)

disallow specifying merge method

68dc112

we only want to allow 'merge', which is the default, not 'squash' or 'rebase'. (SOFTWARE-3564)

comments about resp

a592f6c

(SOFTWARE-3564)

constants for review actions

b34633c

(SOFTWARE-3564)

approve_pr wrapper

b687794

(SOFTWARE-3564)

put osg-bot comments in separate module

bb3edb5

(SOFTWARE-3564)

include up_to_date flag in get_base_head_shas() return value

061df08

(SOFTWARE-3564)

separate exit status for pass-except-out-of-date

d4357c6

(SOFTWARE-3564)

automerge_status_messages list

6daceb9

(SOFTWARE-3564)

publish pr review comment for automerge test results

1596d10

sendit! sendat comment! (SOFTWARE-3564)

publish pr review APPROVE after ci success

2feffad

(SOFTWARE-3564)

comment on pr if eligible for automerge but ci failed

dd32151

...required rearranging status_hook a bit... (SOFTWARE-3564)

hit merge button instead of pushing merge ref

c4e9da1

... if this all works, we'll strip out the stuff to generate the merge commit, and maybe the mailx stuff too (SOFTWARE-3564)

handle http errors; extract merge fail message

7c1db6e

(SOFTWARE-3564)

logger wrappers for github comments

b8a8394

which probably belong in the github module... but one step at a time (SOFTWARE-3564)

edquist added 12 commits February 21, 2019 11:51

do not publish dt_success comment (until ci success)

148ac66

(SOFTWARE-3564)

do not publish merge success comment

bcf42c2

(SOFTWARE-3564)

ci_success is now _the_ success message

b32dd3c

note we also dropped 👍 for mat (SOFTWARE-3564)

fix Response

7adfede

(SOFTWARE-3564)

drop unused _fix_unicode

8e3f2eb

(SOFTWARE-3564)

call publish_pr_review whether or not configured

c42354c

if gh_api_user/token aren't configured, this will just be an empty call (SOFTWARE-3564)

rearrange slightly

d38e841

put validate_request_signature next to validate_webhook_signature (SOFTWARE-3564)

include initial greeting context in ci_failure

d271840

(SOFTWARE-3564)

ci_{success,failure} comments from status hook include sender

98f967e

(SOFTWARE-3564)

use pr sender not status sender

aa6f0d1

status sender is the ci job owner, which is always derek (SOFTWARE-3564)

wait for merge success before publishing comment

acd3a96

(SOFTWARE-3564)

allow publishing pr review with no body

03fb462

(SOFTWARE-3564)

edquist force-pushed the SOFTWARE-3564.github-api-comments branch from fe74251 to 03fb462 Compare February 21, 2019 18:00

edquist removed the WIP label Feb 21, 2019

edquist requested a review from matyasselmeci February 21, 2019 18:04

matyasselmeci reviewed Feb 21, 2019

View reviewed changes

src/webapp/webhook_status_messages.py Outdated Show resolved Hide resolved

matyasselmeci and others added 2 commits February 21, 2019 12:14

heads up

0d97c96

(SOFTWARE-3564) Co-Authored-By: edquist <edquist@users.noreply.github.com>

supply contact registration link

4bb9216

(SOFTWARE-3564)

brianhlin reviewed Feb 22, 2019

View reviewed changes

brianhlin and others added 2 commits February 25, 2019 15:08

tweak message per BrianL

005a794

(SOFTWARE-3564) Co-Authored-By: edquist <edquist@users.noreply.github.com>

say that osg help is on the way after merge failure

891c7c9

(SOFTWARE-3564)

matyasselmeci reviewed Feb 27, 2019

View reviewed changes

matyasselmeci approved these changes Feb 27, 2019

View reviewed changes

edquist and others added 3 commits February 26, 2019 20:48

pass method parameter in Request constructor

0ba3616

Apparently in python3.3+, Request takes a method parameter, rather than needing to provide a get_method function. And since we only care about python3.6+, we might as well just do that. Suggested by Mat. (SOFTWARE-3564)

Merge branch 'master' into SOFTWARE-3564.github-api-comments

a6c2f3c

Merge branch 'master' into SOFTWARE-3564.github-api-comments

037ef52

edquist merged commit eb7c829 into opensciencegrid:master Feb 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use github api for osg-bot PR interaction (SOFTWARE-3564) #444

Use github api for osg-bot PR interaction (SOFTWARE-3564) #444

edquist commented Feb 13, 2019

edquist commented Feb 14, 2019

edquist commented Feb 21, 2019

brianhlin Feb 22, 2019

edquist Feb 25, 2019

brianhlin Feb 22, 2019

edquist commented Feb 25, 2019

matyasselmeci Feb 27, 2019

edquist Feb 27, 2019

matyasselmeci commented Feb 27, 2019

Use github api for osg-bot PR interaction (SOFTWARE-3564) #444

Use github api for osg-bot PR interaction (SOFTWARE-3564) #444

Conversation

edquist commented Feb 13, 2019

edquist commented Feb 14, 2019

edquist commented Feb 21, 2019

brianhlin Feb 22, 2019

Choose a reason for hiding this comment

edquist Feb 25, 2019

Choose a reason for hiding this comment

brianhlin Feb 22, 2019

Choose a reason for hiding this comment

edquist commented Feb 25, 2019

matyasselmeci Feb 27, 2019

Choose a reason for hiding this comment

edquist Feb 27, 2019

Choose a reason for hiding this comment

matyasselmeci commented Feb 27, 2019