support GeoLite2 ASN DB, which is availably only as tarball #16

sbma44 · 2018-03-19T19:38:03Z

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 · 2018-03-20T04:27:25Z

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 · 2018-03-20T04:46:39Z

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

- sprinkle in some ES6 syntax - drop node 4 testing, add node 9

…-mirror into add-v2-asn

codecov · 2018-03-20T15:30:15Z

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 · 2018-03-20T15:37:52Z

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.

msimerson

Much better, thanks! There's a few more issues I've commented on inline.

msimerson · 2018-03-20T16:16:23Z

package.json

@@ -8,7 +8,6 @@
    "url": "http://matt.simerson.net"
  },
  "contributors": {
-    "name": "Carl Banbury",


restore this line please.

msimerson · 2018-03-20T16:17:31Z

bin/maxmind-geolite-mirror

 const fs    = require('fs');
 const http  = require('http');
 const zlib  = require('zlib');
-
+const path = require('path');


please restore the blank line here and include path in the top list of "built-in" modules. config is separated by whitespace because it's a local dependency.

msimerson · 2018-03-20T16:18:22Z

bin/maxmind-geolite-mirror

 const config = require('../lib/config');
-


please restore all the blank lines. Whitespace is free and I prefer it.

msimerson · 2018-03-20T16:19:23Z

bin/maxmind-geolite-mirror

 function httpReqOpts () {
    return {
        method: 'HEAD',
        hostname: config.hostName,
        port: config.hostPort,
        headers: {
-            'User-Agent': config.userAgent,
+            'User-Agent': config.userAgent


Please restore the dangling comma.

sbma44 · 2018-03-20T16:26:55Z

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 · 2018-03-20T16:29:50Z

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.

msimerson

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

sbma44 · 2018-03-20T16:43:03Z

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.

support GeoLite2 ASN DB, which is availably only as tarball

20e8347

sbma44 force-pushed the add-v2-asn branch from ab9b172 to 20e8347 Compare March 19, 2018 19:39

sbma44 mentioned this pull request Mar 19, 2018

support GeoLite2 ASN DB, which is availably only as tarball mapbox/maxmind-geolite-mirror#1

Merged

sbma44 added 2 commits March 19, 2018 15:57

avoid duplicative var names

383bdc8

use renameSync to avoid warning

0018f31

Repository owner deleted a comment from codecov bot Mar 20, 2018

msimerson added a commit that referenced this pull request Mar 20, 2018

add geolite2-ASN db to list (from #16 by Tom Lee)

aca145e

- sprinkle in some ES6 syntax - drop node 4 testing, add node 9

msimerson mentioned this pull request Mar 20, 2018

add geolite2-ASN db to list #17

Merged

msimerson added a commit that referenced this pull request Mar 20, 2018

add geolite2-ASN db to list (from #16 by Tom Lee)

da3fba7

- sprinkle in some ES6 syntax - drop node 4 testing, add node 9

Merge branch 'master' of https://github.com/msimerson/maxmind-geolite…

380d077

…-mirror into add-v2-asn

sbma44 force-pushed the add-v2-asn branch from 0018f31 to 380d077 Compare March 20, 2018 15:30

preserve whitespace to keep diff clean

44dc15b

msimerson requested changes Mar 20, 2018

View reviewed changes

restore whitespace & Mr. Banbury

6e9371d

sbma44 force-pushed the add-v2-asn branch from 33e5a33 to 6e9371d Compare March 20, 2018 16:25

fix trailing whitespace

7108971

msimerson approved these changes Mar 20, 2018

View reviewed changes

msimerson merged commit b337dcc into msimerson:master Mar 20, 2018

sbma44 deleted the add-v2-asn branch March 20, 2018 16:41

msimerson mentioned this pull request Mar 20, 2018

test downloaded files and assure correct format #18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support GeoLite2 ASN DB, which is availably only as tarball #16

support GeoLite2 ASN DB, which is availably only as tarball #16

sbma44 commented Mar 19, 2018

msimerson commented Mar 20, 2018 •

edited

Loading

msimerson commented Mar 20, 2018

codecov bot commented Mar 20, 2018 •

edited

Loading

sbma44 commented Mar 20, 2018 •

edited

Loading

msimerson left a comment

msimerson Mar 20, 2018

msimerson Mar 20, 2018

msimerson Mar 20, 2018

msimerson Mar 20, 2018

sbma44 commented Mar 20, 2018

msimerson commented Mar 20, 2018

msimerson left a comment

sbma44 commented Mar 20, 2018

support GeoLite2 ASN DB, which is availably only as tarball #16

support GeoLite2 ASN DB, which is availably only as tarball #16

Conversation

sbma44 commented Mar 19, 2018

msimerson commented Mar 20, 2018 • edited Loading

msimerson commented Mar 20, 2018

codecov bot commented Mar 20, 2018 • edited Loading

Codecov Report

sbma44 commented Mar 20, 2018 • edited Loading

msimerson left a comment

Choose a reason for hiding this comment

msimerson Mar 20, 2018

Choose a reason for hiding this comment

msimerson Mar 20, 2018

Choose a reason for hiding this comment

msimerson Mar 20, 2018

Choose a reason for hiding this comment

msimerson Mar 20, 2018

Choose a reason for hiding this comment

sbma44 commented Mar 20, 2018

msimerson commented Mar 20, 2018

msimerson left a comment

Choose a reason for hiding this comment

sbma44 commented Mar 20, 2018

msimerson commented Mar 20, 2018 •

edited

Loading

codecov bot commented Mar 20, 2018 •

edited

Loading

sbma44 commented Mar 20, 2018 •

edited

Loading