Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ node-webkit.app
npm-debug.log
.nyc_output
coverage
test/*/package-lock.json
test/*/package-lock.json
test/app7/build-tmp-napi-*
Comment thread
bmacnaughton marked this conversation as resolved.
3 changes: 3 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ services:
# have more than one run with the same node version
# otherwise they will clobber each other
#####
env:
node_pre_gyp_mock_s3=true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great. Even better I think would be another matrix/job so that:

  • We test both mocking and no mocking
  • Only the mocking job is used with coverage is tested and uploaded

Sound good? If so, could you add another job that uses node_pre_gyp_mock_s3=false? You should be able to do this by moving the env into each matrix item.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's easy enough to move the env into the matrix and add the false env setting.

but i'm not sure what you mean by "only the mocking job is with coverage is tested and uploaded".

and maybe it's related, but if mocking is false won't all external PR tests always fail?


matrix:
include:
Expand All @@ -17,6 +19,7 @@ matrix:
node_js: 10
before_install:
- npm install request
- npm install --package-lock-only
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking we should move to npm ci, which I think does this. Do you know if there is a functional or important difference between npm ci and npm install --package-lock-only?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the command that is executed further down is npm ci and that fails because package.json and package-lock.json are out of sync. at least that's what i was seeing. that's why only this test was failing - it's the only one that did npm install request

script:
- npm run lint
- npm run coverage
Expand Down
3 changes: 3 additions & 0 deletions appveyor.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
os: Visual Studio 2015

environment:
global:
node_pre_gyp_mock_s3: true

matrix:
- nodejs_version: 10
- nodejs_version: 12
Expand Down
8 changes: 4 additions & 4 deletions lib/info.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ const s3_setup = require('./util/s3_setup.js');
const config = require('rc')('node_pre_gyp', { acl: 'public-read' });

