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

Integrate with MaxMind out of the box #766

Merged
merged 12 commits into from Dec 14, 2020
Merged

Conversation

chadwhitacre
Copy link
Member

@chadwhitacre chadwhitacre commented Dec 9, 2020

Closes #310.

Todo

@chadwhitacre
Copy link
Member Author

@chadwhitacre chadwhitacre changed the title Integrate with MaxMind GeoIP2 out of the box Integrate with MaxMind out of the box Dec 9, 2020
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
nginx/nginx.conf Outdated Show resolved Hide resolved
nginx/nginx.conf Outdated Show resolved Hide resolved
chadwhitacre added a commit to getsentry/develop that referenced this pull request Dec 9, 2020
@chadwhitacre chadwhitacre marked this pull request as ready for review December 9, 2020 22:43
chadwhitacre added a commit to getsentry/develop that referenced this pull request Dec 9, 2020
@BYK
Copy link
Collaborator

BYK commented Dec 10, 2020

Looks pretty good so far, great job!

@chadwhitacre
Copy link
Member Author

Greetings! 👋 My name is ChadBot 🤖, and I will be performing your manual continuous integration services for this PR. 🐭

Commit

c53d8a9

Test Matrix

conf mmdb expected
missing missing error
missing present error
bad present error
good present done
bad missing error
good missing done

Test Run

$ ./install/geoip.sh
Setting up IP address geolocation ...
Installing (empty) IP address geolocation database ... done.
IP address geolocation is not configured for updates.
See https://develop.sentry.dev/self-hosted/geolocation/ for instructions.
Error setting up IP address geolocation.
$
$
$ ./install/geoip.sh
Setting up IP address geolocation ...
IP address geolocation database already exists.
IP address geolocation is not configured for updates.
See https://develop.sentry.dev/self-hosted/geolocation/ for instructions.
Error setting up IP address geolocation.
$
$
$ touch geoip/GeoIP.conf
$
$ ./install/geoip.sh
Setting up IP address geolocation ...
IP address geolocation database already exists.
IP address geolocation is configured for updates.
Updating IP address geolocation database ...
Creating sentry_onpremise_geoipupdate_run ...
Creating sentry_onpremise_geoipupdate_run ... done
error loading configuration file /sentry/GeoIP.conf: the `EditionIDs` option is required
Error updating IP address geolocation database.
Error setting up IP address geolocation.
$
$
$ cp ~/Desktop/GeoIP.conf geoip 
$
$ ./install/geoip.sh           
Setting up IP address geolocation ...
IP address geolocation database already exists.
IP address geolocation is configured for updates.
Updating IP address geolocation database ... 
Creating sentry_onpremise_geoipupdate_run ... 
Creating sentry_onpremise_geoipupdate_run ... done
Done updating IP address geolocation database.
Done setting up IP address geolocation.
$
$
$ rm geoip/GeoIP.conf
$ rm geoip/GeoLite2-City.mmdb
$ touch geoip/GeoIP.conf
$
$ ./install/geoip.sh
Setting up IP address geolocation ...
Installing (empty) IP address geolocation database ... done.
IP address geolocation is configured for updates.
Updating IP address geolocation database ...
Creating sentry_onpremise_geoipupdate_run ...
Creating sentry_onpremise_geoipupdate_run ... done
error loading configuration file /sentry/GeoIP.conf: the `EditionIDs` option is required
Error updating IP address geolocation database.
Error setting up IP address geolocation.
$
$
$ rm geoip/GeoIP.conf 
$ rm geoip/GeoLite2-City.mmdb
$ cp ~/Desktop/GeoIP.conf geoip 
$
$ ./install/geoip.sh 
Setting up IP address geolocation ...
Installing (empty) IP address geolocation database ... done.
IP address geolocation is configured for updates.
Updating IP address geolocation database ... 
Creating sentry_onpremise_geoipupdate_run ... 
Creating sentry_onpremise_geoipupdate_run ... done
Done updating IP address geolocation database.
Done setting up IP address geolocation.
$
$
$ ls -l geoip 
total 65864
-rw-r--r--  1 chadwhitacre  staff        70 Dec 11 10:02 GeoIP.conf
-rw-r--r--  1 chadwhitacre  staff  66456088 Dec 11 10:02 GeoLite2-City.mmdb
-rw-r--r--  1 chadwhitacre  staff      1055 Dec 10 11:47 GeoLite2-City.mmdb.empty
$

Result

💃

@chadwhitacre chadwhitacre mentioned this pull request Dec 11, 2020
README.md Outdated Show resolved Hide resolved
geoip/.gitignore Outdated Show resolved Hide resolved
@@ -325,6 +325,10 @@ if [[ ! -f "$RELAY_CREDENTIALS_JSON" ]]; then
echo "Relay credentials written to $RELAY_CREDENTIALS_JSON"
fi


./install/geoip.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know install.sh is getting a bit unwieldy but does splitting geoip part only help much?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. By splitting out the geoip part I am able to test it in isolation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ehh, I don't like the fact that we are doing this only for one part of the file. Let's do this only if you have a plan for refactoring the remainder of ./install.sh file without bloating the root directory with many .sh files and replicating all common things across them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't either. :) I think we should refactor install.sh to make it more modular and therefore more testable ... and we should test it! 😁 I don't see this bloating the root directory with many .sh files, you can see here I split the subscript into an install/ directory. Yes we can and will share components between these scripts ... again, one step at a time.

install/geoip.sh Show resolved Hide resolved
relay/config.example.yml Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@@ -325,6 +325,10 @@ if [[ ! -f "$RELAY_CREDENTIALS_JSON" ]]; then
echo "Relay credentials written to $RELAY_CREDENTIALS_JSON"
fi


./install/geoip.sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ehh, I don't like the fact that we are doing this only for one part of the file. Let's do this only if you have a plan for refactoring the remainder of ./install.sh file without bloating the root directory with many .sh files and replicating all common things across them.

install/geoip.sh Show resolved Hide resolved
install/geoip.sh Show resolved Hide resolved
@chadwhitacre chadwhitacre merged commit a623e72 into master Dec 14, 2020
@chadwhitacre chadwhitacre deleted the chadwhitacre/geoip branch December 14, 2020 16:56
install/geoip.sh Show resolved Hide resolved
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GEOIP_PATH_MMDB support
3 participants