This is an automated email from the ASF dual-hosted git repository.
400Ping pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/mahout.git
The following commit(s) were added to refs/heads/main by this push:
new e23c955cf [CI] Require two committer approvals before a PR can be
merged (#1530)
e23c955cf is described below
commit e23c955cfca32a5c69bfd394cedf2eb1ddff7dc5
Author: Jie-Kai Chang <[email protected]>
AuthorDate: Wed Oct 14 14:18:15 2026 +0800
[CI] Require two committer approvals before a PR can be merged (#1530)
Signed-off-by: 400Ping <[email protected]>
---
.asf.yaml | 2 +-
.github/scripts/check_pr_approval.py | 44 +++++++++++++++++------
.github/workflows/pr-approval-check.yml | 13 ++++---
docs/community/pr-policy-and-review-guidelines.md | 9 +++--
4 files changed, 46 insertions(+), 22 deletions(-)
diff --git a/.asf.yaml b/.asf.yaml
index c1ca308e7..3212511b5 100644
--- a/.asf.yaml
+++ b/.asf.yaml
@@ -61,7 +61,7 @@ github:
# protected_branches entry for main.
#
# PR policy (docs/community/pr-policy-and-review-guidelines.md): a PR needs
- # at least one approving review from a committer before it can be merged,
+ # at least two approving reviews from committers before it can be merged,
# unless it was opened by a PMC member (.github/pmc-members.txt).
# GitHub's own required_approving_review_count (as used by Airflow) cannot
# exempt PMC members, so the rule is enforced through the required status
diff --git a/.github/scripts/check_pr_approval.py
b/.github/scripts/check_pr_approval.py
index d3daa1fcb..53cb7e447 100644
--- a/.github/scripts/check_pr_approval.py
+++ b/.github/scripts/check_pr_approval.py
@@ -14,7 +14,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
-"""Check that a pull request has at least one approving review from a
committer.
+"""Check that a pull request has enough approving reviews from committers.
This backs the ``Committer approval`` required status check on ``main``
(see ``.github/workflows/pr-approval-check.yml`` and ``.asf.yaml``).
@@ -22,16 +22,17 @@ This backs the ``Committer approval`` required status check
on ``main``
Rules:
* A pull request opened by a PMC member (``.github/pmc-members.txt``) is
exempt.
-* Otherwise, at least one reviewer whose *latest* review is ``APPROVED`` must
have
- write access to the repository. Per ASF policy every committer has write
access,
- so "has write access" is used as the definition of "is a committer".
+* Otherwise, at least ``REQUIRED_APPROVALS`` (default 2) distinct reviewers
whose
+ *latest* review is ``APPROVED`` must have write access to the repository. Per
+ ASF policy every committer has write access, so "has write access" is used as
+ the definition of "is a committer".
* Following GitHub's own semantics, a ``COMMENTED`` review does not revoke an
earlier approval, while ``CHANGES_REQUESTED`` or a dismissal does.
Only the Python standard library is used so the script runs in CI and locally:
GITHUB_TOKEN=$(gh auth token) python .github/scripts/check_pr_approval.py
\\
- --repo apache/mahout --pr 1234
+ --repo apache/mahout --pr 1234 [--required-approvals 2]
Exit status is 0 when the PR may be merged and 1 otherwise.
"""
@@ -50,6 +51,8 @@ from typing import Any
API_URL = os.environ.get("GITHUB_API_URL", "https://api.github.com";)
DEFAULT_PMC_FILE = Path(__file__).resolve().parents[1] / "pmc-members.txt"
+# Committer approvals a non-PMC PR needs (REQUIRED_APPROVALS in the workflow).
+DEFAULT_REQUIRED_APPROVALS = 2
# Review states that change a reviewer's standing. COMMENTED is deliberately
# excluded: on GitHub a comment-only review leaves a previous approval in
place.
DECISIVE_STATES = {"APPROVED", "CHANGES_REQUESTED", "DISMISSED"}
@@ -131,7 +134,11 @@ def has_write_access(token: str, repo: str, login: str) ->
bool:
def check(
- token: str, repo: str, pr_number: int, pmc_members: set[str]
+ token: str,
+ repo: str,
+ pr_number: int,
+ pmc_members: set[str],
+ required_approvals: int = DEFAULT_REQUIRED_APPROVALS,
) -> tuple[bool, list[str]]:
"""Evaluate the rule. Returns ``(ok, log_lines)``."""
lines: list[str] = []
@@ -168,9 +175,12 @@ def check(
f"Approved by @{login} (not a committer; does not satisfy the
rule)."
)
- if committer_approvers:
+ if len(committer_approvers) >= required_approvals:
return True, lines
- lines.append("No approving review from a committer.")
+ lines.append(
+ f"{len(committer_approvers)} of {required_approvals} required
committer "
+ "approvals."
+ )
return False, lines
@@ -195,6 +205,13 @@ def main(argv: list[str] | None = None) -> int:
default=DEFAULT_PMC_FILE,
help=f"PMC member list (default: {DEFAULT_PMC_FILE})",
)
+ parser.add_argument(
+ "--required-approvals",
+ type=int,
+ default=int(os.environ.get("REQUIRED_APPROVALS",
DEFAULT_REQUIRED_APPROVALS)),
+ help="committer approvals a non-PMC PR needs "
+ f"(default: $REQUIRED_APPROVALS or {DEFAULT_REQUIRED_APPROVALS})",
+ )
args = parser.parse_args(argv)
token = os.environ.get("GITHUB_TOKEN")
@@ -202,6 +219,8 @@ def main(argv: list[str] | None = None) -> int:
parser.error(
"GITHUB_TOKEN, --repo/$GITHUB_REPOSITORY and --pr/$PR_NUMBER are
required"
)
+ if args.required_approvals < 1:
+ parser.error("--required-approvals must be at least 1")
pmc_members = load_pmc_members(args.pmc_file)
if not pmc_members:
@@ -211,15 +230,18 @@ def main(argv: list[str] | None = None) -> int:
)
try:
- ok, lines = check(token, args.repo, int(args.pr), pmc_members)
+ ok, lines = check(
+ token, args.repo, int(args.pr), pmc_members,
args.required_approvals
+ )
except GitHubError as exc:
print(f"::error::{exc}")
return 1
+ n = args.required_approvals
if ok:
- verdict = "PASS: at least one committer approval (or PMC author)."
+ verdict = f"PASS: at least {n} committer approval(s) (or PMC author)."
else:
- verdict = "FAIL: needs at least one approving review from a committer."
+ verdict = f"FAIL: needs at least {n} approving review(s) from
committers."
lines.append(verdict)
print("\n".join(lines))
diff --git a/.github/workflows/pr-approval-check.yml
b/.github/workflows/pr-approval-check.yml
index 819ab5452..011758aa6 100644
--- a/.github/workflows/pr-approval-check.yml
+++ b/.github/workflows/pr-approval-check.yml
@@ -15,11 +15,12 @@
# limitations under the License.
#
# Enforces the PR policy (docs/community/pr-policy-and-review-guidelines.md):
-# a pull request needs at least one approving review from a committer before
-# it can be merged, unless it was opened by a PMC member
+# a pull request needs at least REQUIRED_APPROVALS (2) approving reviews from
+# committers before it can be merged, unless it was opened by a PMC member
# (.github/pmc-members.txt). The "Committer approval" job is a required status
-# check on main via .asf.yaml, mirroring Airflow's
-# required_approving_review_count: 1 but with the PMC exemption.
+# check on main via .asf.yaml, mirroring GitHub's
+# required_approving_review_count: 2 but with the PMC exemption.
+# PMC members can bypass it in an emergency (main ruleset in .asf.yaml).
#
name: PR Approval Check
@@ -59,8 +60,10 @@ jobs:
.github/scripts/check_pr_approval.py
sparse-checkout-cone-mode: false
- - name: Check for a committer approval
+ - name: Check for committer approvals
env:
GITHUB_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number }}
+ # Committer approvals a non-PMC PR needs. Keep the docs in sync.
+ REQUIRED_APPROVALS: "2"
run: python3 .github/scripts/check_pr_approval.py
diff --git a/docs/community/pr-policy-and-review-guidelines.md
b/docs/community/pr-policy-and-review-guidelines.md
index fc8ab65f4..f0db6a90f 100644
--- a/docs/community/pr-policy-and-review-guidelines.md
+++ b/docs/community/pr-policy-and-review-guidelines.md
@@ -74,7 +74,7 @@ For non-trivial code contributions, contributors are expected
to have an ASF ICL
When deciding whether to merge, committers should verify all of the following:
- Required CI checks are green (enforced automatically, see below).
-- At least one committer approves (enforced automatically, see below).
+- At least two committers approve (enforced automatically, see below).
- All blocking comments are resolved.
#### Auto-merge and CI
@@ -115,13 +115,12 @@ decision.
The `Committer approval` status check
([`pr-approval-check.yml`](https://github.com/apache/mahout/blob/main/.github/workflows/pr-approval-check.yml))
is required on `main` and re-runs whenever a review is submitted, edited, or
dismissed:
-- The PR must have at least one reviewer whose latest review is an approval
and who has write access to the repository (i.e. a committer). Approvals from
non-committers are welcome but do not satisfy the check.
+- The PR must have at least **two** distinct reviewers whose latest review is
an approval and who have write access to the repository (i.e. committers).
Approvals from non-committers are welcome but do not count. The number is set
by `REQUIRED_APPROVALS` in the workflow.
- Following GitHub semantics, a comment-only review keeps an earlier approval
in place; "Request changes" or dismissing the review removes it.
- PRs opened by PMC members are exempt. The list of PMC GitHub accounts lives
in
[`.github/pmc-members.txt`](https://github.com/apache/mahout/blob/main/.github/pmc-members.txt)
and must be updated when the PMC roster changes.
+- This applies to auto-merge and manual merges alike; PMC members can bypass
it in an emergency (see above).
-The two-approval expectation for high-impact changes below is not enforced by
tooling and remains a committer responsibility.
-
-The following changes should receive at least two committer approvals and
prior public discussion (issue or `dev@` thread):
+The following changes should additionally have prior public discussion (issue
or `dev@` thread):
- API or behavior-breaking changes.
- Architecture or major dependency changes.
