Skip to content

feat: update node_npm actions and workflow to support building in source directory #462

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.

Sign up for GitHub

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

Merged
torresxb1 merged 4 commits into from
Mar 2, 2023
Merged

feat: update node_npm actions and workflow to support building in source directory #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d30238
feat: update node_npm actions and workflow to support building in sou…
torresxb1 Mar 1, 2023
7860fbc
remove unused import
torresxb1 Mar 1, 2023
ea0bd17
Merge branch 'develop' into build-in-source-node-npm
torresxb1 Mar 2, 2023
5744b59
fix tests for windows
torresxb1 Mar 2, 2023
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
52 changes: 32 additions & 20 deletions aws_lambda_builders/workflows/nodejs_npm/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
"""

import logging
from typing import Optional

from aws_lambda_builders.actions import ActionFailedError, BaseAction, Purpose
from aws_lambda_builders.utils import extract_tarfile

from .npm import NpmExecutionError
from .npm import NpmExecutionError, SubprocessNpm

LOG = logging.getLogger(__name__)

Expand Down Expand Up @@ -82,21 +83,22 @@ class NodejsNpmInstallAction(BaseAction):
DESCRIPTION = "Installing dependencies from NPM"
PURPOSE = Purpose.RESOLVE_DEPENDENCIES

def __init__(self, install_dir, subprocess_npm):
def __init__(self, install_dir: str, subprocess_npm: SubprocessNpm, install_links: Optional[bool] = False):
"""
:type install_dir: str
:param install_dir: Dependencies will be installed in this directory.

:type subprocess_npm: aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm
:param subprocess_npm: An instance of the NPM process wrapper

:type is_production: bool
:param is_production: NPM installation mode is production (eg --production=false to force dev dependencies)
Parameters
----------
install_dir : str
Dependencies will be installed in this directory.
subprocess_npm : SubprocessNpm
An instance of the NPM process wrapper
install_links : Optional[bool]
Uses the --install-links npm option if True, by default False
"""

super(NodejsNpmInstallAction, self).__init__()
self.install_dir = install_dir
self.subprocess_npm = subprocess_npm
self.install_links = install_links

def execute(self):
"""
Expand All @@ -107,9 +109,11 @@ def execute(self):
try:
LOG.debug("NODEJS installing in: %s", self.install_dir)

self.subprocess_npm.run(
["install", "-q", "--no-audit", "--no-save", "--unsafe-perm", "--production"], cwd=self.install_dir
)
command = ["install", "-q", "--no-audit", "--no-save", "--unsafe-perm", "--production"]
if self.install_links:
command.append("--install-links")

self.subprocess_npm.run(command, cwd=self.install_dir)

except NpmExecutionError as ex:
raise ActionFailedError(str(ex))
Expand All @@ -128,18 +132,22 @@ class NodejsNpmCIAction(BaseAction):
DESCRIPTION = "Installing dependencies from NPM using the CI method"
PURPOSE = Purpose.RESOLVE_DEPENDENCIES

def __init__(self, install_dir, subprocess_npm):
def __init__(self, install_dir: str, subprocess_npm: SubprocessNpm, install_links: Optional[bool] = False):
"""
:type install_dir: str
:param install_dir: Dependencies will be installed in this directory.

:type subprocess_npm: aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm
:param subprocess_npm: An instance of the NPM process wrapper
Parameters
----------
install_dir : str
Dependencies will be installed in this directory.
subprocess_npm : SubprocessNpm
An instance of the NPM process wrapper
install_links : Optional[bool]
Uses the --install-links npm option if True, by default False
"""

super(NodejsNpmCIAction, self).__init__()
self.install_dir = install_dir
self.subprocess_npm = subprocess_npm
self.install_links = install_links

def execute(self):
"""
Expand All @@ -151,7 +159,11 @@ def execute(self):
try:
LOG.debug("NODEJS installing ci in: %s", self.install_dir)

self.subprocess_npm.run(["ci"], cwd=self.install_dir)
command = ["ci"]
if self.install_links:
command.append("--install-links")
Copy link
Contributor

@sriram-mv sriram-mv Mar 1, 2023

Choose a reason for hiding this comment

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

something to keep in mind, is that we are appending in certain places and doing a + operation in other places, let's default to one.

Copy link
Contributor Author

@torresxb1 torresxb1 Mar 1, 2023

Choose a reason for hiding this comment

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

I don't think there's a problem with using both 🤔 . Appending to add one element, using "+" to concatenate two lists.

Copy link
Contributor

@mildaniel mildaniel Mar 1, 2023

Choose a reason for hiding this comment

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

I don't have any strong opinion here, but we can also use .extend() to concatenate two lists if we feel that's cleaner moving forward, though I think += is usually descriptive enough.

Copy link
Contributor

@sriram-mv sriram-mv Mar 1, 2023

Choose a reason for hiding this comment

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

I understand that its not a technical problem, its about cohesion.


self.subprocess_npm.run(command, cwd=self.install_dir)
Copy link
Contributor Author

@torresxb1 torresxb1 Mar 1, 2023

Choose a reason for hiding this comment

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

I've realized the CI action maybe should've just been a subclass of the normal install action, and in general I'm not sure why we don't use the same flags. But it might be too late to change now.


except NpmExecutionError as ex:
raise ActionFailedError(str(ex))
Expand Down
149 changes: 104 additions & 45 deletions aws_lambda_builders/workflows/nodejs_npm/workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
"""

import logging
import os
from typing import Optional

from aws_lambda_builders.actions import (
CleanUpAction,
CopyDependenciesAction,
CopySourceAction,
LinkSinglePathAction,
MoveDependenciesAction,
)
from aws_lambda_builders.path_resolver import PathResolver
Expand Down Expand Up @@ -43,7 +46,7 @@ class NodejsNpmWorkflow(BaseWorkflow):
CONFIG_PROPERTY = "aws_sam"

DEFAULT_BUILD_DIR = BuildDirectory.ARTIFACTS
BUILD_IN_SOURCE_SUPPORT = BuildInSourceSupport.NOT_SUPPORTED
BUILD_IN_SOURCE_SUPPORT = BuildInSourceSupport.OPTIONALLY_SUPPORTED

def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtime=None, osutils=None, **kwargs):
super(NodejsNpmWorkflow, self).__init__(
Expand All @@ -52,6 +55,7 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim

if osutils is None:
osutils = OSUtils()
self.osutils = osutils

if not osutils.file_exists(manifest_path):
LOG.warning("package.json file not found. Continuing the build without dependencies.")
Expand All @@ -62,6 +66,8 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim

tar_dest_dir = osutils.joinpath(scratch_dir, "unpacked")
tar_package_dir = osutils.joinpath(tar_dest_dir, "package")
# TODO: we should probably unpack straight into artifacts dir, rather than unpacking into tar_dest_dir and
# then copying into artifacts. Just make sure EXCLUDED_FILES are not included, or remove them.
npm_pack = NodejsNpmPackAction(
tar_dest_dir, scratch_dir, manifest_path, osutils=osutils, subprocess_npm=subprocess_npm
)
Expand All @@ -75,39 +81,82 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
]

if self.download_dependencies:
# install the dependencies into artifact folder
self.actions.append(
NodejsNpmWorkflow.get_install_action(source_dir, artifacts_dir, subprocess_npm, osutils, self.options)
NodejsNpmWorkflow.get_install_action(
source_dir=source_dir,
install_dir=self.build_dir,
subprocess_npm=subprocess_npm,
osutils=osutils,
build_options=self.options,
install_links=self.build_dir == self.source_dir,
Copy link
Contributor Author

@torresxb1 torresxb1 Mar 1, 2023

Choose a reason for hiding this comment

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

We want to use install-links so npm installs local dependencies without any symbolic links and without dev dependencies or temporary files, but instead just like any other dependency.

)
)

artifacts_cleanup_actions = [
NodejsNpmrcCleanUpAction(artifacts_dir, osutils=osutils),
NodejsNpmLockFileCleanUpAction(artifacts_dir, osutils=osutils),
]
if self.download_dependencies and self.build_dir == self.source_dir:
self.actions += self._actions_for_linking_source_dependencies_to_artifacts

# if no dependencies dir, just cleanup artifacts and we're done
if not self.dependencies_dir:
self.actions += artifacts_cleanup_actions
self.actions += self._actions_for_cleanup
return

# if we downloaded dependencies, update dependencies_dir
if self.download_dependencies:
# clean up the dependencies folder first
self.actions.append(CleanUpAction(self.dependencies_dir))
# if combine_dependencies is set, we should keep dependencies and source code in the artifact folder
# while copying the dependencies. Otherwise we should separate the dependencies and source code
dependencies_dir_update_action = (
CopyDependenciesAction if self.combine_dependencies else MoveDependenciesAction
)
self.actions.append(dependencies_dir_update_action(source_dir, artifacts_dir, self.dependencies_dir))
self.actions += self._actions_for_updating_dependencies_dir
# otherwise if we want to use the dependencies from dependencies_dir and we want to combine them,
# then copy them into the artifacts dir
elif self.combine_dependencies:
self.actions.append(CopySourceAction(self.dependencies_dir, artifacts_dir))
self.actions.append(
CopySourceAction(
self.dependencies_dir, artifacts_dir, maintain_symlinks=self.build_dir == self.source_dir
)
)

# cleanup
self.actions += artifacts_cleanup_actions
self.actions.append(NodejsNpmLockFileCleanUpAction(self.dependencies_dir, osutils=osutils))
self.actions += self._actions_for_cleanup

@property
def _actions_for_cleanup(self):
actions = [NodejsNpmrcCleanUpAction(self.artifacts_dir, osutils=self.osutils)]

# we don't want to cleanup the lockfile in the source code's symlinked node_modules
if self.build_dir != self.source_dir:
actions.append(NodejsNpmLockFileCleanUpAction(self.artifacts_dir, osutils=self.osutils))
if self.build_dir != self.source_dir and self.dependencies_dir:
actions.append(NodejsNpmLockFileCleanUpAction(self.dependencies_dir, osutils=self.osutils))

return actions

@property
def _actions_for_linking_source_dependencies_to_artifacts(self):
source_dependencies_path = os.path.join(self.source_dir, "node_modules")
artifact_dependencies_path = os.path.join(self.artifacts_dir, "node_modules")
return [LinkSinglePathAction(source=source_dependencies_path, dest=artifact_dependencies_path)]

@property
def _actions_for_updating_dependencies_dir(self):
# clean up the dependencies folder first
actions = [CleanUpAction(self.dependencies_dir)]
# if combine_dependencies is set, we should keep dependencies and source code in the artifact folder
# while copying the dependencies. Otherwise we should separate the dependencies and source code
if self.combine_dependencies:
actions.append(
CopyDependenciesAction(
source_dir=self.source_dir,
artifact_dir=self.artifacts_dir,
destination_dir=self.dependencies_dir,
maintain_symlinks=self.build_dir == self.source_dir,
)
)
else:
actions.append(
MoveDependenciesAction(
source_dir=self.source_dir,
artifact_dir=self.artifacts_dir,
destination_dir=self.dependencies_dir,
)
)

return actions

def get_resolvers(self):
"""
Expand All @@ -116,30 +165,36 @@ def get_resolvers(self):
return [PathResolver(runtime=self.runtime, binary="npm")]

@staticmethod
def get_install_action(source_dir, install_dir, subprocess_npm, osutils, build_options):
def get_install_action(
source_dir: str,
install_dir: str,
subprocess_npm: SubprocessNpm,
osutils: OSUtils,
build_options: Optional[dict],
install_links: Optional[bool] = False,
):
"""
Get the install action used to install dependencies at artifacts_dir

:type source_dir: str
:param source_dir: an existing (readable) directory containing source files

:type install_dir: str
:param install_dir: Dependencies will be installed in this directory.

:type osutils: aws_lambda_builders.workflows.nodejs_npm.utils.OSUtils
:param osutils: An instance of OS Utilities for file manipulation

:type subprocess_npm: aws_lambda_builders.workflows.nodejs_npm.npm.SubprocessNpm
:param subprocess_npm: An instance of the NPM process wrapper

:type build_options: Dict
:param build_options: Object containing build options configurations

:type is_production: bool
:param is_production: NPM installation mode is production (eg --production=false to force dev dependencies)

:rtype: BaseAction
:return: Install action to use
Get the install action used to install dependencies.

Parameters
----------
source_dir : str
an existing (readable) directory containing source files
install_dir : str
Dependencies will be installed in this directory
subprocess_npm : SubprocessNpm
An instance of the NPM process wrapper
osutils : OSUtils
An instance of OS Utilities for file manipulation
build_options : Optional[dict]
Object containing build options configurations
install_links : Optional[bool]
Uses the --install-links npm option if True, by default False

Returns
-------
BaseAction
Install action to use
"""
lockfile_path = osutils.joinpath(source_dir, "package-lock.json")
shrinkwrap_path = osutils.joinpath(source_dir, "npm-shrinkwrap.json")
Expand All @@ -149,6 +204,10 @@ def get_install_action(source_dir, install_dir, subprocess_npm, osutils, build_o
npm_ci_option = build_options.get("use_npm_ci", False)

if (osutils.file_exists(lockfile_path) or osutils.file_exists(shrinkwrap_path)) and npm_ci_option:
return NodejsNpmCIAction(install_dir=install_dir, subprocess_npm=subprocess_npm)
return NodejsNpmCIAction(
install_dir=install_dir, subprocess_npm=subprocess_npm, install_links=install_links
)

return NodejsNpmInstallAction(install_dir=install_dir, subprocess_npm=subprocess_npm)
return NodejsNpmInstallAction(
install_dir=install_dir, subprocess_npm=subprocess_npm, install_links=install_links
)
Loading