Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(registry): Add support for specifying registry #47

Merged
merged 9 commits into from
Mar 6, 2019

Conversation

quinnturner
Copy link
Member

@quinnturner quinnturner commented Jan 27, 2019

From @memelet:

When using a private repo like nexus that does not response correctly to audit, its necessary to use --registry=https://registry.npmjs.org.

This PR supports specifying a registry for NPM (Yarn is not supported yet) by adding the --registry argument. It is an optional parameter, defaulting to the package manager's standard registry.

Addresses: #46

lib/npm-auditer.js Outdated Show resolved Hide resolved
@quinnturner
Copy link
Member Author

@andy-patt could use some assistance. When running locally, the npm-auditer test 'fails with error code ETIMEDOUT on an invalid site' throws an Error with code ETIMEDOUT included in the message (expected and the desired behaviour). In the CI environments, they just hang and never actually time out; always relying on the Mocha timeout (not desired behaviour).

Perhaps this is due to a proxy setting in the CI environments?

One approach is to do something like https://stackoverflow.com/a/23720300/4400318 and explicitly handle the Mocha timeout case. However, we don't actually want this to happen. If for some reason a user is experiencing the same symptoms as the CI environment (behind a proxy?), they should be delivered the code ETIMEDOUT as I do.

@andy-patt
Copy link
Collaborator

@quinnturner I need a bit more context here. You're expecting ETIMEDOUT from an https call to registry.npmjs.co, right?
nslookup reports:

Name:	registry.npmjs.co
Address: 199.59.242.151

So it looks like it's a valid name, but it just isn't serving https traffic. I feel like that could change in the future.

Are we setting a timeout on the https client at all? As far as I know, the limit for connection if the client library doesn't implement its own timeouts, is configured at the kernel level. So the difference in kernels could be playing a role there. I'm not sure how you want to be testing connection failures in an automated way - I'm not sure if it's necessary. Let me know what your motivation is behind these tests.

@quinnturner
Copy link
Member Author

quinnturner commented Mar 6, 2019

Your nslookup was helpful, I didn't think registry.npmjs.co was a valid domain! I was trying to mimic a typo in the --registry <domain> and make sure we handle it "OK". I was not aware that the timeouts could be configured at the kernel level, I am running on Windows 10 with Node@10.x. When I change the domain to a non-existent domain I get ENOTFOUND, which definitely makes more sense that ETIMEDOUT. I will be changing the test case to that.

The test was intended to test for a registry with a typo.
The previous test's domain existed, which was unintended.
To gracefully handle this, the audit should error with `code ENOTFOUND`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants