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

gojko · 2018-12-08T18:34:02Z

Issue #, if available: #798

Description of changes: White-list nodejs for aws-lambda-builders, required to use the nodejs_npm builder from SAM CLI. This should be merged only after aws/aws-lambda-builders#44 is merged

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

jfuss · 2018-12-10T15:44:26Z

@gojko Thanks for submitting this. I have just merged the NPM building into aws-lambda-builders. We still need to test this end-to-end from sam cli and publish a new version of the aws-lambda-builders. We will need to include that version into this PR otherwise, building node will not work.

I am doing some testing locally and using the --use-container flag. I can submit the integ tests and version bump here if you would like?

jfuss · 2018-12-10T16:27:19Z

Tested with nodejs8.10:

sam init --runtime nodejs8.10 --name node-builder-testing
cd node-builder-testing
sam build
# remove cache created by previous sam build
rm -rf .aws-sam/

sam build --use-container

The sam build on my machine (without --use-container) worked as expected. I don't have node 8.10 locally though but not sure if that is expected to fail.

node -v
v8.9.1

When running sam build --use-container, I get this error:

Build Failed
'nodejs' runtime has not been validated!

This is due to the cli that is within the aws-lambda-builders doing validation. Looks like we missed this during the implementation for the node builder.

I am looking at how to address this now.

gojko · 2018-12-10T16:44:17Z

any version of node is OK for this (it's executing the npm binary). I think even ancient versions such as 0.10 will still pack the files correctly. There might be problems executing custom scripts, but in that case NPM would explode with a reasonable error message and users would know how to fix it.

I've not tried the --use-container option when testing locally, I guess that's a good note for future builder implementations to remember.

jfuss · 2018-12-10T16:48:48Z

Testing locally with a dev build of the container is not trivial right now. :(

@gojko How should we go about validating? The build for python checks for explicit versions. By the sounds of it, we only care about npm? I am not super familiar with the nodejs world and how to go about this right now.

gojko · 2018-12-10T16:58:50Z

@jfuss I think the python validation problem is specific because SAM is a python app building other python apps potentially needing different virtualenvs. My best guess is that for node we only care about npm existing.

Until someone complains and actually has a problem that we could test against I suggest we don't do any additional validation apart from checking for NPM.

jfuss · 2018-12-11T22:51:15Z

@gojko I updated the nodejs init examples and updated the aws-lambda-builders to 0.0.4 but can't push this to your remote so it shows up in this PR. Then the only thing left would be to write integ tests for building node.

jfuss · 2018-12-12T15:55:34Z

@thesriram When you get some time, can you please review.

@gojko If you could look at the changes to the nodejs example I made, I would really appreciate it. I want to make sure the example aligns with what the community would expect. My one concern is that the package.json file in the function folder makes it hard to test the application through unit, functional, or integration tests. You have to cd into that directory in order to run npm run test and not sure if there is a practice to get around this.

sriram-mv · 2018-12-12T18:04:08Z

samcli/lib/build/app_builder.py

@@ -51,6 +51,12 @@ def _get_workflow_config(runtime):
            dependency_manager="pip",
            application_framework=None,
            manifest_name="requirements.txt")
+    elif runtime.startswith("nodejs"):


Noo, lets not do if else's again, this might become like our entrypoint file. Given that there may be more runtimes that we support building.

What is your suggestion then?

Suggestion is to be encapsulate in a dictionary, and invoke corresponding actions based on a match. But I will not block on this. we can address this separately, I'll create an issue.

sriram-mv · 2018-12-12T18:05:36Z

...init/templates/cookiecutter-aws-sam-hello-nodejs/{{cookiecutter.project_name}}/template.yaml

@@ -16,7 +16,7 @@ Resources:
    HelloWorldFunction:
        Type: AWS::Serverless::Function # More info about Function Resource: https://github.com/awslabs/serverless-application-model/blob/master/versions/2016-10-31.md#awsserverlessfunction
        Properties:
-            CodeUri: hello_world/
+            CodeUri: hello-world/


just curious: any reason for the change in the name?

Conversations with people familiar with Node.js said dashes are used not underscores. So I changed this to make it match what the community is doing/the Node.js style

sriram-mv · 2018-12-12T18:06:05Z

tests/integration/buildcmd/test_build_cmd.py

+
+    FUNCTION_LOGICAL_ID = "Function"
+
+    @parameterized.expand([


Brilliant! thanks for adding all cases!

sriram-mv · 2018-12-12T18:18:29Z

tests/integration/buildcmd/test_build_cmd.py

+        self._verify_resource_property(str(self.built_template),
+                                       "OtherRelativePathResource",
+                                       "BodyS3Location",
+                                       os.path.relpath(


Thats a lotta path heh.

Mostly stolen from the python above.

sriram-mv

Approved. 👍

gojko · 2018-12-13T08:48:39Z

@gojko If you could look at the changes to the nodejs example I made, I would really appreciate it. I want to make sure the example aligns with what the community would expect. My one concern is that the package.json file in the function folder makes it hard to test the application through unit, functional, or integration tests. You have to cd into that directory in order to run npm run test and not sure if there is a practice to get around this.

It looks fine to me. My workflow is usually to whitelist files/directories by including them into package.json files section, so instead of excluding tests via .npmignore, I'd create something like

files: ['app.js']

but both approaches are equally valid. Regarding test execution, I think this is OK. There are tools like lerna that help manage multi-directory projects, but there isn't anything that could be called standard, at least not that I know of it. I usually just script it using a makefile.

gojko added 3 commits November 29, 2018 23:49

added node builder

1d04f9d

added a test for the node workflow

3e8df36

list node engines with version

a3b4f22

gojko and others added 3 commits December 11, 2018 23:10

Merge branch 'develop' of github.com:awslabs/aws-sam-cli into develop

a5efdc1

Update sam init template for nodejs

39274a1

Version bump aws-lambda-builders to 0.0.4

4cd6523

jfuss mentioned this pull request Dec 12, 2018

Simplify built-in quickstarts to a simple hello world #849

Merged

18 tasks

jfuss added 3 commits December 12, 2018 07:07

Add integ tests for building NodeJs functions

60a6c38

Make npm/node available in travis

804e571

fix linting to allow make pr to pass

14ba361

sriram-mv self-requested a review December 12, 2018 18:01

sriram-mv reviewed Dec 12, 2018

View reviewed changes

Update nodejs init README

216fc4a

sriram-mv approved these changes Dec 12, 2018

View reviewed changes

jfuss merged commit bd7b669 into aws:develop Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gojko commented Dec 8, 2018

jfuss commented Dec 10, 2018

jfuss commented Dec 10, 2018

gojko commented Dec 10, 2018

jfuss commented Dec 10, 2018

gojko commented Dec 10, 2018

jfuss commented Dec 11, 2018

jfuss commented Dec 12, 2018

sriram-mv Dec 12, 2018

jfuss Dec 12, 2018

sriram-mv Dec 12, 2018

sriram-mv Dec 12, 2018

jfuss Dec 12, 2018

sriram-mv Dec 12, 2018

sriram-mv Dec 12, 2018

jfuss Dec 12, 2018

sriram-mv left a comment

gojko commented Dec 13, 2018

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

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

Conversation

gojko commented Dec 8, 2018

jfuss commented Dec 10, 2018

jfuss commented Dec 10, 2018

gojko commented Dec 10, 2018

jfuss commented Dec 10, 2018

gojko commented Dec 10, 2018

jfuss commented Dec 11, 2018

jfuss commented Dec 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sriram-mv left a comment

Choose a reason for hiding this comment

gojko commented Dec 13, 2018