spacedentist/spr

Can no longer land complaining about no approving review even though reviewers approved it

Opened this issue · 2 comments

opqpop commented

This has just started happening 2-3 days ago and I'm not sure why. Is it possible it has to do with the recent commits that happened since 3 days ago? Pinging the authors too in case it might be related (#159 cc @andrewhamon, #153 cc @cadolphs, #162 cc @spacedentist)

Repro:

  1. git checkout -b branch
  2. make change, git commit (hash: 123)
  3. spr diff, PR is created
  4. have someone stamp it
  5. go to main and get latest changes to prepare for rebase: git checkout origin/main; git pull origin main --rebase
  6. cherry-pick the commit to land git cherry-pick 123
  7. spr diff -m "rebase"
  8. spr land, get below error
> spr land                                          
847b2ea [drac] fix up add wallet (1/n)
  #️⃣   Pull Request #1789
  🛫  Getting started...
  ❌  GitHub Pull Request merge failed
  🛑  GitHub: At least 1 approving review is required by reviewers with write access.
      Documentation URL: https://docs.github.com/articles/about-protected-branches

Let me know if there's anything else I can help with

opqpop commented

Hmm it might be because we've recently added pre-commit hooks that require an approval before it can be merged, as well as passing a build check

but my approval + build check is all against the parent diff instead of the main branch, so when spr land swaps it to be against the main branch, it fails the pre-commit hook until the newly build check that got kicked off finishes.

Any suggestions for how to best get around this? Is there a way for spr to bypass pre-commit hooks vs main, as long as they passed when it was against the parent diff?

  • 👍1
dukeofcool199 commented

we ran into a similar issue recently, ours occured in a flow like this.

  1. commit A onto main
  2. spr diff
  3. commit B onto main
  4. spr diff
  5. B gets review with "changes requested"
  6. B requested changes get made
  7. B and A get re-ordered so that B can be merged first
  8. B gets accepted
  9. B cannot be landed because it claims it is not accepted