mapbox / npm-internal

helps you package internal npm modules and upload them to your own s3 bucket.
MIT License
11 stars 3 forks source link

when target package is a git repo, check for stray files #11

Closed sbma44 closed 9 years ago

sbma44 commented 9 years ago

npm's packaging behavior tars up everything in the working directory. This has bitten me several times, often due to redis dropping dump.rdb in a terminal I forgot I had launched it from. This resulted, most recently, in a much-too-large tarball and mysterious testfails on CircleCI. AFAIK we have never leaked sensitive information this way, but that's possible, too.

This PR:

This code shells out to list files with git and tar (it also depends on awk being available). I think there isn't a strong case for using node-native modules, but if anyone disagrees lmk and I can nodeify this up.

cc @yhahn @mick @freenerd

sbma44 commented 9 years ago

@mick any thoughts on this? my own npm-internal install is in limbo at the moment, would love to either fish or cut bait on this

mick commented 9 years ago

@sbma44 yeah this looks good. We also should figure out a better way to do this with npm / public modules.

One question, why us exec-sync module over the built in execSync function

sbma44 commented 9 years ago

One question, why us exec-sync module over the built in execSync function

Ignorance! I'll fix then merge if everything's passing

sbma44 commented 9 years ago

woof, took me a while to track down why travis was failing -- different awk behavior on OS X. nasty.

sbma44 commented 9 years ago

worth noting that child_process.execSync requires node 0.12. Presumably not a problem within mapbox.

sbma44 commented 9 years ago

@mick turns out 0.12 is a problem for a ton of stuff with node-pre-gyp dependencies. I'm going to go back to exec-sync for the sake of the rest of the team -- @jfirebaugh tells we're running 0.10 by and large

mick commented 9 years ago

make sense, I didnt see that it was 0.12 only.. lets use a exec sync module that doesnt have "this is no longer maintained" as the first line of the readme, like: https://www.npmjs.com/package/sync-exec You could even polyfill with that module.