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

chore: remove dns-lookup-cache #3389

Merged
merged 1 commit into from
Nov 29, 2022
Merged

Conversation

Walther
Copy link
Contributor

@Walther Walther commented Nov 29, 2022

What this PR does / why we need it:

Fixes the regression in local Docker Desktop Kubernetes introduced in #3374 due to some adverse interaction with the DNS caching library we used.

Which issue(s) this PR fixes:

Replaces the need to revert the changes with #3387

Special notes for your reviewer:

The dns-lookup-cache library has its latest commit at nearly 3 years ago, so we should remove it anyway. If we absolutely need a DNS caching solution, we should figure out an alternative.

@Orzelius
Copy link
Contributor

Wow good job on catching this, I was scratching my head pretty bad with this

@vvagaytsev
Copy link
Collaborator

Great job, @Walther! Thank you! 🚀

Copy link
Contributor

@Orzelius Orzelius left a comment

Choose a reason for hiding this comment

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

Code changes look good, but lets see the test results before merging. I presume you tested basic mac functionality on your machine.

@Walther Walther merged commit 3524401 into main Nov 29, 2022
@Walther Walther deleted the 2022-11-29-remove-dns-lookup-cache branch November 29, 2022 11:08
@edvald
Copy link
Collaborator

edvald commented Nov 29, 2022

FYI, the original reason for including that library was an issue for users running Rancher, bumping into dropped DNS requests and therefore connection issues with clusters. We should be aware of potential implications but I still agree that we should likely drop this library.

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.

4 participants