From 8abb53747cfc9a574c3b24143134423f4ad043bb Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 9 Dec 2024 16:08:01 +0100 Subject: [PATCH 01/11] chore(IDX): updates to bot check script (#76) * chore(IDX): updates to bot check script * updaet shell script * udpate error * remove error checking * lint --- .github/workflows/repo_policies.yml | 1 + .../bot_checks/check_bot_approved_files.py | 20 ++++----- .../tests/test_repo_policies.py | 43 +++++++++---------- 3 files changed, 31 insertions(+), 33 deletions(-) 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() From 23dbe615201047dcf43804a81514358b44bbe941 Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 9 Dec 2024 16:46:41 +0100 Subject: [PATCH 02/11] chore(IDX): fix bot workflow (#79) * chore(IDX): fix bot workflow * add error message --- .github/workflows/repo_policies.yml | 2 -- .../repo_policies/bot_checks/check_bot_approved_files.py | 4 ++++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index 886af08..a333f40 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -12,8 +12,6 @@ jobs: steps: - name: Checkout uses: actions/checkout@v4 - with: - repository: dfinity/public-workflows - name: Python Setup uses: ./.github/workflows/python-setup 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 297afaa..b3f47dc 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 @@ -26,6 +26,8 @@ def get_changed_files(merge_base_sha: str, branch_head_sha: str) -> list[str]: capture_output=True, text=True, ) + if result.returncode != 0: + raise RuntimeError(f"git diff failed with exit code {result.returncode}: {result.stderr}") changed_files = result.stdout.strip().split("\n") return changed_files @@ -65,6 +67,8 @@ def check_if_pr_is_blocked(env_vars: dict) -> None: config = get_approved_files_config(repo) approved_files = get_approved_files(config) block_pr = not all(file in approved_files for file in changed_files) + print(f"changed_files: {changed_files}") + print(f"approved_files: {approved_files}") if block_pr: 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. From f889663493d6f08f4c44ef4bb73f435967a659ca Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 9 Dec 2024 16:55:05 +0100 Subject: [PATCH 03/11] fix(IDX): try two checkout steps (#80) --- .github/workflows/repo_policies.yml | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index a333f40..558f4c2 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -10,18 +10,26 @@ jobs: # Dont run this workflow on merge queue if: ${{ github.event_name != 'merge_group' }} steps: + # First check out code from public-workflows - name: Checkout uses: actions/checkout@v4 + with: + repository: dfinity/public-workflows + path: public-workflows + + # Then switch back to this repository to make sure it's run from current + - name: Checkout Original Repository + uses: actions/checkout@v4 - name: Python Setup - uses: ./.github/workflows/python-setup + uses: ./public-workflows/.github/workflows/python-setup - 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 + export PYTHONPATH="$PWD/public-workflows/reusable_workflows/" + python public-workflows/reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py shell: bash env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} From cd39b63062ec0ebb14bd545d952bbc96dc2b093c Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 9 Dec 2024 17:29:46 +0100 Subject: [PATCH 04/11] fix(IDX): enable multi-repo checkout (#82) * fix(IDX): enable multi-repo checkout * test * add working dir * Update repo_policies_ruleset.yml * replace default * always run cla dev check * update comment --- .github/workflows/check_cla_dev.yml | 5 ----- .github/workflows/python-setup/action.yml | 6 ++++++ .github/workflows/repo_policies.yml | 4 ++++ 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/.github/workflows/check_cla_dev.yml b/.github/workflows/check_cla_dev.yml index ea1e3a9..27bba88 100644 --- a/.github/workflows/check_cla_dev.yml +++ b/.github/workflows/check_cla_dev.yml @@ -4,11 +4,6 @@ name: CLA Check Dev on: pull_request: - paths: - - .github/workflows/check_cla.yml - - .github/workflows/check_cla_dev.yml - - reusable_workflows/check_cla/** - - reusable_workflows/check_membership/** jobs: call-check-cla: diff --git a/.github/workflows/python-setup/action.yml b/.github/workflows/python-setup/action.yml index 385fbd5..c014c72 100644 --- a/.github/workflows/python-setup/action.yml +++ b/.github/workflows/python-setup/action.yml @@ -1,5 +1,10 @@ name: Python Setup description: Installs Python and necessary dependencies +inputs: + working-directory: + description: 'The path to the workspace' + required: false + default: ${{ github.workspace }} runs: using: composite @@ -12,3 +17,4 @@ runs: - name: Install Dependencies run: pip install -r requirements.txt shell: bash + working-directory: ${{ inputs.working-directory }} diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index 558f4c2..b47e3e6 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -20,9 +20,13 @@ jobs: # Then switch back to this repository to make sure it's run from current - name: Checkout Original Repository uses: actions/checkout@v4 + with: + path: current-repo # need to specify another path to avoid overwriting the first checkout - name: Python Setup uses: ./public-workflows/.github/workflows/python-setup + with: + working-directory: public-workflows - name: Bot Checks id: bot-checks From 28a8a2850e3b0426b8d232a1e2b34b3247c90bc8 Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 9 Dec 2024 18:31:08 +0100 Subject: [PATCH 05/11] fix(IDX): run git commands in a different dir (#83) * fix(IDX): run git commands in a different dir * fix * fix * test * switch back to main --- .github/workflows/repo_policies.yml | 1 + .../repo_policies/bot_checks/check_bot_approved_files.py | 8 ++++++-- reusable_workflows/tests/test_repo_policies.py | 7 +++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index b47e3e6..04b0d91 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -42,3 +42,4 @@ jobs: REPO: ${{ github.event.repository.name }} MERGE_BASE_SHA: ${{ github.event.pull_request.base.sha }} BRANCH_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + REPO_PATH: current-repo 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 b3f47dc..b68f94b 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 @@ -1,4 +1,5 @@ import subprocess +from typing import Optional import github3 @@ -11,12 +12,13 @@ "GH_TOKEN", "GH_ORG", "REPO", + "REPO_PATH", "MERGE_BASE_SHA", "BRANCH_HEAD_SHA", ] -def get_changed_files(merge_base_sha: str, branch_head_sha: str) -> list[str]: +def get_changed_files(merge_base_sha: str, branch_head_sha: str, repo_path: Optional[str] = None) -> list[str]: """ Compares the files changed in the current branch to the merge base. """ @@ -25,6 +27,7 @@ def get_changed_files(merge_base_sha: str, branch_head_sha: str) -> list[str]: ["git", "diff", "--name-only", commit_range], capture_output=True, text=True, + cwd=repo_path, ) if result.returncode != 0: raise RuntimeError(f"git diff failed with exit code {result.returncode}: {result.stderr}") @@ -61,8 +64,9 @@ def check_if_pr_is_blocked(env_vars: dict) -> None: """ gh = github3.login(token=env_vars["GH_TOKEN"]) repo = gh.repository(owner=env_vars["GH_ORG"], repository=env_vars["REPO"]) + repo_path = env_vars["REPO_PATH"] changed_files = get_changed_files( - env_vars["MERGE_BASE_SHA"], env_vars["BRANCH_HEAD_SHA"] + env_vars["MERGE_BASE_SHA"], env_vars["BRANCH_HEAD_SHA"], repo_path ) config = get_approved_files_config(repo) approved_files = get_approved_files(config) diff --git a/reusable_workflows/tests/test_repo_policies.py b/reusable_workflows/tests/test_repo_policies.py index 3dcead3..3b607be 100644 --- a/reusable_workflows/tests/test_repo_policies.py +++ b/reusable_workflows/tests/test_repo_policies.py @@ -26,6 +26,7 @@ def test_get_changed_files(mock_subprocess_run): ["git", "diff", "--name-only", "merge_base_sha..branch_head_sha"], capture_output=True, text=True, + cwd=None, ) @@ -74,6 +75,7 @@ def test_pr_is_blocked_false(gh_login, get_approved_files_config, get_changed_fi "GH_TOKEN": "token", "GH_ORG": "org", "REPO": "repo", + "REPO_PATH": "path", "MERGE_BASE_SHA": "base", "BRANCH_HEAD_SHA": "head", } @@ -89,7 +91,7 @@ def test_pr_is_blocked_false(gh_login, get_approved_files_config, get_changed_fi check_if_pr_is_blocked(env_vars) - get_changed_files.assert_called_once_with("base", "head") + get_changed_files.assert_called_once_with("base", "head", "path") get_approved_files_config.assert_called_once_with(repo) @@ -103,6 +105,7 @@ def test_pr_is_blocked_true(gh_login, get_approved_files_config, get_changed_fil "GH_TOKEN": "token", "GH_ORG": "org", "REPO": "repo", + "REPO_PATH": "path", "MERGE_BASE_SHA": "base", "BRANCH_HEAD_SHA": "head", } @@ -119,7 +122,7 @@ def test_pr_is_blocked_true(gh_login, get_approved_files_config, get_changed_fil with pytest.raises(SystemExit): check_if_pr_is_blocked(env_vars) - get_changed_files.assert_called_once_with("base", "head") + get_changed_files.assert_called_once_with("base", "head", "path") get_approved_files_config.assert_called_once_with(repo) From 838f7b9eb6e8fd2e86f19642e056c5ce24d4f06c Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 9 Dec 2024 18:56:29 +0100 Subject: [PATCH 06/11] chore(IDX): make sure correct branch is checked out (#84) * chore(IDX): make sure correct branch is checked out * disable part of test * update * fix * lint --- .github/workflows/repo_policies.yml | 1 + .../bot_checks/check_bot_approved_files.py | 13 +++++++++++-- reusable_workflows/tests/test_repo_policies.py | 13 +++++++------ 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index 04b0d91..6a9f0d6 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -22,6 +22,7 @@ jobs: uses: actions/checkout@v4 with: path: current-repo # need to specify another path to avoid overwriting the first checkout + ref: ${{ github.head_ref }} - name: Python Setup uses: ./public-workflows/.github/workflows/python-setup 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 b68f94b..253b7ba 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 @@ -18,11 +18,18 @@ ] -def get_changed_files(merge_base_sha: str, branch_head_sha: str, repo_path: Optional[str] = None) -> list[str]: +def get_changed_files( + merge_base_sha: str, branch_head_sha: str, repo_path: Optional[str] = None +) -> list[str]: """ Compares the files changed in the current branch to the merge base. """ commit_range = f"{merge_base_sha}..{branch_head_sha}" + # debug + current_branch = subprocess.run( + ["git", "branch"], capture_output=True, text=True, cwd=repo_path + ) + print(f"current branch: {current_branch.stdout}") result = subprocess.run( ["git", "diff", "--name-only", commit_range], capture_output=True, @@ -30,7 +37,9 @@ def get_changed_files(merge_base_sha: str, branch_head_sha: str, repo_path: Opti cwd=repo_path, ) if result.returncode != 0: - raise RuntimeError(f"git diff failed with exit code {result.returncode}: {result.stderr}") + raise RuntimeError( + f"git diff failed with exit code {result.returncode}: {result.stderr}" + ) 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 3b607be..ef678b9 100644 --- a/reusable_workflows/tests/test_repo_policies.py +++ b/reusable_workflows/tests/test_repo_policies.py @@ -22,12 +22,13 @@ def test_get_changed_files(mock_subprocess_run): 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"], - capture_output=True, - text=True, - cwd=None, - ) + # temporarily disable while debugging + # mock_subprocess_run.assert_called_once_with( + # ["git", "diff", "--name-only", "merge_base_sha..branch_head_sha"], + # capture_output=True, + # text=True, + # cwd=None, + # ) @mock.patch("repo_policies.bot_checks.check_bot_approved_files.download_gh_file") From dd2731714f0b34208bbab6d87c2d435932a9683a Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 9 Dec 2024 19:10:42 +0100 Subject: [PATCH 07/11] chore(IDX): add fetch-depth (#85) --- .github/workflows/repo_policies.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index 6a9f0d6..03eef62 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -23,6 +23,7 @@ jobs: with: path: current-repo # need to specify another path to avoid overwriting the first checkout ref: ${{ github.head_ref }} + fetch-depth: 256 - name: Python Setup uses: ./public-workflows/.github/workflows/python-setup From 3c7cfee442658deb5f7e0443f96286c256c22344 Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Mon, 9 Dec 2024 19:17:01 +0100 Subject: [PATCH 08/11] chore(IDX): remove debug (#86) --- .../bot_checks/check_bot_approved_files.py | 5 ----- reusable_workflows/tests/test_repo_policies.py | 13 ++++++------- 2 files changed, 6 insertions(+), 12 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 253b7ba..4888cc1 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 @@ -25,11 +25,6 @@ def get_changed_files( Compares the files changed in the current branch to the merge base. """ commit_range = f"{merge_base_sha}..{branch_head_sha}" - # debug - current_branch = subprocess.run( - ["git", "branch"], capture_output=True, text=True, cwd=repo_path - ) - print(f"current branch: {current_branch.stdout}") result = subprocess.run( ["git", "diff", "--name-only", commit_range], capture_output=True, diff --git a/reusable_workflows/tests/test_repo_policies.py b/reusable_workflows/tests/test_repo_policies.py index ef678b9..3b607be 100644 --- a/reusable_workflows/tests/test_repo_policies.py +++ b/reusable_workflows/tests/test_repo_policies.py @@ -22,13 +22,12 @@ def test_get_changed_files(mock_subprocess_run): changed_files = get_changed_files("merge_base_sha", "branch_head_sha") assert changed_files == ["file1.py", "file2.py"] - # temporarily disable while debugging - # mock_subprocess_run.assert_called_once_with( - # ["git", "diff", "--name-only", "merge_base_sha..branch_head_sha"], - # capture_output=True, - # text=True, - # cwd=None, - # ) + mock_subprocess_run.assert_called_once_with( + ["git", "diff", "--name-only", "merge_base_sha..branch_head_sha"], + capture_output=True, + text=True, + cwd=None, + ) @mock.patch("repo_policies.bot_checks.check_bot_approved_files.download_gh_file") From 9db3e73376e741ac50c95eda97b8305fb30116a0 Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Tue, 10 Dec 2024 09:25:07 +0100 Subject: [PATCH 09/11] chore(IDX): small updates (#87) * chore(IDX): small updates * update test --- .github/workflows/repo_policies.yml | 2 +- .../repo_policies/bot_checks/check_bot_approved_files.py | 2 +- reusable_workflows/tests/test_repo_policies.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/repo_policies.yml b/.github/workflows/repo_policies.yml index 03eef62..f8275b4 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -23,7 +23,7 @@ jobs: with: path: current-repo # need to specify another path to avoid overwriting the first checkout ref: ${{ github.head_ref }} - fetch-depth: 256 + fetch-depth: 50 - name: Python Setup uses: ./public-workflows/.github/workflows/python-setup 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 4888cc1..b1dd7f4 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 @@ -96,7 +96,7 @@ def main() -> None: check_if_pr_is_blocked(env_vars) else: - print(f"{user} is not a bot. Letting CLA check handle contribution decision.") + print(f"{user} is not a bot. Skipping bot checks.") if __name__ == "__main__": diff --git a/reusable_workflows/tests/test_repo_policies.py b/reusable_workflows/tests/test_repo_policies.py index 3b607be..1ade7a8 100644 --- a/reusable_workflows/tests/test_repo_policies.py +++ b/reusable_workflows/tests/test_repo_policies.py @@ -153,7 +153,7 @@ def test_main_not_a_bot(check_if_pr_is_blocked, is_approved_bot, load_env_vars, captured = capfd.readouterr() assert ( - "user is not a bot. Letting CLA check handle contribution decision." + "user is not a bot. Skipping bot checks." in captured.out ) check_if_pr_is_blocked.assert_not_called() From cdde4f9d60df35651fe791cdd2b2e15a78a2a6b1 Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:59:28 +0100 Subject: [PATCH 10/11] chore(IDX): skip checks from dependabot (#88) --- .../repo_policies/bot_checks/check_bot_approved_files.py | 5 +++++ 1 file changed, 5 insertions(+) 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 b1dd7f4..901b856 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 @@ -90,6 +90,11 @@ def main() -> None: env_vars = load_env_vars(REQUIRED_ENV_VARS) user = env_vars["USER"] + # For now skip checks from dependabot until we decide how to handle them + if user == "dependabot[bot]": + print("Skipping checks for dependabot.") + return + is_bot = is_approved_bot(user) if is_bot: From 768965ff8bda493026c7eaefdb92434b9f80c657 Mon Sep 17 00:00:00 2001 From: Carly Gundy <47304080+cgundy@users.noreply.github.com> Date: Thu, 12 Dec 2024 10:49:41 +0100 Subject: [PATCH 11/11] fix(IDX): remove pending label (#77) * wip * remove --- reusable_workflows/check_cla/check_cla_pr.py | 16 +++++++-------- reusable_workflows/tests/test_cla_pr.py | 21 +++----------------- 2 files changed, 10 insertions(+), 27 deletions(-) diff --git a/reusable_workflows/check_cla/check_cla_pr.py b/reusable_workflows/check_cla/check_cla_pr.py index f474046..f6f9533 100644 --- a/reusable_workflows/check_cla/check_cla_pr.py +++ b/reusable_workflows/check_cla/check_cla_pr.py @@ -69,24 +69,22 @@ def create_cla_issue(self, user: str, pr_url: str) -> GHIssue: user, self.cla_link, user_agreement_message, pr_url ), ) - issue.add_labels(PENDING_LABEL) return issue def handle_cla_signed(self, issue: GHIssue, user: str) -> None: for label in issue.original_labels: if label.name == APPROVED_LABEL: return + + # if a pending label exists, remove it for pending_label in [GH_WORKFLOW_LABEL, PENDING_LABEL]: if label.name == pending_label: - agreement_message = messages.AGREED_MESSAGE.format(user) - issue.create_comment(agreement_message) issue.remove_label(pending_label) - issue.add_labels(APPROVED_LABEL) - return - print( - "No cla labels found - manually check the cla issue to see what state it is in. Exiting program." # noqa - ) - sys.exit(1) + + # once all pending labels have been removed and no approved label was found, add the agreement message with an approved label + agreement_message = messages.AGREED_MESSAGE.format(user) + issue.create_comment(agreement_message) + issue.add_labels(APPROVED_LABEL) def main() -> None: diff --git a/reusable_workflows/tests/test_cla_pr.py b/reusable_workflows/tests/test_cla_pr.py index dcf02e6..65bff83 100644 --- a/reusable_workflows/tests/test_cla_pr.py +++ b/reusable_workflows/tests/test_cla_pr.py @@ -142,7 +142,6 @@ def test_create_cla_issue(): "cla: @username", body=cla_agreement_message, ) - issue.add_labels.assert_called_with("cla:pending") def test_handle_cla_signed_with_agreed_label(): @@ -158,22 +157,19 @@ def test_handle_cla_signed_with_agreed_label(): issue.remove_label.assert_not_called() -def test_handle_cla_signed_with_pending_label(): +def test_handle_cla_signed_with_no_label(): issue = mock.Mock() - label = mock.Mock() - label.name = "cla:gh-wf-pending" - issue.original_labels = [label] + issue.original_labels = [] agreement_message = AGREED_MESSAGE.format("username") cla = CLAHandler(mock.Mock()) cla.handle_cla_signed(issue, "username") issue.create_comment.assert_called_with(agreement_message) - issue.remove_label.assert_called_once() issue.add_labels.assert_called_once() -def test_handle_cla_signed_with_new_pending_label(): +def test_handle_cla_signed_with_old_pending_label(): issue = mock.Mock() label = mock.Mock() label.name = "cla:pending" @@ -188,17 +184,6 @@ def test_handle_cla_signed_with_new_pending_label(): issue.add_labels.assert_called_once() -def test_handle_cla_signed_with_no_label(capfd): - issue = mock.Mock() - issue.original_labels = [] - - with pytest.raises(SystemExit): - cla = CLAHandler(mock.Mock()) - cla.handle_cla_signed(issue, "username") - out, err = capfd.readouterr() - assert out == "No cla labels found - manually check the cla issue to see what state it is in. Exiting program.\n" # fmt: skip - - @mock.patch.dict( os.environ, {"GH_ORG": "my_org", "GH_TOKEN": "secret", "REPO": "repo-name", "PR_ID": "1"},