feat: Add rate limit handling for GitHub client#4926
feat: Add rate limit handling for GitHub client#4926itays333 wants to merge 5 commits intorunatlantis:mainfrom
Conversation
itays333
requested review from
jamengual,
lukemassa and
nitrocode
and removed request for
a team
github-actions
bot
added
dependencies
| return nil, errors.Wrap(err, "error initializing github authentication transport") | ||
| } | ||
|
|
||
| transportWithRateLimit, err := github_ratelimit.NewRateLimitWaiterClient(transport.Transport, github_ratelimit.WithTotalSleepLimit(time.Minute, nil)) |
There was a problem hiding this comment.
regarding WithTotalSleepLimit(time.Minute, nil) - not sure how long we want to keep retry, and if this should be configurable
There was a problem hiding this comment.
We are trying to limit as much as possible adding more flags, so maybe we can add this to the repo.Yaml instead.
There was a problem hiding this comment.
is something like this acceptable? open to suggestions
...
git:
github:
secondary_rate_limit_duration: 60
There was a problem hiding this comment.
yeah, repo config would be a good idea.
There was a problem hiding this comment.
Can I suggest starting with the hard-coded 1 minute value that you currently have, so that we can get this PR finished. It can always be refined later to make it configurable.
Signed-off-by: Simon Heather <[email protected]>
| return nil, errors.Wrap(err, "error initializing github authentication transport") | ||
| } | ||
|
|
||
| transportWithRateLimit, err := github_ratelimit.NewRateLimitWaiterClient(transport.Transport, github_ratelimit.WithTotalSleepLimit(time.Minute, nil)) |
There was a problem hiding this comment.
How about adding a callback to these that log an info message so that we can see when they are being triggered?
| return nil, errors.Wrap(err, "error initializing github authentication transport") | ||
| } | ||
|
|
||
| transportWithRateLimit, err := github_ratelimit.NewRateLimitWaiterClient(transport.Transport, github_ratelimit.WithTotalSleepLimit(time.Minute, nil)) |
There was a problem hiding this comment.
Can I suggest starting with the hard-coded 1 minute value that you currently have, so that we can get this PR finished. It can always be refined later to make it configurable.
Signed-off-by: Simon Heather <[email protected]>
|
This issue is stale because it has been open for 1 month with no activity. Remove stale label or comment or this will be closed in 1 month. |
|
@itays333 Do you have time to address the comments? Thanks. |
|
I suggest this gets closed because #5226 got merged |
|
closed in favour of #5226 |
5 participants
What
Why
Implementation
go-github-ratelimitlibrary, as recommended in the official go-github README:Testing
References