CodeApprove reports a PR approved, Github says it's pending approval

[mtlynch]

We just ran into an unexpected blocker with a PR we reviewed through CodeApprove.

1. Developer sent out PR for review
2. Reviewer 1 sends notes, doesn't mark PR as approved
3. Reviewer 2 (me) chimes in with a suggestion, doesn't mark PR as approved
4. Developer addresses feedback, marks all issues as resolved
5. Reviewer 1 marks PR as approved

Expected behavior

CodeApprove changes approvals in Github to all approved

Actual behavior

CodeApprove changes approvals in Github for Reviewer 1 as approved, Reviewer 2 (me) is still marked as "Changes requested," which blocks the PR from being merged.

Screenshots

[samatcodeapprove]

@mtlynch yikes! I'll see what the logs say. There's also a link at the bottom of every CodeApprove PR page which lets you re-sync the PR in case things get out of whack like this. Mind seeing if that helps here (assuming it's still in this state)?

[samatcodeapprove]

@mtlynch oh wow I just realized this was from 11 days ago 🤦 which means you definitely worked around this. Sorry I let this slip through my inbox! Not sure what happened.

[mtlynch]

@samatcodeapprove - That's okay. Yeah, I just approved it to unblock. We haven't seen it again, but I'm also not sure if we've created the same conditions. I'll bump the thread if we see this again.

[rockwotj]

We've seen this frequently too. Hitting the sync button at the bottom left in codeapprove fixes it.

[mtlynch]

We just hit this again on tiny-pilot/ustreamer-debian#6 but re-syncing didn't fix it.

[samatcodeapprove]

@mtlynch thanks for letting me know! Seems like you got it fixed for that PR by re-approving? I'll check out the logs.

[mtlynch]

The PR already had approval from one other reviewer, but I had commented, so it was blocking the review until I approved as well.

The behavior we want is that one approval is sufficient even if multiple people commented on the PR.

[mtlynch]

We just hit this again on tiny-pilot/tinypilot#1458

The re-sync workaround didn't work.

[samatcodeapprove]

Hmmm thanks for reporting I’ll take a look!

…

On Wed, Jun 28, 2023 at 8:04 AM Michael Lynch ***@***.***> wrote: We just hit this again on tiny-pilot/tinypilot#1458 <tiny-pilot/tinypilot#1458> The re-sync workaround didn't work. — Reply to this email directly, view it on GitHub <#80 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2GUM4AWYFKVEYFIG5OMLZDXNRBYTANCNFSM6AAAAAARXJBU2A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jdeanwallace]

We just hit this again on tiny-pilot/tinypilot#1469

The re-sync workaround didn't work.

[samatcodeapprove]

@jdeanwallace thanks for bumping this again. Just want to lay out what I see.

On CodeApprove:

1. mtlynch reviewed (no approval)
2. jotaen4tinypilot reviewed (no approval)
3. mtlynch reviewed (no approval)
4. mtlynch reviewed (no approval)
5. mtlynch reviewed (approval)

On GitHub:

1. mtlynch requested changes
2. jotaen4tinypilot requested changes
3. mtlynch requested changes
4. mtlynch requested changes
5. mtlynch requested changes (with pending approval)
6. mtlynch auto-approved

So the problem here is that jotaen4tinypilot's review is still listed as "requested changes" which blocks you on GitHub. I can make a change so that hanging reviews like that are dismissed.

[samatcodeapprove]

@jdeanwallace this should not happen any more, thanks for providing a clear case!

[jdeanwallace]

Thanks @samatcodeapprove, looking forward to forgetting about this thread 😆

[mtlynch]

@samatcodeapprove - Just to clarify, the functionality we're hoping for is that CodeApprove unblocks the PR when there's >=1 approval and 0 unresolved threads.

In other words, if five teammates add drive-by comments, and one person gives approval, we want the PR to be considered approved based on the one approval and not wait for any of the other five teammates to approve (as long as their threads are resolved).

Is that CodeApprove's new behavior?

[samatcodeapprove]

@mtlynch that is the intended behavior! Let me know if you're not seeing this.

[mtlynch]

@samatcodeapprove - Gotcha, thanks! We'll bump the thread if we hit something in the future that's unexpected.