Skip to content
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

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

Merged
merged 10 commits into from
Dec 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ matrix:
dist: xenial
sudo: true

before_install:
- nvm install 8.10
- npm --version
- node --version

install:
# Install the code requirements
- make init
Expand Down
2 changes: 1 addition & 1 deletion requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ dateparser~=0.7
python-dateutil~=2.6
pathlib2~=2.3.2; python_version<"3.4"
requests~=2.20.0
aws_lambda_builders==0.0.3
aws_lambda_builders==0.0.4
6 changes: 6 additions & 0 deletions samcli/lib/build/app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ def _get_workflow_config(runtime):
dependency_manager="pip",
application_framework=None,
manifest_name="requirements.txt")
elif runtime.startswith("nodejs"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your suggestion then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return config(
language="nodejs",
dependency_manager="npm",
application_framework=None,
manifest_name="package.json")
else:
raise UnsupportedRuntimeException("'{}' runtime is not supported".format(runtime))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ This is a sample template for {{ cookiecutter.project_name }} - Below is a brief
```bash
.
├── README.md <-- This instructions file
├── hello_world <-- Source code for a lambda function
├── hello-world <-- Source code for a lambda function
│ ├── app.js <-- Lambda function code
│ ├── package.json <-- NodeJS dependencies
│ └── tests <-- Unit tests
Expand All @@ -28,15 +28,21 @@ This is a sample template for {{ cookiecutter.project_name }} - Below is a brief

## Setup process

### Installing dependencies
### Building the project

In this example we use `npm` but you can use `yarn` if you prefer to manage NodeJS dependencies:
[AWS Lambda requires a flat folder](https://docs.aws.amazon.com/lambda/latest/dg/nodejs-create-deployment-pkg.html) with the application as well as its dependencies in a node_modules folder. When you make changes to your source code or dependency manifest,
run the following command to build your project local testing and deployment:

```bash
sam build
```

If your dependencies contain native modules that need to be compiled specifically for the operating system running on AWS Lambda, use this command to build inside a Lambda-like Docker container instead:
```bash
cd hello_world
npm install
cd ../
sam build --use-container
```

By default, this command writes built artifacts to `.aws-sam/build` folder.

### Local development

Expand Down Expand Up @@ -69,7 +75,7 @@ AWS Lambda NodeJS runtime requires a flat folder with all dependencies including
FirstFunction:
Type: AWS::Serverless::Function
Properties:
CodeUri: hello_world/
CodeUri: hello-world/
...
```

Expand Down Expand Up @@ -112,7 +118,8 @@ aws cloudformation describe-stacks \
We use `mocha` for testing our code and it is already added in `package.json` under `scripts`, so that we can simply run the following command to run our tests:

```bash
cd hello_world
cd hello-world
npm install
npm run test
```

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
tests/*
Original file line number Diff line number Diff line change
Expand Up @@ -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/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Handler: app.lambdaHandler
{%- if cookiecutter.runtime == 'nodejs6.10' %}
Runtime: nodejs6.10
Expand Down
74 changes: 71 additions & 3 deletions tests/integration/buildcmd/test_build_cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class TestBuildCommand_PythonFunctions(BuildIntegBase):
"jinja2",
'requirements.txt'}

FUNCTION_LOGICAL_ID = "PythonFunction"
FUNCTION_LOGICAL_ID = "Function"

@parameterized.expand([
("python2.7", False),
Expand All @@ -37,7 +37,7 @@ def test_with_default_requirements(self, runtime, use_container):
self.skipTest("Current Python version '{}' does not match Lambda runtime version '{}'".format(py_version,
runtime))

overrides = {"PythonRuntime": runtime}
overrides = {"Runtime": runtime, "CodeUri": "Python"}
cmdlist = self.get_command_list(use_container=use_container,
parameter_overrides=overrides)

Expand Down Expand Up @@ -113,7 +113,7 @@ def _get_python_version(self):
class TestBuildCommand_ErrorCases(BuildIntegBase):

def test_unsupported_runtime(self):
overrides = {"PythonRuntime": "unsupportedpython"}
overrides = {"Runtime": "unsupportedpython", "CodeUri": "NoThere"}
cmdlist = self.get_command_list(parameter_overrides=overrides)

LOG.info("Running Command: {}", cmdlist)
Expand All @@ -124,3 +124,71 @@ def test_unsupported_runtime(self):
self.assertEquals(1, process.returncode)

self.assertIn("Build Failed", process_stdout)


class TestBuildCommand_NodeFunctions(BuildIntegBase):

EXPECTED_FILES_GLOBAL_MANIFEST = set()
EXPECTED_FILES_PROJECT_MANIFEST = {'node_modules', 'main.js'}
EXPECTED_NODE_MODULES = {'minimal-request-promise'}

FUNCTION_LOGICAL_ID = "Function"

@parameterized.expand([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant! thanks for adding all cases!

("nodejs4.3", False),
("nodejs6.10", False),
("nodejs8.10", False),
("nodejs4.3", "use_container"),
("nodejs6.10", "use_container"),
("nodejs8.10", "use_container")
])
def test_with_default_package_json(self, runtime, use_container):
overrides = {"Runtime": runtime, "CodeUri": "Node"}
cmdlist = self.get_command_list(use_container=use_container,
parameter_overrides=overrides)

LOG.info("Running Command: {}", cmdlist)
process = subprocess.Popen(cmdlist, cwd=self.working_dir)
process.wait()

self._verify_built_artifact(self.default_build_dir, self.FUNCTION_LOGICAL_ID,
self.EXPECTED_FILES_PROJECT_MANIFEST, self.EXPECTED_NODE_MODULES)

self._verify_resource_property(str(self.built_template),
"OtherRelativePathResource",
"BodyS3Location",
os.path.relpath(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats a lotta path heh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly stolen from the python above.

os.path.normpath(os.path.join(str(self.test_data_path), "SomeRelativePath")),
str(self.default_build_dir))
)

def _verify_built_artifact(self, build_dir, function_logical_id, expected_files, expected_modules):

self.assertTrue(build_dir.exists(), "Build directory should be created")

build_dir_files = os.listdir(str(build_dir))
self.assertIn("template.yaml", build_dir_files)
self.assertIn(function_logical_id, build_dir_files)

template_path = build_dir.joinpath("template.yaml")
resource_artifact_dir = build_dir.joinpath(function_logical_id)

# Make sure the template has correct CodeUri for resource
self._verify_resource_property(str(template_path),
function_logical_id,
"CodeUri",
function_logical_id)

all_artifacts = set(os.listdir(str(resource_artifact_dir)))
actual_files = all_artifacts.intersection(expected_files)
self.assertEquals(actual_files, expected_files)

all_modules = set(os.listdir(str(resource_artifact_dir.joinpath('node_modules'))))
actual_files = all_modules.intersection(expected_modules)
self.assertEquals(actual_files, expected_modules)

def _verify_resource_property(self, template_path, logical_id, property, expected_value):

with open(template_path, 'r') as fp:
template_dict = yaml_parse(fp.read())
self.assertEquals(expected_value, template_dict["Resources"][logical_id]["Properties"][property])
Empty file.
11 changes: 11 additions & 0 deletions tests/integration/testdata/buildcmd/Node/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "npmdeps",
"version": "1.0.0",
"description": "",
"keywords": [],
"author": "",
"license": "APACHE2.0",
"dependencies": {
"minimal-request-promise": "*"
}
}
10 changes: 6 additions & 4 deletions tests/integration/testdata/buildcmd/template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@ AWSTemplateFormatVersion : '2010-09-09'
Transform: AWS::Serverless-2016-10-31

Parameteres:
PythonRuntime:
Runtime:
Type: String
CodeUri:
Type: String

Resources:

PythonFunction:
Function:
Type: AWS::Serverless::Function
Properties:
Handler: main.handler
Runtime: !Ref PythonRuntime
CodeUri: Python
Runtime: !Ref Runtime
CodeUri: !Ref CodeUri
Timeout: 600

OtherRelativePathResource:
Expand Down
14 changes: 14 additions & 0 deletions tests/unit/lib/build_module/test_app_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,20 @@ def test_must_work_for_python(self, runtime):
self.assertEquals(result.application_framework, None)
self.assertEquals(result.manifest_name, "requirements.txt")

@parameterized.expand([
("nodejs6.10", ),
("nodejs8.10", ),
("nodejsX.Y", ),
("nodejs", )
])
def test_must_work_for_nodejs(self, runtime):

result = _get_workflow_config(runtime)
self.assertEquals(result.language, "nodejs")
self.assertEquals(result.dependency_manager, "npm")
self.assertEquals(result.application_framework, None)
self.assertEquals(result.manifest_name, "package.json")

def test_must_raise_for_unsupported_runtimes(self):

runtime = "foobar"
Expand Down