-
Notifications
You must be signed in to change notification settings - Fork 731
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
Rate-Limit: Thread fix, reset date #160
Conversation
Kohsuke Kawaguchi » github-api #284 SUCCESS |
@@ -18,7 +18,7 @@ | |||
@Before | |||
public void setUp() throws Exception { | |||
Properties props = new Properties(); | |||
java.io.File f = new java.io.File(System.getProperty("user.home"), ".github.kohsuke2"); | |||
java.io.File f = new java.io.File(System.getProperty("user.home"), ".github"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub.connect()
already looks for ~/.github
, so I need to keep the original line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better cal it .github.test
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general not critical in comparison to thread usage.
This issue is blocker for me :( |
Added two related to rate limit info commits, can move in separate PR if you want. |
Kohsuke Kawaguchi » github-api #287 SUCCESS |
Kohsuke Kawaguchi » github-api #288 SUCCESS |
Kohsuke Kawaguchi » github-api #297 SUCCESS |
Removed one unrelated commit. Also if some trigger plugin uses this library it can stuck with the next execution time:
|
Another one bug is that if you have bad password + rate limit 0, then rate_limit page http answer is treated as NotFound and sets rate limit to 10000 (it think that its enterprise). |
Strategy pattern is better for API rate limit handling as the current behavior is quite valid for batch applications. I'm not taking OkHttp version change so as not to introduce any unneeded version requirement.
I implemented a variant of this in 4093e53 and merged. |
Could you also add "bad password" throw before looping? Sent from my iPad
|
caused regression #198 |
for #159