diff --git a/.github/workflows/check_dependent.yaml b/.github/workflows/check_dependent.yaml deleted file mode 100644 index 65fc3e8de421..000000000000 --- a/.github/workflows/check_dependent.yaml +++ /dev/null @@ -1,33 +0,0 @@ -# Part of the Carbon Language project, under the Apache License v2.0 with LLVM -# Exceptions. See /LICENSE for license information. -# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - -name: Check Dependent Label -on: - pull_request_target: - types: [opened, synchronize, labeled, unlabeled] - -# This workflow runs as `pull_request_target` so that the check can't be -# disabled or bypassed by the PR, but it doesn't need any permissions. -permissions: {} - -jobs: - check_label: - runs-on: ubuntu-latest - steps: - - name: Harden Runner - uses: step-security/harden-runner@58077d3c7e43986b6b15fba718e8ea69e387dfcc # v2.15.1 - with: - disable-sudo: true - egress-policy: block - # prettier-ignore - allowed-endpoints: > - api.github.com:443 - - - name: Check for 'dependent' label - run: | - if [[ "${{ contains(github.event.pull_request.labels.*.name, 'dependent') }}" == "true" ]]; then - echo "PR has 'dependent' label. Blocking merge." - exit 1 - fi - echo "PR does not have 'dependent' label." diff --git a/.github/workflows/check_dependent_pr.yaml b/.github/workflows/check_dependent_pr.yaml index 0d7454fcc9b4..acfa2162edef 100644 --- a/.github/workflows/check_dependent_pr.yaml +++ b/.github/workflows/check_dependent_pr.yaml @@ -14,6 +14,7 @@ concurrency: permissions: contents: read pull-requests: write + statuses: write jobs: check_dependent_prs: diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 8dc763a042ee..c7eee9a243a6 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -150,6 +150,7 @@ repos: - gql >= 2.0.0, < 3.0.0 - PyGitHub - rich + - types-requests # Exclusions are: # - p#### scripts because they're not tested or maintained. # - lit.cfg.py because it has multiple copies, breaking mypy. diff --git a/github_tools/check_dependent_pr.py b/github_tools/check_dependent_pr.py index d26fc6c581d4..aef2e02d5e64 100755 --- a/github_tools/check_dependent_pr.py +++ b/github_tools/check_dependent_pr.py @@ -24,6 +24,7 @@ import json import re import os import sys +import requests from typing import Any, Optional # Do some extra work to support direct runs. @@ -238,13 +239,50 @@ def _parse_and_validate_state( return parsed_open, parsed_merged, first_commit +def _set_commit_status( + sha: str, + state: str, + description: str, + token: str, + dry_run: bool, +) -> None: + """Sets the commit status via the GitHub REST API.""" + url = ( + "https://api.github.com/repos/carbon-language/carbon-lang/" + f"statuses/{sha}" + ) + headers = { + "Authorization": f"bearer {token}", + "Accept": "application/vnd.github.v3+json", + } + payload = { + "state": state, + "description": description, + "context": "PR dependencies check", + } + if dry_run: + _print_err( + f"[Dry-run] Would set commit status on {sha[:8]} to {state} " + f"({description})" + ) + return + + try: + response = requests.post(url, headers=headers, json=payload) + response.raise_for_status() + _print_err(f"Set commit status on {sha[:8]} to {state}") + except Exception as e: + _print_err(f"Error setting commit status on {sha[:8]}: {e}") + + def _process_pr( client: github_helpers.Client, pr_number: int, pr_to_commits: dict[int, list[str]], open_pr_numbers: set[int], label_id: str, - dry_run: bool, + token: str, + dry_run: bool = False, scanning: bool = False, max_merged_pr: int = 10000, ) -> None: @@ -322,9 +360,6 @@ def _process_pr( ) ) - if not open_deps and not existing_comment_id: - return - # Keep tracking previously identified dependencies if they are still open, # even if they no longer pass the subset check (e.g. they got new commits). for pr in parsed_open_deps: @@ -339,6 +374,18 @@ def _process_pr( merged_deps = list(set(parsed_merged_deps + newly_merged_deps)) + if open_deps: + state = "pending" + pr_list_str = ", ".join([f"#{num}" for num in open_deps]) + description = f"This PR has open dependencies: {pr_list_str}" + else: + state = "success" + description = "This PR has no open dependencies" + + _set_commit_status( + pr_node["headRefOid"], state, description, token, dry_run + ) + first_independent_commit_oid = None if open_deps: dependent_oids = set() @@ -527,7 +574,8 @@ def main() -> None: pr_to_commits, open_pr_numbers, label_id, - parsed_args.dry_run, + parsed_args.access_token, + dry_run=parsed_args.dry_run, max_merged_pr=max_merged_pr, ) elif parsed_args.scan: @@ -540,7 +588,8 @@ def main() -> None: pr_to_commits, open_pr_numbers, label_id, - parsed_args.dry_run, + parsed_args.access_token, + dry_run=parsed_args.dry_run, scanning=True, max_merged_pr=max_merged_pr, ) diff --git a/github_tools/check_dependent_pr_test.py b/github_tools/check_dependent_pr_test.py index 3ac21cd1aca2..2ceddab7b163 100644 --- a/github_tools/check_dependent_pr_test.py +++ b/github_tools/check_dependent_pr_test.py @@ -24,6 +24,21 @@ _OID9 = "9" * 40 class TestCheckDependentPR(unittest.TestCase): def setUp(self) -> None: self.mock_client = mock.MagicMock(spec=github_helpers.Client) + # Mock requests.post to avoid network calls and track status updates. + self.requests_post_patcher = mock.patch("requests.post") + self.mock_post = self.requests_post_patcher.start() + + def tearDown(self) -> None: + self.requests_post_patcher.stop() + + def _assert_status(self, sha: str, state: str, description: str) -> None: + """Validates that requests.post was called to set the commit status.""" + self.mock_post.assert_called_once() + args, kwargs = self.mock_post.call_args + self.assertIn(f"statuses/{sha}", args[0]) + self.assertEqual(kwargs["json"]["state"], state) + self.assertEqual(kwargs["json"]["context"], "PR dependencies check") + self.assertEqual(kwargs["json"]["description"], description) def _make_comment( self, @@ -84,9 +99,12 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1]}, open_pr_numbers={1}, label_id="label_id", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 1) + self._assert_status( + _OID1, "success", "This PR has no open dependencies" + ) def test_process_pr_with_overlap(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -100,12 +118,15 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1], 2: [_OID1, _OID2]}, open_pr_numbers={1, 2}, label_id="label_dependent", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 3) calls = self.mock_client.execute.call_args_list self.assertIn("addLabelsToLabelable", calls[1][0][0]) self.assertIn("addComment", calls[2][0][0]) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_dependencies_merged(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -121,11 +142,14 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={3: [_OID1, _OID2]}, open_pr_numbers={3}, label_id="label_dependent", - dry_run=False, + token="test_token", ) calls = self.mock_client.execute.call_args_list self.assertIn("removeLabelsFromLabelable", calls[1][0][0]) self.assertIn("updateIssueComment", calls[2][0][0]) + self._assert_status( + _OID2, "success", "This PR has no open dependencies" + ) def test_process_pr_dependency_got_new_commits(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -141,7 +165,7 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1, _OID4], 3: [_OID1, _OID2]}, open_pr_numbers={1, 3}, label_id="label_dependent", - dry_run=False, + token="test_token", ) calls = self.mock_client.execute.call_args_list update_mutation = calls[1][0][0] @@ -149,6 +173,9 @@ class TestCheckDependentPR(unittest.TestCase): variable_values = calls[1][1]["variable_values"] self.assertIn('"open": [1]', variable_values["body"]) self.assertIn('"merged": [2]', variable_values["body"]) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_non_coherent_prefix(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -162,9 +189,12 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={10: [_OID1, _OID2], 11: [_OID1, _OID3]}, open_pr_numbers={10, 11}, label_id="label_dependent", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 1) + self._assert_status( + _OID2, "success", "This PR has no open dependencies" + ) def test_process_pr_overlap_only_on_head_ref(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -178,12 +208,15 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID2], 9: [_OID1, _OID2]}, open_pr_numbers={1, 9}, label_id="label_dependent", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 3) calls = self.mock_client.execute.call_args_list self.assertIn("addLabelsToLabelable", calls[1][0][0]) self.assertIn("addComment", calls[2][0][0]) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_scanning_no_add(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -197,10 +230,13 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1], 7: [_OID1, _OID2]}, open_pr_numbers={1, 7}, label_id="label_dependent", - dry_run=False, + token="test_token", scanning=True, ) self.assertEqual(self.mock_client.execute.call_count, 1) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_no_changes_needed(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -216,9 +252,12 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1], 6: [_OID1, _OID2]}, open_pr_numbers={1, 6}, label_id="label_dependent", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 1) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_invalid_marker(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -242,7 +281,7 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={5: [_OID1]}, open_pr_numbers={5}, label_id="label_dependent", - dry_run=False, + token="test_token", ) def test_process_pr_hidden_comment(self) -> None: @@ -265,11 +304,14 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1], 14: [_OID1, _OID2]}, open_pr_numbers={1, 14}, label_id="label_dependent", - dry_run=False, + token="test_token", ) calls = self.mock_client.execute.call_args_list self.assertEqual(self.mock_client.execute.call_count, 2) self.assertIn("addComment", calls[1][0][0]) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_sticky_first_commit(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -285,12 +327,15 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1], 11: [_OID1, _OID2, _OID3]}, open_pr_numbers={1, 11}, label_id="label_dependent", - dry_run=False, + token="test_token", ) calls = self.mock_client.execute.call_args_list variable_values = calls[1][1]["variable_values"] self.assertIn(_OID2[:8], variable_values["body"]) self.assertNotIn(_OID1[:8], variable_values["body"]) + self._assert_status( + _OID3, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_rebase_first_commit(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -306,11 +351,14 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID9], 12: [_OID1, _OID2]}, open_pr_numbers={1, 12}, label_id="label_dependent", - dry_run=False, + token="test_token", ) calls = self.mock_client.execute.call_args_list variable_values = calls[1][1]["variable_values"] self.assertIn(_OID1[:8], variable_values["body"]) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_fallback_no_independent_commit(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -326,13 +374,16 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1, _OID2], 13: [_OID1, _OID2]}, open_pr_numbers={1, 13}, label_id="label_dependent", - dry_run=False, + token="test_token", ) calls = self.mock_client.execute.call_args_list variable_values = calls[1][1]["variable_values"] self.assertIn( "unable to identify starting review commit", variable_values["body"] ) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) def test_process_pr_sequence_failure(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -346,9 +397,12 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1], 2: [_OID1, _OID2]}, open_pr_numbers={1, 2}, label_id="label_dependent", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 1) + self._assert_status( + _OID1, "success", "This PR has no open dependencies" + ) def test_process_pr_no_overlap_different_commits(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -362,9 +416,12 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1], 2: [_OID2]}, open_pr_numbers={1, 2}, label_id="label_dependent", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 1) + self._assert_status( + _OID2, "success", "This PR has no open dependencies" + ) def test_process_pr_no_unique_commit(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -378,9 +435,12 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1, _OID2, _OID3], 2: [_OID1, _OID2]}, open_pr_numbers={1, 2}, label_id="label_dependent", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 1) + self._assert_status( + _OID2, "success", "This PR has no open dependencies" + ) def test_process_pr_multiple_non_overlapping_commits(self) -> None: self.mock_client.execute.return_value = self._make_pr_response( @@ -394,11 +454,50 @@ class TestCheckDependentPR(unittest.TestCase): pr_to_commits={1: [_OID1, _OID2], 2: [_OID1, _OID3, _OID4]}, open_pr_numbers={1, 2}, label_id="label_dependent", - dry_run=False, + token="test_token", ) self.assertEqual(self.mock_client.execute.call_count, 3) calls = self.mock_client.execute.call_args_list self.assertIn("addLabelsToLabelable", calls[1][0][0]) + self._assert_status( + _OID4, "pending", "This PR has open dependencies: #1" + ) + + def test_always_sets_status_check_success(self) -> None: + self.mock_client.execute.return_value = self._make_pr_response( + pr_id="pr_1", + head_ref_oid=_OID1, + commits=[_OID1], + ) + check_dependent_pr._process_pr( + self.mock_client, + pr_number=1, + pr_to_commits={1: [_OID1]}, + open_pr_numbers={1}, + label_id="label_id", + token="test_token", + ) + self._assert_status( + _OID1, "success", "This PR has no open dependencies" + ) + + def test_always_sets_status_check_pending(self) -> None: + self.mock_client.execute.return_value = self._make_pr_response( + pr_id="pr_2", + head_ref_oid=_OID2, + commits=[_OID1, _OID2], + ) + check_dependent_pr._process_pr( + self.mock_client, + pr_number=2, + pr_to_commits={1: [_OID1], 2: [_OID1, _OID2]}, + open_pr_numbers={1, 2}, + label_id="label_dependent", + token="test_token", + ) + self._assert_status( + _OID2, "pending", "This PR has open dependencies: #1" + ) if __name__ == "__main__":