Rescue OAuth2 Timeout errors - #166
Conversation
jessieay
commented
Jun 5, 2023
jessieay
commented
- This fix was originally proposed with a different solution via this PR: Rescue Faraday timeout errors #129
- The conclusion there was that we should not rescue Faraday errors because Faraday is not a direct dependency of OAuth2.
- Instead, best to rescue Faraday errors on OAuth2, then rescue OAuth2 errors here
- Those changes to OAuth2 were made here: https://gitlab.com/oauth-xx/oauth2/-/merge_requests/604 and here Rescue Faraday::ConnectionFailed ruby-oauth/oauth2#549
- And released as part of OAuth2 gem version 2.0.2: https://gitlab.com/oauth-xx/oauth2/-/blob/866dc365bfe0d64f6cc56295a38ccd5b24143836/CHANGELOG.md#L74
* This fix was originally proposed with a different solution via this PR: omniauth#129 * The conclusion there was that we should not rescue Faraday errors because Faraday is not a direct dependency of OAuth2. * Instead, best to rescue Faraday errors on OAuth2, then rescue OAuth2 errors here * Those changes to OAuth2 were made here: https://gitlab.com/oauth-xx/oauth2/-/merge_requests/604 * And released as part of OAuth2 gem version 2.0.2: https://gitlab.com/oauth-xx/oauth2/-/blob/866dc365bfe0d64f6cc56295a38ccd5b24143836/CHANGELOG.md#L74
| Gem::Specification.new do |gem| | ||
| gem.add_dependency "oauth2", [">= 1.4", "< 3"] | ||
| gem.add_dependency "omniauth", "~> 2.0" | ||
| gem.add_dependency "omniauth", "~> 2.0.2" |
There was a problem hiding this comment.
Because the errors we are rescuing are introduced in version 2.0.2 of omniauth this seemed like a good update.
There was a problem hiding this comment.
@nov Good point, this is somewhat inflexible, isn't it? Maybe we should change to:
gem.add_dependency "omniauth", "~> 2.0.2, "~> 2.1"Not sure if this is valid syntax for a gemspec but could give it a try
There was a problem hiding this comment.
Specify like we do oauth2 above
There was a problem hiding this comment.
and I realized you should specify v2.0.2 of oauth2 gem, not omniauth.
|
Hi @BobbyMcWho - let me know what you think of this PR. I tried to include as much context in the description as possible to jog your memory on the why behind this change |
|
Hey @BobbyMcWho - just following up on this. I can see that GitHub actions are failing but I cannot see why. |
|
I'm not sure what's causing CI fail, can you try rebasing onto main and seeing if that helps? |
|
namespace is wrong. |
|
what's the status of this PR / #169? |
3 participants
