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

jasonterando · 2023-01-25T22:22:14Z

Issue #: 430

Description of changes:

Addition of support for Metadata Include property that can be set to a file mask, or list of file masks, of files in the source directory, and copy those to the artifact (output) directory after build/packaging
An action called CopyResourceAction and utilities glob_copy and get_option_from_args have been added to support the functionality

Note: esbuild will work with the SAM CLI as-is, but for npm (without esbuild), SAM CLI will need to be updated to support sending include (currently, it will only send use_npm_ci and no other Metadata options).

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

…orkflows

qingchm · 2023-01-30T16:46:27Z

Thanks for opening this pull request! We will look at your feature request and see if your changes can be accepted!

jasonterando · 2023-02-01T18:19:21Z

I'll take a look and see what is causing unit tests to fail in Windows

mildaniel

Thanks for taking the time to raise this PR! I've left a few comments but overall it looks good. I do wonder if this should be applied to other runtimes since it seems it's applicable to more than just Node.js (maybe compiled languages).

Would you also be able to raise the corresponding SAM-CLI PR for sending this parameter down?

mildaniel · 2023-02-01T23:15:07Z

aws_lambda_builders/utils.py

+    known_paths = []
+    for item in items:
+        if Path(item).is_absolute():
+            raise ValueError('"{item}" is not a relative path'.format(item=item))


Let's use custom exceptions instead of raising ValueError.

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}"

mildaniel · 2023-02-01T23:17:54Z

aws_lambda_builders/utils.py

+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:


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

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

jasonterando · 2023-02-02T15:23:47Z

Would you also be able to raise the corresponding SAM-CLI PR for sending this parameter down?

Assuming we can get this shepherded through, sure!

jasonterando · 2023-02-02T19:00:29Z

I've updated the following:

Created a FileOperationError to raise when there is a problem copying
Refactored get_option_from_args to implement the suggested .get syntax
Added function robust_rmtree to facilitate retries for shutil.rmtree to workaround a Python on Windows issue (Python issue 40143)

mildaniel

Another thing to consider, and I'll take ownership of this, is that this will require a documentation update to publicize this new option.

mildaniel · 2023-02-03T17:43:14Z

aws_lambda_builders/exceptions.py

+    Raised when a file operation fails
+    """
+
+    MESSAGE = "{operation_name} - {reason}"


Make sure to run make black to reformat according to our style guide.

Sorry, missed that

mildaniel · 2023-02-03T19:33:27Z

aws_lambda_builders/utils.py

@@ -222,3 +225,53 @@ 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:


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

mildaniel · 2023-02-03T19:34:33Z

aws_lambda_builders/utils.py

+    dest_path = Path(destination)
+    known_paths = []
+    for item in items:
+        if os.path.isabs(item.replace('\\', '/')):


Do we need the item.replace()? I think os.path.isabs() should already handle this, I could be wrong though.

I had to have the replace in there to get the "absolute" detection to work properly under Windows. "\foo" is considered a relative path, but "/foo" is absolute (go figure). The purposes of this call is to make sure that the requested asset 's path is relative to the project, so either "\foo" or "/foo" should be treated as non-relative.

mildaniel · 2023-02-03T19:35:44Z

aws_lambda_builders/workflows/nodejs_npm/workflow.py

+        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")


Let's use a custom exception here too.

mildaniel · 2023-02-03T19:37:52Z

aws_lambda_builders/utils.py

+            shutil.copyfile(file, save_to)
+
+
+def robust_rmtree(path, timeout=1):


If this is only used in tests, we should move it there.

Moved to test/functional/test_utils.py

jasonterando · 2023-02-03T20:16:18Z

If I take a stab at working on this stuff over the weekend, can you enable me to have Build & Test checks run. I promise not to submit 100 times.

mildaniel · 2023-02-03T21:16:44Z

If I take a stab at working on this stuff over the weekend, can you enable me to have Build & Test checks run. I promise not to submit 100 times.

Already enabled 👍

jasonterando · 2023-02-06T15:59:09Z

Hi, latest changes committed - make pr passed and I got all but one Gradle integration test to run (is there a Dockerfile available to set up Java, Go, Ruby, .NET, NodeJS?).

With regard to documentation, happy to submit it if you tell me where it should go and in what format. Here's some Markdown I've put together as a first pass.

## **Include** Resources (optional)

This optional build property for nodejs_npm and nodejs_npm_esbuild workflows will copy files to the output (artifacts) directory, after bundling, for inclusion in deployment.

Valid value types include string, dictionary, or a list containing either strings and/or dictionaries.

### String Parameter

Use this to deep-copy files matching the specfied glob-style parameter to the artifact directory.

Example:

```yaml
Resources:
  MyLambda:
    Type: AWS::Serverless::Function
    Metadata:
       BuildMethod: esbuild
       BulidProperties:
         Include: resources/*.json
```

*Project (Source) Directory*
```
index.ts
  /resources
    file1.json
    file2.json
      /data
        file3.json
```

*Artifact (Destination) Directory*

```
index.js
file1.json
file2.json
/data
  file3.json
```

### Dictionary (Object) Parameter

Use this to control where files are copied to in the artifact directory and whether they are deep-copied.

Example:

```yaml
Resources:
  MyLambda:
    Type: AWS::Serverless::Function
    Metadata:
      BuildMethod: esbuild
      BuildProperties:
        Include:
          Source: resources/*.json
          Destination: resources
          Recursive: False
```

*Project (Source) Directory*
```
index.ts
  /resources
    file1.json
    file2.json
      /data
        file3.json
```

*Artifact (Destination) Directory*

```
index.js
  /resources
    file1.json
    file2.json
```

## List Parameter

A list of strings and dictionaries as described above can be used to trigger multiple copy operations.

moelasmar

I have a general comment on this PR, I feel that this action is not related to a specific Runtime, and instead it can be part of sam build command or may be sam package but not part of aws-lambda-builders library. I will raise this PR with the team to discuss what is the better approach to implement this feature.

I will make this PR as draft till we discuss it, and will update it later.

jasonterando · 2023-05-30T22:22:31Z

I have a general comment on this PR, I feel that this action is not related to a specific Runtime, and instead it can be part of sam build command or may be sam package but not part of aws-lambda-builders library. I will raise this PR with the team to discuss what is the better approach to implement this feature.

I will make this PR as draft till we discuss it, and will update it later.

I don't disagree that, ideally, this would be a Runtime-independent feature. However, even within NodeJS, there are differences in when you implement it, based upon whether esbuild is being used or not. You could go the route of creating some kind of event-driven mechanism so that the individual Runtime workflows can take actions (before build, after build, copy resources after build, etc.), but that isn't there today.

Anyways, it's months later and I had to end up going a different route (orchestrating esbuild external to SAM). If you want to nuke this PR, that's fine, we have a workaround, although it would have been nicer to use the integrated esbuild workflow.

mildaniel · 2023-06-12T23:05:26Z

Closing until we figure out the correct path forward. Consolidating include/exclude functionality design and discussion into a single issue #516.

Jason Terando added 5 commits January 24, 2023 21:22

Addition of include resource(s) action to NodeJS and NodeJS/ESBuild w…

e891892

…orkflows

Raise ValueError if include resource glob not found

fb98729

Raise error if expected resource include is missing

9e954a4

Update unit tests

c6f627d

Final testing and formatting clean-up

b575481

jasonterando requested a review from a team as a code owner January 25, 2023 22:22

jasonterando requested review from mndeveci and lucashuy January 25, 2023 22:22

github-actions bot added area/workflow/node_npm pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Jan 25, 2023

jasonterando added 2 commits January 25, 2023 16:22

Merge branch 'develop' into issue-430

d8aede9

Merge branch 'develop' into issue-430

52e7436

torresxb1 removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Jan 31, 2023

Merge branch 'develop' into issue-430

63c938f

mildaniel requested changes Feb 1, 2023

View reviewed changes

Updated per PR requests, added Windows-friendly rmtree

8074e1f

Jason Terando added 2 commits February 2, 2023 13:00

Add 2nd blank line in front of robust_rmtree

66f5284

Merge upstream develop changes

4d857dc

mildaniel requested changes Feb 3, 2023

View reviewed changes

Jason Terando added 3 commits February 3, 2023 18:04

Merge remote-tracking branch 'upstream/develop' into issue-430

831032c

Updates to copy resource/glob action and test

3a6eea4

Clean up calls to build CopyResourceAction

f054638

moelasmar reviewed May 25, 2023

View reviewed changes

moelasmar marked this pull request as draft May 25, 2023 05:52

mildaniel mentioned this pull request Jun 12, 2023

Feature request: Include/Exclude Interface for AWS Lambda Builders #516

Open

mildaniel closed this Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jasonterando commented Jan 25, 2023

qingchm commented Jan 30, 2023

jasonterando commented Feb 1, 2023

mildaniel left a comment •

edited

Loading

mildaniel Feb 1, 2023

jasonterando Feb 1, 2023 •

edited

Loading

mildaniel Feb 1, 2023 •

edited

Loading

jasonterando Feb 2, 2023

jasonterando commented Feb 2, 2023

jasonterando commented Feb 2, 2023

mildaniel left a comment

mildaniel Feb 3, 2023

jasonterando Feb 3, 2023

mildaniel Feb 3, 2023

jasonterando Feb 3, 2023

mildaniel Feb 3, 2023

jasonterando Feb 3, 2023

mildaniel Feb 3, 2023

jasonterando Feb 3, 2023

mildaniel Feb 3, 2023

jasonterando Feb 3, 2023

jasonterando commented Feb 3, 2023

mildaniel commented Feb 3, 2023

jasonterando commented Feb 6, 2023

moelasmar left a comment •

edited

Loading

jasonterando commented May 30, 2023

mildaniel commented Jun 12, 2023

		shutil.copyfile(file, save_to)


		def robust_rmtree(path, timeout=1):

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

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

Conversation

jasonterando commented Jan 25, 2023

qingchm commented Jan 30, 2023

jasonterando commented Feb 1, 2023

mildaniel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonterando Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

mildaniel Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonterando commented Feb 2, 2023

jasonterando commented Feb 2, 2023

mildaniel left a 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

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

jasonterando commented Feb 3, 2023

mildaniel commented Feb 3, 2023

jasonterando commented Feb 6, 2023

moelasmar left a comment • edited Loading

Choose a reason for hiding this comment

jasonterando commented May 30, 2023

mildaniel commented Jun 12, 2023

mildaniel left a comment •

edited

Loading

jasonterando Feb 1, 2023 •

edited

Loading

mildaniel Feb 1, 2023 •

edited

Loading

moelasmar left a comment •

edited

Loading