From b5f0773589c3115eaca57ca2e16d31b41f077922 Mon Sep 17 00:00:00 2001 From: Carly Gundy Date: Mon, 9 Dec 2024 13:24:06 +0100 Subject: [PATCH 1/5] chore(IDX): updates to bot check script --- .github/workflows/repo_policies.yml | 1 + .../bot_checks/check_bot_approved_files.py | 23 +++++----- .../tests/test_repo_policies.py | 43 ++++++++++--------- 3 files changed, 35 insertions(+), 32 deletions(-) diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index ed78718..ce921c3 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 -eufo 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..342fbd5 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,8 +22,13 @@ 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], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + text=True, ) + if result.returncode != 0: + raise RuntimeError(f"git diff failed: {result.stderr}") changed_files = result.stdout.strip().split("\n") return changed_files @@ -51,7 +56,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 +69,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 +84,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..1a9adb7 100644 --- a/reusable_workflows/tests/test_repo_policies.py +++ b/reusable_workflows/tests/test_repo_policies.py @@ -6,24 +6,27 @@ 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, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, text=True, ) @@ -86,9 +89,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 +118,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() From ab2b8b4d76593510a5782b0bde634801f8785cd0 Mon Sep 17 00:00:00 2001 From: Carly Gundy Date: Mon, 9 Dec 2024 14:57:28 +0100 Subject: [PATCH 2/5] updaet shell script --- .github/workflows/repo_policies.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index ce921c3..886af08 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -21,7 +21,7 @@ jobs: - name: Bot Checks id: bot-checks run: | - set -eufo pipefail + set -euo pipefail export PYTHONPATH="$PWD/reusable_workflows/" python reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py shell: bash From d7d5d93bbd376daa471302a690053cd0e19ce6c2 Mon Sep 17 00:00:00 2001 From: Carly Gundy Date: Mon, 9 Dec 2024 14:57:36 +0100 Subject: [PATCH 3/5] udpate error --- .../repo_policies/bot_checks/check_bot_approved_files.py | 5 +---- reusable_workflows/tests/test_repo_policies.py | 3 +-- 2 files changed, 2 insertions(+), 6 deletions(-) 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 342fbd5..6436938 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,10 +22,7 @@ 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.DEVNULL, - stderr=subprocess.DEVNULL, - text=True, + ["git", "diff", "--name-only", commit_range], capture_output=True, text=True, ) if result.returncode != 0: raise RuntimeError(f"git diff failed: {result.stderr}") diff --git a/reusable_workflows/tests/test_repo_policies.py b/reusable_workflows/tests/test_repo_policies.py index 1a9adb7..8c8bb91 100644 --- a/reusable_workflows/tests/test_repo_policies.py +++ b/reusable_workflows/tests/test_repo_policies.py @@ -25,8 +25,7 @@ def test_get_changed_files(mock_subprocess_run): 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.DEVNULL, - stderr=subprocess.DEVNULL, + capture_output=True, text=True, ) From fb0125896830295484c0ee029b86e7d01b176959 Mon Sep 17 00:00:00 2001 From: Carly Gundy Date: Mon, 9 Dec 2024 15:01:06 +0100 Subject: [PATCH 4/5] remove error checking --- .../repo_policies/bot_checks/check_bot_approved_files.py | 2 -- 1 file changed, 2 deletions(-) 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 6436938..24f6093 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 @@ -24,8 +24,6 @@ def get_changed_files(merge_base_sha: str, branch_head_sha: str) -> list[str]: result = subprocess.run( ["git", "diff", "--name-only", commit_range], capture_output=True, text=True, ) - if result.returncode != 0: - raise RuntimeError(f"git diff failed: {result.stderr}") changed_files = result.stdout.strip().split("\n") return changed_files From d95591d1057f9b7e22537ec22c8d937a9c7eb7b4 Mon Sep 17 00:00:00 2001 From: Carly Gundy Date: Mon, 9 Dec 2024 15:53:05 +0100 Subject: [PATCH 5/5] lint --- .../repo_policies/bot_checks/check_bot_approved_files.py | 4 +++- reusable_workflows/tests/test_repo_policies.py | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) 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 24f6093..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], capture_output=True, text=True, + ["git", "diff", "--name-only", commit_range], + capture_output=True, + text=True, ) changed_files = result.stdout.strip().split("\n") return changed_files diff --git a/reusable_workflows/tests/test_repo_policies.py b/reusable_workflows/tests/test_repo_policies.py index 8c8bb91..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