[ADD] Pull request review script #346

madman-bob · 2018-06-07T14:00:15Z

Add a script to automatically review pull requests in the manner described in #203.

jgrandguillaume · 2018-06-11T07:30:04Z

Hi @madman-bob

Thanks for your proposal ! On my side it looks good. I have one request for testing/fixing purpose:

Can you add a --dry-run option that will print the results instead of making them ?

Other than that, I've asked my colleague to review it as well.

Regards,

Joël

madman-bob · 2018-06-11T10:25:11Z

Seems reasonable

pedrobaeza

Wow, excellent work! Thanks for this. Little comments, but very good

pedrobaeza · 2018-06-11T12:48:09Z

tools/review_pull_request.py

@@ -0,0 +1,167 @@
+#!/usr/bin/env python


Is this working both on 2.7/3.x?

Tested in 2.7. Works in 3.6 with a minor change to config.py. Not using any particularly exciting language features, so should work in all 3.x.

pedrobaeza · 2018-06-11T18:13:45Z

tools/review_pull_request.py

+        pr_is_old = now - pull_request.created_at > timedelta(days=31 * 6)
+
+        if pr_is_old and full_pull_request.comments_count == 0:
+            pull_request.create_comment("Please re-open if necessary")


We can put here a longer text for not being rude, like:

This is an automatic message for telling you that this PR is going to be closed due to a long inactivity period. Please ask for reopening if needed. """

Just a nitpicking comment here: isn't better to use the last update date? If some big PR is having a long discussion it will be auto-closed after 6 months even if there is recent conversations/reviews.

EDIT: Pedro already said that in the next review comment 😅 #346 (comment)

Then the comments_count could be removed (there are inactive PRs with comments)

pedrobaeza · 2018-06-11T18:15:51Z

tools/review_pull_request.py

+        now = datetime.now(pull_request.created_at.tzinfo)
+        pr_is_old = now - pull_request.created_at > timedelta(days=31 * 6)
+
+        if pr_is_old and full_pull_request.comments_count == 0:


We can have PRs with comments, but also abandoned. I think a better metric that the number of comments is the date of the PR < 6 months if no comment, but the date of the last comment < 6 months in case that you have comments.

I was just following the requirements as lain out in the original issue. You may well be right, but I don't really want to get into discussing what the best metrics are. It should be easy enough to change, and feel free to do so, if you want to.

Yeah, thanks, but as you know the API, please do it you. I give you my approval on behalf the @OCA/board

pedrobaeza · 2018-06-11T18:16:27Z

tools/review_pull_request.py

+        full_pull_request = repository.pull_request(pull_request.number)
+
+        now = datetime.now(pull_request.created_at.tzinfo)
+        pr_is_old = now - pull_request.created_at > timedelta(days=31 * 6)


Put directly 365 / 2 ~= 183

pedrobaeza · 2018-06-12T06:28:09Z

I'm thinking right now about the GitHub rate limits, that can be a problem making this process. Look how I handled these limits when pulling Transifex translations. I wrap any GitHub call this way:

maintainer-tools/tools/tx_pull.py

Lines 137 to 166 in 20557fa

    
           def wrap_gh_call(func, args=None, kwargs=None): 
        
               """Intercept all GH calls to wait when the API rate limit is reached.""" 
        
               retry = 0 
        
               while True: 
        
                   try: 
        
                       if args is None: 
        
                           args = [] 
        
                       if kwargs is None: 
        
                           kwargs = {} 
        
                       return func(*args, **kwargs) 
        
                   except (KeyboardInterrupt, SystemExit): 
        
                       raise 
        
                   except GitHubError as e: 
        
                       if e.code == 403: 
        
                           print("WARNING: %s. Sleeping 300 seconds" % e.message) 
        
                           time.sleep(300) 
        
                       elif e.code == 405: 
        
                           retry += 1 
        
                           if retry < 4: 
        
                               print("WARNING: Temporary error: %s. Retrying..." % ( 
        
                                   e.message 
        
                               )) 
        
                               time.sleep(5) 
        
                           else: 
        
                               print("WARNING: GitHub error: %s. Aborting..." % ( 
        
                                   e.message 
        
                               )) 
        
                               break 
        
                       else: 
        
                           raise

and call it this way:

maintainer-tools/tools/tx_pull.py

Line 277 in 20557fa

gh_branch = wrap_gh_call(gh_repo.branch, [oca_branch])

hbrunn · 2018-06-14T11:57:54Z

tools/review_pull_request.py

+    contributions = get_contribution_counts()
+
+    for pull_request in wrap_github_call(repository.pull_requests):
+        if contributions[pull_request.user.id] < 3:


I think this (and all other hardcoded values further down) should be arguments with the curernt value as default value

hbrunn · 2018-06-14T11:58:28Z

tools/review_pull_request.py

+        )
+
+        now = datetime.now(pull_request.created_at.tzinfo)
+        pr_is_old = now - pull_request.created_at > timedelta(days=31 * 6)


same thing as above

hbrunn · 2018-06-14T12:00:18Z

tools/review_pull_request.py

+        tag_new_pull_requests(repository_name, dry_run=args.dry_run)
+
+    if args.tag_all_new_pull_requests:
+        tag_all_new_pull_requests(dry_run=args.dry_run)


if you pass args to the functions, it's much simpler to implement my proposals above

@hbrunn OTOH passing args as arguments would make these functions harder to compose when we will want to run them behind a webhook.

sbidoul · 2018-06-17T13:20:09Z

tools/review_pull_request.py

+        github.repository,
+        [OCA_USERNAME, repository_name]
+    )
+    contributions = get_contribution_counts()


I have concerns about the performance of this.

In what way?

…ied time to determine age Rather than time created

madman-bob · 2018-06-25T09:06:59Z

Have rebased onto master, to resolve merge conflicts.

max3903 · 2018-07-03T12:22:35Z

@madman-bob Thank you for your contribution. The @OCA/board will be testing your script in the next few months so the merge may only happen at the OCA code sprint in Belgium in October. If you want to claim the bounty earlier, please start the process (I think you have to initiate the process).

madman-bob · 2018-07-03T13:01:08Z

@max3903 Thanks, but I'm only allowed to start the bounty claim once the original issue has been closed.

max3903 · 2018-07-03T13:03:11Z

@madman-bob Done

madman-bob · 2018-07-03T13:10:01Z

@max3903 Thank you very much. Claim submitted.

pedrobaeza · 2018-11-13T13:18:17Z

@sbidoul can this be closed as it has been moved to specialized repo?

sbidoul · 2018-11-13T13:57:38Z

Yes!

max3903 added the needs review label Jun 7, 2018

max3903 assigned madman-bob Jun 7, 2018

jgrandguillaume mentioned this pull request Jun 11, 2018

OCA Github Bot #203

Closed

jgrandguillaume requested review from sbidoul, gurneyalex and pedrobaeza June 11, 2018 07:30

pedrobaeza requested changes Jun 11, 2018

View reviewed changes

hbrunn requested changes Jun 14, 2018

View reviewed changes

sbidoul reviewed Jun 17, 2018

View reviewed changes

madman-bob added 7 commits June 25, 2018 09:59

Add OCA_USERNAME variable to tools/oca_projects.py

d96c144

Add GitHub pull request review automation script

ce5a1a0

Add --dry-run option to GitHub pull request review script

3606efd

Fix tools/config.py to work in Python 2

088f46a

Add GitHub call retry wrapping to tools/github_login.py

00ec0ee

Wrap pull request review script GitHub requests with retries

ba1f839

Change GitHub abandoned pull request closing script to use last modif…

a41d950

…ied time to determine age Rather than time created

This was referenced Sep 22, 2018

[ADD] task to set ready to merge label OCA/oca-github-bot#1

Merged

add "ready to merge" label #374

Merged

sbidoul closed this Nov 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADD] Pull request review script #346

[ADD] Pull request review script #346

madman-bob commented Jun 7, 2018

jgrandguillaume commented Jun 11, 2018

madman-bob commented Jun 11, 2018

pedrobaeza left a comment

pedrobaeza Jun 11, 2018

madman-bob Jun 12, 2018

pedrobaeza Jun 11, 2018

LoisRForgeFlow Jun 12, 2018 •

edited

Loading

LoisRForgeFlow Jun 12, 2018 •

edited

Loading

pedrobaeza Jun 11, 2018

madman-bob Jun 12, 2018

pedrobaeza Jun 12, 2018

pedrobaeza Jun 11, 2018

pedrobaeza commented Jun 12, 2018

hbrunn Jun 14, 2018

hbrunn Jun 14, 2018

hbrunn Jun 14, 2018

sbidoul Jun 17, 2018

sbidoul Jun 17, 2018

madman-bob Jun 18, 2018

madman-bob commented Jun 25, 2018

max3903 commented Jul 3, 2018

madman-bob commented Jul 3, 2018

max3903 commented Jul 3, 2018

madman-bob commented Jul 3, 2018

pedrobaeza commented Nov 13, 2018

sbidoul commented Nov 13, 2018

[ADD] Pull request review script #346

[ADD] Pull request review script #346

Conversation

madman-bob commented Jun 7, 2018

jgrandguillaume commented Jun 11, 2018

madman-bob commented Jun 11, 2018

pedrobaeza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LoisRForgeFlow Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

LoisRForgeFlow Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrobaeza commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madman-bob commented Jun 25, 2018

max3903 commented Jul 3, 2018

madman-bob commented Jul 3, 2018

max3903 commented Jul 3, 2018

madman-bob commented Jul 3, 2018

pedrobaeza commented Nov 13, 2018

sbidoul commented Nov 13, 2018

LoisRForgeFlow Jun 12, 2018 •

edited

Loading

LoisRForgeFlow Jun 12, 2018 •

edited

Loading