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

Update the GeoIp database download method [BREAKING] #1990

Merged
merged 2 commits into from
Jan 21, 2020
Merged

Conversation

sgdc3
Copy link
Member

@sgdc3 sgdc3 commented Jan 20, 2020

Now GeoIp database updates require a ClientID and a LicenseKey, which can be obtained for free at https://www.maxmind.com/en/accounts/current/license-key

Now GeoIp database updates require a ClientID and a LicenseKey, which can be obtained for free at https://www.maxmind.com/en/accounts/current/license-key
@sgdc3
Copy link
Member Author

sgdc3 commented Jan 20, 2020

#1979

@Xephi
Copy link
Contributor

Xephi commented Jan 20, 2020

Maybe we can let users choose between downloading the whole database or using the api directly ?
https://github.com/maxmind/GeoIP2-java

@ljacqu
Copy link
Member

ljacqu commented Jan 21, 2020

Great job! I have two questions about error scenarios:

  1. if one of the config values is empty, could we log something meaningful? (Pointing them to the config.yml, maybe to the properties‘ path)
  2. if the download somehow fails, the plugin should still work and start up normally, right?

@sgdc3
Copy link
Member Author

sgdc3 commented Jan 21, 2020

@Xephi the api isn't as fast as the local db, i personally had issues with antibot plugins using the rest api, due to rate limits and the performance impact

@sgdc3
Copy link
Member Author

sgdc3 commented Jan 21, 2020

@ljacqu

  1. Ok, there is already a message but it doesn't point to the config path
  2. Yes, the geoip check will just be ignored, and a warning message will be printed on startup

@sgdc3 sgdc3 merged commit a43127d into master Jan 21, 2020
@sgdc3 sgdc3 deleted the geoip-update branch January 21, 2020 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants