diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index ed78718..886af08 100644 --- a/.github/workflows/repo_policies.yml +++ b/.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 diff --git a/reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py b/reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py index aca0690..297afaa 100644 --- a/reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py +++ b/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__": diff --git a/reusable_workflows/tests/test_repo_policies.py b/reusable_workflows/tests/test_repo_policies.py index a835012..3dcead3 100644 --- a/reusable_workflows/tests/test_repo_policies.py +++ b/reusable_workflows/tests/test_repo_policies.py @@ -1,4 +1,3 @@ -import subprocess from unittest import mock import github3 @@ -6,24 +5,26 @@ 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()