diff --git a/miss_islington/delete_branch.py b/miss_islington/delete_branch.py index f7087608..174782d0 100644 --- a/miss_islington/delete_branch.py +++ b/miss_islington/delete_branch.py @@ -1,27 +1,31 @@ -import asyncio - -import gidgethub import gidgethub.routing +from kombu import exceptions as kombu_ex +from redis import exceptions as redis_ex import stamina +from . import tasks + router = gidgethub.routing.Router() @router.register("pull_request", action="closed") -@stamina.retry(on=gidgethub.GitHubException, timeout=120) async def delete_branch(event, gh, *args, **kwargs): """ Delete the branch once miss-islington's PR is closed. """ if event.data["pull_request"]["user"]["login"] == "miss-islington": branch_name = event.data["pull_request"]["head"]["ref"] - url = f"/repos/miss-islington/cpython/git/refs/heads/{branch_name}" - if event.data["pull_request"]["merged"]: - await gh.delete(url) - else: - # this is delayed to ensure that the bot doesn't remove the branch - # if PR was closed and reopened to rerun checks (or similar) - await asyncio.sleep(60) - updated_data = await gh.getitem(event.data["pull_request"]["url"]) - if updated_data["state"] == "closed": - await gh.delete(url) + merged = event.data["pull_request"]["merged"] + pr_url = event.data["pull_request"]["url"] + installation_id = event.data["installation"]["id"] + _queue_delete_task(branch_name, pr_url, merged, installation_id) + + +@stamina.retry(on=(redis_ex.ConnectionError, kombu_ex.OperationalError), timeout=30) +def _queue_delete_task(branch_name, pr_url, merged, installation_id): + tasks.delete_branch_task.delay( + branch_name, + pr_url, + merged, + installation_id=installation_id + ) diff --git a/miss_islington/tasks.py b/miss_islington/tasks.py index 3154512c..6f1cd745 100644 --- a/miss_islington/tasks.py +++ b/miss_islington/tasks.py @@ -178,6 +178,58 @@ async def backport_task_asyncio( cp.abort_cherry_pick() +@app.task() +def delete_branch_task(branch_name, pr_url, merged, *, installation_id): + """Delete a branch from the miss-islington/cpython fork.""" + loop = asyncio.get_event_loop() + loop.run_until_complete( + _delete_branch_task_asyncio( + branch_name, pr_url, merged, installation_id=installation_id + ) + ) + + +async def _delete_branch_task_asyncio(branch_name, pr_url, merged, *, installation_id): + """Delete a branch, with delayed deletion for non-merged PRs.""" + if not util.is_cpython_repo(): + if "cpython" in os.listdir("."): + os.chdir("./cpython") + else: + print(f"Cannot delete branch: cpython repo not found. " + f"pwd: {os.getcwd()}, listdir: {os.listdir('.')}") + return + + if merged: + _git_delete_branch(branch_name) + else: + await asyncio.sleep(60) + async with aiohttp.ClientSession() as session: + gh = gh_aiohttp.GitHubAPI(session, "python/cpython", cache=cache) + installation_access_token = await apps.get_installation_access_token( + gh, + installation_id=installation_id, + app_id=os.environ.get("GH_APP_ID"), + private_key=os.environ.get("GH_PRIVATE_KEY") + ) + gh.oauth_token = installation_access_token["token"] + updated_data = await gh.getitem(pr_url) + if updated_data["state"] == "closed": + _git_delete_branch(branch_name) + + +def _git_delete_branch(branch_name): + """Delete a branch from the origin remote using git.""" + try: + subprocess.check_output( + ["git", "push", "origin", "--delete", branch_name], + stderr=subprocess.STDOUT + ) + print(f"Deleted branch {branch_name}") + except subprocess.CalledProcessError as e: + print(f"Failed to delete branch {branch_name}: {e.output.decode()}") + raise + + class InitRepoStep(bootsteps.StartStopStep): def start(self, c): print("Initialize the repository.") diff --git a/tests/test_delete_branch.py b/tests/test_delete_branch.py index 32c8fa18..19fe0bb3 100644 --- a/tests/test_delete_branch.py +++ b/tests/test_delete_branch.py @@ -1,159 +1,102 @@ -import asyncio +import os +from unittest import mock from gidgethub import sansio +import pytest -from miss_islington import delete_branch +os.environ.setdefault("HEROKU_REDIS_MAROON_URL", "someurl") - -class FakeGH: - def __init__(self, *, getitem=None): - self._getitem_return = getitem - self.getitem_url = None - self.post_data = None - - async def getitem(self, url): - self.getitem_url = url - to_return = self._getitem_return[self.getitem_url] - return to_return - - async def delete(self, url): - self.delete_url = url +from miss_islington import delete_branch, tasks -async def noop_sleep(delay, result=None): +class FakeGH: pass -async def test_branch_deleted_when_pr_merged(): +async def test_branch_deletion_queued_when_pr_merged(): data = { "action": "closed", "pull_request": { "number": 5722, "user": {"login": "miss-islington"}, "merged": True, - "merged_by": {"login": "miss-islington"}, - "head": {"ref": "backport-17ab8f0-3.7"}, - }, - } - event = sansio.Event(data, event="pull_request", delivery_id="1") - - gh = FakeGH() - await delete_branch.router.dispatch(event, gh) - assert gh.post_data is None # does not leave a comment - assert ( - gh.delete_url - == f"/repos/miss-islington/cpython/git/refs/heads/{data['pull_request']['head']['ref']}" - ) - - -async def test_branch_deleted_and_thank_committer(): - data = { - "action": "closed", - "pull_request": { - "number": 5722, - "user": {"login": "miss-islington"}, - "merged": True, - "merged_by": {"login": "Mariatta"}, - "head": {"ref": "backport-17ab8f0-3.7"}, - }, - } - event = sansio.Event(data, event="pull_request", delivery_id="1") - - gh = FakeGH() - await delete_branch.router.dispatch(event, gh) - assert gh.post_data is None # does not leave a comment - assert ( - gh.delete_url - == f"/repos/miss-islington/cpython/git/refs/heads/{data['pull_request']['head']['ref']}" - ) - - -async def test_branch_deleted_and_thanks(): - data = { - "action": "closed", - "pull_request": { - "number": 5722, - "user": {"login": "miss-islington"}, - "merged": True, - "merged_by": {"login": "miss-islington"}, - "head": {"ref": "backport-17ab8f0-3.7"}, - }, - } - event = sansio.Event(data, event="pull_request", delivery_id="1") - - gh = FakeGH() - await delete_branch.router.dispatch(event, gh) - assert gh.post_data is None # does not leave a comment - assert ( - gh.delete_url - == f"/repos/miss-islington/cpython/git/refs/heads/{data['pull_request']['head']['ref']}" - ) - - -async def test_branch_deleted_when_pr_closed(monkeypatch): - data = { - "action": "closed", - "pull_request": { - "number": 5722, - "user": {"login": "miss-islington"}, - "merged": False, - "merged_by": {"login": None}, "head": {"ref": "backport-17ab8f0-3.7"}, "url": "https://api.github.com/repos/python/cpython/pulls/5722", }, + "installation": {"id": 123}, } event = sansio.Event(data, event="pull_request", delivery_id="1") - getitem = { - "https://api.github.com/repos/python/cpython/pulls/5722": {"state": "closed"}, - } - monkeypatch.setattr(asyncio, "sleep", noop_sleep) - gh = FakeGH(getitem=getitem) - await delete_branch.router.dispatch(event, gh) - assert gh.post_data is None # does not leave a comment - assert ( - gh.delete_url - == f"/repos/miss-islington/cpython/git/refs/heads/{data['pull_request']['head']['ref']}" - ) + gh = FakeGH() + with mock.patch.object(tasks.delete_branch_task, "delay") as mock_delay: + await delete_branch.router.dispatch(event, gh) + mock_delay.assert_called_once_with( + "backport-17ab8f0-3.7", + "https://api.github.com/repos/python/cpython/pulls/5722", + True, + installation_id=123 + ) -async def test_branch_not_deleted_when_pr_closed_and_reopened(monkeypatch): +async def test_branch_deletion_queued_when_pr_closed_not_merged(): data = { "action": "closed", "pull_request": { "number": 5722, "user": {"login": "miss-islington"}, "merged": False, - "merged_by": {"login": None}, "head": {"ref": "backport-17ab8f0-3.7"}, "url": "https://api.github.com/repos/python/cpython/pulls/5722", }, + "installation": {"id": 456}, } event = sansio.Event(data, event="pull_request", delivery_id="1") - getitem = { - "https://api.github.com/repos/python/cpython/pulls/5722": {"state": "opened"}, - } - monkeypatch.setattr(asyncio, "sleep", noop_sleep) - gh = FakeGH(getitem=getitem) - await delete_branch.router.dispatch(event, gh) - assert gh.post_data is None # does not leave a comment - assert not hasattr(gh, "delete_url") + gh = FakeGH() + with mock.patch.object(tasks.delete_branch_task, "delay") as mock_delay: + await delete_branch.router.dispatch(event, gh) + mock_delay.assert_called_once_with( + "backport-17ab8f0-3.7", + "https://api.github.com/repos/python/cpython/pulls/5722", + False, + installation_id=456 + ) -async def test_ignore_non_miss_islingtons_prs(): +async def test_ignore_non_miss_islington_prs(): data = { "action": "closed", "pull_request": { "number": 5722, "user": {"login": "Mariatta"}, "merged": True, - "merged_by": {"login": "Mariatta"}, "head": {"ref": "backport-17ab8f0-3.7"}, + "url": "https://api.github.com/repos/python/cpython/pulls/5722", }, + "installation": {"id": 123}, } event = sansio.Event(data, event="pull_request", delivery_id="1") + gh = FakeGH() - await delete_branch.router.dispatch(event, gh) - assert gh.post_data is None # does not leave a comment - assert not hasattr(gh, "delete_url") + with mock.patch.object(tasks.delete_branch_task, "delay") as mock_delay: + await delete_branch.router.dispatch(event, gh) + mock_delay.assert_not_called() + + +def test_git_delete_branch_success(): + with mock.patch("subprocess.check_output") as mock_subprocess: + tasks._git_delete_branch("backport-17ab8f0-3.7") + mock_subprocess.assert_called_once_with( + ["git", "push", "origin", "--delete", "backport-17ab8f0-3.7"], + stderr=mock.ANY + ) + + +def test_git_delete_branch_failure(): + with mock.patch("subprocess.check_output") as mock_subprocess: + import subprocess + mock_subprocess.side_effect = subprocess.CalledProcessError( + 1, "git", output=b"error: unable to delete" + ) + with pytest.raises(subprocess.CalledProcessError): + tasks._git_delete_branch("backport-17ab8f0-3.7")