support GeoLite2 ASN DB, which is availably only as tarball

[sbma44]

unlike the other databases, curl -I http://geolite.maxmind.com/download/geoip/database/GeoLite2-ASN.mmdb.gz returns a 404. It is available as a .tar.gz file linked from https://dev.maxmind.com/geoip/geoip2/geolite2/ . In order to support this, I've added handling for config.geoIpDbs entries that point to gzipped tarballs.

[msimerson]

As I noted in DEVELOP.md, I prefer keeping the dependency list to a minimum. How would you feel about? :

• encapsulating your block of tar.gz expansion code in a named (vs anonymous) function, and instead of a giant else, use this manner of flow control:

if (/\.tar\.gz$/.test(opts.path)) {
    myUntarFunction();
    return;
}

var file = fs.createWriteStream(dest + '.tmp');
...

• using tar-stream instead (it used to be ~10x faster, maybe still is?)
• declare tmp and tar-stream as optionalDependencies, and lazy loading them when a tar.gz file is encountered

[msimerson]

Interesting. I just added the GeoLite2-ASN declaration to config.js and was surprised to find that it downloaded the file just fine:

[matt@imac27] ~/git/maxmind-geolite-mirror $ npm test

> maxmind-geolite-mirror@1.0.8 test /Users/matt/git/maxmind-geolite-mirror
> mocha

  maxmind-geolite-mirror
    ✓ can be required

  config
    ✓ has dbDir
    ✓ has custom dbDir
    ✓ has geoIpDbs
    ✓ has UserAgent
    ✓ has hostName
    ✓ has hostPort
    ✓ has urlPath

  8 passing (49ms)

downloading /usr/local/share/GeoIP/GeoIP.dat
saved /usr/local/share/GeoIP/GeoIP.dat
downloading /usr/local/share/GeoIP/GeoIPCity.dat
saved /usr/local/share/GeoIP/GeoIPCity.dat
downloading /usr/local/share/GeoIP/GeoIPCityv6.dat
saved /usr/local/share/GeoIP/GeoIPCityv6.dat
downloading /usr/local/share/GeoIP/GeoIPv6.dat
saved /usr/local/share/GeoIP/GeoIPv6.dat
downloading /usr/local/share/GeoIP/GeoIPASNum.dat
saved /usr/local/share/GeoIP/GeoIPASNum.dat
downloading /usr/local/share/GeoIP/GeoIPASNumv6.dat
saved /usr/local/share/GeoIP/GeoIPASNumv6.dat
downloading /usr/local/share/GeoIP/GeoLite2-Country.mmdb
saved /usr/local/share/GeoIP/GeoLite2-Country.mmdb
downloading /usr/local/share/GeoIP/GeoLite2-City.mmdb
saved /usr/local/share/GeoIP/GeoLite2-City.mmdb
downloading /usr/local/share/GeoIP/GeoLite2-ASN.mmdb
saved /usr/local/share/GeoIP/GeoLite2-ASN.mmdb

[codecov[bot]]

Codecov Report

Merging #16 into master will not change coverage. The diff coverage is n/a.

@@          Coverage Diff          @@
##           master    #16   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           6      6           
  Branches        1      1           
=====================================
  Hits            6      6

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b360e6c...7108971. Read the comment docs.

[sbma44]

Thanks for the quick response!

I have switched dependencies to tar-stream and moved the code into a named function. tar-stream's interface is a better fit and allowed me to ditch two other dependencies. Thank you for the suggestion.

I am less sure about the optionalDependency setup's benefits -- the default config state will wind up with that code always getting invoked. I love zero-dependency packages too but I think Maxmind's decision (and likely future policy?) has made that untenable. But I defer to your sense of this. If you will accept tar-stream as a dependency I can tidy away that try...except block.

Interesting. I just added the GeoLite2-ASN declaration to config.js and was surprised to find that it downloaded the file just fine

You had success adding the gzipped tarball to the config with the existing code because the code gunzips the file and then renames whatever comes out to the desired output filename. The tarball is a valid gzip so no error is thrown. But in this case the result will be a (misnamed) uncompressed tar file, not a valid mmdb.

Ideally that would have been caught by tests. The test script as it stands checks for the ability to require() but that's more or less a syntax check. If you'd like to add a functional test I can add one without much trouble, but this will require a devDependency and the addition of a Maxmind tarball (or two, to cover the gzip case) as a test fixture so as to avoid making network requests in the test script.

[sbma44]

Apologies to Mr. Banbury -- I thought he was removed in your merge to master. The excision wasn't intentional. The killed blank lines were the result of an overly greedy regex attempting to satisfy the trailing whitespace linting rule. We will have to agree to disagree on the wisdom of trailing commas but I have made the change you requested.

[msimerson]

I am less sure about the optionalDependency setup's benefits

As an optionalDependency, the package will still get installed. The only difference is that if the install fails for some reason, npm can still successfully install this package.

But in this case the result will be a (misnamed) uncompressed tar file, not a valid mmdb.

Uh oh! That's no good.

A test case is definitely preferable to catch this. There are a couple ways it could be done. One that doesn't would be to "taste" the file with the file utility, or perhaps just with fs.

I don't have strong biases against dev deps but I do like to keep the package nice and small, and downloading an entire tarball from maxmind just to test would likely bloat the test times a fair bit. Dropping a sample file in tests/fixtures with a tar header to test the code would however be very fast.

[sbma44]

I'll create a new issue regarding a test for tarball.

Sounds good. I will keep an eye out for this; in order to account for the seemingly-correct behavior you observed I believe it will be necessary to stub http.request and export the full download function.