feat(governance): enforce author-dependent Governance Council approval rules - #59
feat(governance): enforce author-dependent Governance Council approval rules#59carolinerg1 wants to merge 7 commits into
Conversation
9b109e2 to
f783e45
Compare
| is_satisfied = approved_count >= effective_req.min_approvals | ||
| if ( | ||
| require_total_approvals is not None | ||
| and len(approver_usernames) < require_total_approvals |
There was a problem hiding this comment.
len(approver_usernames) < require_total_approvals
Can you please double check and confirm the logic here?
I haven't looked too closely but I think there may be an edge-case bug here: approver_usernames can contain non-approving reviewers (e.g., users who only commented or requested changes), causing len(approver_usernames) to pass even when there isn't a second actual approval.
To fix this, we should count distinct valid approvals (excluding the PR author):
total_valid_approvals = len([u for u in approver_usernames if u != author])
if total_valid_approvals < require_total_approvals:
is_satisfied = FalseThere was a problem hiding this comment.
Thanks for double-checking @amithanda - that’s actually already handled upstream in _get_all_approvers_and_assigned_usernames():
approver_usernamesis only populated from reviews wherestate == ReviewState.APPROVED(commenters and change requestors are placed intoassigned_usernamesinstead).- The author is already filtered out there (
if user != pr.author), soapprover_usernamesis guaranteed to contain only distinct, non-author approving reviewers.
Because of that, len(approver_usernames) accurately reflects the total number of distinct valid approvals on the PR.
Description
Simplify the Governance Council (GC) dynamic approval rules in
pr_validator.pyto match the new policy.min_approvalsfrom the repository's rules configuration is respectedCategory (Required)
Please select one or more categories that apply to this change.
ucp-schematool (resolver, linter, validator). (Requires Maintainer approval)Related Issues
Checklist
!for breaking changes).Screenshots / Logs (if applicable)