function info(gyp, argv, callback) {
const AWS = require('aws-sdk');
const package_json = gyp.package_json;
const opts = versioning.evaluate(package_json, gyp.opts);

s3_setup.detect(opts.hosted_path, config);
AWS.config.update(config);
const s3 = new AWS.S3();
const s3_opts = { Bucket: config.bucket,
const s3 = s3_setup.get_s3(config);
const s3_opts = {
Bucket: config.bucket,
Prefix: config.prefix
};
s3.listObjects(s3_opts, (err, meta) => {
Expand Down
12 changes: 11 additions & 1 deletion lib/node-pre-gyp.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ module.exports = exports;
* Module dependencies.
*/

// load mocking control function for accessing s3 via https. the function is a noop always returning
// false if not mocking.
exports.mockS3Http = require('./util/s3_setup').get_mockS3Http();
exports.mockS3Http('on');
const mocking = exports.mockS3Http('get');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a little more detail on why this is needed? My question is coming from the deeper question of whether there is any way we could avoid having this code in lib/. If not I don't think this is a blocker, but want to understand more why it is needed.

Copy link
Copy Markdown
Collaborator Author

@bmacnaughton bmacnaughton Jan 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my original goal (in the original PR) was to not modify any of the code in lib (or as little as possible). but to do so meant modifying the script command in package.json which led to more problems (different env var settings for windows/linux, changes in what program was actually run, etc. - that stuff is still in the other PR if you want to compare).

i originally put this here because i am not sure whether nock patches the http/https modules after they have been loaded or only when they are loaded. if only when they are loaded, i wanted to load nock as early as possible. i did not get around to testing that so i don't know.

but one way or the other the code has to go into lib/. the reason is that it is lib/install that needs to use the mocked https endpoint, not the test module. while test/fetch.test.js requires lib/install.js and invokes the function directly, the test/build.test.js tests execute bin/node-pre-gyp via a subprocess and there is no good way that i came up with (see original PR for not-so-good way) to have the mock code in the same node instance in that case. my original approach was to prefix the commands with node -r mocking-module <regular command> but that had the knock-on consequences of fiddling with the commands in order to get them to work.

Copy link
Copy Markdown
Collaborator Author

@bmacnaughton bmacnaughton Jan 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and, if not in lib/node-pre-gyp.js it would have to go into lib/install.js which seemed worse.

a couple options:

i can make the code slightly less invasive by not requiring that it be turned on.

i could also just let the code check the env var again. i didn't because i was trying to localize knowledge of the env var to the single s3_setup module. if i do that it can look like:

require('./util/s3_setup').get_mockS3Http();

and if you want to avoid the 'mock' reference it could just be changed to get_s3Http().



const fs = require('fs');
const path = require('path');
const nopt = require('nopt');
Expand Down Expand Up @@ -38,6 +45,10 @@ const aliases = {};
// differentiate node-pre-gyp's logs from npm's
log.heading = 'node-pre-gyp';

if (mocking) {
log.warn(`mocking s3 to ${process.env.node_pre_gyp_mock_s3}`);
}

exports.find = require('./pre-binding').find;

//
Expand Down Expand Up @@ -287,4 +298,3 @@ Object.defineProperty(proto, 'version', {
},
enumerable: true
});

24 changes: 14 additions & 10 deletions lib/publish.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ const url = require('url');
const config = require('rc')('node_pre_gyp', { acl: 'public-read' });

function publish(gyp, argv, callback) {
const AWS = require('aws-sdk');
const package_json = gyp.package_json;
const napi_build_version = napi.get_napi_build_version_from_command_args(argv);
const opts = versioning.evaluate(package_json, gyp.opts, napi_build_version);
Expand All @@ -24,16 +23,19 @@ function publish(gyp, argv, callback) {
if (!found) {
return callback(new Error('Cannot publish because ' + tarball + ' missing: run `node-pre-gyp package` first'));
}

log.info('publish', 'Detecting s3 credentials');
s3_setup.detect(opts.hosted_path, config);

const s3 = s3_setup.get_s3(config);
const s3_opts = {
Bucket: config.bucket,
Prefix: config.prefix
};
const key_name = url.resolve(config.prefix, opts.package_name);
log.info('publish', 'Authenticating with s3');
AWS.config.update(config);
log.info('publish', AWS.config);
const s3 = new AWS.S3();
const s3_opts = { Bucket: config.bucket,
Key: key_name
};
log.info('publish', config);

const remote_package = 'https://' + s3_opts.Bucket + '.s3.amazonaws.com/' + s3_opts.Key;
log.info('publish', 'Checking for existing binary at ' + remote_package);
s3.headObject(s3_opts, (err, meta) => {
Expand All @@ -42,15 +44,17 @@ function publish(gyp, argv, callback) {
// we are safe to publish because
// the object does not already exist
log.info('publish', 'Preparing to put object');
const s3_put = new AWS.S3();
const s3_put_opts = { ACL: config.acl,

// const s3_put = new AWS.S3();
const s3_put_opts = {
ACL: config.acl,
Body: fs.createReadStream(tarball),
Bucket: config.bucket,
Key: key_name
};
log.info('publish', 'Putting object', s3_put_opts.ACL, s3_put_opts.Bucket, s3_put_opts.Key);
try {
s3_put.putObject(s3_put_opts, (err2, resp) => {
s3.putObject(s3_put_opts, (err2, resp) => {
log.info('publish', 'returned from putting object');
if (err2) {
log.info('publish', 's3 putObject error: "' + err2 + '"');
Expand Down
8 changes: 4 additions & 4 deletions lib/unpublish.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,15 @@ const url = require('url');
const config = require('rc')('node_pre_gyp', { acl: 'public-read' });

function unpublish(gyp, argv, callback) {
const AWS = require('aws-sdk');
const package_json = gyp.package_json;
const napi_build_version = napi.get_napi_build_version_from_command_args(argv);
const opts = versioning.evaluate(package_json, gyp.opts, napi_build_version);

s3_setup.detect(opts.hosted_path, config);
AWS.config.update(config);
const s3 = s3_setup.get_s3(config);
const key_name = url.resolve(config.prefix, opts.package_name);
const s3 = new AWS.S3();
const s3_opts = { Bucket: config.bucket,
const s3_opts = {
Bucket: config.bucket,
Key: key_name
};
s3.headObject(s3_opts, (err, meta) => {
Expand Down
129 changes: 129 additions & 0 deletions lib/util/s3_setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
module.exports = exports;

const url = require('url');
const fs = require('fs');
const path = require('path');

module.exports.detect = function(to, config) {
const uri = url.parse(to);
Expand All @@ -24,3 +26,130 @@ module.exports.detect = function(to, config) {
}
}
};

module.exports.get_s3 = function(config) {

if (process.env.node_pre_gyp_mock_s3) {
// here we're mocking. node_pre_gyp_mock_s3 is the scratch directory
// for the mock code.
const AWSMock = require('mock-aws-s3');
const os = require('os');

AWSMock.config.basePath = `${os.tmpdir()}/mock`;

const s3 = AWSMock.S3();

// wrapped callback maker. fs calls return code of ENOENT but AWS.S3 returns
// NotFound.
const wcb = (fn) => (err, ...args) => {
if (err && err.code === 'ENOENT') {
err.code = 'NotFound';
}
return fn(err, ...args);
};

return {
listObjects(params, callback) {
return s3.listObjects(params, wcb(callback));
},
headObject(params, callback) {
return s3.headObject(params, wcb(callback));
},
deleteObject(params, callback) {
return s3.deleteObject(params, wcb(callback));
},
putObject(params, callback) {
return s3.putObject(params, wcb(callback));
}
};
}

// if not mocking then setup real s3.
const AWS = require('aws-sdk');

AWS.config.update(config);
const s3 = new AWS.S3();

// need to change if additional options need to be specified.
return {
listObjects(params, callback) {
return s3.listObjects(params, callback);
},
headObject(params, callback) {
return s3.headObject(params, callback);
},
deleteObject(params, callback) {
return s3.deleteObject(params, callback);
},
putObject(params, callback) {
return s3.putObject(params, callback);
}
};



};

//
// function to get the mocking control function. if not mocking it returns a no-op.
//
// if mocking it sets up the mock http interceptors that use the mocked s3 file system
// to fulfill reponses.
module.exports.get_mockS3Http = function() {
let mock_s3 = false;
if (!process.env.node_pre_gyp_mock_s3) {
return () => mock_s3;
}

const nock = require('nock');
// the bucket used for testing, as addressed by https.
const host = 'https://mapbox-node-pre-gyp-public-testing-bucket.s3.us-east-1.amazonaws.com';
const mockDir = process.env.node_pre_gyp_mock_s3 + '/mapbox-node-pre-gyp-public-testing-bucket';

// function to setup interceptors. they are "turned off" by setting mock_s3 to false.
const mock_http = () => {
// eslint-disable-next-line no-unused-vars
function get(uri, requestBody) {
const filepath = path.join(mockDir, uri.replace('%2B', '+'));

try {
fs.accessSync(filepath, fs.constants.R_OK);
} catch (e) {
return [404, 'not found\n'];
}

// the mock s3 functions just write to disk, so just read from it.
return [200, fs.createReadStream(filepath)];
}

// eslint-disable-next-line no-unused-vars
return nock(host)
.persist()
.get(() => mock_s3) // mock any uri for s3 when true
.reply(get);
};

// setup interceptors. they check the mock_s3 flag to determine whether to intercept.
mock_http(nock, host, mockDir);
// function to turn matching all requests to s3 on/off.
const mockS3Http = (action) => {
const previous = mock_s3;
if (action === 'off') {
mock_s3 = false;
} else if (action === 'on') {
mock_s3 = true;
} else if (action !== 'get') {
throw new Error(`illegal action for setMockHttp ${action}`);
}
return previous;
};

// call mockS3Http with the argument
// - 'on' - turn it on
// - 'off' - turn it off (used by fetch.test.js so it doesn't interfere with redirects)
// - 'get' - return true or false for 'on' or 'off'
return mockS3Http;
};



53 changes: 51 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"codecov": "^3.8.1",
"eslint": "^7.16.0",
"eslint-plugin-node": "^11.1.0",
"mock-aws-s3": "^4.0.1",
"nock": "^12.0.3",
"node-addon-api": "^3.1.0",
"nyc": "^15.1.0",
Expand Down
Loading