feat: support both faraday 1.x and 2.x #618
Conversation
DocX
force-pushed
the
support-faraday-2
branch
3 times, most recently
from
1d74e8a to
6803e3a
Compare
|
cc @cben please review when you get time |
|
Looks great, just some questions on the version range, should it really be "any version" |
|
ref: https://lostisland.github.io/faraday/middleware/authentication documents both v2 and v1 APIs used here ✔️ |
good point, I've added condition for >= 1 and < 3 - so that matches 1.x and 2.x. |
|
cc @cben I've fixed the faraday version range |
| @@ -15,6 +15,7 @@ jobs: | |||
| strategy: | |||
| matrix: | |||
| ruby: [ '2.7', '3.0', '3.1', '3.2', 'ruby-head', 'truffleruby-head' ] | |||
| gemfile: ["test/gemfiles/Gemfile.faraday-1", "test/gemfiles/Gemfile.faraday-2"] | |||
There was a problem hiding this comment.
nice 👍
This approach won't scale to many libraries, but for now it's OK and addresses a real concern that people making changes won't realize they need to to test both versions.
There was a problem hiding this comment.
We could separate it to its own single test to test the Faraday 1 with just one specific ruby version. And use Faraday 2 in the all ruby versions test. I think ruby version should not influence the faraday behaviour, and it is more orthogonal test to testing all ruby versions as well.
kubeclient.gemspec
Outdated
| @@ -32,8 +32,8 @@ Gem::Specification.new do |spec| | |||
| spec.add_development_dependency 'net-smtp' | |||
| spec.add_development_dependency 'forking_test_runner' | |||
|
|
|||
| spec.add_dependency 'faraday', '~> 1.1' | |||
| spec.add_dependency 'faraday_middleware', '~> 1.0' | |||
| spec.add_dependency 'faraday' | |||
There was a problem hiding this comment.
[Oops, this were the "some questions" I thought I sent but left "Pending" 🤦♂️
Good thing you inferred my meaning anyway 👍 LGTM now]
-
Should we keep
>= 1.1constraint? I don't remember/see any record why Replace rest_client with faraday #466 started from 1.1 in the first place, probably it was simply the latest version then?
I don't see obviously-critical things in faraday changelog but at this point it's imho safer to keep it (if anybody really needs a faraday older then 1.1, the onus should be on them to test and make a PR). -
Should this add a
< 3.0constraint, as we're only testing 1.y & 2.y and can't know what future breaking changes 3.0 may bring?
faraday-follow_redirectsgem that is compatible with 1.x and 2.x as well - instead of deprecatedfaraday_middlewaregem