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

fix(#124): Improve localhost handling, add cache for ipv6 & ipv4 check #1136

Merged
merged 1 commit into from Dec 4, 2023

Conversation

BrandonGillis
Copy link
Contributor

@BrandonGillis BrandonGillis commented Dec 4, 2023

Description

this fix #1114 PR (#124 issue) , by properly implementing localhost, @helloanoop this time I did multiple tests on linux & windows and it works nicely, as a bonus, it also fix [::1] axios issue. Could you check if it's good for you too ?

The cache can create issue though, if you have your localhost listening on ipv6, doing a request with bruno, then changing localhost to ipv4, bruno will still try to reach ipv6, anyway that's a minor issue and a restart of bruno will clear the cache

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

@BrandonGillis
Copy link
Contributor Author

BrandonGillis commented Dec 4, 2023

Before merging, I still need to handle the case of doing a dns.lookup first, if localhost have a custom IP inside the hosts file.
Anyway if you have time could you test this change on your machine to see if this is going to the right direction ?

@BrandonGillis BrandonGillis marked this pull request as draft December 4, 2023 12:56
@helloanoop
Copy link
Contributor

Awesome! I confirm that this is working on my machine @BrandonGillis !

@helloanoop
Copy link
Contributor

Before merging, I still need to handle the case of doing a dns.lookup first, if localhost have a custom IP inside the hosts file.

@BrandonGillis Can we do that as a separate PR? I think we can ship this

@BrandonGillis
Copy link
Contributor Author

Before merging, I still need to handle the case of doing a dns.lookup first, if localhost have a custom IP inside the hosts file.

@BrandonGillis Can we do that as a separate PR? I think we can ship this

No problem for me, we can do that! :)

@helloanoop helloanoop marked this pull request as ready for review December 4, 2023 13:50
@helloanoop helloanoop merged commit 6d3a518 into usebruno:main Dec 4, 2023
2 checks passed
@helloanoop helloanoop mentioned this pull request Dec 4, 2023
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.

None yet

2 participants