allow prod & stage s3 targets

[bmacnaughton]

allow the binary.host property (the s3 target) to be set at execution time.

for this to take effect requires all the following to be true.

• binary.host is falsey
• binary.staging_host is not empty
• binary.production_host is not empty

if any of the previous are not true then binary.host is used, as it is now.

the command line option, --s3_host can specify staging or production, e.g.,
--s3_host=staging. if the value is not staging or production and exception is
thrown.

defaults when --s3_host is not specified:

• if command is "publish" then the default is set to "binary.staging_host"
• if command is not "publish" the the default is set to "binary.production_host"

these defaults were chosen so that any command other than "publish" uses "production"
as the default without requiring any command-line options while "publish" requires
'--s3_host production_host' to be specified in order to really publish. publishing
to staging can be done freely without worrying about disturbing any production releases.

the main change this required is to centralize the reading (and preprocessing) or package.json. there is a duplicate of
the minor fix in the linux-differentiation PR to verify required binary properties.

[springmeyer]

Why did you remove this? Would be ideal to leave this in across all the JS files.

[bmacnaughton]

i removed it because eslint flagged it as unncessary. maybe @mapbox/eslint-config-mapbox/.eslint.rc should have root: true set? i can put them back in but was just going with eslint.

[bmacnaughton]

the rule in my upper-level eslintrc file is:

    "strict": [2, "global"],

but that seems like it should allow a single 'use strict' at the top of files based on eslint doc.

[springmeyer]

Okay. I'm not seeing that locally / or on travis. So, can you bring back the strict here and I'll investigate more this week.

[bmacnaughton]

i think you're not seeing it because you don't have a upper-level directory with an eslintrc file that supplies defaults for settings not specified by your eslintrc files. i think if you add root: true (or "root": true) to your file then it won't pick up mine.

eslint doc - search for root: true.

[bmacnaughton]

i in-place edited node_modules/@mapbox/eslinit-config-mapbox/base.js to start with:

module.exports = {
  extends: 'eslint:recommended',
  root: true,
  env: {
    es6: true
  },

and restarted vscode. eslint no longer complains about 'use strict'.

[springmeyer]

Okay, can you please revert your removal of use strict?

[bmacnaughton]

am doing so now. will ping you when done.

[springmeyer]

thanks

[springmeyer]

looks like you missed restoring one use strict, the one referenced by this comment

[springmeyer]

Ping me once the tests are passing and I'll do a deeper review.

[bmacnaughton]

@springmeyer - tests are running

[springmeyer]

Okay, I've reviewed once more. Code looks good - no more feedback. Next steps before merging:

• Need to get travis running (I notice only windows tests are running). I don't think the problem is the PR, but rather something with travis. I just fixed a setting in the travis UI which might have been the problem. So can you try pushing another commit to confirm that travis runs for this PR?
• coverage: once travis is working this PR should then start tracking whether code coverage has increased or decreased. It will need to not decrease significantly.
• tests: I figure this support will break in the future if there are not tests. The tests for node-pre-gyp are integration tests, which make them useful but somewhat difficult to add/understand. But could you take a shot at adding some tests for this either standalone or in test/build.test.js?

[bmacnaughton]

I will add some test(s) specific to this and push that. I'll let you know when I've pushed them.

[bmacnaughton]

@springmeyer - i added tests and it is running in travis. codecov has come back with results that i don't understand. looking at the details it's not clear how coverage was reduced as much as reported. for example, the file lib/publish.js shows abs 0.0 relative 0.0, change -70.94. when looking at the diffs i don't see how the changes could change the coverage. i also do not understand how 0 absolute and relative changes could results in a 70% decrease.

i added these tests to versioning.test.js because it seemed like the majority of what i'm testing in this pass relates to whether the host variable is being set correctly. let me know if you think these should be moved to a separate test file.

i'll add tests that actually exercise publishing with different destinations in build.test.js next. but because i don't have access to the mapbox s3 buckets that will involve changing/rewriting the app?/package.json files in order for me to test. do you have any thoughts on how to best approach this?

[bmacnaughton]

oops. my bad. left an only in there. am fixing.

[bmacnaughton]

ok, i rebased on your master, pushed, and things look a little better. but i still don't understand codecov's results. the changes in lib/util are your changes; my edits changed nothing. and the changes to lib/<command>.js are almost entirely the change of the one line JSON.parse(fs.readFileSync(...)) to gyp.package.json - and that line is always going to be executed.

i haven't used codecov before so maybe i'm missing something obvious.

[springmeyer]

As far as code coverage I think you are being unfairly penalized for touching files that lacked some coverage to begin with. So for the purposes of this PR I'd focus only on the coverage of the new code added to lib/node-pre-gyp.js inside setBinaryHostProperty. You can see that here: https://codecov.io/gh/mapbox/node-pre-gyp/src/abbd9b466772b34c812914cd7057744d676bcfc2/lib/node-pre-gyp.js. Yellow lines in that display indicate code covered but only by one branch. Often this is paired with a red line below since the branch was not taken. So if you could ensure you have tests that hit these lines that would be ideal:

• 226
• 235
• 246
• 65

[bmacnaughton]

thanks for the perspective. i have made changes to testing and those four lines should be covered now.

when reviewing the logic for the expanded tests i thought a couple additional changes were needed as well.

i removed the try {} catch (e) {} around the common JSON.parse(fs.readFileSync(...)) in the Run constructor. previously the individual commands would throw without catching and catching it here resulted in less obvious errors later. i don't think i ever should have caught the error.

there is also a minor change in setBinaryHostProperty() so it returns the already set host if already set. it makes testing easier and it distinguishes between an error and just calling the function one extra time.

[springmeyer]

Great, thanks - coverage looks perfect now (for what matters). Just one thing before merging:

• now that you always return a value from setBinaryHostProperty perhaps this logging should either be removed or adjusted? https://github.com/mapbox/node-pre-gyp/pull/533/files#diff-996ac3b3c6454eec7ba3acb92ebbf3bc6bf4c3e2592bf34feb411d3973036a70R83
• It seems to me like it should only log when the host has been overriden, but it looks to me like it will effectively log now no matter what.

[bmacnaughton]

now that you always return a value from setBinaryHostProperty perhaps this logging should either be removed or adjusted?

it doesn't always return a truthy value; i think the message is ok as it is. setBinaryHostProperty() returns a truthy value if binary.host is falsey, both binary.staging_host and binary.production_host are truthy, and the --s3_host option is either 'staging' or 'production'. it will also return a truthy value if it has been called once before and met those criteria (meaning it has already set binary.host to either 'staging_host' or 'production_host') in which case it just returns the value it was set to.

the flag isn't set until completion, so i think log.info should only be called if it did change binary.host. (it would log also if it were the second call to setBinaryHostProperty, but i don't think that happens.)

make sense? it's a bit confusing with the binaryHostSet flag maintaining state.

[springmeyer]

Okay, I'll trust you on this - thanks. Okay, last things are:

• Add a mention about this option to the v1.0.0 changelog
• I'll approve and merge

I'm still mulling on what other changes will go into v1.0.0 so hopefully you can live with using HEAD for now to give me time to get a v1.0.0 out in the ~ next month.

[bmacnaughton]

just pushed CHANGELOG.md addition. i appreciate your working with me on this.

[bmacnaughton]

@springmeyer - don't do a release based on master. i believe i need to do a fix so that it will work when -C/--directory is specified. i missed that interaction. i'll add a test to catch that as well.

[springmeyer]

👍