chore(IDX): updates to bot check script

.github/workflows/repo_policies.yml

@@ -21,6 +21,7 @@ jobs:
       - name: Bot Checks
         id:  bot-checks
         run: |
+          set -euo pipefail
           export PYTHONPATH="$PWD/reusable_workflows/"
           python reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py
         shell: bash

reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py

@@ -22,7 +22,9 @@ def get_changed_files(merge_base_sha: str, branch_head_sha: str) -> list[str]:
     """
     commit_range = f"{merge_base_sha}..{branch_head_sha}"
     result = subprocess.run(
-        ["git", "diff", "--name-only", commit_range], stdout=subprocess.PIPE, text=True
+        ["git", "diff", "--name-only", commit_range],
+        capture_output=True,
+        text=True,
     )
     changed_files = result.stdout.strip().split("\n")
     return changed_files
@@ -51,7 +53,7 @@ def get_approved_files(config_file: str) -> list[str]:
     return approved_files
 
 
-def pr_is_blocked(env_vars: dict) -> bool:
+def check_if_pr_is_blocked(env_vars: dict) -> None:
     """
     Logic to check if the Bot's PR can be merged or should be blocked.
     """
@@ -64,13 +66,12 @@ def pr_is_blocked(env_vars: dict) -> bool:
     approved_files = get_approved_files(config)
     block_pr = not all(file in approved_files for file in changed_files)
     if block_pr:
-        print(
-            f"""Blocking PR because the changed files are not in the list of approved files.
+        message = f"""Blocking PR because the changed files are not in the list of approved files.
                 Update config at: {BOT_APPROVED_FILES_PATH} if necessary.
             """
-        )
-
-    return block_pr
+        raise SystemExit(message)
+    else:
+        print("Changed files are in list of approved files.")
 
 
 def main() -> None:
@@ -80,13 +81,10 @@ def main() -> None:
     is_bot = is_approved_bot(user)
 
     if is_bot:
-        block_pr = pr_is_blocked(env_vars)
+        check_if_pr_is_blocked(env_vars)
 
     else:
         print(f"{user} is not a bot. Letting CLA check handle contribution decision.")
-        block_pr = False
-
-    subprocess.run(f"""echo 'block_pr={block_pr}' >> $GITHUB_OUTPUT""", shell=True)
 
 
 if __name__ == "__main__":

reusable_workflows/tests/test_repo_policies.py

@@ -1,29 +1,30 @@
-import subprocess
 from unittest import mock
 
 import github3
 import pytest
 
 from repo_policies.bot_checks.check_bot_approved_files import (
     BOT_APPROVED_FILES_PATH,
+    check_if_pr_is_blocked,
     get_approved_files,
     get_approved_files_config,
     get_changed_files,
     main,
-    pr_is_blocked,
 )
 
 
-@mock.patch("subprocess.run")
+@mock.patch("repo_policies.bot_checks.check_bot_approved_files.subprocess.run")
 def test_get_changed_files(mock_subprocess_run):
-    mock_subprocess_run.return_value = mock.Mock(stdout="file1.py\nfile2.py\n")
+    mock_subprocess_run.return_value = mock.Mock(
+        stdout="file1.py\nfile2.py\n", returncode=0, stderr=""
+    )
 
     changed_files = get_changed_files("merge_base_sha", "branch_head_sha")
 
     assert changed_files == ["file1.py", "file2.py"]
     mock_subprocess_run.assert_called_once_with(
         ["git", "diff", "--name-only", "merge_base_sha..branch_head_sha"],
-        stdout=subprocess.PIPE,
+        capture_output=True,
         text=True,
     )
 
@@ -86,9 +87,8 @@ def test_pr_is_blocked_false(gh_login, get_approved_files_config, get_changed_fi
     ).read()
     get_approved_files_config.return_value = config_file
 
-    blocked = pr_is_blocked(env_vars)
+    check_if_pr_is_blocked(env_vars)
 
-    assert blocked is False
     get_changed_files.assert_called_once_with("base", "head")
     get_approved_files_config.assert_called_once_with(repo)
 
@@ -116,42 +116,41 @@ def test_pr_is_blocked_true(gh_login, get_approved_files_config, get_changed_fil
     ).read()
     get_approved_files_config.return_value = config_file
 
-    blocked = pr_is_blocked(env_vars)
+    with pytest.raises(SystemExit):
+        check_if_pr_is_blocked(env_vars)
 
-    assert blocked is True
     get_changed_files.assert_called_once_with("base", "head")
     get_approved_files_config.assert_called_once_with(repo)
 
 
 @mock.patch("repo_policies.bot_checks.check_bot_approved_files.load_env_vars")
 @mock.patch("repo_policies.bot_checks.check_bot_approved_files.is_approved_bot")
-@mock.patch("repo_policies.bot_checks.check_bot_approved_files.pr_is_blocked")
-@mock.patch("subprocess.run")
-def test_main_succeeds(subprocess_run, pr_is_blocked, is_approved_bot, load_env_vars):
+@mock.patch("repo_policies.bot_checks.check_bot_approved_files.check_if_pr_is_blocked")
+def test_main_succeeds(check_if_pr_is_blocked, is_approved_bot, load_env_vars, capfd):
     env_vars = {"GH_TOKEN": "token", "USER": "user"}
     load_env_vars.return_value = env_vars
     is_approved_bot.return_value = True
-    pr_is_blocked.return_value = False
+    check_if_pr_is_blocked.return_value = False
 
     main()
 
-    subprocess_run.assert_called_once_with(
-        "echo 'block_pr=False' >> $GITHUB_OUTPUT", shell=True
-    )
+    captured = capfd.readouterr()
+    assert "" == captured.out
 
 
 @mock.patch("repo_policies.bot_checks.check_bot_approved_files.load_env_vars")
 @mock.patch("repo_policies.bot_checks.check_bot_approved_files.is_approved_bot")
-@mock.patch("repo_policies.bot_checks.check_bot_approved_files.pr_is_blocked")
-@mock.patch("subprocess.run")
-def test_main_not_a_bot(subprocess_run, pr_is_blocked, is_approved_bot, load_env_vars):
+@mock.patch("repo_policies.bot_checks.check_bot_approved_files.check_if_pr_is_blocked")
+def test_main_not_a_bot(check_if_pr_is_blocked, is_approved_bot, load_env_vars, capfd):
     env_vars = {"GH_TOKEN": "token", "USER": "user"}
     load_env_vars.return_value = env_vars
     is_approved_bot.return_value = False
 
     main()
 
-    subprocess_run.assert_called_once_with(
-        "echo 'block_pr=False' >> $GITHUB_OUTPUT", shell=True
+    captured = capfd.readouterr()
+    assert (
+        "user is not a bot. Letting CLA check handle contribution decision."
+        in captured.out
     )
-    pr_is_blocked.assert_not_called()
+    check_if_pr_is_blocked.assert_not_called()