[BUG] Minimal approval workflow blocks PR merge

[shiv0408]

Describe the bug

Github Action Minimum Approval workflow blocks the PR merge if PR already has approval but a new commit is pushed. As the trigger for this action is "pull_request_review", this action is only triggered if a approval is given or removed or description is edited.

OpenSearch/.github/workflows/maintainer-approval.yml

Line 4 in 3c07461

pull_request_review:

Related component

Other

To Reproduce

1. Raise a PR
2. Get approval on the PR from a maintainer
3. Push a commit to the PR branch

Expected behavior

The workflow should run all the times we have updated the PR or after getting approval as well.

Additional Details

No response

[shiv0408]

Example #8218 currently we can't merged because this expected check is not getting triggered. So, it is neither failing or succeeding.

[shiv0408]

We should modify the trigger to use "pull_request" webhook with opened, reopened and synchronized action.
@peternied as the original author of the workflow, do you have any comments or suggestion on this?

[peternied]

I believe the root cause of the issue is that PR checks are associated with a commit SHA, whereas PR approvals are associated with a PR, this puts us in a bad state because if the PR changes, the workflow is not triggered dismiss or re-approval and it looks like the check is stalled.

Here are some possible approaches and issues they might encounter.

Use "pull_request_target" trigger

This would work, but it would only be run after new commits have been pushed. If you've got code that isn't change and then approved by a maintainer, it would not get the update.

Use both "pull_request_target" & "pull_request_review" triggers

With how the backing approval check works you'd get two maintainer-approvals checks, one for each trigger source, this would create a different and strange bottle neck where you'd need to make changes / get more approvals till they both lit up green.

[Recommendation] Separate the trigger source from the check

By decoupling the result of peternied/required-approval from the check on the PR this would allow any number of sources from restarting the check on the PR workflow which could add or updates an existing check. This requires making changes to that GitHub action.

I'd be happy to review a PR if there are other ideas, or on the required-approval GitHub action.

[peternied]

[Triage - attendees 1 2 3 4 5]
@shiv0408 Thanks for filing, there could be improvements in this space, we'd gladly review a pull request to improve.