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 ed78718..f8275b4 100644 --- a/.github/workflows/repo_policies.yml +++ b/.github/workflows/repo_policies.yml @@ -10,19 +10,32 @@ 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 + with: + path: current-repo # need to specify another path to avoid overwriting the first checkout + ref: ${{ github.head_ref }} + fetch-depth: 50 - name: Python Setup - uses: ./.github/workflows/python-setup + uses: ./public-workflows/.github/workflows/python-setup + with: + working-directory: public-workflows - name: Bot Checks id: bot-checks run: | - export PYTHONPATH="$PWD/reusable_workflows/" - python reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py + set -euo pipefail + 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 }} @@ -31,3 +44,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/check_cla/check_cla_pr.py b/reusable_workflows/check_cla/check_cla_pr.py index 06b139c..dde810e 100644 --- a/reusable_workflows/check_cla/check_cla_pr.py +++ b/reusable_workflows/check_cla/check_cla_pr.py @@ -66,24 +66,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/repo_policies/bot_checks/check_bot_approved_files.py b/reusable_workflows/repo_policies/bot_checks/check_bot_approved_files.py index aca0690..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 @@ -1,4 +1,5 @@ import subprocess +from typing import Optional import github3 @@ -11,19 +12,29 @@ "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. """ 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, + cwd=repo_path, ) + 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 @@ -51,42 +62,46 @@ 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. """ 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) 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: - 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: 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: - 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) + print(f"{user} is not a bot. Skipping bot checks.") if __name__ == "__main__": diff --git a/reusable_workflows/tests/test_cla_pr.py b/reusable_workflows/tests/test_cla_pr.py index fe69cff..1ab3621 100644 --- a/reusable_workflows/tests/test_cla_pr.py +++ b/reusable_workflows/tests/test_cla_pr.py @@ -151,7 +151,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(): @@ -167,22 +166,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" @@ -197,17 +193,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"}, diff --git a/reusable_workflows/tests/test_repo_policies.py b/reusable_workflows/tests/test_repo_policies.py index a835012..1ade7a8 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,25 +5,28 @@ 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, + cwd=None, ) @@ -73,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", } @@ -86,10 +89,9 @@ 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_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", } @@ -116,42 +119,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_changed_files.assert_called_once_with("base", "head", "path") 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. Skipping bot checks." + in captured.out ) - pr_is_blocked.assert_not_called() + check_if_pr_is_blocked.assert_not_called()