Skip to content

adds function to get geolocation from domain names using round robin dns#58

Open
kf0jvt wants to merge 6 commits intoyolothreat:masterfrom
kf0jvt:roundrobin
Open

adds function to get geolocation from domain names using round robin dns#58
kf0jvt wants to merge 6 commits intoyolothreat:masterfrom
kf0jvt:roundrobin

Conversation

@kf0jvt
Copy link
Contributor

@kf0jvt kf0jvt commented May 21, 2015

This came up for discussion in our python code review club meeting earlier this week. The domain_to_geo function passes the domain directly to pygeoip which returns a location. However if the domain being searched uses round-robin DNS for some reason then the domain may resolve to multiple IP addresses which could be in several places.

This PR adds a function called domain_to_mgeo which will get all the ip addresses that the domain resolves to and then get location info for each of them, returning a list of dictionaries. In order to honor the promises made by previous functions, the existing domain_to_geo function continues to work the way it did before but it does raise a warning if a domain resolves to multiple ip addresses.

@krmaxwell
Copy link
Member

Once #59 is merged into master, you can merge that into here (or just do that from your own branch) so we can get happy ✅

Copy link
Member

Choose a reason for hiding this comment

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

We need a test case that exercises this conditional. Maybe google.com or such?

Copy link
Member

Choose a reason for hiding this comment

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

For context, we only have one line that is not getting covered by tests.

@kf0jvt
Copy link
Contributor Author

kf0jvt commented May 22, 2015

you mean we need to add a test to test_domain_to_geo where we pass something like heroku.com so that it tests the conditional? Or do we need to write the test so that it checks if a warning is raised? Because that second one seems more difficult.

@krmaxwell
Copy link
Member

Oops, missed your question here. The first one - it'll test the conditional and raise the warning and that'll be good.

@sroberts
Copy link
Member

This shows as good to go, is it?

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