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

Support multiple address resolution in DNS requests #49020

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

sarchar
Copy link
Contributor

@sarchar sarchar commented May 24, 2021

This change was previously merged into 2.1 as commit 010a343 but I don't see any build that actually was released with this change. The feature is extremely useful for certain network load balancing.

I've git cherry-pick'd and resolved the conflicts, and updated a few of the lines to make sure they work in 3.x.

Let me know if anything needs fixing.

Cheers

@akien-mga
Copy link
Member

Some comments:

  • PR should be made against the master branch in priority. Otherwise that's yet another feature which you might have in 3.x and not in 4.0+, leading to the exact same situation again.
  • The commit message should be amended to state what the commit does, not what you did to create it. Referencing the cherry-picked commit is good information but it should go in the body of the commit, the title should actually say what is being implemented.
  • I have no clue where this 2.1 commit comes from. There's no PR from this author that has been merged. It's also pretty weird that I would have merged this in 2.1 and not in master at the time...

@sarchar
Copy link
Contributor Author

sarchar commented May 24, 2021

This is the pull request from the original author: #12397 - it looks like it was "merged manually", by you.

I'll update the commit message to be what it does and issue a new PR against master.

@akien-mga
Copy link
Member

akien-mga commented May 24, 2021

Thanks for finding it, GitHub search had failed me. Looks like it was indeed fully reviewed and approved by @Faless, but my comment asking for a master port was left unanswered. Thanks for solving this 4 years later :D

You can make a new PR for master and keep this one for 3.x if the code is not trivially cherry-pickable between the two.

@akien-mga akien-mga requested a review from a team May 24, 2021 10:34
@sarchar
Copy link
Contributor Author

sarchar commented May 24, 2021

Looks like the only issue to master is the IP_Address -> IPAddress change. I'll have a new PR for master soon.

drivers/unix/ip_unix.h Outdated Show resolved Hide resolved
Add two new functions to the IP class that returns all addresses/aliases associated with a given address.

This is a cherry-pick merge from 010a343 which was merged in 2.1, and has been updated to build with the latest code.

This merge adds two new methods IP.resolve_hostname_addresses and IP.get_resolve_item_addresses that returns a List of all addresses returned from the DNS request.
@akien-mga akien-mga changed the title Get all addresses associated with dns request Support multiple address resolution in DNS requests Jun 9, 2021
@akien-mga akien-mga merged commit 1c86395 into godotengine:3.x Jun 9, 2021
@akien-mga
Copy link
Member

Thanks!

@sarchar
Copy link
Contributor Author

sarchar commented Jun 9, 2021

Thanks!

Excellent!

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

Successfully merging this pull request may close these issues.

None yet

2 participants