Merge branch 'main' into fix-cla-bot

.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:

.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 }}

.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

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__":

reusable_workflows/tests/test_repo_policies.py

@@ -1,30 +1,32 @@
-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,
+        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()