search

codeapprove / feedback

Feedback for CodeApprove
0 stars 0 forks source link

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

Closed mtlynch closed 1 year ago

mtlynch commented 1 year ago

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

image

image

image

samatcodeapprove commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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

mtlynch commented 1 year ago

We just hit this again on https://github.com/tiny-pilot/ustreamer-debian/pull/6 but re-syncing didn't fix it.

samatcodeapprove commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

We just hit this again on https://github.com/tiny-pilot/tinypilot/pull/1458

The re-sync workaround didn't work.

samatcodeapprove commented 1 year ago

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 https://github.com/tiny-pilot/tinypilot/pull/1458

The re-sync workaround didn't work.

— Reply to this email directly, view it on GitHub https://github.com/codeapprove/feedback/issues/80#issuecomment-1611613785, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2GUM4AWYFKVEYFIG5OMLZDXNRBYTANCNFSM6AAAAAARXJBU2A . You are receiving this because you were mentioned.Message ID: @.***>

jdeanwallace commented 1 year ago

We just hit this again on https://github.com/tiny-pilot/tinypilot/pull/1469

The re-sync workaround didn't work.

samatcodeapprove commented 1 year ago

@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 commented 1 year ago

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

jdeanwallace commented 1 year ago

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

mtlynch commented 1 year ago

@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 commented 1 year ago

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

mtlynch commented 1 year ago

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