Skip to content
This repository was archived by the owner on Mar 29, 2022. It is now read-only.

Add error handling for GitHub 503s #461

Merged
merged 2 commits into from
Aug 28, 2020
Merged

Conversation

katjuell
Copy link
Contributor

Description

This PR addresses the known limitations of the GitHub API, encountered last year through 503s in the sign up process. By adding error handling for Octokit::ServiceUnavailable exceptions, we can provide a better user experience.

@katjuell katjuell self-assigned this Aug 19, 2020
@katjuell katjuell requested a review from MattIPv4 August 19, 2020 21:07
@@ -4,6 +4,7 @@ class ApplicationController < ActionController::Base
before_action :set_current_user
rescue_from Faraday::ClientError, with: :api_error
rescue_from Octokit::Unauthorized, with: :github_unauthorized_error
rescue_from Octokit::ServiceUnavailable, with: :api_error
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should recuse from Octokit::ServerError instead, so we cleanly handle all 5xx that GitHub might throw at us?

What do you think about handling other client errors from Octokit, or are we happy to let the app do what it does currently for those until we see an issue arrise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, makes sense to handle all 5xx –– let's do that.

As far as the other errors –– I wish we had more data from last year! Let me at least add Octokit::AccountSuspended, which was mentioned in #431

Copy link
Contributor

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

Lgtm -- don't really have a way to test but looks correct.

@katjuell katjuell merged commit 6795265 into master Aug 28, 2020
@katjuell katjuell deleted the kjuell/github-signin-errors branch August 28, 2020 21:14
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