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

Support ruby 2.4 #247

Merged
merged 1 commit into from
May 3, 2017
Merged

Support ruby 2.4 #247

merged 1 commit into from
May 3, 2017

Conversation

cben
Copy link
Collaborator

@cben cben commented May 1, 2017

Bump to Webmock 2.3.1 which fixed .close stubbing for ruby 2.4.
(It's a dev dependency, shouldn't affect users of kubeclient.)

Update tests using user:pass@... urls — webmock 2.x treats basic auth as a header, not url part.
https://github.com/bblimke/webmock/blob/2.0/CHANGELOG.md#200

Fixes #242. @jrafanie please review.

Bump to Webmock 2.3.1 which fixed .close stubbing
for ruby 2.4.

Update tests using user:pass@... urls - webmock 2.x
treats basic auth as a header, not url part.
https://github.com/bblimke/webmock/blob/2.0/CHANGELOG.md#200
@jrafanie
Copy link
Member

jrafanie commented May 2, 2017

LGTM @cben 👍

Copy link
Collaborator

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@moolitayer
Copy link
Collaborator

cc @abonas @simon3z

@abonas
Copy link
Member

abonas commented May 3, 2017

Given that the gem supports ruby starting version 2.0, how does this PR influence users that have ruby < 2.4 ? especially given the bump of webmock which looks like has a different way of handling basic auth - won't it cause regressions?

@cben
Copy link
Collaborator Author

cben commented May 3, 2017

webmock is a develoment dependency.
Other projects depending only on gem 'kubeclient' won't even get webmock installed. Projects depending on both can use whatever webmock they want:

> cat Gemfile
source 'https://rubygems.org'
gem 'kubeclient', :git => 'https://github.com/cben/kubeclient.git', :branch => 'ruby-2.4'
gem 'webmock', '= 1.0.0'

> bundle install
Fetching https://github.com/cben/kubeclient.git
Fetching gem metadata from https://rubygems.org/...........
Fetching version metadata from https://rubygems.org/.
Fetching dependency metadata from https://rubygems.org/.
Resolving dependencies...
Using public_suffix 2.0.5
Using unf_ext 0.0.7.4
Using http-form_data 1.0.1
Using http_parser.rb 0.6.0
Using recursive-open-struct 1.0.4 (was 1.0.0)
Using mime-types-data 3.2016.0521
Using netrc 0.11.0
Using bundler 1.13.1
Using addressable 2.5.1
Using unf 0.1.4
Using mime-types 3.1
Installing webmock 1.0.0
Using domain_name 0.5.20170404
Using http-cookie 1.0.3
Using http 2.2.2 (was 0.9.8)
Using rest-client 2.0.2
Using kubeclient 2.3.0 from https://github.com/cben/kubeclient.git (at ruby-2.4@1065d39)
Bundle complete! 2 Gemfile dependencies, 17 gems now installed.
Use `bundle show [gemname]` to see where a bundled gem is installed.

@jrafanie
Copy link
Member

jrafanie commented May 3, 2017

Given that the gem supports ruby starting version 2.0, how does this PR influence users that have ruby < 2.4 ? especially given the bump of webmock which looks like has a different way of handling basic auth - won't it cause regressions?

@abonas In addition to what @cben mentioned about using kubeclient in a project already using an older webmock, the tests should pick up regressions since we now test 2.0 through 2.4.

@abonas abonas merged commit 2770480 into ManageIQ:master May 3, 2017
jrafanie added a commit to jrafanie/foreman_api_client that referenced this pull request May 4, 2017
[1] Older webmock is not compatible with ruby 2.4
[2] webmock 2+ needs VCR 3.0.2+ [2]
[3] Script from [2] to convert userinfo in uri to auth header (in
cassettes)
[4] An alternative script
[5] thanks to cben for digging into this

[1] Ruby 2.4.0 removed the closed? check in the conditional in: s.close if
!s.closed?
Webmock was changed to add close to StubSocket along with another
change...
ruby/ruby@f845a9e
bblimke/webmock@8f2176a

WebMock 2.3.1+ fixed the issue with ruby 2.4.0 by adding
StubSocket#close.

[2] vcr/vcr#570 (comment)

[3] https://gist.github.com/glaszig/9170b1cf2186674faeead74a68606c5d

[4] https://gist.github.com/ujh/594c99385b6cbe92e32b1bbfa8578a45

[5] ManageIQ/kubeclient#247
jrafanie added a commit to jrafanie/manageiq that referenced this pull request May 4, 2017
[1] Older webmock is not compatible with ruby 2.4
[2] webmock 2+ needs VCR 3.0.2+ [2]
[3] Script from [2] to convert userinfo in uri to auth header (in
cassettes)
[4] An alternative script
[5] thanks to cben for digging into this
[6] very similar to the change for foreman_api_client

[1] Ruby 2.4.0 removed the closed? check in the conditional in: s.close if
!s.closed?
Webmock was changed to add close to StubSocket along with another
change...
ruby/ruby@f845a9e
bblimke/webmock@8f2176a

WebMock 2.3.1+ fixed the issue with ruby 2.4.0 by adding
StubSocket#close.

[2] vcr/vcr#570 (comment)

[3] https://gist.github.com/glaszig/9170b1cf2186674faeead74a68606c5d

[4] https://gist.github.com/ujh/594c99385b6cbe92e32b1bbfa8578a45

[5] ManageIQ/kubeclient#247

[6] ManageIQ/foreman_api_client#7
cben pushed a commit to cben/kubeclient that referenced this pull request Jan 22, 2018
cben added a commit to cben/kubeclient that referenced this pull request Jan 22, 2018
This reverts commit 263ff40 from ManageIQ#271.
Those backport changes are no longer needed:
- after backporting ManageIQ#195 and ManageIQ#233, v2.x now has
  Kubeclient::HttpError and Kubeclient::ResourceNotFoundError.
- after backporting ManageIQ#247, v2.x now tests with webmock 2.x,
  it takes `basic_auth` rather than user:pw in URL.
- ns/namespace change was just cosmetic.

In other words, v2.x branch is now closer to original ManageIQ#262.
cben pushed a commit to cben/kubeclient that referenced this pull request Jan 25, 2018
cben added a commit to cben/kubeclient that referenced this pull request Jan 25, 2018
This reverts commit 263ff40 from ManageIQ#271.
Those backport changes are no longer needed:
- after backporting ManageIQ#195 and ManageIQ#233, v2.x now has
  Kubeclient::HttpError and Kubeclient::ResourceNotFoundError.
- after backporting ManageIQ#247, v2.x now tests with webmock 2.x,
  it takes `basic_auth` rather than user:pw in URL.
- ns/namespace change was just cosmetic.

In other words, v2.x branch is now closer to original ManageIQ#262.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade webmock for ruby 2.4+ support
4 participants