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

Replacing Net::HTTP::Persistent with faraday-net_http_persistent #1250

Merged
merged 4 commits into from Mar 26, 2021
Merged

Replacing Net::HTTP::Persistent with faraday-net_http_persistent #1250

merged 4 commits into from Mar 26, 2021

Conversation

@MikeRogers0
Copy link
Contributor

@MikeRogers0 MikeRogers0 commented Mar 19, 2021

Description

Using https://github.com/lostisland/faraday-net_http_persistent over storing the code with in this repo :)

Part of v2 Wishlist: #953

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • Documentation

Additional Notes

Optional section

@MikeRogers0
Copy link
Contributor Author

@MikeRogers0 MikeRogers0 commented Mar 19, 2021

Your bundle requires gems that depend on each other, creating an infinite loop. Please remove either gem 'faraday' or gem 'faraday-net_http_persistent' and try again.

I need to remove faraday as a dependency - I'll update the other repo & come back to this :)

@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented Mar 19, 2021

I need to remove faraday as a dependency - I'll update the other repo & come back to this :)

Now that PR has been merged - thanks a lot, @MikeRogers0!

@MikeRogers0
Copy link
Contributor Author

@MikeRogers0 MikeRogers0 commented Mar 19, 2021

@iMacTia What is the release procedure for https://github.com/lostisland/faraday-net_http_persistent/blob/master/.github/workflows/publish.yml - Do I just push up a tag & add the release notes?

@olleolleolle
Copy link
Member

@olleolleolle olleolleolle commented Mar 19, 2021

@MikeRogers0 I think the Released event is created in the GitHub API events universe when hitting the "GitHub Releases" create button. A big green one, in the UI.

Creating a git tag locally and pushing it wouldn't create that event, I assume.

@iMacTia
Copy link
Member

@iMacTia iMacTia commented Mar 19, 2021

@MikeRogers0 As @olleolleolle said, there's no need to create a tag locally and pushing it, just use the GitHub UI to release a new version (remember to bump the VERSION file first, I always forget!) and that will automatically create a tag for you and trigger the Action to deploy on Rubygems 👍

@MikeRogers0
Copy link
Contributor Author

@MikeRogers0 MikeRogers0 commented Mar 19, 2021

👀 Ruby 3.0 is failing due to:

Bundler could not find compatible versions for gem "net-http-persistent":
  In Gemfile:
    net-http-persistent (>= 3.0)

    faraday was resolved to 1.3.0, which depends on
faraday-net_http_persistent (~> 1.0) was resolved to 1.0.1, which depends
on
        net-http-persistent (~> 3.1)

I will update the requirements in the faraday-net_http_persistent gem to be >= 3.0 to match what is in the Gemfile this weekend & see if that solves the issue :)

@iMacTia
Copy link
Member

@iMacTia iMacTia commented Mar 22, 2021

@MikeRogers0 actually, I think we should remove Net::HTTP::Persistent direct dependency from Faraday Gemfile, as that should be decided by faraday-net_http_persistent.

I think the problem you're having may be with @grosser's adding support for Ruby 3 to Net::HTTP::Persistent
If you check our Gemfile, we actually have a special directive to manage Ruby 3.0 as a special case (https://github.com/lostisland/faraday/blob/master/Gemfile#L28). So those 2 lines could be moved from Faraday's Gemfile to your gem's one.

@MikeRogers0 MikeRogers0 marked this pull request as ready for review Mar 25, 2021
@MikeRogers0
Copy link
Contributor Author

@MikeRogers0 MikeRogers0 commented Mar 25, 2021

Woo @iMacTia I think that worked! Are you able to give it a sanity check when you have some free time?

Thank you btw! It's been pretty fun diving through this code :D

Copy link
Member

@olleolleolle olleolleolle left a comment

The changes look 💯.

👍 Thanks for joining this effort, @MikeRogers0!

♥️ 💛 💚 💙 💜

@@ -24,9 +24,6 @@ group :test, :development do
gem 'excon', '>= 0.27.4'
gem 'httpclient', '>= 2.2'
gem 'multipart-parser'
# TODO: remove this once v4 is released
options = (RUBY_VERSION.start_with?('3') ? { github: 'grosser/net-http-persistent', branch: 'grosser/spec' } : {})
gem 'net-http-persistent', '>= 3.0', **options

This comment has been minimized.

@olleolleolle

olleolleolle Mar 26, 2021
Member

So very nice to move this out of the way!

Copy link
Member

@iMacTia iMacTia left a comment

Thank you so much @MikeRogers0 🙏!
One more out of the way 🥳🎉🍾!

@iMacTia iMacTia merged commit 14d0dd7 into lostisland:master Mar 26, 2021
7 checks passed
7 checks passed
@github-actions
linting
Details
@github-actions
build 2.4
Details
@github-actions
build 2.5
Details
@github-actions
build 2.6
Details
@github-actions
build 2.7
Details
@github-actions
build 3.0
Details
codeclimate 1 fixed issue
Details
@MikeRogers0 MikeRogers0 deleted the MikeRogers0:feature/faraday-net_http_persistent-gem branch Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants