Skip to content

Update faraday version in gemspec #794

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.

Sign up for GitHub

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

Jump to bottom
Merged
krainboltgreene merged 3 commits into from
Feb 8, 2020
Merged

Update faraday version in gemspec #794

krainboltgreene merged 3 commits into from
Feb 8, 2020

Conversation

@d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Feb 4, 2020

Faraday got updated and the manageiq folks were hoping maybe that you all might be amenable to letting people update so we can support ruby 2.7.

Thanks!

@d-m-u d-m-u mentioned this pull request Feb 4, 2020
Closed
21 tasks
@d-m-u d-m-u changed the title Update faraday version in gemspec [WIP] Update faraday version in gemspec Feb 4, 2020
@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 4, 2020 •
edited
Loading

WIP because it's failing tests.

@olleolleolle
Copy link
Member

olleolleolle commented Feb 4, 2020

#788

There are a few differences as well, which can be seen in tests (it wants adapter to be called, not use)

olleolleolle
olleolleolle reviewed Feb 4, 2020
View reviewed changes
Copy link
Member

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

A note.

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 4, 2020

Ah, I guess possibly https://github.com/vcr/vcr/blob/master/script/ci.sh#L89 needs to change?

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 4, 2020 •
edited
Loading

Hey @olleolleolle thanks for the help, any chance you might be able to tell me what's up with the last failure ?

@d-m-u d-m-u changed the title [WIP] Update faraday version in gemspec Update faraday version in gemspec Feb 6, 2020
@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 6, 2020 •
edited
Loading

lostisland/faraday#750 changed the way that the handlers are stored. The default adapter is no longer stored in the list of handlers so the test that was previously failing is bogus, because it no longer warns, and these needed to be updated in order to no longer test that the adapter was in the list. These updated specs will only work on faraday 1.0.0 and above. Thanks @jrafanie for the 👀

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 6, 2020

@krainboltgreene could I get you to take a look at this, please?

jrafanie
jrafanie reviewed Feb 6, 2020
View reviewed changes
jrafanie
jrafanie reviewed Feb 6, 2020
View reviewed changes
spec/lib/vcr/library_hooks/faraday_spec.rb
it 'prints a warning if the faraday connection stack contains a middleware after the HTTP adapter' do
conn = Faraday.new(:url => 'http://sushi.com') do |builder|
builder.use Faraday::Adapter::NetHttp
builder.use Faraday::Response::Logger
Copy link

@jrafanie jrafanie Feb 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

✂️ Nice! Yeah, agreed. This test is no longer valid with faraday 1.0.0 as the warning doesn't happen because the adapter is managed differently now.

jrafanie
jrafanie reviewed Feb 6, 2020
View reviewed changes
spec/spec_helper.rb
require "codeclimate-test-reporter"
CodeClimate::TestReporter.start
require 'simplecov'
SimpleCov.start
Copy link

@jrafanie jrafanie Feb 6, 2020

Choose a reason for hiding this comment

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

I'm late to this PR, was it failing on master because of this?

Copy link
Contributor Author

@d-m-u d-m-u Feb 6, 2020

Choose a reason for hiding this comment

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

Yes. It was deprecated in https://github.com/codeclimate/ruby-test-reporter/blob/master/CHANGELOG.md#changes-1

@krainboltgreene
Copy link
Contributor

krainboltgreene commented Feb 8, 2020

@d-m-u Ready to merge?

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 8, 2020

@krainboltgreene should be good to go, yeah! Thanks!!

@krainboltgreene krainboltgreene merged commit a61ce03 into vcr:master Feb 8, 2020
@d-m-u d-m-u deleted the updating_faraday branch February 8, 2020 13:44
@d-m-u d-m-u mentioned this pull request Feb 8, 2020
Closed
@d-m-u
Copy link
Contributor Author

d-m-u commented Apr 20, 2020 •
edited
Loading

hey @krainboltgreene or @olleolleolle could we please get a version cut that uses the new faraday? just kidding, thanks jrafanie

@jrafanie
Copy link

jrafanie commented Jun 1, 2020

looks like v6.0.0 tag has it @d-m-u: https://github.com/vcr/vcr/blob/v6.0.0/CHANGELOG.md

@AxelTheGerman AxelTheGerman mentioned this pull request Jun 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@olleolleolle olleolleolle olleolleolle left review comments

+2 more reviewers

@jrafanie jrafanie jrafanie left review comments

@krainboltgreene krainboltgreene krainboltgreene approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@d-m-u @olleolleolle @krainboltgreene @jrafanie