pelias / openaddresses

Pelias import pipeline for OpenAddresses.
MIT License
51 stars 43 forks source link

Revert "Update tmp to the latest version 🚀" #473

Closed missinglink closed 3 years ago

missinglink commented 3 years ago

Reverts pelias/openaddresses#466

Unfortunately I have to revert this change because the authors of npm tmp don't adhere to the semantic versioning spec and have recently been publishing a bunch of breaking changes under the minor versions 0.2.x 😢

More specifically, the following change was introduced in a minor version which broke our download script:

# version 0.1.0
> require('tmp').tmpNameSync({prefix: 'foo', dir: '/code', postfix: '.zip'})
'/code/foo16hKdNdr67yY0D.zip'

# version 0.2.1
> require('tmp').tmpNameSync({prefix: 'foo', dir: '/code', postfix: '.zip'})
Thrown:
Error: dir option must be relative to "/tmp", found "/code".
    at _assertIsRelative (/code/pelias/openaddresses/node_modules/tmp/lib/tmp.js:599:13)
    at _assertAndSanitizeOptions (/code/pelias/openaddresses/node_modules/tmp/lib/tmp.js:513:5)
    at Object.tmpNameSync (/code/pelias/openaddresses/node_modules/tmp/lib/tmp.js:105:3)
missinglink commented 3 years ago

related https://github.com/raszi/node-tmp/issues/258

orangejulius commented 3 years ago

Ah, that's unfortunate. I think the semantic versioning spec does allow for breaking changes at the 0.x level for pre version 1 packages though: https://semver.org/#spec-item-4

orangejulius commented 3 years ago

It's always been frustrating though, that we don't have any tests for the download process. This is probably because it's hard and quite a bit of work to mock out all of the "actual" work a downloader does, and in that case those tests would have probably not covered the breakage created by this upgrade anyway.

But do you think there are things we could do in our tests to detect future breakage like this?

missinglink commented 3 years ago

I was thinking about that today.

What I did to test it manually was to create a custom pelias.json file with the imports.openaddresses section defined.

Then I created the download dir and ran PELIAS_CONFIG=xxx npm run download and looked at the output and checked the download dir contained the extracted files.

This had to be done twice, since the presence of the files property makes it use a different script.

This could potentially be automated and run before confirming the image is valid, although I doubt we'd want to wait for the global file to download every time!

IIRC there is an env var which can be used to change the host name used for downloads, this could be changed to a static s3 bucket containing small files for testing.

missinglink commented 3 years ago

Or we just delete all the bash scripts and replace them with node scripts.

missinglink commented 3 years ago

This https://semver.org/#spec-item-4 clause is scary:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

For the npm tmp module, which is 6 years old and describes itself as "a widely used library" this is double scary.

We should probably have a policy of not using anything 0.x.x in production.

orangejulius commented 3 years ago

We should probably have a policy of not using anything 0.x.x in production.

Yeah, minimizing them definitely makes sense. We have quite a lot of them though:

$ grep '\^0' */package.json
analysis/package.json:    "remove-accents": "^0.4.1",
api/package.json:    "@mapbox/geojson-extent": "^0.3.1",
api/package.json:    "geojson": "^0.5.0",
api/package.json:    "locale": "^0.1.0",
api/package.json:    "markdown": "^0.5.0",
api/package.json:    "pelias-compare": "^0.1.16",
api/package.json:    "retry": "^0.12.0",
api/package.json:    "stable": "^0.1.8",
api/package.json:    "istanbul": "^0.4.2",
compare/package.json:    "@fortawesome/vue-fontawesome": "^0.1.9",
compare/package.json:    "regenerator-runtime": "^0.13.3",
csv-importer/package.json:    "temp": "^0.9.0"
dbclient/package.json:    "intercept-stdout": "^0.1.2",
geocoder-test-suite/package.json:    "pelias-fuzzy-tester": "^0.10.6",
geonames/package.json:    "cli-table": "^0.3.0",
geonames/package.json:    "unzipper": "^0.10.0"
interpolation/package.json:    "cli-table3": "^0.6.0",
leaflet-plugin/package.json:    "leaflet": "^0.7.x || ^1.0.0 || >=1.0.0-beta.1 || >=1.0.0-rc.1"
mars-importer/package.json:    "deep-diff": "^0.3.3",
mars-importer/package.json:    "temp": "^0.8.3"
openaddresses/package.json:    "temp": "^0.9.1",
openstreetmap/package.json:    "istanbul": "^0.4.3",
parser/package.json:    "cluster": "^0.7.7",
parser/package.json:    "remove-accents": "^0.4.2",
pip-service/package.json:    "temp": "^0.9.0"
placeholder/package.json:    "remove-accents": "^0.4.0",
placeholder/package.json:    "sorted-intersect": "^0.1.4",
polylines/package.json:    "@mapbox/geojson-extent": "^0.3.2",
scripts-batch-search/package.json:    "csv-stream": "^0.2.0",
spatial/package.json:    "handlebars-helpers": "^0.10.0",
spatial/package.json:    "locale": "^0.1.0",
spatial/package.json:    "wkx": "^0.4.8",
whosonfirst/package.json:    "iso3166-1": "^0.5.0",
whosonfirst/package.json:    "temp": "^0.9.0"
wof-admin-lookup/package.json:    "stable": "^0.1.8",
wof-admin-lookup/package.json:    "intercept-stdout": "^0.1.2",
wof-admin-lookup/package.json:    "temp": "^0.9.0"
wof/package.json:    "tmp": "^0.1.0",