Selaa lähdekoodia

Switch to a manual check status for dependent PRs (#7117)

The labeling script will now directly set a check status for the PR as
`pending` when it marks something as dependent, and clear it when it no
longer is. This emulates a check that starts when marked as dependent
and runs until the last dependency lands, allowing automerge and other
workflows to work cleanly.

The branch protection rule will have to be updated to the new spelling.

This should do the same key thing as #7113, but integrated to the new
script.

Assisted-by: Antigravity with Gemini
Chandler Carruth 4 päivää sitten
vanhempi
sitoutus
0e308e0739

+ 0 - 33
.github/workflows/check_dependent.yaml

@@ -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."

+ 1 - 0
.github/workflows/check_dependent_pr.yaml

@@ -14,6 +14,7 @@ concurrency:
 permissions:
   contents: read
   pull-requests: write
+  statuses: write
 
 jobs:
   check_dependent_prs:

+ 1 - 0
.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.

+ 55 - 6
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,
             )

+ 116 - 17
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__":