Skip to content

Github rate limiting should always fail fast#63

Merged
samrocketman merged 1 commit intojenkinsci:masterfrom
i386:rate-limit-handling
Oct 24, 2016
Merged

Github rate limiting should always fail fast#63
samrocketman merged 1 commit intojenkinsci:masterfrom
i386:rate-limit-handling

Conversation

@i386
Copy link
Contributor

@i386 i386 commented Oct 23, 2016

Requires #62 to be merged.

We see a lot of errors such as https://gist.github.com/michaelneale/d471fd6c1d44f29b94fe380ced8539d1

The Github client should always fail fast so that it never locks up web threads and cause Jenkins to become unresponsive.

PTAL @samrocketman

@i386 i386 changed the title Rate limit handling Github rate limiting should always fail fast Oct 23, 2016
@i386 i386 mentioned this pull request Oct 23, 2016
@samrocketman
Copy link
Member

samrocketman commented Oct 24, 2016

#62 merged

@i386 i386 force-pushed the rate-limit-handling branch from b72163a to 13deca9 Compare October 24, 2016 00:49
@i386
Copy link
Contributor Author

i386 commented Oct 24, 2016

@samrocketman rebased.

@i386
Copy link
Contributor Author

i386 commented Oct 24, 2016

Ahh this is failing because one of the mocks isn't setup right. Ill take a look.

@samrocketman
Copy link
Member

Okay, I'm also troubleshooting this by trying to replicate the CI environment.

@samrocketman
Copy link
Member

Strange, when I build with:

  • Apache Maven 3.3.9
  • Oracle java version "1.8.0_112"

I get a build success.

@i386 i386 force-pushed the rate-limit-handling branch from 13deca9 to 1917c8a Compare October 24, 2016 01:28
@i386 i386 force-pushed the rate-limit-handling branch from 1917c8a to 44ee646 Compare October 24, 2016 01:29
@i386
Copy link
Contributor Author

i386 commented Oct 24, 2016

@samrocketman OK I fixed this, rebased and squished the commit.

@i386
Copy link
Contributor Author

i386 commented Oct 24, 2016

Side effect of this change is that JENKINS-37581 is resolved.

@samrocketman
Copy link
Member

Side effect is now JENKINS-37581 is resolved.

Cool I'll test that.

@samrocketman
Copy link
Member

samrocketman commented Oct 24, 2016

Unfortunately, this does not resolve JENKINS-37581.

When I attempt to log in after setting and exporting environment variables I get redirected to https://github.com/login/oauth/authorize?client_id=$GITHUB_OAUTH_CLIENTID&scope=read:org,user:email. It still literally interprets $GITHUB_OAUTH_CLIENTID.

That doesn't block me from merging this but it doesn't fix JENKINS-37581.

@samrocketman
Copy link
Member

samrocketman commented Oct 24, 2016

I tested to ensure functionality wasn't negatively affected. +2 from me.

@samrocketman samrocketman merged commit c98657d into jenkinsci:master Oct 24, 2016
@i386
Copy link
Contributor Author

i386 commented Oct 24, 2016

Ahh will have to address environment variables later.

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