-
Notifications
You must be signed in to change notification settings - Fork 352
Add support for Gun 2.0 - Feat: update hackney #625
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
Add support for Gun 2.0 - Feat: update hackney #625
Conversation
test/tesla/adapter/gun_test.exs
Outdated
@@ -186,14 +186,14 @@ defmodule Tesla.Adapter.GunTest do | |||
assert response.status == 500 | |||
end | |||
|
|||
test "error on socks proxy" do | |||
test "error waiting for response on socks proxy" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yordis maybe we just can get rid of this test since socks proxy is already supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see why not, but what about v1
people? I am just wondering if this will impose a major Tesla version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you are right about the v1. In my opinion, I'd go for enforcing >= 2.0 and release a major version of tesla.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭 😭 😭 😭 😭 😭 😭 😭 😭 😭 😭 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..... right 😭
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible, and we did that for poison 3.x but I just noticed that this test was removed in #584 🧐
There is also missing test checking current codebase against 1.0.0 tests 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I see si you rely on a test mix.lock file, dark magic 🧙 . Cool, I'll do it in that way 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok guys. Please take a look again. I thought about defining two modules based on the version and duplicating the whole files but, from now, I think that we can keep it like that as the differences are very small.
Notice that I'm doing the check more explicitly, using Version
instead of function_exported?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: please, could you re-run the failed tests? I think they are flaky ones [test/tesla/middleware/retry_test.exs]. Edit: it looks like shared the RetryTest is using an adapter that is not able to be shared in concurrent runs, I've selected the only test that we should care about in gun 1.0
in the workflow.
I've also tried to add tests for Elixir 1.15 but it looks like there is something still pending to be done, according to the errors: https://github.com/elixir-tesla/tesla/actions/runs/6517890595/job/17702775428?pr=625
What is the minimal required version of |
Co-authored-by: Yordis Prieto <[email protected]>
@carrascoacd I just made a mistake ... do you mind opening the PR against We can close the |
* chore(deps): bump gun from 1.3.3 to 2.0.1 Bumps [gun](https://github.com/ninenines/gun) from 1.3.3 to 2.0.1. - [Commits](ninenines/gun@1.3.3...2.0.1) --- updated-dependencies: - dependency-name: gun dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Yordis Prieto <[email protected]> * fix on exit assertion * Add support for Gun 2.0 - Feat: update hackney (#625) * Remove notice for socks proxy support and use gun:headers instead of gun:request * Fix lint errors * Add backwards-compatibility for gun1 * Use gun 1.x * Fix linting * Add Elixir 1.15 and OTP 26.1 to test matrix * Fix matrix * Fix matrix typo * Update hackney * Add concurrency to avoid shared access * Run only gun tests to avoid concurrency problems * Make explicit the gun job * Leave tests for Elixir 1.15 and OTP 26 for another iteration * Fix warnings and improve doc * Update .github/workflows/test.yml Co-authored-by: Yordis Prieto <[email protected]> * Move start_link to a separate process in order to avoid race conditions --------- Co-authored-by: Yordis Prieto <[email protected]> --------- Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Yordis Prieto <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Antonio <[email protected]>
Thanks! Nothing to sorry about. |
Solves the compatibility problems detected on Gun 2.0 #587 cc/ @yordis