Fall back to main branch protection rules for PRs targeting branche…

…s without protection rules (#1900) * Skip reminders for PRs that do not target main * Remove references to non-existent repositories * Fallback to main branch protection rules rather than skipping non-main PRs
WordPress · May 2, 2023 · 2710483 · 2710483
1 parent afafd62
commit 2710483
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 29 deletions.
diff --git a/catalog/dags/maintenance/pr_review_reminders/pr_review_reminders.py b/catalog/dags/maintenance/pr_review_reminders/pr_review_reminders.py
@@ -3,6 +3,8 @@
 from collections import defaultdict
 from dataclasses import dataclass
 
+from requests import HTTPError
+
 from common.github import GitHubAPI
 
 
@@ -135,9 +137,7 @@ def base_repo_name(pr: dict):
 _BRANCH_PROTECTION_CACHE = defaultdict(dict)
 
 
-def get_branch_protection(gh: GitHubAPI, pr: dict) -> dict:
-    repo = base_repo_name(pr)
-    branch_name = pr["base"]["ref"]
+def get_branch_protection(gh: GitHubAPI, repo: str, branch_name: str) -> dict:
     if branch_name not in _BRANCH_PROTECTION_CACHE[repo]:
         _BRANCH_PROTECTION_CACHE[repo][branch_name] = gh.get_branch_protection(
             repo, branch_name
@@ -147,7 +147,20 @@ def get_branch_protection(gh: GitHubAPI, pr: dict) -> dict:
 
 
 def get_min_required_approvals(gh: GitHubAPI, pr: dict) -> int:
-    branch_protection_rules = get_branch_protection(gh, pr)
+    repo = base_repo_name(pr)
+    branch_name = pr["base"]["ref"]
+
+    try:
+        branch_protection_rules = get_branch_protection(gh, repo, branch_name)
+    except HTTPError as e:
+        # If the base branch does not have protection rules, the request
+        # above will 404. In that case, fall back to the rules for `main`
+        # as a safe default.
+        if e.response is not None and e.response.status_code == 404:
+            branch_protection_rules = get_branch_protection(gh, repo, "main")
+        else:
+            raise e
+
     return branch_protection_rules["required_pull_request_reviews"][
         "required_approving_review_count"
     ]

diff --git a/catalog/tests/dags/maintenance/test_pr_review_reminders.py b/catalog/tests/dags/maintenance/test_pr_review_reminders.py
@@ -2,6 +2,7 @@
 from datetime import datetime, timedelta
 
 import pytest
+from requests import HTTPError, Request, Response
 
 from catalog.dags.maintenance.pr_review_reminders.pr_review_reminders import (
     Urgency,
@@ -100,6 +101,13 @@ def get_reviews(*args, **kwargs):
     def get_branch_protection(*args, **kwargs):
         repo = args[1]
         branch = args[2]
+        if repo not in branch_protection or branch not in branch_protection[repo]:
+            response = Response()
+            response.status_code = 404
+            request = Request(method="GET", url="https://api.github.com/")
+            response.request = request
+            raise HTTPError(response=response)
+
         return branch_protection[repo][branch]
 
     def patch_gh_fn(fn, impl):
@@ -131,11 +139,16 @@ def freeze_friday(freeze_time):
 
 
 def _setup_branch_protection(github: dict, pr: dict, min_required_approvals: int = 2):
-    branch_protection = make_branch_protection(min_required_approvals)
-
     repo = pr["base"]["repo"]["name"]
     branch = pr["base"]["ref"]
 
+    _setup_branch_protection_for_branch(github, repo, branch, min_required_approvals)
+
+
+def _setup_branch_protection_for_branch(
+    github: dict, repo: str, branch: str, min_required_approvals: int = 2
+):
+    branch_protection = make_branch_protection(min_required_approvals)
     github["branch_protection"][repo][branch] = branch_protection
 
 
@@ -247,22 +260,25 @@ def test_does_reping_past_due_if_reminder_is_outdated(github, urgency):
     # tested elsewhere.
     ("APPROVED",) + UNAPPROVED_REVIEW_STATES,
 )
+@pytest.mark.parametrize("base_branch", ("main", "not_main"))
 def test_does_ping_if_pr_has_less_than_min_required_approvals(
-    github, urgency, first_review_state, second_review_state
+    github, urgency, first_review_state, second_review_state, base_branch
 ):
     reviews = [
         make_review(state)
         for state in (first_review_state, second_review_state)
         if state is not None
     ]
-    past_due_pull = make_pull(urgency, past_due=True)
+    past_due_pull = make_pull(urgency, past_due=True, base_branch=base_branch)
     past_due_pull["requested_reviewers"] = [
         make_requested_reviewer(f"reviewer-due-{i}") for i in range(2)
     ]
 
-    _setup_branch_protection(
+    # Always use `main` to exercise fallback for non-main branches
+    _setup_branch_protection_for_branch(
         github,
-        pr=past_due_pull,
+        repo="openverse",
+        branch="main",
         min_required_approvals=2,
     )
 
@@ -275,17 +291,26 @@ def test_does_ping_if_pr_has_less_than_min_required_approvals(
 
 
 @parametrize_urgency
-def test_does_not_ping_if_pr_has_min_required_approvals(github, urgency):
-    past_due_pull = make_pull(urgency, past_due=True)
+@pytest.mark.parametrize(
+    ("base_branch"),
+    (
+        "main",
+        "not_main",  # should fallback to use `main`
+    ),
+)
+def test_does_not_ping_if_pr_has_min_required_approvals(github, urgency, base_branch):
+    past_due_pull = make_pull(urgency, past_due=True, base_branch=base_branch)
     past_due_pull["requested_reviewers"] = [
         make_requested_reviewer(f"reviewer-due-{i}") for i in range(2)
     ]
 
     min_required_approvals = 4
 
-    _setup_branch_protection(
+    # Always use `main` to exercise fallback for non-main branches
+    _setup_branch_protection_for_branch(
         github,
-        pr=past_due_pull,
+        repo="openverse",
+        branch="main",
         min_required_approvals=min_required_approvals,
     )
 

diff --git a/catalog/tests/factories/github/__init__.py b/catalog/tests/factories/github/__init__.py
@@ -37,7 +37,7 @@ def walk_backwards_in_time_until_weekday_count(today: datetime.datetime, count:
 _pr_count = 1
 
 
-def make_pull(urgency: Urgency, past_due: bool) -> dict:
+def make_pull(urgency: Urgency, past_due: bool, base_branch: str = "main") -> dict:
     """
     Create a PR object like the one returned by the GitHub API.
 
@@ -70,6 +70,10 @@ def make_pull(urgency: Urgency, past_due: bool) -> dict:
 
     pull["updated_at"] = _gh_date(updated_at)
 
+    base = pull["base"]
+    base["ref"] = base_branch
+    base["label"] = f"WordPress:{base_branch}"
+
     return pull
 
 

diff --git a/catalog/tests/factories/github/pull.json b/catalog/tests/factories/github/pull.json
@@ -272,12 +272,12 @@
       "subscribers_url": "https://api.github.com/repos/helpful-contributor/openverse/subscribers",
       "subscription_url": "https://api.github.com/repos/helpful-contributor/openverse/subscription",
       "svn_url": "https://github.com/helpful-contributor/openverse",
-      "tags_url": "https://api.github.com/repos/helpful-contributor/openverse-frontend/tags",
-      "teams_url": "https://api.github.com/repos/helpful-contributor/openverse-frontend/teams",
+      "tags_url": "https://api.github.com/repos/helpful-contributor/openverse/tags",
+      "teams_url": "https://api.github.com/repos/helpful-contributor/openverse/teams",
       "topics": [],
-      "trees_url": "https://api.github.com/repos/helpful-contributor/openverse-frontend/git/trees{/sha}",
+      "trees_url": "https://api.github.com/repos/helpful-contributor/openverse/git/trees{/sha}",
       "updated_at": "2022-03-28T08:10:33Z",
-      "url": "https://api.github.com/repos/helpful-contributor/openverse-frontend",
+      "url": "https://api.github.com/repos/helpful-contributor/openverse",
       "visibility": "public",
       "watchers": 0,
       "watchers_count": 0
@@ -304,9 +304,9 @@
       "url": "https://api.github.com/users/helpful-contributor"
     }
   },
-  "html_url": "https://github.com/WordPress/openverse-frontend/pull/0",
+  "html_url": "https://github.com/WordPress/openverse/pull/0",
   "id": 123412341234,
-  "issue_url": "https://api.github.com/repos/WordPress/openverse-frontend/issues/0",
+  "issue_url": "https://api.github.com/repos/WordPress/openverse/issues/0",
   "labels": [
     {
       "color": "ffcc00",
@@ -315,7 +315,7 @@
       "id": 3029513057,
       "name": "\ud83d\udfe8 priority: medium",
       "node_id": "MDU6TGFiZWwzMDI5NTEzMDU3",
-      "url": "https://api.github.com/repos/WordPress/openverse-frontend/labels/%F0%9F%9F%A8%20priority:%20medium"
+      "url": "https://api.github.com/repos/WordPress/openverse/labels/%F0%9F%9F%A8%20priority:%20medium"
     },
     {
       "color": "ffffff",
@@ -324,7 +324,7 @@
       "id": 3029513140,
       "name": "\ud83d\udee0 goal: fix",
       "node_id": "MDU6TGFiZWwzMDI5NTEzMTQw",
-      "url": "https://api.github.com/repos/WordPress/openverse-frontend/labels/%F0%9F%9B%A0%20goal:%20fix"
+      "url": "https://api.github.com/repos/WordPress/openverse/labels/%F0%9F%9B%A0%20goal:%20fix"
     },
     {
       "color": "04338c",
@@ -333,7 +333,7 @@
       "id": 3029513145,
       "name": "\ud83d\udcc4 aspect: text",
       "node_id": "MDU6TGFiZWwzMDI5NTEzMTQ1",
-      "url": "https://api.github.com/repos/WordPress/openverse-frontend/labels/%F0%9F%93%84%20aspect:%20text"
+      "url": "https://api.github.com/repos/WordPress/openverse/labels/%F0%9F%93%84%20aspect:%20text"
     }
   ],
   "locked": false,
@@ -342,16 +342,16 @@
   "milestone": null,
   "node_id": "PR_asdfasdfasdfasdfasdfasdf",
   "number": 0,
-  "patch_url": "https://github.com/WordPress/openverse-frontend/pull/0.patch",
+  "patch_url": "https://github.com/WordPress/openverse/pull/0.patch",
   "requested_reviewers": [],
   "requested_teams": [],
-  "review_comment_url": "https://api.github.com/repos/WordPress/openverse-frontend/pulls/comments{/number}",
-  "review_comments_url": "https://api.github.com/repos/WordPress/openverse-frontend/pulls/0/comments",
+  "review_comment_url": "https://api.github.com/repos/WordPress/openverse/pulls/comments{/number}",
+  "review_comments_url": "https://api.github.com/repos/WordPress/openverse/pulls/0/comments",
   "state": "open",
-  "statuses_url": "https://api.github.com/repos/WordPress/openverse-frontend/statuses/asdfasdfasdfasdfasdfasdfasdf",
+  "statuses_url": "https://api.github.com/repos/WordPress/openverse/statuses/asdfasdfasdfasdfasdfasdfasdf",
   "title": "Super cool PR to add amazing new features.",
   "updated_at": "2022-06-16T11:57:28Z",
-  "url": "https://api.github.com/repos/WordPress/openverse-frontend/pulls/0",
+  "url": "https://api.github.com/repos/WordPress/openverse/pulls/0",
   "user": {
     "avatar_url": "https://avatars.githubusercontent.com/u/0?v=4",
     "events_url": "https://api.github.com/users/helpful-contributor/events{/privacy}",