Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add retries to GitHub land API call #168

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jozef-mokry
Copy link
Contributor

In Cord we notice that when working on a stack of diffs, then
sometimes spr land fails with the following error:

  ❌  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

This problem goes away if we go to the PR and manually change the PR's
base branch to master. Then spr land works fine. This is surprising
because in the PR history I can see that spr changed the base branch to
master too when we tried to spr land the first time.

I initially added a 5s sleep call, and the problem went away. Hence, I
suspect this is an issue on GitHub's end. Hence, I added a retry
mechanism around the API call.

I added a utility function do_with_retry() to do this. Not as much for
code re-use, but because it makes the code easier to read IMO. There is
code above my changes that also uses retries, but I did not refactor it
to use do_with_retry() because that code looked more complex πŸ™ƒ

Test Plan: I will be using spr with this change for the next few days

Created using spr 1.3.5-beta.1
Copy link
Contributor

@jwatzman jwatzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jozef-mokry do we still need this? If so, looks reasonable to me. I don't know enough Rust machinery to know what the build failure is about but I assume you do :)

If we don't need this, can just close out?

Created using spr 1.3.5-beta.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants