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

Add --cafile command line option. #837

Merged
merged 2 commits into from
Dec 10, 2015
Merged

Add --cafile command line option. #837

merged 2 commits into from
Dec 10, 2015

Conversation

bnoordhuis
Copy link
Member

R=@rvagg?

@@ -444,3 +407,60 @@ function install (gyp, argv, callback) {
}

}

function download (gyp, env, url, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

taking a cb option that's only used when erroring is a bit gross, perhaps it'd closer to idiomatic if the thrown error bubbled up and the try/catch was done at the download() call?

@rvagg
Copy link
Member

rvagg commented Dec 9, 2015

A bit smelly, I understand that it's hard to do much better without a more heavy refactor. Things I don't like:

  • the use of cb only for errors, the fact that it has a cb argument at all in fact
  • the fs.readFileSync() to read the cert file

A begrudging lgtm since I understand the challenges of doing a better job, I would prefer the cb thing be handled but if you feel strongly enough that the way you've implemented it is good enough then go ahead.

Yet another refactor for the future I suppose.

@bnoordhuis
Copy link
Member Author

I added a commit that lets the exception bubble up. I'm a bit on the fence on whether it's an improvement; let me know what you think.

@rvagg
Copy link
Member

rvagg commented Dec 10, 2015

I'm happy with the clarity of the changes, but generally unhappy with the structure of the call chain but that's a matter for another day! lgtm

Move the function around so it can be tested and add a regression test.

As a policy vs. mechanism thing, change the control flow to handle
exceptions at the call site, not inside the download function.

PR-URL: #837
Reviewed-By: Rod Vagg <rod@vagg.org>
Add an option for overriding the default CA chain that is used when
downloading the tarball.  This matches the npm option of the same
name and gets implicitly passed through the `npm_config_cafile`
environment variable.

Fixes: #695
PR-URL: #837
Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants