Allow authentication with registry.npmjs.com

[Ianfeather]

This adds functionality for #93

Making config.uplink an object and including auth as a property would be a little nicer imo, but it would be a breaking change and I'm not sure this warrants that change.

Could you give me some pointers on how to run the tests? Do I need to run them within the container? Currently when I run them locally they're almost all returned as pending. Let me know and i'll add tests for this functionality.

cc @dgautsch

[dgautsch]

Looks good so far, can you add update the unit tests? @jdxcode does the package.json need to be updated? Doesn't npm publish handle that?

[Ianfeather]

Hey @dgautsch, thanks for getting back to me. Regarding the unit tests:

Could you give me some pointers on how to run the tests? Do I need to run them within the container? Currently when I run them locally they're almost all returned as pending. Let me know and i'll add tests for this functionality.

I'm more than happy to help fix up existing tests too, but at the moment I have a totally unstable set of tests locally so I assume I'm running them incorrectly. Thanks.

[jdx]

I definitely wouldn't change the version in a PR. I recommend using npm version or the np tool to do that when you publish a release. Either of those will create a git tag at the same time.

[Ianfeather]

@dgautsch @jdxcode If you could tell me how I can run the tests locally I can add unit tests and land this patch?

[dgautsch]

@Ianfeather hi Ian, apologies for the slow response, I was hoping jd would jump in here because the s3 tests are going to fail, looks like they're running through his personal amazon cloud. Running tests is done from the command line via npm test. For now lets add tests for the file system until S3 is figured out. Let me know if you hit issues.

[jdx]

running locally you just need to set the AWS env vars to a valid S3 bucket. Running on circle is a different story, however. I'm not sure how to enable the public to run the tests without potentially exposing the credentials

[dgautsch]

@Ianfeather revert this file, I will make sure it's updated when we republish.

[Ianfeather]

Will do

[Ianfeather]

@jdxcode Thanks, that should give me enough to get going.

Would you be interested in an update which mocks s3 for the unit tests? You could potentially have a couple of integration tests that run separately to the core unit tests and actually hit an s3 bucket

[jdx]

that sounds like a good idea, I wonder if there is something like nock for s3 that would be useful here

[Ianfeather]

There's this https://www.npmjs.com/package/mock-aws-s3, I've never used it though.

I'll circle back to that in a different PR

[Ianfeather]

I added test documentation in #95

[Ianfeather]

@dgautsch This now has tests and all are passing locally.

There are commits that are in this and also in #95 - I'd recommend merging 95 if you can first, then i'll rebase this branch. I think they're both good to land though.

Let me know what you think from here.

[dgautsch]

Ok thanks Ian. I'll take a look tonight after work.

…

On Nov 21, 2017 12:48 PM, "Ianfeather" ***@***.***> wrote: @dgautsch <https://github.com/dgautsch> This now has tests and all are passing locally. There are commits that are in this and also in #95 <#95> - I'd recommend merging 95 if you can first, then i'll rebase this branch. I think they're both good to land though. Let me know what you think from here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#94 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKHIENmIDNFyqk3le_eBSGwyMWHcx7cks5s4wztgaJpZM4P6kEC> .

[dgautsch]

@Ianfeather There are a couple conflicts, can you resolve?

[codecov]

Codecov Report

Merging #94 into master will increase coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   89.46%   89.63%   +0.16%     
==========================================
  Files          27       27              
  Lines         427      434       +7     
==========================================
+ Hits          382      389       +7     
  Misses         45       45

Impacted Files	Coverage Δ
lib/config.js	100% <ø> (ø)	⬆️
lib/npm.js	75.47% <100%> (+3.73%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 262e563...df47615. Read the comment docs.

[Ianfeather]

I think this might finally be ready to go! Integration tests pass locally

[Ianfeather]

@dgautsch @jdxcode Are you ok to land this patch now?

[jdx]

👍

[Ianfeather]

@dgautsch Sorry to bug you. Are you happy to merge and publish this new version?

[dgautsch]

Hey I'll publish, I think I still need npm rights. If I do, I'll do it today! On Jan 8, 2018 6:48 PM, "Ianfeather" <[email protected]> wrote: @dgautsch <https://github.com/dgautsch> Sorry to bug you. Are you happy to merge and publish this new version? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#94 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKHIOzucF3GinmJm5k8-BZkid4PcWuXks5tIqllgaJpZM4P6kEC> .

[jdx]

you should have it @dgautsch!

[Ianfeather]

Fantastic, thanks both of you