-
Notifications
You must be signed in to change notification settings - Fork 152
Extend Node.js NPM support for .npmrc configuration #53
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
Changes from all commits
3100bd4
6ebe8b1
54d3f31
b6ad3fc
ca45e1b
b9e51d1
b2b26be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| optional=false |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| //excluded | ||
| const x = 1; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| //included | ||
| const x = 1; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| { | ||
| "name": "npmdeps", | ||
| "version": "1.0.0", | ||
| "description": "", | ||
| "files": ["included.js"], | ||
| "keywords": [], | ||
| "author": "", | ||
| "license": "APACHE2.0", | ||
| "dependencies": { | ||
| "fake-http-request": "*" | ||
| }, | ||
| "optionalDependencies": { | ||
| "minimal-request-promise": "*" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,8 @@ | |
| from mock import patch | ||
|
|
||
| from aws_lambda_builders.actions import ActionFailedError | ||
| from aws_lambda_builders.workflows.nodejs_npm.actions import NodejsNpmPackAction, NodejsNpmInstallAction | ||
| from aws_lambda_builders.workflows.nodejs_npm.actions import \ | ||
| NodejsNpmPackAction, NodejsNpmInstallAction, NodejsNpmrcCopyAction, NodejsNpmrcCleanUpAction | ||
| from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError | ||
|
|
||
|
|
||
|
|
@@ -78,3 +79,77 @@ def test_raises_action_failed_when_npm_fails(self, SubprocessNpmMock): | |
| action.execute() | ||
|
|
||
| self.assertEqual(raised.exception.args[0], "NPM Failed: boom!") | ||
|
|
||
|
|
||
| class TestNodejsNpmrcCopyAction(TestCase): | ||
|
|
||
| @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") | ||
| def test_copies_npmrc_into_a_project(self, OSUtilMock): | ||
| osutils = OSUtilMock.return_value | ||
| osutils.joinpath.side_effect = lambda a, b: "{}/{}".format(a, b) | ||
|
|
||
| action = NodejsNpmrcCopyAction("artifacts", | ||
| "source", | ||
| osutils=osutils) | ||
| osutils.file_exists.side_effect = [True] | ||
| action.execute() | ||
|
|
||
| osutils.file_exists.assert_called_with("source/.npmrc") | ||
| osutils.copy_file.assert_called_with("source/.npmrc", "artifacts") | ||
|
|
||
| @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") | ||
| def test_skips_copying_npmrc_into_a_project_if_npmrc_doesnt_exist(self, OSUtilMock): | ||
| osutils = OSUtilMock.return_value | ||
| osutils.joinpath.side_effect = lambda a, b: "{}/{}".format(a, b) | ||
|
|
||
| action = NodejsNpmrcCopyAction("artifacts", | ||
| "source", | ||
| osutils=osutils) | ||
| osutils.file_exists.side_effect = [False] | ||
| action.execute() | ||
|
|
||
| osutils.file_exists.assert_called_with("source/.npmrc") | ||
| osutils.copy_file.assert_not_called() | ||
|
|
||
| @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") | ||
| def test_raises_action_failed_when_copying_fails(self, OSUtilMock): | ||
| osutils = OSUtilMock.return_value | ||
| osutils.joinpath.side_effect = lambda a, b: "{}/{}".format(a, b) | ||
|
|
||
| osutils.copy_file.side_effect = OSError() | ||
|
|
||
| action = NodejsNpmrcCopyAction("artifacts", | ||
| "source", | ||
| osutils=osutils) | ||
|
|
||
| with self.assertRaises(ActionFailedError): | ||
| action.execute() | ||
|
|
||
|
|
||
| class TestNodejsNpmrcCleanUpAction(TestCase): | ||
|
|
||
| @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") | ||
| def test_removes_npmrc_if_npmrc_exists(self, OSUtilMock): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also add a tests for when npmrc does not exist, to make sure that branch is also covered?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, I'll take a look at it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| osutils = OSUtilMock.return_value | ||
| osutils.joinpath.side_effect = lambda a, b: "{}/{}".format(a, b) | ||
|
|
||
| action = NodejsNpmrcCleanUpAction( | ||
| "artifacts", | ||
| osutils=osutils) | ||
| osutils.file_exists.side_effect = [True] | ||
| action.execute() | ||
|
|
||
| osutils.remove_file.assert_called_with("artifacts/.npmrc") | ||
|
|
||
| @patch("aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils") | ||
| def test_skips_npmrc_removal_if_npmrc_doesnt_exist(self, OSUtilMock): | ||
| osutils = OSUtilMock.return_value | ||
| osutils.joinpath.side_effect = lambda a, b: "{}/{}".format(a, b) | ||
|
|
||
| action = NodejsNpmrcCleanUpAction( | ||
| "artifacts", | ||
| osutils=osutils) | ||
| osutils.file_exists.side_effect = [False] | ||
| action.execute() | ||
|
|
||
| osutils.remove_file.assert_not_called() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the order matter at all? Does
npm_copy_npmrcjust need to be betweennpm_packand '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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs to come between npm_pack and npm_install because
npm packwill exclude it from the package, but it's needed fornpm installto correctly function (it may contain URLs to module repositories etc)