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

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

openverse_catalog/dags/maintenance/pr_review_reminders/pr_review_reminders_dag.py

@@ -21,17 +21,14 @@
 
 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
 
 
 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') }}",
+        },
     )

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

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