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

:octocat: handle GitHub API rate limits #4298

Merged
merged 2 commits into from
Jun 28, 2024
Merged

Conversation

Copy link
Contributor

github-actions bot commented Jun 27, 2024

Test Results

3 072 tests  ±0   3 071 ✅ ±0   1m 22s ⏱️ -1s
  363 suites ±0       1 💤 ±0 
   27 files   ±0       0 ❌ ±0 

Results for commit d5df8cb. ± Comparison against base commit a187a80.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@czunker czunker left a comment

Choose a reason for hiding this comment

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

Nice!

Just some minor things on the secondary limit.

providers/github/connection/connection.go Outdated Show resolved Hide resolved
if resp.Header.Get("x-ratelimit-remaining") == "0" {
unix, err := strconv.ParseInt(resp.Header.Get("x-ratelimit-reset"), 10, 64) // x-ratelimit-reset - The time at which the current rate limit window resets, in UTC epoch seconds
if err != nil { // Must be impossible to hit errors here, but just in case
return time.Second * 8 // Github starts with 8s jailtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you find this? I only found a one minute pause on the secondary limit.

Do we need to handle primary and secondary separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't find at this point.
What would you suggest for error handling?

I don't think so, no, it'll be reflected in wait time. But, I think we could add the maximum wait time and throw error after that, because otherwise the scan can be stuck up to an hour and provide no data

Copy link
Contributor

@czunker czunker Jun 28, 2024

Choose a reason for hiding this comment

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

Ok, I understood your comment in a way that it is some default GitHub setting.

I like the idea with the max wait time.

@slntopp slntopp requested a review from czunker June 28, 2024 14:37
@slntopp slntopp merged commit 2739c68 into main Jun 28, 2024
15 checks passed
@slntopp slntopp deleted the mik/github-provider-handle-rate-limits branch June 28, 2024 14:41
@github-actions github-actions bot locked and limited conversation to collaborators Jun 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants