search

opensearch-project / OpenSearch

🔎 Open source distributed and RESTful search engine.
https://opensearch.org/docs/latest/opensearch/index/
Apache License 2.0
9.61k stars 1.76k forks source link

[BUG] Unable to merge even after maintainers approval #11864

Open deshsidd opened 8 months ago

deshsidd commented 8 months ago

Describe the bug

Problem PR: https://github.com/opensearch-project/OpenSearch/pull/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):

Additional context Add any other context about the problem here.

dblock commented 8 months ago

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

msfroh commented 8 months ago

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 commented 8 months ago

@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

deshsidd commented 8 months ago

Thanks for the followup @peternied @msfroh

andrross commented 8 months ago

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 commented 7 months ago

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

peternied commented 7 months ago

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 commented 3 months ago

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 image

Can't we enable those options instead?

andrross commented 3 months ago

@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 commented 3 months ago

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.