[WIP] support for building nodejs_npm functions #44

gojko · 2018-12-01T12:09:03Z

Description of changes: Initial support for building nodejs_npm, for early code review. This does not yet support building projects with local dependencies.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Release v0.0.3

sanathkr

Thank you so much for contributing. Really appreciate all the hardwork you have put in here.

It might look like there are a lot of comments. But most of my comments are in general about Python or the conventions used in this project. So feel free to ignore or let me know if you disagree. I am happy to hear your thoughts. I believe we can get to a better design by collaborating this way.

aws_lambda_builders/workflows/nodejs_npm/actions.py

sanathkr · 2018-12-03T02:42:50Z

aws_lambda_builders/workflows/nodejs_npm/actions.py

+
+            LOG.debug("NODEJS packaging %s to %s", package_path, self.scratch_dir)
+
+            tarfile_name = self.subprocess_npm.main(['pack', '-q', package_path], cwd=self.scratch_dir)


Is tarball the only option? We need to tar and untar again unnecessarily.

yes, as far as I know. npm pack produces a tarball (https://docs.npmjs.com/cli/pack)

okay, cool. Let's stick with this then.

One downside of this is, when we implement hot-reloading we have to modify this implementation. But that's for later :)

ok, should I leave some kind of todo note there?

also, in theory, we can do hot reloading to check if package.json got modified. if yes, then rerun the whole tar thing. if only a file within the project got modified, and not package.json, then it's pretty safe to assume that packaging didn't change and dependencies did not change as well, so it's OK to just safe-reload that file.

aws_lambda_builders/workflows/nodejs_npm/actions.py

tests/functional/workflows/python_pip/test_utils.py

tests/integration/workflows/nodejs_npm/test_nodejs_npm.py

tests/integration/workflows/nodejs_npm/testdata/npm-deps/package.json

sanathkr · 2018-12-03T03:08:16Z

Copying conversation from slack on difference between claudia pack and this implementation for sake of tracking:

`claudia pack` vs `node_npm` workflow from @gojko

There are features in claudia pack that are not in the code yet, but 90% of the common cases will be covered by what I submitted in the PR

There is a lot of complexity to handle local file dependencies, which I find very useful for my work as I like to keep multiple projects in the same repository and just refer locally to them as dependencies. Once I know that the overall code design is OK, I’ll port that over.
Another thing very useful in claudia pack that’s not here is the ability to exclude optional dependencies (we discussed that earlier, I will add an flag that depends on an environment variable to activate/deactivate that).
The third thing I also think is worth porting over from claudia pack is an option to run a post-package script for any kind of project specific build. NPM has the ability to put build phase scripts (similar to Maven) into the manifest, not just dependencies... so this would be a way for developers to customise the stuff sent up to lambda in an automated way

- restored old osutils into python_pip to avoid unnecessary changes - removed the guard for manifest not existing (as per #44 (comment))

sanathkr

Couple of other minor comments..

This is starting to shape up really well :)

aws_lambda_builders/workflows/nodejs_npm/npm.py

sriram-mv · 2018-12-03T18:44:30Z

aws_lambda_builders/workflows/nodejs_npm/npm.py

+        self.osutils = osutils
+
+        if npm_exe is None:
+            npm_exe = 'npm'


In the future if there are multiple paths of npm on your system, how would you imagine passing them in? or maybe thats not a big concern.

I started a PR here: #35 that would help with that. But I'm going to take a step back and maybe write a mini design doc to get started on it. I was curious to know if you had any comments on that?

#44 (comment)) - using ValueError for argument type error (as per #44 (comment))

sanathkr · 2018-12-03T18:58:49Z

@gojko we also need to setup Travis & AppVeyor to run tests with a node environment. This is going to be interesting. I am happy to punt this work to a separate PR because it requires more wrangling..

#44 (comment))

gojko · 2018-12-03T19:29:56Z

It looks like the overall design of the code is OK? I'll then add unit tests and we can start closing off the first release of this.

Regarding NPM, we could somehow split out integration tests that require NPM from the rest, so you can run them on two different integration test environments. Let me know if that's an option.

aws_lambda_builders/workflows/nodejs_npm/actions.py

aws_lambda_builders/workflows/nodejs_npm/workflow.py

sanathkr · 2018-12-03T20:48:39Z

Everything looks good, except for the unique_dir comment. Do start writing unit tests. I will create a separate Travis/Appveyor PR to run integ tests

sanathkr · 2018-12-03T20:49:11Z

Also, is there a min version of Nodejs & NPM that this implementation assumes?

gojko · 2018-12-03T22:10:12Z

Also, is there a min version of Nodejs & NPM that this implementation assumes?

this just runs the NPM executable, so as long as there is one on the path, it will work. NPM tends to ignore parameters that it does not understand, so --no-audit and --no-save will have no effect on earlier versions where they are not supported.

sanathkr

We are in the homestretch here. Logic all looks great! One minor comment about exceptions, and everything else is docstrings.

Can you add docstrings to the classes & methods? Following a reStructuredText doc format (like this: https://github.com/awslabs/aws-lambda-builders/blob/develop/aws_lambda_builders/workflow.py#L84-L117)

I will also keep the PR open for unit tests

aws_lambda_builders/workflows/nodejs_npm/npm.py

aws_lambda_builders/workflows/nodejs_npm/actions.py

sanathkr · 2018-12-04T18:52:50Z

@gojko I just pushed a commit to your branch fixing appveyor & travis - 7eaad47

Also, can you update the PR to send against develop branch instead of master?

The code looks good to me! I think we are good to merge once the integ tests pass

gojko · 2018-12-04T18:58:04Z

Also, can you update the PR to send against develop branch instead of master?

I've updated the base to merge into develop using the github UI, not sure if that will do the trick or not. if this looks wrong to you, i'll create a completely clean PR

sanathkr · 2018-12-04T19:02:07Z

Yup, that's good enough :)

Looks like the integ tests are failing on Windows because its not able to find node/npm :(

gojko · 2018-12-04T19:06:26Z

I wonder if npm is a separate package you need to install there? it looks like node is installed, but it's not finding NPM.

sanathkr · 2018-12-04T19:09:44Z

that's surprising. I tested the appveyor config here - sanathkr@713d5b2. It did print npm version - https://ci.appveyor.com/project/sanathkr/aws-lambda-builders/builds/20767787/job/5k44u8p3pujh7kg3

jfuss

Overall this looks really good @gojko Nothing super critical in my feedback.

aws_lambda_builders/workflows/nodejs_npm/actions.py

jfuss · 2018-12-04T21:33:56Z

aws_lambda_builders/workflows/nodejs_npm/npm.py

+
+        if npm_exe is None:
+            if osutils.is_windows():
+                npm_exe = 'npm.cmd'


Does npm have a npm.exe at all?

no it doesn't unfortunately. I verified on a windows box

aws_lambda_builders/workflows/nodejs_npm/utils.py

tests/functional/workflows/nodejs_npm/test_utils.py

tests/unit/workflows/nodejs_npm/test_workflow.py

tests/unit/workflows/nodejs_npm/test_npm.py

jfuss · 2018-12-04T21:45:59Z

tests/unit/workflows/nodejs_npm/test_workflow.py

+
+from aws_lambda_builders.workflows.nodejs_npm.actions import NodejsNpmPackAction, NodejsNpmInstallAction
+
+from aws_lambda_builders.actions import CopySourceAction


PEP 8 imports

hi, I'm not actively working with python do I don't know what you mean by this. What would you like me to change?

resolved in 6863067

sanathkr · 2018-12-05T00:32:18Z

Just realized, we never wrote a design document for this. This is just a simple markdown file that explains what the workflow does, like this - https://github.com/awslabs/aws-lambda-builders/blob/develop/aws_lambda_builders/workflows/python_pip/DESIGN.md.

Goal is to let other implementors understand why certain decisions were made. It helps us set context for future design changes.

If you got time, can you add a short design document? If you feel the PR is already big, we can do it in a separate PR later

(as per #44 (comment))

#44 (comment))

gojko · 2018-12-05T08:53:07Z

If you got time, can you add a short design document? If you feel the PR is already big, we can do it in a separate PR later

I added this here 09d4190

jfuss

Two small comments on the design doc. This looks really good!

aws_lambda_builders/workflows/nodejs_npm/DESIGN.md

sriram-mv · 2018-12-05T21:03:27Z

aws_lambda_builders/workflows/nodejs_npm/npm.py

+        :param osutils: An instance of OS Utilities for file manipulation
+
+        :type npm_exe: str
+        :param npm_exe: Path to the NPM binary. If not set,


I'm ok with keeping this here for the PR, but eventually we need the workflow to start with absolute paths to dependency managers, language etc to ensure a correct build?

I have started a PR here with design notes: https://github.com/awslabs/aws-lambda-builders

Let me know on your thoughts!

@thesriram I guess as long as you pass an executable into the workflow as a standard parameter, we can easily pass it to the NPM wrapper there, so it should be a single line of code change once it is ready.

sriram-mv · 2018-12-05T21:07:00Z

The PR looks good to me. My only concern was to be able plugin path resolvers and path validators appropriately without much effort after this gets merged, which is what I have commented.

sanathkr · 2018-12-06T01:19:33Z

@gojko Please confirm this contribution is under the terms of the Apache 2.0 license. Thanks.

gojko · 2018-12-06T05:39:13Z

@sanathkr I confirm that my contribution is made under the terms of the Apache 2.0 license.

sanathkr and others added 6 commits November 26, 2018 17:21

Merge pull request #41 from awslabs/develop

140fd87

Release v0.0.3

simple flow - trivial copying and executing npm

03bddad

clean up

50b55cf

use npm pack to build the source

bab2c3d

tests for changes in osutils

de32212

integration tests for nodejs_npm

ccf9e6d

sanathkr suggested changes Dec 3, 2018

View reviewed changes

gojko added 4 commits December 3, 2018 09:35

styling fixes

3ac1a38

remove executable autodetection, as per #44 (comment)

3810cd8

remove executable autodetection, as per #44 (comment)

a08723a

fix copy/paste error

9ac2e18

jfuss mentioned this pull request Dec 3, 2018

npm link causes sam local to error aws/aws-sam-cli#756

Open

gojko added 2 commits December 3, 2018 19:30

- moved osutils into nodejs_npm (as per #44 (comment))

cec44ae

- restored old osutils into python_pip to avoid unnecessary changes - removed the guard for manifest not existing (as per #44 (comment))

- removing guard against manifest missing, as per #44 (comment)

4a00c1e

sanathkr suggested changes Dec 3, 2018

View reviewed changes

aws_lambda_builders/workflows/nodejs_npm/npm.py Outdated Show resolved Hide resolved

aws_lambda_builders/workflows/nodejs_npm/npm.py Outdated Show resolved Hide resolved

aws_lambda_builders/workflows/nodejs_npm/npm.py Outdated Show resolved Hide resolved

sriram-mv reviewed Dec 3, 2018

View reviewed changes

- using "run" as the npm method entry point (as per

3e945b2

#44 (comment)) - using ValueError for argument type error (as per #44 (comment))

using standard copysource action (as per

d91c537

#44 (comment))

sanathkr reviewed Dec 3, 2018

View reviewed changes

aws_lambda_builders/workflows/nodejs_npm/actions.py Outdated Show resolved Hide resolved

aws_lambda_builders/workflows/nodejs_npm/workflow.py Outdated Show resolved Hide resolved

- using more specific purpose, as per #44 (comment)

2542cdb

sanathkr reviewed Dec 3, 2018

View reviewed changes

aws_lambda_builders/workflows/nodejs_npm/workflow.py Show resolved Hide resolved

- simplify target directory naming, as per #44 (comment)

d183932

sanathkr suggested changes Dec 3, 2018

View reviewed changes

aws_lambda_builders/workflows/nodejs_npm/npm.py Outdated Show resolved Hide resolved

aws_lambda_builders/workflows/nodejs_npm/actions.py Show resolved Hide resolved

aws_lambda_builders/workflows/nodejs_npm/actions.py Outdated Show resolved Hide resolved

initial tests for nodejs_npm actions

1008117

Install nodejs8.10 in travis & appveyor

7eaad47

gojko changed the base branch from master to develop December 4, 2018 18:57

gojko added 3 commits December 4, 2018 20:22

experiment to see if this will fix it for windows

bbb92a5

try fixing for windows

9270a81

better testing for windows

6f23b11

jfuss suggested changes Dec 4, 2018

View reviewed changes

sanathkr approved these changes Dec 5, 2018

View reviewed changes

gojko added 5 commits December 5, 2018 08:34

calling parent constructor for actions, as per #44 (comment)

53f9d34

Using platform to check for windows, to make it consistent with SAM CLI

2c5d780

(as per #44 (comment))

using unittest instead of pytest (as per

ed23979

#44 (comment))

use self.assert* instead of assert (as per #44 (comment), #44 (comment))

fcd47a9

added design document, as per #44 (comment)

09d4190

gojko added 2 commits December 5, 2018 10:05

added a note on package locking

607079e

group imports according to PEP8, as per #44 (comment)

6863067

jfuss approved these changes Dec 5, 2018

View reviewed changes

aws_lambda_builders/workflows/nodejs_npm/DESIGN.md Outdated Show resolved Hide resolved

aws_lambda_builders/workflows/nodejs_npm/DESIGN.md Outdated Show resolved Hide resolved

sriram-mv reviewed Dec 5, 2018

View reviewed changes

clarify the need for package customisation

fdc2707

gojko mentioned this pull request Dec 8, 2018

[WIP] White-list nodejs for aws-lambda-builders aws/aws-sam-cli#845

Merged

jfuss merged commit 84980da into aws:develop Dec 10, 2018

sriram-mv mentioned this pull request Dec 11, 2018

Cannot use 3d party libraries aws/aws-sam-cli#125

Closed


		LOG.debug("NODEJS packaging %s to %s", package_path, self.scratch_dir)

		tarfile_name = self.subprocess_npm.main(['pack', '-q', package_path], cwd=self.scratch_dir)


		from aws_lambda_builders.workflows.nodejs_npm.actions import NodejsNpmPackAction, NodejsNpmInstallAction

		from aws_lambda_builders.actions import CopySourceAction

[WIP] support for building nodejs_npm functions #44

[WIP] support for building nodejs_npm functions #44

Uh oh!

Conversation

gojko commented Dec 1, 2018

Uh oh!

sanathkr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanathkr commented Dec 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

claudia pack vs node_npm workflow from @gojko

Uh oh!

sanathkr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanathkr commented Dec 3, 2018

Uh oh!

gojko commented Dec 3, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanathkr commented Dec 3, 2018

Uh oh!

sanathkr commented Dec 3, 2018

Uh oh!

gojko commented Dec 3, 2018

Uh oh!

sanathkr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanathkr commented Dec 4, 2018

Uh oh!

gojko commented Dec 4, 2018

Uh oh!

sanathkr commented Dec 4, 2018

Uh oh!

gojko commented Dec 4, 2018

Uh oh!

sanathkr commented Dec 4, 2018

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sanathkr commented Dec 3, 2018 •

edited

Loading

`claudia pack` vs `node_npm` workflow from @gojko