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

Faraday 2.0 requires locking to ~> 1.0 or switching to faraday-net_http #1663

Closed
trevorturk opened this issue Jan 4, 2022 · 9 comments · Fixed by #1678
Closed

Faraday 2.0 requires locking to ~> 1.0 or switching to faraday-net_http #1663

trevorturk opened this issue Jan 4, 2022 · 9 comments · Fixed by #1678
Assignees
Projects
Milestone

Comments

@trevorturk
Copy link

trevorturk commented Jan 4, 2022

Issue Description

Faraday 2.0 was released with major changes: https://github.com/lostisland/faraday/blob/main/UPGRADING.md#thats-great-what-should-i-change-in-my-code-immediately-after-upgrading

I'd suggest switching to faraday-net_http and setting Faraday.default_adapter = :net_http since this will remove a number of dependencies. Alternatively, it seems you could lock to ~> 1.0, or consider using net_http directly instead of relying on Faraday to sidestep this issue entirely.

I'm not sure what's best... locking Faraday to ~> 1.0 might be easiest in the short term, but may cause issues later as users may want to upgrade to Faraday ~> 2.0 for other reasons. Perhaps sentry-ruby could detect which version is being used somehow, and switch based on that, but I don't believe you can customize the gem dependencies, so you may need to pick a "winner" between the two in the long term

I ran into a similar problem with gem dependencies here: geokit/geokit#250

So perhaps dropping the dependency might be best, but that may result in users needing code changes in order to use Sentry, for example if they're setting transport_configuration.http_adapter etc, and of course even that change could significant work on your end.

For now, I believe I can just manually lock Faraday to ~> 1.0 in my Gemfile, even though I'm only using Faraday because of Sentry.

Anyway, this is a tricky problem -- I'm happy to help if I'm able. Thank you for your attention!

@st0012
Copy link
Collaborator

st0012 commented Jan 4, 2022

@trevorturk Thank you for reporting the issue with a detailed analysis. It's very thoughtful.

Dropping faraday has been on my list for a while but I didn't expect that faraday 2.0 would bring dependency-level breaking changes 😬. It left us no choice but to leave it as quickly as possible. So my plan to drop it with other breaking changes in v5 need to be updated now. Here's my current plan:

  1. Rollout v4.8.2 that'll lock faraday at ~> 1.0 today or tomorrow.
  2. Rollout v4.9.0 with all the merged changes, which will also lock faraday at ~> 1.0 later this week.
  3. Drop faraday in the next 2 weeks or so and release v5.0.0 with only that change. So most users can easily upgrade and drop faraday without much effort.
  4. All the remaining feature work for 4.9.0 will be pushed to 5.1.0.
  5. All the work planned in [Plan] Version 6 #1279 will be pushed to 6.0.0.

Wdyt?

@sl0thentr0py I'd like to hear your opinion on this too 🙂

@st0012 st0012 added this to To do in 4.x via automation Jan 4, 2022
@st0012 st0012 added this to the 4.9.0 milestone Jan 4, 2022
@trevorturk
Copy link
Author

I think that's a reasonable plan, thank you!

I'm sorry about the faraday change. I totally see where they're coming from and I like the idea in theory, but I think this is going to impact a lot of gems in a similar way. I was thinking you might be able to "just" add faraday-net_http to your gem dependencies, but I don't think that'll work since that'll likely depend on faraday ~> 2.0. So, in the end, I suspect a lot of gems will remove the faraday dependency, which may be for the best in the long run, anyway 🤷‍♂️

Anyway, thank you for your help!

@sl0thentr0py
Copy link
Member

sl0thentr0py commented Jan 4, 2022

Hey @st0012, I'd definitely be up for dropping faraday if we can. From seeing several issues/customer reports linked to the faraday dependency to having also personally dealt with similar dependency hell in a huge rails app as an end user, I think it makes a lot of sense.

Do you plan to go with plain Net::HTTP? I assume the complexity there is doing both the SSL and proxy cases.

@st0012
Copy link
Collaborator

st0012 commented Jan 4, 2022

@trevorturk Yeah the faraday ~> 2.0 requirement really blocked that path. I'll keep you updated about the progress!

@sl0thentr0py Yes I'm going with Net::HTTP and I've done some preparations for the migration a while ago:

This week I'll focus on releasing 4.9.0 and I think next weekend I can make a release candidate for 5.0.

This was referenced Jan 4, 2022
@st0012
Copy link
Collaborator

st0012 commented Jan 10, 2022

Updates:

  • Version 4.8.3 has gone out last week with faraday ~> 1.0.
  • Version 4.9.0 just went out as well.
  • The PR to drop faraday is almost ready and just waits for migrations guide - Goodbye faraday 👋  #1678

Since v5.0 will essentially be v4.9.0 without faraday, I'll wait a week to see if there's any issue on v4.9.0 first before releasing it.

@ojab
Copy link
Contributor

ojab commented Jan 12, 2022

I guess this one could be closed, since faraday-net_http was re-added lostisland/faraday#1366

@st0012
Copy link
Collaborator

st0012 commented Jan 12, 2022

Wow that's great news. I'll still follow our original plan to drop it though. And I'll close this with that 🙂

@trevorturk
Copy link
Author

This is great, thanks for the heads up @ojab!

@st0012 st0012 modified the milestones: 4.9.0, 5.0.0 Jan 14, 2022
@trevorturk
Copy link
Author

I just wanted to say that I was able to unlock and upgrade all gems including faraday after the faraday-net_http change and things are working well. I'm excited for sentry-ruby-core to use Net::HTTP instead, but things are nice and smooth now in any case. Thanks, all! (Feel free to close this issue whenever you'd like.)

4.x automation moved this from To do to Done Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.x
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants