Do not merge PRs with unresolved conversations

[evancharlton]

It would be great if there was a way for me to configure to only merge when all PR conversations have been marked as resolved. The use case here is that I will often find a typo during review, leave a comment, and approve the PR. I don't want bulldozer to bulldoze it on in immediately, but once they mark the conversation resolved (or outdated) it should be merged in.

[bluekeyes]

An alternative approach to this is to find a separate bot that converts comment resolution status into a commit status check that can be required using the existing Bulldozer features. A quick search didn't find anything like this, but I don't think it would be hard to build and would provide good signal about the state of the pull request.

That said, I recognize that running an additional service isn't always an option. In that case, adding something like this to Bulldozer could work, although I wouldn't want it to be on by default: it seems easy to forget to resolve a conversation and then wonder why your pull request isn't merging if you aren't aware of this feature.

If this was added to Bulldozer, I think the implementation could be a bit involved. I didn't see the resolved state in the response from the v3 REST API, which means converting to the v4 GraphQL API for at least this feature.

[evancharlton]

That idea about using a separate service to apply status checks is very intriguing to me -- I could whip that up in some spare time. However, would that lead to a race condition? The unresolved-comments-status-check app would need to be sure to run before the Bulldozer app.

[bluekeyes]

I think you can avoid race conditions by either making the "unresolved comments" check a required status check as part of GitHub branch protection (if you want it to apply to humans), or by adding it to the required_statuses option in your Bulldozer configuration (if you only want it to apply to Bulldozer.) In both cases, Bulldozer will wait for the status check to pass before attempting to merge the pull request.

[evancharlton]

Hmmm would that fix the race condition, though? I'm not super familiar with GitHub's pipelines, but here's the scenario I'm playing out in my head:

1. Alice send Bob a PR to review and add the [auto-merge] label so Bulldozer will merge it when everything is happy.
2. There are no unresolved comments; the [no-unresolved-comments] check passes.
3. Bob reviews the PR, and adds a comment ("You made a typo in this comment."), but still approves the PR.
4. Bulldozer merges the PR
5. [no-unresolved-comments] fails to pass

I guess the app behind no-unresolved-comments would have to only flip the bit after approval has been granted. I'm not super happy about that because then the status indicator on the PR list page would be ❌ even though it's actually passing.

Certainly an interesting idea, though.

[bluekeyes]

Thanks for the example, I was thinking about how the bots interact when a new commit is pushed instead.

I agree that only setting the status to success after approval is given should work (or only if there is at least one comment). Maybe using the pending status prior to that, rather than failed, would clean up the PR listing page?

[ajRiverav]

hey @evancharlton, any new thoughts on this? Have you found alternatives in the github actions marketplace?

[evancharlton]

Thanks for following up @ajRiverav ! I have moved to a new employer who doesn't use GitHub (in deference to Azure DevOps), so I no longer have a vested interest in this feature. However, I still believe that it has merit! (ADO has this built-in and it's very convenient)

[chenxeed]

Hi, I recently found out that the github branch protection has added this option:

https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/about-protected-branches#require-conversation-resolution-before-merging

(at the time I write this comment, it was still in beta)

[ivanrein]

hi @chenxeed, are you able to get that option working for you? mine still merge it regardless

[SamuelCabralCruz]

@ivanrein I just tried on one of my repositories and I have the same problem as you.

[evancharlton]

(Sorry for using this as a public debugging session when I could just try it, but that requires that I enlist someone to help me)

Have you enabled this option, too? Without it, I don't believe that you'd be able to block yourself on your own repo. (It's not enabled by default)

[SamuelCabralCruz]

@evancharlton I did enable the feature and I see the check detecting the unresolved conversation, but nothing seems to prevent the merge in the case of failing check As you can see above, the merge button is still available.

[evancharlton]

Thanks for elaborating, @SamuelCabralCruz . I'm out of ideas at the moment, sorry.

[SamuelCabralCruz]

@evancharlton if you guys are only looking for a solution, I do have an action that does exactly this, but it is less accurate and does not integrate as gracefully as a built-in feature. Even I was looking to move all my repositories to the built-in feature.

[SamuelCabralCruz]

I started a discussion in the github feedback repository.