Skip to content

refactor: simplify node npm workflow conditionals #458

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 3 commits into from
Feb 22, 2023
Merged

refactor: simplify node npm workflow conditionals #458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c05e4f3
refactor: simplify node npm workflow conditionals
torresxb1 Feb 21, 2023
b4bd768
make black
torresxb1 Feb 21, 2023
342357c
update test
torresxb1 Feb 21, 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
97 changes: 33 additions & 64 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 @@ -60,35 +60,6 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim

subprocess_npm = SubprocessNpm(osutils)

self.actions = self.actions_without_bundler(
source_dir, artifacts_dir, scratch_dir, manifest_path, osutils, subprocess_npm
)

def actions_without_bundler(self, source_dir, artifacts_dir, scratch_dir, manifest_path, osutils, subprocess_npm):
Copy link
Contributor Author

@torresxb1 torresxb1 Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this was originally meant for when the workflow would have actions_with_bundler (for esbuild) and actions_without_bundler. But the bundler actions were split into a separate workflow (esbuild), so there's not much use for having actions_without_bundler in a separate function. So I just moved the logic into the workflow constructor.

"""
Generate a list of Nodejs build actions without a bundler

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

:type artifacts_dir: str
:param artifacts_dir: an existing (writable) directory where to store the output.

:type scratch_dir: str
:param scratch_dir: an existing (writable) directory for temporary files

:type manifest_path: str
:param manifest_path: path to package.json of an NPM project with the source to pack

: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

:rtype: list
:return: List of build actions to execute
"""
tar_dest_dir = osutils.joinpath(scratch_dir, "unpacked")
tar_package_dir = osutils.joinpath(tar_dest_dir, "package")
npm_pack = NodejsNpmPackAction(
Expand All @@ -97,48 +68,46 @@ def actions_without_bundler(self, source_dir, artifacts_dir, scratch_dir, manife

npm_copy_npmrc_and_lockfile = NodejsNpmrcAndLockfileCopyAction(tar_package_dir, source_dir, osutils=osutils)

actions = [
self.actions = [
npm_pack,
npm_copy_npmrc_and_lockfile,
CopySourceAction(tar_package_dir, artifacts_dir, excludes=self.EXCLUDED_FILES),
]

if self.download_dependencies:
# installed the dependencies into artifact folder
install_action = NodejsNpmWorkflow.get_install_action(
source_dir, artifacts_dir, subprocess_npm, osutils, self.options
# install the dependencies into artifact folder
self.actions.append(
NodejsNpmWorkflow.get_install_action(source_dir, artifacts_dir, subprocess_npm, osutils, self.options)
)

artifacts_cleanup_actions = [
NodejsNpmrcCleanUpAction(artifacts_dir, osutils=osutils),
NodejsNpmLockFileCleanUpAction(artifacts_dir, osutils=osutils),
]

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

# if we downloaded dependencies, update dependencies_dir
if self.download_dependencies:
Copy link
Contributor

@sriram-mv sriram-mv Feb 21, 2023

Choose a reason for hiding this comment

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

are we branching on the same condition twice?

Copy link
Contributor

@sriram-mv sriram-mv Feb 21, 2023

Choose a reason for hiding this comment

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

One other general comment is we have more than a few branches, we should go down a handler route where we mutate on a a given object all the way through.

eg: self.actions is the one we mutate in this workflow, something like below would be useful to grok through it.

for action_mapper in action_mappers:
    action_mapper.apply(*args, **kwargs)

Copy link
Contributor Author

@torresxb1 torresxb1 Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

are we branching on the same condition twice?

The first time it doesn't depend on self.dependencies_dir. The second time we can assume we have self.dependencies_dir (since we checked it before).

Copy link
Contributor

@sriram-mv sriram-mv Feb 22, 2023

Choose a reason for hiding this comment

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

L77 and L99

the conditions are the same, what am I missing?

Copy link
Contributor Author

@torresxb1 torresxb1 Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this part in the middle of those:

aws-lambda-builders/aws_lambda_builders/workflows/nodejs_npm/workflow.py

Lines 89 to 91 in 342357c

if not self.dependencies_dir:
self.actions += artifacts_cleanup_actions
return

Copy link
Contributor Author

@torresxb1 torresxb1 Feb 22, 2023

Choose a reason for hiding this comment

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

The first time it doesn't care about, or depend on, self.dependencies_dir. The second time, since we have checked dependencies_dir, we can assume we have it and we don't have to check for it again. Otherwise it would require some conditional nesting or repetitive conditions, which is what I'm trying to refactor out here 😅

Copy link
Contributor

@sriram-mv sriram-mv Feb 22, 2023

Choose a reason for hiding this comment

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

to me this is not intuitive and requires a further refactor.

aws-lambda-builders/aws_lambda_builders/workflows/nodejs_npm/workflow.py

Line 109 in 342357c

self.actions += artifacts_cleanup_actions
and

aws-lambda-builders/aws_lambda_builders/workflows/nodejs_npm/workflow.py

Line 90 in 342357c

self.actions += artifacts_cleanup_actions
is also repeated.

Copy link
Contributor Author

@torresxb1 torresxb1 Feb 22, 2023

Choose a reason for hiding this comment

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

^ the above is also due to something similar. One is performed when there's no dependencies dir and one is performed when there is.

To me the refactor is easier to read than before, but let me know if you disagree. Also open to suggestions on how to further refactor.

# 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
)
actions.append(install_action)

# if dependencies folder exists, copy or move dependencies from artifact folder to dependencies folder
# depends on the combine_dependencies flag
if self.dependencies_dir:
# clean up the dependencies folder first
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
if self.combine_dependencies:
actions.append(CopyDependenciesAction(source_dir, artifacts_dir, self.dependencies_dir))
else:
actions.append(MoveDependenciesAction(source_dir, artifacts_dir, self.dependencies_dir))
else:
# if dependencies folder exists and not download dependencies, simply copy the dependencies from the
# dependencies folder to artifact folder
if self.dependencies_dir and self.combine_dependencies:
actions.append(CopySourceAction(self.dependencies_dir, artifacts_dir))
else:
LOG.info(
"download_dependencies is False and dependencies_dir is None. Copying the source files into the "
"artifacts directory. "
)
Comment on lines -130 to -133
Copy link
Contributor Author

@torresxb1 torresxb1 Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I thought this message was a bit weird and not totally accurate, so I removed it. The message could be printed even if dependencies_dir is not None (if combine_dependencies is False) . Let me know if you think I should bring this message back.

Copy link
Contributor Author

@torresxb1 torresxb1 Feb 21, 2023

Choose a reason for hiding this comment

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

Note that the conditions that lead to this message being output is not a valid SAM CLI use case anyway (shouldn't be very common):
https://github.com/aws/aws-lambda-builders/pull/272/files#r701280071


actions.append(NodejsNpmrcCleanUpAction(artifacts_dir, osutils=osutils))
actions.append(NodejsNpmLockFileCleanUpAction(artifacts_dir, osutils=osutils))

if self.dependencies_dir:
actions.append(NodejsNpmLockFileCleanUpAction(self.dependencies_dir, osutils=osutils))

return actions
self.actions.append(dependencies_dir_update_action(source_dir, artifacts_dir, self.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))

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

def get_resolvers(self):
"""
Expand Down
5 changes: 0 additions & 5 deletions tests/integration/workflows/nodejs_npm/test_nodejs_npm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,6 @@ def test_builds_project_with_remote_dependencies_without_download_dependencies_w
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

mock_info.assert_called_with(
"download_dependencies is False and dependencies_dir is None. Copying the source files into the "
"artifacts directory. "
)

@parameterized.expand([("nodejs12.x",), ("nodejs14.x",), ("nodejs16.x",), ("nodejs18.x",)])
def test_builds_project_without_combine_dependencies(self, runtime):
source_dir = os.path.join(self.TEST_DATA_FOLDER, "npm-deps")
Expand Down