Delete branches as an async task with the correct auth (#726)

Author: ambv

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
+    )

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.")

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")