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

fix(github): chunk get pull requests #330

Merged
merged 4 commits into from
Mar 16, 2023
Merged

Conversation

raffis
Copy link
Contributor

@raffis raffis commented Mar 15, 2023

What does this change

Currently it is impossible to use the merge command for github if it targets a certain amount of pull requests.
if the data sent to the graphql server is too large the request fails.
I get:
encountered error during GraphQL query: Something went wrong while executing your query. This may be the result of a timeout, or it could be a GitHub bug. Please include xxx when reporting this issue.

What issue does it fix

This PR chunks the GetPullRequest() method into a static chunk of 50.

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

  • Made sure the PR follows the CONTRIBUTING.md guidelines
  • Tests if something new is added

Sorry, something went wrong.

Signed-off-by: Raffael Sahli <raffael.sahli@doodle.com>
Copy link
Owner

@lindell lindell left a comment

Choose a reason for hiding this comment

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

Thanks for finding and fixing this problem!

The main issue is that we should not do parallel requests as per the GitHub API docs (more info in a comment).

internal/scm/github/github.go Outdated Show resolved Hide resolved
internal/scm/github/github.go Outdated Show resolved Hide resolved
internal/scm/github/github.go Outdated Show resolved Hide resolved
internal/scm/github/util.go Show resolved Hide resolved
@raffis raffis requested a review from lindell March 16, 2023 08:05
Copy link
Owner

@lindell lindell left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks for the changes!

@lindell lindell merged commit 488cd63 into lindell:master Mar 16, 2023
@github-actions
Copy link
Contributor

Included in release v0.44.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.

None yet

2 participants