Extend Node.js NPM support for .npmrc configuration #53
Conversation
There was a problem hiding this comment.
@simalexan This looks really good! Thanks for taking the time to make the addition.
Just a few small comments.
| except OSError as ex: | ||
| raise ActionFailedError(str(ex)) | ||
|
|
||
| class NodejsCleanUpAction(BaseAction): |
There was a problem hiding this comment.
This name indicates we are cleaning up more than .npmrc. Can we scope the name down to be Npmrc, like how you have done for the Copy Action? Or was the goal to make one Npm Clean up action?
Side thought: I wonder if this will be a pattern that develops (adding then removing the same set of files), to the point where it's worth codifying.
There was a problem hiding this comment.
there will be some other clean up actions to add later (such as npm dedupe). Would you prefer each step to be a separate action, or do we keep it all inside a single action?
on a related note, is it worth adding another Purpose value there? This isn't really COPY_SOURCE, but nothing else fit.
| class TestNodejsCleanUpAction(TestCase): | ||
|
|
||
| @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") | ||
| def test_removes_npmrc_if_npmrc_exists(self, OSUtilMock): |
There was a problem hiding this comment.
Can you also add a tests for when npmrc does not exist, to make sure that branch is also covered?
There was a problem hiding this comment.
sure, I'll take a look at it
There was a problem hiding this comment.
| self.actions = [ | ||
| npm_pack, | ||
| npm_copy_npmrc, |
There was a problem hiding this comment.
Does the order matter at all? Does npm_copy_npmrc just need to be between npm_pack and 'npm_install` or must is happen before the copy source?
Trying to understand if there is some documentation we need to add here for the future.
There was a problem hiding this comment.
it needs to come between npm_pack and npm_install because npm pack will exclude it from the package, but it's needed for npm install to correctly function (it may contain URLs to module repositories etc)
|
@thesriram Thank you! :) Waiting for approval of both of Jacob and you. |
|
Thank you both @jfuss and @thesriram ! |
|
@thesriram thank you! |
Description of changes:
NPM can be configured to use proxies or local company repositories using a local file,
.npmrc.The packaging process from step 1 normally excludes this file, so it needs to be copied before dependency installation, and then removed.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.