Update to inherit from correct Faraday error #1154

Gasparila · 2019-09-27T17:59:08Z

The update to Faraday is causing require "octokit" to break. Updating
to Faraday::ClientError fixes it and is correct based on Faraday code:
https://github.com/lostisland/faraday/blob/master/lib/faraday/error.rb

Before:

script/test
...
/Users/kevinmiller/octokit.rb/lib/octokit/middleware/follow_redirects.rb:14:in `<module:Middleware>': uninitialized constant Faraday::Error::ClientError (NameError)
Did you mean?  Faraday::ClientError

After:

script/test
...
Finished in 26.83 seconds (files took 0.79837 seconds to load)
775 examples, 0 failures

The update to Faraday is causing `require "octokit"` to break. Updating to `Faraday::ClientError` fixes it and is correct based on Faraday code: https://github.com/lostisland/faraday/blob/master/lib/faraday/error.rb

Gasparila · 2019-09-27T18:10:26Z

Looks like the failed test is flaky given that it is to do with caching and unrelated to my change? 🤞

BanzaiMan · 2019-09-27T18:22:02Z

This will then assume that Faraday version is 0.16.0 and up. Some sort of fallback is more desirable.

ushis · 2019-09-27T18:24:59Z

@gas You could add faraday as dependency to the gemspec with >= 0.16.0

michaelherold · 2019-09-27T18:27:46Z

This will then assume that Faraday version is 0.16.0 and up. Some sort of fallback is more desirable.

v0.15.0 uses this organization but has an alias that allows it to still work.

This organization has been the current one since at least 0.9.0 (which was a long-standing release). They just removed the backwards-compatibility with 0.16.0.

I suggest this change be merged.

michaelherold · 2019-09-27T18:30:00Z

Specifically, it's this commit where they remove the aliases (see here for the specific lines).

avondohren · 2019-09-27T18:32:16Z

If the change was present in Faraday v0.9.0, then we would only need to update the gemspec with >= 0.9.0, correct?

Gasparila · 2019-09-27T18:38:13Z

@avondohren I agree, currently "> 0.8" is required. Added >= 0.9 explicit requirement in 11c65a6b

Faraday has been maintaining a backward-compatibility layer for errors in the `Faraday::Error::` "namespace". These were removed with v0.16.0, which caused issues for dependents that use semantic versioning constraints. It would be best if these were deprecated with a warning message and then dropped in v1.0 in order to make sure transitive dependencies do not break. See octokit/octokit.rb#1154

michaelherold · 2019-09-27T18:50:04Z

I submitted a patch to restore the backward-compatibility layer to Faraday's 0.16.x releases. I think they want to drop it for 1.0 so this change should still be merged into Octokit.

michaelherold · 2019-09-27T19:06:02Z

octokit.gemspec

@@ -6,6 +6,7 @@ require 'octokit/version'
 Gem::Specification.new do |spec|
  spec.add_development_dependency 'bundler', '>= 1', '< 3'
  spec.add_dependency 'sawyer', '>= 0.5.3', '~> 0.8.0'
+  spec.add_dependency 'faraday', '>= 0.9', '< 2.0'


This will pick up v0.16.0, which is the affected version. I think we just want to change the constant.

Suggested change

spec.add_dependency 'faraday', '>= 0.9', '< 2.0'

spec.add_dependency 'faraday', '>= 0.9'

If we want a version constraint, we want it as permissive as possible.

tarebyte · 2019-09-27T20:29:27Z

@Gasparila thanks for the PR, am I right in thinking that requiring Faraday >= 9.0 that we'll break the gem for users who are using Sawyer versions that are older than 0.8.2?

At least that's how I understand what I'm reading on https://rubygems.org/gems/sawyer/versions/0.8.1

Gasparila · 2019-09-27T21:40:21Z

@tarebyte it looks like Sawyer supported 0.9 since version 0.1 (https://rubygems.org/gems/sawyer/versions/0.1.0)

The ~> 0.8 is shorthand for >= 0.8, < 1.0 (the last digit can fluctuate)

tarebyte · 2019-09-27T21:55:31Z

The ~> 0.8 is shorthand for >= 0.8, < 1.0 (the last digit can fluctuate)

TIL I have no idea how Rubygems short hand works.

If you wouldn't mind accepting #1154 (comment) I think this'll be good to merge.

Thanks ✨

Gasparila · 2019-09-27T23:36:47Z

If you wouldn't mind accepting #1154 (comment) I think this'll be good to merge

Hmmm I disagree that we should be permissive of major version bumps, and all of the other dependencies specify a cap of the next major version. Originally, I had ~> 0.9, but that would disallow 1.x releases and this seems more permissive. Happy to make the change, but that would make faraday handled differently from the other dependencies (sawyer and bundler)

BobbyMcWho · 2019-09-28T01:24:48Z

@Gasparila IMO you shouldn't restrict versions on the upperside unless you know for sure that a new version of the dependency is going to break something that this package uses. If you leave the top open, then you allow folks using this along with other gems that have the same dependency to keep upgrading. Once a breaking change is released, then you cap the upper bound if you need to as a patch release, or release a compatibility fix as appropriate.

…ctokit.rb into kevin/fix_faraday_inheritance

Gasparila · 2019-09-28T02:00:33Z

Once a breaking change is released, then you cap the upper bound if you need to as a patch release, or release a compatibility fix as appropriate.

Or you don't notice it and then your gem is broken, necessitating another PR like this one 😉

Regardless, I care most about getting this fixed so updated in d4d77dc

michaelherold · 2019-09-28T21:22:05Z

Or you don't notice it and then your gem is broken, necessitating another PR like this one

In this case, the Faraday maintainers leaned into the "less than 1.0 means all bets are off" clause of SemVer. 😁

Because Faraday is such a common "plumbing" gem that ends up being a transitive dependency across libraries, I've found that keeping it with a relaxed constraint leads to a better user experience -- even when they break API compatibility -- because it avoids dependency hell where you have to fork it to unblock unrelated gem updates.

Faraday has been maintaining a backward-compatibility layer for errors in the `Faraday::Error::` "namespace". These were removed with v0.16.0, which caused issues for dependents that use semantic versioning constraints. It would be best if these were deprecated with a warning message and then dropped in v1.0 in order to make sure transitive dependencies do not break. See octokit/octokit.rb#1154

acidprime · 2019-10-02T22:53:06Z

Any update on this PR, whats needed or what it needs to be merged , I'm using slack-ruby-client and this gem which is in a dependency loop until this is solved by one or all of the gems. Really don't want to fork my production deployment if possible.

BobbyMcWho · 2019-10-02T22:59:00Z

@acidprime try a bundle update faraday, version 0.16.2 should have resolved most downstream issues

acidprime · 2019-10-03T18:55:27Z

@acidprime try a bundle update faraday, version 0.16.2 should have resolved most downstream issues

Interesting, I tried pinning the following and I'm still getting the error

ruby '2.5.1'

source 'https://rubygems.org'

gem 'async-websocket', '~>0.8.0'
gem 'aws-sdk', '3.0.1'
gem 'faraday','0.16.2'
gem 'octokit', '4.13.0'
gem 'rugged', '0.27.5'
gem 'sawyer', '0.8.0'
gem 'slack-ruby-bot', :git => 'https://github.com/slack-ruby/slack-ruby-bot.git'

gets me

NameError: uninitialized constant Faraday::Error::ClientError
Did you mean?  Faraday::ClientError
  /usr/local/bundle/gems/octokit-4.13.0/lib/octokit/middleware/follow_redirects.rb:14:in `<module:Middleware>'
  /usr/local/bundle/gems/octokit-4.13.0/lib/octokit/middleware/follow_redirects.rb:11:in `<module:Octokit>'
  /usr/local/bundle/gems/octokit-4.13.0/lib/octokit/middleware/follow_redirects.rb:9:in `<top (required)>'
  /usr/local/bundle/gems/octokit-4.13.0/lib/octokit/default.rb:1:in `require'
  /usr/local/bundle/gems/octokit-4.13.0/lib/octokit/default.rb:1:in `<top (required)>'
  /usr/local/bundle/gems/octokit-4.13.0/lib/octokit.rb:4:in `require'
  /usr/local/bundle/gems/octokit-4.13.0/lib/octokit.rb:4:in `<top (required)>'

