Use latest review state in maintainer-approval - #5323
fallintoplace wants to merge 2 commits into
Conversation
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
| // GitHub returns the full review history, so we collapse it to each user's | ||
| // latest state before evaluating whether the PR is currently approved. | ||
| function latestReviewsByUser(reviews) { |
There was a problem hiding this comment.
this cannot just take all reviews.entries() but must only consider opinionated reviews (similar to GitHub's latestOpinionatedReviews API), i.e. APPROVED or CHANGES_REQUESTED.
Otherwise, an APPROVED followed by a COMMENTED will undo the approval. Needs to be covered by a unit test as well.
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
|
Would you mind taking another look when you have a chance? |
|
Superseded by #6709 |
2 participants
Summary
Why
The workflow currently treats historical
APPROVEDreviews as current approval. If a reviewer later switches toCHANGES_REQUESTED, the older approval can still satisfy the check.Fixes #5322.
Test plan
node --test .github/scripts/owners.test.js .github/workflows/maintainer-approval.test.js