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.

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

Update faraday version in gemspec #794

Merged
merged 3 commits into from
Feb 8, 2020
Merged

Update faraday version in gemspec #794

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

WIP because it's failing tests.

@olleolleolle
Copy link
Member

#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.

spec/lib/vcr/library_hooks/faraday_spec.rb Outdated Show resolved Hide resolved
@d-m-u d-m-u force-pushed the updating_faraday branch 2 times, most recently from 0165273 to 1c1b3ba Compare February 4, 2020 20:00
@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

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

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 d-m-u force-pushed the updating_faraday branch 2 times, most recently from 17e89bf to 82707b8 Compare February 6, 2020 17:20
@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
script/ci.sh Show resolved Hide resolved
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

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

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

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

@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

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

@jrafanie jrafanie jrafanie left review comments

@krainboltgreene krainboltgreene krainboltgreene approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@d-m-u @olleolleolle @krainboltgreene @jrafanie