BobbyMcWho · 2019-10-03T19:19:05Z

And you're 100% sure you ran bundle update faraday after pinning it?

acidprime · 2019-10-07T17:13:39Z

And you're 100% sure you ran bundle update faraday after pinning it?

Its all ran in docker with no cache, so there is no "update" that would be relevant I believe.

... until octokit has an actual release. See: octokit/octokit.rb#1154

@see octokit/octokit.rb#1154

This can be removed when octokit release a new version with octokit/octokit.rb#1154

Without this, we get failing builds with: ``` NameError: uninitialized constant Faraday::Error::ClientError Did you mean? Faraday::ClientError ``` This is documented in octokit/octokit.rb#1154.

Update to inherit from correct Faraday error

3e9054b

The update to Faraday is causing `require "octokit"` to break. Updating to `Faraday::ClientError` fixes it and is correct based on Faraday code: https://github.com/lostisland/faraday/blob/master/lib/faraday/error.rb

BanzaiMan mentioned this pull request Sep 27, 2019

Build not playing well faraday version bump? travis-ci/dpl#1108

Closed

Add faraday requirement

11c65a6

michaelherold mentioned this pull request Sep 27, 2019

Restore backward-compatibility layer for errors lostisland/faraday#1030

Merged

michaelherold reviewed Sep 27, 2019

View reviewed changes

stuyam mentioned this pull request Sep 27, 2019

Faraday 0.16.0 release breaking Octokit #1155

Closed

Merge branch 'master' into kevin/fix_faraday_inheritance

2422c9c

Gasparila added 2 commits September 27, 2019 18:54

Relax dependency

d4d77dc

Merge branch 'kevin/fix_faraday_inheritance' of github.com:flexport/o…

bf32723

…ctokit.rb into kevin/fix_faraday_inheritance

ma8ma mentioned this pull request Sep 28, 2019

Update requirements for dependencies (gtkmm >= 2.18) JDimproved/JDim#124

Merged

dawidd6 mentioned this pull request Sep 28, 2019

build: bump faraday from 0.15.4 to 0.16.0 in /docs Homebrew/brew#6499

Closed

michaelherold mentioned this pull request Sep 30, 2019

Make Hashie play nice with Rails 6 Hash#except method hashie/hashie#479

Merged

Merge branch 'master' into kevin/fix_faraday_inheritance

33a4ef0

indigok merged commit ae5838a into octokit:master Oct 17, 2019

orien mentioned this pull request Nov 28, 2019

Faraday deprecation notice #1170

Closed

rick added a commit to lronhodl/mikado-graph that referenced this pull request Dec 6, 2019

Work around faraday deprecation notice

0d7f054

... until octokit has an actual release. See: octokit/octokit.rb#1154

DannyBen mentioned this pull request Dec 12, 2019

Faraday::Error::ClientError deprecation warning is disruptive lostisland/faraday#1089

Closed

plutino mentioned this pull request Dec 30, 2019

New release? #1177

Closed

parkr mentioned this pull request Jan 1, 2020

Faraday ClientError issue due to version mismatch github/pages-gem#665

Closed

4 tasks

bobbyshaw added a commit to bobbyshaw/tomrobertshaw.net that referenced this pull request Jan 1, 2020

Downgrade faraday to be compatible with Octokit

d90d990

@see octokit/octokit.rb#1154

waipeng added a commit to waipeng/tutorials that referenced this pull request Jan 2, 2020

Downgrade Faraday until Octokit is fixed

1b76101

This can be removed when octokit release a new version with octokit/octokit.rb#1154

waipeng mentioned this pull request Jan 2, 2020

Downgrade Faraday until Octokit is fixed NeCTAR-RC/tutorials#93

Closed

mlinksva mentioned this pull request Jan 14, 2020

Bump octokit from 4.14.0 to 4.15.0 github/opensource.guide#1268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to inherit from correct Faraday error #1154

Update to inherit from correct Faraday error #1154

Gasparila commented Sep 27, 2019 •

edited

Loading

Gasparila commented Sep 27, 2019

BanzaiMan commented Sep 27, 2019

ushis commented Sep 27, 2019

michaelherold commented Sep 27, 2019

michaelherold commented Sep 27, 2019

avondohren commented Sep 27, 2019

Gasparila commented Sep 27, 2019

michaelherold commented Sep 27, 2019

michaelherold Sep 27, 2019

tarebyte commented Sep 27, 2019

Gasparila commented Sep 27, 2019 •

edited

Loading

tarebyte commented Sep 27, 2019

Gasparila commented Sep 27, 2019

BobbyMcWho commented Sep 28, 2019

Gasparila commented Sep 28, 2019

michaelherold commented Sep 28, 2019

acidprime commented Oct 2, 2019

BobbyMcWho commented Oct 2, 2019

acidprime commented Oct 3, 2019 •

edited

Loading

BobbyMcWho commented Oct 3, 2019

acidprime commented Oct 7, 2019

	spec.add_dependency 'faraday', '>= 0.9', '< 2.0'
	spec.add_dependency 'faraday', '>= 0.9'

Update to inherit from correct Faraday error #1154

Update to inherit from correct Faraday error #1154

Conversation

Gasparila commented Sep 27, 2019 • edited Loading

Gasparila commented Sep 27, 2019

BanzaiMan commented Sep 27, 2019

ushis commented Sep 27, 2019

michaelherold commented Sep 27, 2019

michaelherold commented Sep 27, 2019

avondohren commented Sep 27, 2019

Gasparila commented Sep 27, 2019

michaelherold commented Sep 27, 2019

michaelherold Sep 27, 2019

Choose a reason for hiding this comment

tarebyte commented Sep 27, 2019

Gasparila commented Sep 27, 2019 • edited Loading

tarebyte commented Sep 27, 2019

Gasparila commented Sep 27, 2019

BobbyMcWho commented Sep 28, 2019

Gasparila commented Sep 28, 2019

michaelherold commented Sep 28, 2019

acidprime commented Oct 2, 2019

BobbyMcWho commented Oct 2, 2019

acidprime commented Oct 3, 2019 • edited Loading

BobbyMcWho commented Oct 3, 2019

acidprime commented Oct 7, 2019

Gasparila commented Sep 27, 2019 •

edited

Loading

Gasparila commented Sep 27, 2019 •

edited

Loading

acidprime commented Oct 3, 2019 •

edited

Loading