Skip to content

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

Conversation

carrascoacd
Copy link
Contributor

@carrascoacd carrascoacd commented Oct 10, 2023

Solves the compatibility problems detected on Gun 2.0 #587 cc/ @yordis

@carrascoacd carrascoacd changed the title Remove notice for socks proxy support and use gun:headers instead of … Add support for Gun 2.0 Oct 10, 2023
@@ -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
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

#587 (comment)

nevermind

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

😭 😭 😭 😭 😭 😭 😭 😭 😭 😭 😭 😭

Copy link
Member

Choose a reason for hiding this comment

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

..... right 😭

Copy link
Member

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 🤔

Copy link
Contributor Author

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 😀

Copy link
Contributor Author

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

Copy link
Contributor Author

@carrascoacd carrascoacd Oct 13, 2023

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.0in 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

@yordis
Copy link
Member

yordis commented Oct 10, 2023

What is the minimal required version of :gun dep?

@carrascoacd carrascoacd changed the title Add support for Gun 2.0 Add support for Gun 2.0 - Feat: update hackney; Elixir 1.15/OTP 26 compatibility Oct 14, 2023
@carrascoacd carrascoacd changed the title Add support for Gun 2.0 - Feat: update hackney; Elixir 1.15/OTP 26 compatibility Add support for Gun 2.0 - Feat: update hackney Oct 14, 2023
@yordis yordis merged commit ea09209 into elixir-tesla:dependabot/hex/gun-2.0.1 Oct 16, 2023
@yordis
Copy link
Member

yordis commented Oct 25, 2023

@carrascoacd I just made a mistake ... do you mind opening the PR against master instead of the dependabot branch instead?

We can close the dependabot/hex/gun-2.0.1 and use yours! I am sorry!

yordis added a commit that referenced this pull request Oct 25, 2023
* 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]>
@yordis
Copy link
Member

yordis commented Oct 25, 2023

#630 I fixed at #630

Your contributions are there!

My apologies for the mess, I realized the target branch wasn't master after I sent the wrong message to dependabot ... sorry!

@carrascoacd
Copy link
Contributor Author

Thanks! Nothing to sorry about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants