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

Add Include resource functionality to npm and npm/esbuild #431

Closed
wants to merge 14 commits into from
Closed
30 changes: 29 additions & 1 deletion aws_lambda_builders/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from typing import Set, Iterator, Tuple

from aws_lambda_builders import utils
from aws_lambda_builders.utils import copytree
from aws_lambda_builders.utils import copytree, glob_copy

LOG = logging.getLogger(__name__)

Expand Down Expand Up @@ -45,6 +45,9 @@ class Purpose(object):
# Action is compiling source code
COMPILE_SOURCE = "COMPILE_SOURCE"

# Action is copying resources for deployment
COPY_RESOURCES = "COPY_RESOURCES"

# Action is cleaning up the target folder
CLEAN_UP = "CLEAN_UP"

Expand Down Expand Up @@ -220,6 +223,31 @@ def execute(self):
os.remove(target_path)


class CopyResourceAction(BaseAction):
"""
Class to copy a relative glob-defined or list of glob-defined resources to the specified destination
"""

NAME = "CopyResource"

DESCRIPTION = "Copying resource files for deployment"

PURPOSE = Purpose.COPY_RESOURCES

def __init__(self, source_dir, source_globs, dest_dir):
self.source_dir = source_dir
self.source_globs = source_globs
self.dest_dir = dest_dir

def execute(self):
old_dir = os.getcwd()
try:
os.chdir(self.source_dir)
glob_copy(self.source_globs, self.dest_dir)
finally:
os.chdir(old_dir)


class DependencyManager:
"""
Class for handling the management of dependencies between directories
Expand Down
32 changes: 31 additions & 1 deletion aws_lambda_builders/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
import sys
import os
import logging
from glob import glob
from pathlib import Path
from typing import Union
from typing import Union, Dict

from aws_lambda_builders.architecture import ARM64

Expand Down Expand Up @@ -222,3 +223,32 @@ def extract_tarfile(tarfile_path: Union[str, os.PathLike], unpack_dir: Union[str
raise tarfile.ExtractError("Attempted Path Traversal in Tar File")

tar.extractall(unpack_dir)


def glob_copy(source: Union[str, list], destination: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add type hints and docstrings to all functions.

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing

items = source if isinstance(source, list) else [source]
dest_path = Path(destination)
known_paths = []
for item in items:
if Path(item).is_absolute():
raise ValueError('"{item}" is not a relative path'.format(item=item))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use custom exceptions instead of raising ValueError.

Copy link
Author

@jasonterando jasonterando Feb 1, 2023

Choose a reason for hiding this comment

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

Any specific one you want me to use, or should I roll one? Would something like this work?

class FileOperationError(LambdaBuilderError):
    """
    Raised when a file operation fails
    """

    MESSAGE = "{operation_name} - {reason}"

files = glob(item, recursive=True)
if len(files) == 0:
raise ValueError('"{item}" not found'.format(item=item))
for file in files:
save_to = str(dest_path.joinpath(file))
LOG.debug("Copying resource file from source (%s) to destination (%s)", file, save_to)
save_to_dir = os.path.dirname(save_to)
if save_to_dir not in known_paths:
os.makedirs(save_to_dir, exist_ok=True)
known_paths.append(save_to_dir)
shutil.copyfile(file, save_to)


def get_option_from_args(args: Union[None, Dict[str, any]], option_name: str) -> any:
if args is not None:
if "options" in args:
if args["options"] is not None:
Copy link
Contributor

@mildaniel mildaniel Feb 1, 2023

Choose a reason for hiding this comment

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

Let's do

if args.get("options") is not None: 

instead so that if options isn't there, it doesn't raise a key error. And the same thing for wherever else we do dictionary accesses like this

Copy link
Author

Choose a reason for hiding this comment

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

Roger that. Is this the correct way - should I be doing something more "pythonic"?

def get_option_from_args(args: Union[None, Dict[str, any]], option_name: str) -> any:
    if args is not None:
        options = args.get("options", None)
        if options is not None:
            return options.get(option_name, None)
    return None

if option_name in args["options"]:
return args["options"][option_name]
return None
8 changes: 8 additions & 0 deletions aws_lambda_builders/workflows/nodejs_npm/workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
CleanUpAction,
CopyDependenciesAction,
MoveDependenciesAction,
CopyResourceAction,
)

from .actions import (
Expand All @@ -23,6 +24,7 @@
)
from .utils import OSUtils
from .npm import SubprocessNpm
from ...utils import get_option_from_args

LOG = logging.getLogger(__name__)

Expand Down Expand Up @@ -65,6 +67,12 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
source_dir, artifacts_dir, scratch_dir, manifest_path, osutils, subprocess_npm
)

include = get_option_from_args(kwargs, "include")
if isinstance(include, (list, str)):
self.actions.append(CopyResourceAction(source_dir, include, artifacts_dir))
elif include is not None:
raise ValueError("Resource include items must be strings or lists of strings")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a custom exception here too.

Copy link
Author

Choose a reason for hiding this comment

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

Yes sir


def actions_without_bundler(self, source_dir, artifacts_dir, scratch_dir, manifest_path, osutils, subprocess_npm):
"""
Generate a list of Nodejs build actions without a bundler
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
MoveDependenciesAction,
BaseAction,
LinkSourceAction,
CopyResourceAction,
)
from aws_lambda_builders.utils import which
from aws_lambda_builders.utils import which, get_option_from_args
from .actions import (
EsbuildBundleAction,
EsbuildCheckVersionAction,
Expand Down Expand Up @@ -74,6 +75,12 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
source_dir, scratch_dir, artifacts_dir, bundler_config, osutils, subprocess_npm, subprocess_esbuild
)

include = get_option_from_args(kwargs, "include")
if isinstance(include, (list, str)):
self.actions.append(CopyResourceAction(source_dir, include, artifacts_dir))
elif include is not None:
raise ValueError("Resource include items must be strings or lists of strings")

def actions_with_bundler(
self, source_dir, scratch_dir, artifacts_dir, bundler_config, osutils, subprocess_npm, subprocess_esbuild
) -> List[BaseAction]:
Expand Down
80 changes: 78 additions & 2 deletions tests/functional/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from unittest import TestCase

from aws_lambda_builders.utils import copytree, get_goarch, extract_tarfile
from aws_lambda_builders.utils import copytree, get_goarch, extract_tarfile, glob_copy, get_option_from_args


class TestCopyTree(TestCase):
Expand Down Expand Up @@ -78,11 +78,70 @@ def test_raise_exception_for_unsafe_tarfile(self):
tar_filename = "path_reversal_win.tgz" if platform.system().lower() == "windows" else "path_reversal_uxix.tgz"
test_tar = os.path.join(os.path.dirname(__file__), "testdata", tar_filename)
test_dir = tempfile.mkdtemp()
self.assertRaisesRegexp(
self.assertRaisesRegex(
ExtractError, "Attempted Path Traversal in Tar File", extract_tarfile, test_tar, test_dir
)


class TestGlobCopy(TestCase):
def setUp(self):
self.save_dir = os.getcwd()
self.source = tempfile.mkdtemp()
self.dest = tempfile.mkdtemp()

def tearDown(self):
shutil.rmtree(self.source)
shutil.rmtree(self.dest)
os.chdir(self.save_dir)

def test_copy_single_file(self):
os.chdir(self.source)
file(".", "a", "file.txt")
glob_copy(os.path.join(".", "a", "file.txt"), self.dest)
self.assertTrue(os.path.exists(os.path.join(self.dest, "a", "file.txt")))

def test_copy_single_wildcard(self):
os.chdir(self.source)
file(".", "a", "b", "file1.txt")
file(".", "a", "b", "file2.txt")
glob_copy(os.path.join(".", "a", "b", "file*.txt"), self.dest)
self.assertTrue(os.path.exists(os.path.join(self.dest, "a", "b", "file1.txt")))
self.assertTrue(os.path.exists(os.path.join(self.dest, "a", "b", "file2.txt")))

def test_copy_list_with_wildcards(self):
os.chdir(self.source)
file(".", "a", "file1.txt")
file(".", "a", "file2.txt")
file(".", "b", "file3.txt")
file(".", "c", "file4.txt")
file(".", "c", "file5.txt")
glob_copy(
[os.path.join(".", "a", "file*.txt"), os.path.join(".", "b", "file3.txt"), os.path.join(".", "c", "*")],
self.dest,
)
self.assertTrue(os.path.exists(os.path.join(self.dest, "a", "file1.txt")))
self.assertTrue(os.path.exists(os.path.join(self.dest, "a", "file2.txt")))
self.assertTrue(os.path.exists(os.path.join(self.dest, "b", "file3.txt")))
self.assertTrue(os.path.exists(os.path.join(self.dest, "c", "file4.txt")))
self.assertTrue(os.path.exists(os.path.join(self.dest, "c", "file5.txt")))

def test_raise_exception_for_single_absolute_glob(self):
test = "\\foo" if os.name == "nt" else "/foo"
self.assertRaisesRegex(
ValueError, '"{test}" is not a relative path'.format(test=test), glob_copy, test, "./dest"
)

def test_raise_exception_for_list_item_absolute_glob(self):
test = "\\bar" if os.name == "nt" else "/bar"
self.assertRaisesRegex(
ValueError, '"{test}" is not a relative path'.format(test=test), glob_copy, [test], "./dest"
)

def test_raise_exception_for_not_found(self):
test = "./not-going-to-exist-in-100-years"
self.assertRaisesRegex(ValueError, '"{test}" not found'.format(test=test), glob_copy, test, "./dest")


def file(*args):
path = os.path.join(*args)
basedir = os.path.dirname(path)
Expand All @@ -91,3 +150,20 @@ def file(*args):

# empty file
open(path, "a").close()


class TestGetOptionFromArgs(TestCase):
def test_returns_none_on_no_args(self):
self.assertEqual(None, get_option_from_args(None, "foo"))

def test_returns_none_on_no_options_in_args(self):
self.assertEqual(None, get_option_from_args({}, "foo"))

def test_returns_none_on_none_options_in_args(self):
self.assertEqual(None, get_option_from_args({"options": None}, "foo"))

def test_returns_none_on_no_matching_option_in_args(self):
self.assertEqual(None, get_option_from_args({"options": {}}, "foo"))

def test_returns_value_on_matching_option_in_args(self):
self.assertEqual("bar", get_option_from_args({"options": {"foo": "bar"}}, "foo"))
4 changes: 2 additions & 2 deletions tests/integration/test_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def test_copy_dependencies_action(self, source_folder):
copy_dependencies_action = CopyDependenciesAction(empty_source, test_folder, target)
copy_dependencies_action.execute()

self.assertEqual(os.listdir(test_folder), os.listdir(target))
self.assertEqual(sorted(os.listdir(test_folder)), sorted(os.listdir(target)))


class TestMoveDependenciesAction(TestCase):
Expand All @@ -56,4 +56,4 @@ def test_move_dependencies_action(self, source_folder):
move_dependencies_action = MoveDependenciesAction(empty_source, test_source, target)
move_dependencies_action.execute()

self.assertEqual(os.listdir(test_folder), os.listdir(target))
self.assertEqual(sorted(os.listdir(test_folder)), sorted(os.listdir(target)))
76 changes: 75 additions & 1 deletion tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,84 @@ def test_builds_project_without_dependencies(self, runtime):
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

@parameterized.expand([("nodejs12.x",), ("nodejs14.x",), ("nodejs16.x",), ("nodejs18.x",)])
def test_builds_project_without_dependencies_and_resource_wildcard(self, runtime):
source_dir = os.path.join(self.TEST_DATA_FOLDER, "no-deps-with-resources")

self.builder.build(
source_dir,
self.artifacts_dir,
self.scratch_dir,
os.path.join(source_dir, "package.json"),
runtime=runtime,
options={"include": "resources/file*"},
)

expected_files1 = {"package.json", "included.js", "resources"}
output_files1 = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files1, output_files1)

expected_files2 = {"file1.txt", "file2.txt"}
output_files2 = set(os.listdir(os.path.join(self.artifacts_dir, "resources")))
self.assertEqual(expected_files2, output_files2)

@parameterized.expand([("nodejs12.x",), ("nodejs14.x",), ("nodejs16.x",), ("nodejs18.x",)])
def test_builds_project_without_dependencies_and_resource_list(self, runtime):
source_dir = os.path.join(self.TEST_DATA_FOLDER, "no-deps-with-resources")

self.builder.build(
source_dir,
self.artifacts_dir,
self.scratch_dir,
os.path.join(source_dir, "package.json"),
runtime=runtime,
options={"include": ["resources/file1.txt", "resources/file2.txt"]},
)

expected_files1 = {"package.json", "included.js", "resources"}
output_files1 = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files1, output_files1)

expected_files2 = {"file1.txt", "file2.txt"}
output_files2 = set(os.listdir(os.path.join(self.artifacts_dir, "resources")))
self.assertEqual(expected_files2, output_files2)

@parameterized.expand([("nodejs12.x",), ("nodejs14.x",), ("nodejs16.x",), ("nodejs18.x",)])
def test_builds_project_without_dependencies_and_no_include_option(self, runtime):
source_dir = os.path.join(self.TEST_DATA_FOLDER, "no-deps-with-resources")

self.builder.build(
source_dir,
self.artifacts_dir,
self.scratch_dir,
os.path.join(source_dir, "package.json"),
runtime=runtime,
options={},
)

expected_files = {"package.json", "included.js"}
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

@parameterized.expand([("nodejs12.x",), ("nodejs14.x",), ("nodejs16.x",), ("nodejs18.x",)])
def test_error_project_without_dependencies_and_invalid_resource(self, runtime):
source_dir = os.path.join(self.TEST_DATA_FOLDER, "no-deps-with-resources")

self.assertRaisesRegex(
ValueError,
"Resource include items must be strings or lists of strings",
self.builder.build,
source_dir,
self.artifacts_dir,
self.scratch_dir,
os.path.join(source_dir, "package.json"),
runtime=runtime,
options={"include": {}},
)

@parameterized.expand([("nodejs12.x",), ("nodejs14.x",), ("nodejs16.x",), ("nodejs18.x",)])
def test_builds_project_without_manifest(self, runtime):
source_dir = os.path.join(self.TEST_DATA_FOLDER, "no-manifest")

with mock.patch.object(logger, "warning") as mock_warning:
self.builder.build(
source_dir,
Expand Down
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,9 @@
{
"name": "nodeps",
"version": "1.0.0",
"description": "",
"files": ["included.js"],
"keywords": [],
"author": "",
"license": "APACHE2.0"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
file1
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
file2
Loading