[BUG] Unable to merge even after maintainers approval

[deshsidd]

Describe the bug

Problem PR: #11582

For the above PR, even after getting a maintainers approval (@msfroh ), we were unable to merge the PR as the Minimum approval count check did not pass.

We had to dismiss and re-review to make it pass. Let's please fix this for future contributors.

Related component

Other

To Reproduce

1. Create a PR
2. Approve by maintainer
3. Will not be allowed to merge
4. Dismiss the approval and re-approve will now work

Not sure when this happens and if it happens every time. Might need to do some testing.

Expected behavior

Approval by maintainer should pass the Minimum approval count check.

Additional Details

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

• OS: [e.g. iOS]
• Version [e.g. 22]

Additional context
Add any other context about the problem here.

[dblock]

@peternied was this an issue in an action or something wasn't dismissed?

[msfroh]

I could be mistaken, but I think I've noticed a pattern. If a maintainer approves, then the PR is modified (e.g. add a commit, rebase on main), the action restarts and waits for approval again.

Since that forces approval on the latest version of the PR, it might not really be a bug.

In particular, it helps us avoid the following scenario (that was previously possible, I think):

1. Contributor submits a PR.
2. Maintainer reviews and approves the PR, but doesn't merge right away (maybe retrying Gradle check).
3. Contributor adds another commit that is somehow harmful (malicious, broken, etc).
4. Maintainer returns to the PR, sees the big green checkmark (since they previously approved and now the Gradle checks passed), and merges the PR (without noticing the extra unreviewed commit).

IMO, we should keep an eye out for "failures" of the action to see if they only happen when there are changes after approval. If so, then I think it's a good thing.

[peternied]

@dblock There was an issue with the only way to trigger the 'maintainer approval' was by a
'new approval', where a previous approval was dismissed, non-intuitive. There was a small change in [1] that allows the action to be retriggered on review action.

I'll keep an eye on this issue, and if I haven't heard of any funkiness in the next 2 weeks I'll close on or after the 1/31

• [1] Bump peternied/required-approval from v1.2 to v1.3 #11820

[deshsidd]

Thanks for the followup @peternied @msfroh

[andrross]

I could be mistaken, but I think I've noticed a pattern. If a maintainer approves, then the PR is modified (e.g. add a commit, rebase on main), the action restarts and waits for approval again.

@msfroh I've noticed this behavior and also agree this is desirable. I was always surprised that the previous logic would accept an approval against an old revision of the code given that arbitrary changes could have been made since then.

[bbarani]

@peternied @andrross Can we close this issue? Can you please provide an update?

[peternied]

I don't think we should close this issue, we've seen some behavior that is outside user expectation. There is no harm in leaving this open IMO.

[gaiksaya]

Wondering if we really need this workflow. We have an option in GitHub to dismiss old reviews when a code change is pushed as well as minimum approval count before merging the PR

Can't we enable those options instead?

[andrross]

@gaiksaya You're referring to the maintainer-approval workflow, right? I don't know the history of that workflow, but on the surface it looks like it is duplicating GitHub functionality and seems like it should not be needed. @peternied Why did we need a custom workflow for this?

[peternied]

This is a new approval check that effectively doubles as the existing CODEOWNERS checks on this repository - which will allow that code owners check to be removed in favor of this one.

This check pulls the list of maintainers from GitHub repository settings. This also codifies the number of approvals needed in the repository in the event we increase or alter these requirements.

• From Maintainer approval check #11378

• Background on why this check was added in the first place [RFC] [Bug] New pull request automatically assigns and notifies every maintainer. #10613