Implemented better handling for GitHub API rate limit errors #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… call to get_all_posts
GFLEMING133
reviewed
Jun 8, 2019
| <head> | ||
| <title>Rate Limit Error</title> | ||
| <meta name="viewport" content="width=device-width,initial-scale=1"> | ||
| <style> |
There was a problem hiding this comment.
Maybe this is a little too picky, but could we put the styles in a separate file? Just to separate our concerns (content from styling) a bit more? Thanks!
There was a problem hiding this comment.
Possibly, let me check. The pages in the public folder a little bit different then our standard ruby on rails pages in the app directory. I modeled these pages after the default 404, 422, and 500 error pages which include the style tag in those html pages.
There was a problem hiding this comment.
All right, to be consistent I suppose it's all right if we leave those that way.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request does a few different things.
I Implemented a module for handling errors at a global level. The only error it handles now is the Octokit::TooManyRequests error and it will redirect to a custom error page. Currently it uses the default Ruby on Rails error page styling which I copy and pasted from the default 404 page. We can change this later if we want.
I looked into the 1 test that was failing due to the Octokit::TooManyRequests and I changed it to mock the GitHub Service calls instead of going to the GitHub API which should fix the build. I'm thinking it started throwing the Octokit::TooManyRequests error since I had a lot of commits in master which triggered builds before I realized that CI was working. I hope we don't run into that error again, but it's something we need to keep in mind for the future. Due to this dev should be fine to build so I added it back into the branches section in travis.
I changed all requests in the GitHub service to rely on basic GitHub authentication. Authenticated requests get 5000 requests per hours and unauthenticated requests get 60 requests per hour. So, in all methods in the GitHub service should use an oauth token besides the authenticate method. The oauth token is currently gets stored in the rails session after they logged in. Currently, the oauth token currently being stored is incorrect but that's a separate issue.
After this I think we should merge this to master right away. This seems important for future element on this editor.