Skip to content
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

Rescue OAuth2 Timeout errors #166

Closed
wants to merge 1 commit into from
Closed

Conversation

jessieay
Copy link

@jessieay jessieay commented Jun 5, 2023

* 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
@@ -4,7 +4,7 @@ require "omniauth-oauth2/version"

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"
Copy link
Author

Choose a reason for hiding this comment

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

Because the errors we are rescuing are introduced in version 2.0.2 of omniauth this seemed like a good update.

Copy link
Contributor

Choose a reason for hiding this comment

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

rejecting 2.1+?

Copy link
Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

Specify like we do oauth2 above

Copy link
Contributor

Choose a reason for hiding this comment

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

and I realized you should specify v2.0.2 of oauth2 gem, not omniauth.

@jessieay
Copy link
Author

jessieay commented Jun 5, 2023

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

@jessieay
Copy link
Author

Hey @BobbyMcWho - just following up on this. I can see that GitHub actions are failing but I cannot see why.

@BobbyMcWho
Copy link
Member

I'm not sure what's causing CI fail, can you try rebasing onto main and seeing if that helps?

@nov
Copy link
Contributor

nov commented Jul 11, 2023

namespace is wrong.
ref.) #169

@nov
Copy link
Contributor

nov commented Jul 15, 2023

what's the status of this PR / #169?

@BobbyMcWho BobbyMcWho closed this Jul 20, 2023
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.

3 participants