From d9172c9b14bb687ffa31abaef6f238fb759acedd Mon Sep 17 00:00:00 2001
From: Becky Smith <becky.smith@thedatalab.org>
Date: Mon, 13 Nov 2023 12:17:09 +0000
Subject: [PATCH] fix: Return False if upstream codelist check fails in CI

---
 opensafely/codelists.py | 29 +++++++++++++++++++++--------
 tests/test_codelists.py | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/opensafely/codelists.py b/opensafely/codelists.py
index 79875ec..61598fb 100644
--- a/opensafely/codelists.py
+++ b/opensafely/codelists.py
@@ -135,18 +135,29 @@ def check_upstream(codelists_dir=None):
         # if any codelists in codelists.csv don't match the expected pattern. These should all
         # be fixable by `opensafely codelists update`.
         if "error" in response["data"]:
-            exit_with_prompt(
+            error_message = (
                 f"Error checking upstream codelists: {response['data']['error']}\n"
             )
-        if response["data"]["added"] or response["data"]["removed"]:
-            exit_with_prompt(
+        elif response["data"]["added"] or response["data"]["removed"]:
+            error_message = (
                 "Codelists have been added or removed\n\n"
                 "For details, run:\n\n  opensafely codelists check\n"
             )
-        changed = "\n  ".join(response["data"]["changed"])
-        exit_with_prompt(
-            f"Some codelists are out of date\nCodelists affected:\n  {changed}\n"
-        )
+        else:
+            changed = "\n  ".join(response["data"]["changed"])
+            error_message = (
+                f"Some codelists are out of date\nCodelists affected:\n  {changed}\n"
+            )
+
+        # If we're running in CI, we don't want to fail the entire action due to out of
+        # date upstream codelists, as users may have valid reasons for not wanting to update
+        # them  (i.e. if they have already run jobs that use the backend database). In this
+        # case, just print the error message instead of exiting.
+        if os.environ.get("GITHUB_WORKFLOW"):
+            print(error_message)
+            return False
+        exit_with_prompt(error_message)
+
     print("Codelists OK")
     return True
 
@@ -210,7 +221,7 @@ def check():
         )
 
     try:
-        return check_upstream(codelists_dir)
+        upstream_check = check_upstream(codelists_dir)
     except requests.exceptions.ConnectionError:
         print(
             f"Local codelists OK; could not contact {OPENCODELISTS_BASE_URL} for upstream check,"
@@ -218,6 +229,8 @@ def check():
         )
         return True
 
+    return upstream_check
+
 
 def make_temporary_manifest(codelists_dir):
     with tempfile.TemporaryDirectory() as tmpdir:
diff --git a/tests/test_codelists.py b/tests/test_codelists.py
index 99eac55..6019e98 100644
--- a/tests/test_codelists.py
+++ b/tests/test_codelists.py
@@ -29,6 +29,12 @@ def _mock(response):
     return _mock
 
 
+@pytest.fixture(autouse=True)
+def not_ci(monkeypatch):
+    if "GITHUB_WORKFLOW" in os.environ:
+        monkeypatch.delenv("GITHUB_WORKFLOW")
+
+
 def test_codelists_update(tmp_path, requests_mock):
     codelist_dir = tmp_path / "codelists"
     codelist_dir.mkdir()
@@ -227,3 +233,29 @@ def test_codelists_check_upstream_with_changes(codelists_path, mock_check, respo
     mock_check(response={"status": "error", "data": response})
     with pytest.raises(SystemExit):
         codelists.check_upstream(codelists_path / "codelists")
+
+
+def test_codelists_check_with_upstream_changes(codelists_path, mock_check):
+    mock_check(
+        response={
+            "status": "error",
+            "data": {"added": [], "removed": [], "changed": ["org/foo/123"]},
+        }
+    )
+    os.chdir(codelists_path)
+    with pytest.raises(SystemExit):
+        codelists.check()
+
+
+def test_codelists_check_with_upstream_changes_in_CI(
+    codelists_path, mock_check, monkeypatch
+):
+    monkeypatch.setenv("GITHUB_WORKFLOW", "test")
+    mock_check(
+        response={
+            "status": "error",
+            "data": {"added": [], "removed": [], "changed": ["org/foo/123"]},
+        }
+    )
+    os.chdir(codelists_path)
+    assert codelists.check() is False