search

django / django-docker-box

Run the Django test suite across all supported databases and python versions
116 stars 37 forks source link

Fix Maxmind GeoIP downloads #14

Closed orf closed 7 months ago

orf commented 4 years ago

As per https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-geolite2-databases/, the GeoIP database for Maxmind won’t be available without a usage key. When this change is made the docker image will no longer build.

Missing these tests is a shame, but I cannot see how we can include the database in the image at all from now on.

ngnpope commented 3 years ago

@orf I was actually thinking about this the other day as I had some improvements I wanted to make and my copy of the database then caused a bunch of test failures because it was clearly different from the version that is being used on Jenkins. What's more, as part of the terms of using the database, MaxMind require you to ensure that you keep downloading new versions - something that probably doesn't happen on Jenkins. (IIRC this is because they get requests to remove data that must be complied with.)

So I did a little digging and came across this article which mentions that there are some test databases available. I was thinking that we could pull one of these in and configure the default settings in the test suite to always specify the path to that database? Then we wouldn't need to worry about downloading database updates periodically for simple test purposes. What do you think?

orf commented 3 years ago

Yes! I really like this idea. I think we should perhaps just vendor a few of them in Django itself - there are a number of files there for edge-cases, but we just really want a database with a couple of entries to test with. Having something that auto-downloads these, or even a submodule, might be overkill.

What do you think? Should be simple to add to core.

ngnpope commented 3 years ago

Yup. I'll see if I can get around to it this week. Shouldn't be difficult.

orf commented 3 years ago

FYI I've done this here: https://github.com/django/django/pull/14215

felixxm commented 7 months ago

Fixed by https://github.com/django/django/commit/a93375e8ab26e7cb22c6ee7e4bf23bf2d6ed170d

ngnpope commented 7 months ago

Strictly speaking we should remove GEOIP_PATH from settings/test_*_gis.py in this repo to fully solve this.

By the way, I'm working on overhauling this to fix a load of issues. It's rather hack and slash at the moment -- see https://github.com/ngnpope/django-docker-box/commit/e9d8633fef4a9dc1add36f7b1c16741d50acb8c9. Wondering if it's easier to just have a "fix all the things" approach than splitting out into small pieces as things have stagnated for so long... 🤔 Anyway - a few more things I want to resolve first.

felixxm commented 7 months ago

The current version works for me so I'm not sure that we need so massive changes, but I will not stand against them :shrug:

ngnpope commented 7 months ago

Sure, some things work, but many other things either don't or could work better. Let's see where I get with it - there's some other things I wanted to sort out and more testing required before I put up a PR.