Close PR that tries to merge maintenance branch into master by Mariatta · Pull Request #60 · python/bedevere

Mariatta · 2017-09-18T04:29:57Z

This will close invalid PRs, so at least one less boring chore for core devs.
People in codeowners still get notified though...

A PR is considered invalid if it tries to merge python:<maint_branch> into python:master.
For example python:3.6 into python:master.

Closes python/core-workflow#168

codecov · 2017-09-18T04:31:41Z

Codecov Report

Merging #60 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #60   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     14    +2     
  Lines         780    827   +47     
  Branches       44     45    +1     
=====================================
+ Hits          780    827   +47

Impacted Files	Coverage Δ
tests/test_close_pr.py	`100% <100%> (ø)`
bedevere/close_pr.py	`100% <100%> (ø)`
bedevere/__main__.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48cc643...c3f8051. Read the comment docs.

brettcannon

The approach LGTM, but needs tests and there's some minor formatting style stuff to tweak to match PEP 8.

brettcannon · 2017-09-19T18:01:08Z

bedevere/close_pr.py

+@router.register("pull_request", action="opened")
+@router.register("pull_request", action="synchronize")
+async def close_invalid_pr(event, gh, *args, **kwargs):
+    """


Drop the newline from the opening docstring (for style consistency in the codebase).

brettcannon · 2017-09-19T18:01:31Z

bedevere/close_pr.py

+@router.register("pull_request", action="synchronize")
+async def close_invalid_pr(event, gh, *args, **kwargs):
+    """
+    Close the invalid PR


Missing a period and the summary line on its own line.

brettcannon · 2017-09-19T18:01:37Z

bedevere/close_pr.py

@@ -0,0 +1,28 @@
+"""Automatically close PR that tries to merge maintenance branch into master"""


Missing a period.

brettcannon · 2017-09-19T18:01:47Z

bedevere/close_pr.py

+import re
+
+import gidgethub.routing
+


Missing a newline between the last import and the first global definition.

brettcannon · 2017-09-19T18:03:41Z

bedevere/close_pr.py

+    if PYTHON_MAINT_BRANCH_RE.match(head_label) and \
+        base_label == "python:master":
+        data = {'state': 'closed',
+                'maintainer_can_modify': True


Either pull the closing brace up to this line, or move all dict lines on their own, have them all end with a comma, and have the closing brace be indented to the same level as data.

Mariatta · 2017-09-23T03:43:17Z

🆗 Made the requested changes.

brettcannon

There are a couple of minor style changes, but I trust you plenty to do it before merging so I'm just going to approve this now.

Thanks for the PR! I'm sure all of us who get auto-assigned to PRs will appreciate this. 😄

brettcannon · 2017-09-26T18:05:08Z

bedevere/__main__.py

 from gidgethub import sansio

-from . import backport, bpo, news, stage
+from . import backport, bpo, news, stage, close_pr


Alphabetize.

brettcannon · 2017-09-26T18:05:11Z

bedevere/__main__.py



-router = routing.Router(backport.router, bpo.router, news.router, stage.router)
+router = routing.Router(backport.router, bpo.router, news.router, stage.router,


Alphabetize.

brettcannon · 2017-09-26T18:05:20Z

bedevere/close_pr.py

+@router.register("pull_request", action="synchronize")
+async def close_invalid_pr(event, gh, *args, **kwargs):
+    """Close the invalid PR.
+    PR is considered invalid if:


Missing a blank line.

brettcannon · 2017-09-26T18:06:21Z

bedevere/close_pr.py

+async def close_invalid_pr(event, gh, *args, **kwargs):
+    """Close the invalid PR.
+    PR is considered invalid if:
+    base_label is 'python:master', and


Might as well make this a list, i.e.:

* base_label is 'python:master' * head_label is 'python:<some maintenance branch>'

Otherwise put a period at the end of the sentence.

brettcannon · 2017-09-26T18:09:46Z

tests/test_close_pr.py

+    }
+    event = sansio.Event(data, event="pull_request", delivery_id="12345")
+    gh = FakeGH(getitem=pr_data)
+    await close_pr.close_invalid_pr(event, gh)


To help test the router for the module, it's best to call through it, e.g.:

bedevere/tests/test_bpo.py

Line 63 in 48cc643

await bpo.router.dispatch(event, gh)

I've learned this lesson the hard way. 😉

brettcannon · 2017-09-26T18:10:20Z

tests/test_close_pr.py

+    gh = FakeGH(getitem=pr_data)
+    await close_pr.close_invalid_pr(event, gh)
+    patch_data = gh.patch_data
+    assert patch_data is None


Missing a newline.

Mariatta · 2017-09-29T03:07:48Z

I made the suggested changes, but now I don't know how to make codecov to pass...
Pretty sure I have all the necessary test cases.

brettcannon · 2017-09-29T17:13:20Z

I found the issue with why the tests weren't reaching 100% coverage (missing the "action" clause in the test data for the failing case). Now I'm just waiting for verification that coverage is good and then this is ready for merging!

brettcannon · 2017-09-29T17:18:18Z

Since I think we are all tired of these accidental PRs I have gone ahead and merged to get this out sooner rather than later. 😄

brettcannon · 2017-09-29T17:18:32Z

And obviously thanks for putting in the time to do this!

Mariatta · 2017-09-29T17:22:34Z

Awesome! Thanks for merging and fixing the unit test 😸

Close PR that tries to merge maintenance branch into master

156a1c4

Closes python/core-workflow#168

the-knights-who-say-ni added the CLA signed label Sep 18, 2017

brettcannon requested changes Sep 19, 2017

View reviewed changes

Mariatta added 2 commits September 22, 2017 20:35

Add tests and PEP 8 it

1c58e31

cleanup FakeGH

fe564c1

brettcannon approved these changes Sep 26, 2017

View reviewed changes

Mariatta added 3 commits September 28, 2017 18:55

Use router.dispatch

614b6e4

test close pr on action = "synchronize"

0047faa

typo: on_open -> on_synchronize

b8d8a3f

brettcannon closed this Sep 29, 2017

brettcannon reopened this Sep 29, 2017

Specify the action the hook was triggered for in the failing case

c3f8051

brettcannon merged commit 9927086 into python:master Sep 29, 2017

Mariatta deleted the close-pr branch September 29, 2017 17:22

Mariatta mentioned this pull request Oct 19, 2017

Dismiss review request for PRs that tries to merge from maintenance branch. #69

Closed

		@@ -0,0 +1,28 @@
		"""Automatically close PR that tries to merge maintenance branch into master"""



		router = routing.Router(backport.router, bpo.router, news.router, stage.router)
		router = routing.Router(backport.router, bpo.router, news.router, stage.router,

Uh oh!

Conversation

Mariatta commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Sep 23, 2017

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Sep 29, 2017

Uh oh!

brettcannon commented Sep 29, 2017

Uh oh!

brettcannon commented Sep 29, 2017

Uh oh!

brettcannon commented Sep 29, 2017

Uh oh!

Mariatta commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mariatta commented Sep 18, 2017 •

edited

Loading

codecov bot commented Sep 18, 2017 •

edited

Loading