From 63cd4e3975715f25694abc279a2ead196ba4f179 Mon Sep 17 00:00:00 2001
From: Madison Swain-Bowden <bowdenm@spu.edu>
Date: Tue, 10 Jan 2023 07:22:33 -0800
Subject: [PATCH] Allow no content responses from GitHub (#937)

* Replace top level Variable.get calls with template renders

This also fixes an issue where the dry run variable could never be set to "false" locally since it was not JSON decoded.

* Add GitHubAPI tests, pook testing library

* Don't attempt to decode nothing, remove inaccurate return type
---
 openverse_catalog/dags/common/github.py       |  4 +-
 .../pr_review_reminders_dag.py                | 12 ++--
 requirements_dev.txt                          |  1 +
 tests/dags/common/test_github.py              | 56 +++++++++++++++++++
 4 files changed, 67 insertions(+), 6 deletions(-)
 create mode 100644 tests/dags/common/test_github.py

diff --git a/openverse_catalog/dags/common/github.py b/openverse_catalog/dags/common/github.py
index 2ad587033..c2c81586d 100644
--- a/openverse_catalog/dags/common/github.py
+++ b/openverse_catalog/dags/common/github.py
@@ -9,11 +9,13 @@ def __init__(self, pat: str):
         self.session = requests.Session()
         self.session.headers["Authorization"] = f"token {pat}"
 
-    def _make_request(self, method: str, resource: str, **kwargs) -> requests.Response:
+    def _make_request(self, method: str, resource: str, **kwargs):
         response = getattr(self.session, method.lower())(
             f"https://api.github.com/{resource}", **kwargs
         )
         response.raise_for_status()
+        if response.status_code == 204:
+            return None
         return response.json()
 
     def get_issue(self, repo: str, issue_number: int, owner: str = "WordPress"):
diff --git a/openverse_catalog/dags/maintenance/pr_review_reminders/pr_review_reminders_dag.py b/openverse_catalog/dags/maintenance/pr_review_reminders/pr_review_reminders_dag.py
index fd1e951ea..f2ba486ef 100644
--- a/openverse_catalog/dags/maintenance/pr_review_reminders/pr_review_reminders_dag.py
+++ b/openverse_catalog/dags/maintenance/pr_review_reminders/pr_review_reminders_dag.py
@@ -21,7 +21,7 @@
 
 from datetime import datetime, timedelta
 
-from airflow.models import DAG, Variable
+from airflow.models import DAG
 from airflow.operators.python import PythonOperator
 from common.constants import DAG_DEFAULT_ARGS
 from maintenance.pr_review_reminders import pr_review_reminders
@@ -29,9 +29,6 @@
 
 DAG_ID = "pr_review_reminders"
 MAX_ACTIVE_TASKS = 1
-ENVIRONMENT = Variable.get("ENVIRONMENT", default_var="dev")
-DRY_RUN = Variable.get("PR_REVIEW_REMINDER_DRY_RUN", default_var=(ENVIRONMENT == "dev"))
-GITHUB_PAT = Variable.get("GITHUB_API_KEY", default_var="not_set")
 
 dag = DAG(
     dag_id=DAG_ID,
@@ -50,11 +47,16 @@
     # Use the docstring at the top of the file as md docs in the UI
     doc_md=__doc__,
     tags=["maintenance"],
+    render_template_as_native_obj=True,
 )
 
 with dag:
     PythonOperator(
         task_id="pr_review_reminder_operator",
         python_callable=pr_review_reminders.post_reminders,
-        op_kwargs={"github_pat": GITHUB_PAT, "dry_run": DRY_RUN},
+        op_kwargs={
+            "github_pat": "{{ var.value.get('GITHUB_API_KEY', 'not_set') }}",
+            "dry_run": "{{ var.json.get('PR_REVIEW_REMINDER_DRY_RUN', "
+            "var.value.ENVIRONMENT != 'prod') }}",
+        },
     )
diff --git a/requirements_dev.txt b/requirements_dev.txt
index 2aa57dd8a..cd6269af6 100644
--- a/requirements_dev.txt
+++ b/requirements_dev.txt
@@ -4,6 +4,7 @@
 
 flaky==3.7.0
 ipython
+pook==1.0.2
 pytest-mock==3.10.0
 pytest-raises==0.11
 pytest-socket==0.5.1
diff --git a/tests/dags/common/test_github.py b/tests/dags/common/test_github.py
new file mode 100644
index 000000000..dae9c47ee
--- /dev/null
+++ b/tests/dags/common/test_github.py
@@ -0,0 +1,56 @@
+import pook
+import pytest
+from common.github import GitHubAPI
+from requests import HTTPError
+
+
+SAMPLE_PAT = "foobar"
+
+
+@pytest.fixture
+def github():
+    return GitHubAPI(pat=SAMPLE_PAT)
+
+
+@pytest.mark.parametrize(
+    "method, resource, status_code, response_text, expected",
+    [
+        # Standard replies
+        ("get", "repos/WordPress/openverse/pulls", 200, "[]", []),
+        (
+            "put",
+            "repos/WordPress/openverse/pull",
+            200,
+            '{"key":"value","a":2}',
+            {"key": "value", "a": 2},
+        ),
+        # 204 No Content reply
+        ("delete", "repos/WordPress/openverse/comment/555", 204, "", None),
+        # Failure replies
+        pytest.param(
+            "get",
+            "repos/WordPress/openverse/pulls",
+            400,
+            "[]",
+            [],
+            marks=pytest.mark.raises(exception=HTTPError),
+        ),
+        pytest.param(
+            "get",
+            "repos/WordPress/openverse/pulls",
+            500,
+            "[]",
+            [],
+            marks=pytest.mark.raises(exception=HTTPError),
+        ),
+    ],
+)
+@pook.on
+def test_github_make_request(
+    method, resource, status_code, response_text, expected, github
+):
+    pook_func = getattr(pook, method.lower())
+    pook_func(f"https://api.github.com/{resource}").reply(status_code).body(
+        response_text
+    )
+    assert github._make_request(method, resource) == expected