-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Enable download of GeoLite2 databases #4896
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #4896 +/- ##
==========================================
- Coverage 58.5% 58.44% -0.06%
==========================================
Files 88 88
Lines 6719 6729 +10
==========================================
+ Hits 3931 3933 +2
- Misses 2359 2365 +6
- Partials 429 431 +2
Continue to review full report at Codecov.
|
/retest |
3 similar comments
/retest |
/retest |
/retest |
This patch cherrypicks the commit 74944b9 to allow building without the maxmind geoip2 database.
geoIPPath = "/etc/nginx/geoip" | ||
|
||
geoLiteCityDB = "GeoLite2-City" | ||
geoLiteASNDB = "GeoLite2-ASN" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commercial GeoIP2 databases are more accurate than GeoLite2 ones. We use GeoIP2-ISP
instead of GeoLite2-ASN
. So with this design I had to create a placeholder GeoLite2-ASN.mmdb
file to trick the controller into believing this file exist so that it does not force disable use-geoip2
setting.
I think instead of this the controller should get the comma separated list of DB file names as a command line arrgument and then check for existence of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be more dramatic than that. Right now those files are present in the docker image and are thirteen months old. The old geoip module databases are not maintained anymore.
Before any change, I think we should consider drop support for the old geoip module.
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #4881Special notes for your reviewer: