-
Notifications
You must be signed in to change notification settings - Fork 139
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
feat: add symlink support in actions #461
Changes from 4 commits
4c0542e
f0a2910
a832d83
1c87209
dda3bd6
5f760de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,10 +6,10 @@ | |
import os | ||
import shutil | ||
from pathlib import Path | ||
from typing import Iterator, Set, Tuple | ||
from typing import Iterator, Set, Tuple, Union | ||
|
||
from aws_lambda_builders import utils | ||
from aws_lambda_builders.utils import copytree | ||
from aws_lambda_builders.utils import copytree, create_symlink_or_copy | ||
|
||
LOG = logging.getLogger(__name__) | ||
|
||
|
@@ -105,13 +105,19 @@ class CopySourceAction(BaseAction): | |
|
||
PURPOSE = Purpose.COPY_SOURCE | ||
|
||
def __init__(self, source_dir, dest_dir, excludes=None): | ||
def __init__(self, source_dir, dest_dir, excludes=None, maintain_symlinks=False): | ||
self.source_dir = source_dir | ||
self.dest_dir = dest_dir | ||
self.excludes = excludes or [] | ||
self.maintain_symlinks = maintain_symlinks | ||
|
||
def execute(self): | ||
copytree(self.source_dir, self.dest_dir, ignore=shutil.ignore_patterns(*self.excludes)) | ||
copytree( | ||
self.source_dir, | ||
self.dest_dir, | ||
ignore=shutil.ignore_patterns(*self.excludes), | ||
maintain_symlinks=self.maintain_symlinks, | ||
) | ||
|
||
|
||
class LinkSourceAction(BaseAction): | ||
|
@@ -138,24 +144,48 @@ def execute(self): | |
utils.create_symlink_or_copy(str(source_path), str(destination_path)) | ||
|
||
|
||
class LinkSinglePathAction(BaseAction): | ||
NAME = "LinkSource" | ||
|
||
DESCRIPTION = "Creates symbolic link at dest, pointing to source" | ||
|
||
PURPOSE = Purpose.LINK_SOURCE | ||
|
||
def __init__(self, source: Union[str, os.PathLike], dest: Union[str, os.PathLike]): | ||
self._source = source | ||
self._dest = dest | ||
|
||
def execute(self): | ||
destination_path = Path(self._dest) | ||
if not destination_path.exists(): | ||
os.makedirs(destination_path.parent, exist_ok=True) | ||
utils.create_symlink_or_copy(str(self._source), str(destination_path)) | ||
|
||
|
||
class CopyDependenciesAction(BaseAction): | ||
NAME = "CopyDependencies" | ||
|
||
DESCRIPTION = "Copying dependencies while skipping source file" | ||
|
||
PURPOSE = Purpose.COPY_DEPENDENCIES | ||
|
||
def __init__(self, source_dir, artifact_dir, destination_dir): | ||
def __init__(self, source_dir, artifact_dir, destination_dir, maintain_symlinks=False): | ||
self.source_dir = source_dir | ||
self.artifact_dir = artifact_dir | ||
self.dest_dir = destination_dir | ||
self.maintain_symlinks = maintain_symlinks | ||
|
||
def execute(self): | ||
deps_manager = DependencyManager(self.source_dir, self.artifact_dir, self.dest_dir) | ||
|
||
for dependencies_source, new_destination in deps_manager.yield_source_dest(): | ||
if os.path.isdir(dependencies_source): | ||
copytree(dependencies_source, new_destination) | ||
if os.path.islink(dependencies_source) and self.maintain_symlinks: | ||
os.makedirs(os.path.dirname(new_destination), exist_ok=True) | ||
linkto = os.readlink(dependencies_source) | ||
create_symlink_or_copy(linkto, new_destination) | ||
shutil.copystat(dependencies_source, new_destination, follow_symlinks=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We are symlinking and copying? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same question here ... may be I am missing something, but will the copy ovwerwrite the created symlink ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. copystat copies the metadata: https://docs.python.org/3/library/shutil.html#shutil.copystat Since we are "copying" the symlink by creating a new one that points to the same location as the other one, I thought we might want to copy over the metadata. Let me know if you think this is not necessary. |
||
elif os.path.isdir(dependencies_source): | ||
copytree(dependencies_source, new_destination, maintain_symlinks=self.maintain_symlinks) | ||
else: | ||
os.makedirs(os.path.dirname(new_destination), exist_ok=True) | ||
shutil.copy2(dependencies_source, new_destination) | ||
|
@@ -209,7 +239,9 @@ def execute(self): | |
target_path = os.path.join(self.target_dir, name) | ||
LOG.debug("Clean up action: %s is deleted", str(target_path)) | ||
|
||
if os.path.isdir(target_path): | ||
if os.path.islink(target_path): | ||
os.unlink(target_path) | ||
elif os.path.isdir(target_path): | ||
shutil.rmtree(target_path) | ||
else: | ||
os.remove(target_path) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
LOG = logging.getLogger(__name__) | ||
|
||
|
||
def copytree(source, destination, ignore=None, include=None): | ||
def copytree(source, destination, ignore=None, include=None, maintain_symlinks=False): | ||
torresxb1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
Similar to shutil.copytree except that it removes the limitation that the destination directory should | ||
be present. | ||
|
@@ -74,8 +74,12 @@ def copytree(source, destination, ignore=None, include=None): | |
LOG.debug("File (%s) doesn't satisfy the include rule, skipping it", name) | ||
continue | ||
|
||
if os.path.isdir(new_source): | ||
copytree(new_source, new_destination, ignore=ignore, include=include) | ||
if os.path.islink(new_source) and maintain_symlinks: | ||
linkto = os.readlink(new_source) | ||
create_symlink_or_copy(linkto, new_destination) | ||
shutil.copystat(new_source, new_destination, follow_symlinks=False) | ||
torresxb1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
elif os.path.isdir(new_source): | ||
copytree(new_source, new_destination, ignore=ignore, include=include, maintain_symlinks=maintain_symlinks) | ||
else: | ||
LOG.debug("Copying source file (%s) to destination (%s)", new_source, new_destination) | ||
shutil.copy2(new_source, new_destination) | ||
|
@@ -193,7 +197,8 @@ def create_symlink_or_copy(source: str, destination: str) -> None: | |
os.symlink(Path(source).absolute(), Path(destination).absolute()) | ||
except OSError as ex: | ||
LOG.warning( | ||
"Symlink operation is failed, falling back to copying files", | ||
"Symbolic link creation failed, falling back to copying files instead. To optimize speed, " | ||
"consider enabling the necessary settings or privileges on your system to support symbolic links.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be prescriptive on the necessary settings? ex: set x There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that we can pinpoint exactly why it failed or how to resolve it. It might vary by OS and version. |
||
exc_info=ex if LOG.isEnabledFor(logging.DEBUG) else None, | ||
) | ||
copytree(source, destination) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
import os | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added lines 34-91, and 121-127. The rest were just moved from it's previous location ( |
||
from pathlib import Path | ||
import tempfile | ||
from unittest import TestCase | ||
from parameterized import parameterized | ||
|
||
|
||
from aws_lambda_builders.actions import CopyDependenciesAction, LinkSinglePathAction, MoveDependenciesAction | ||
from aws_lambda_builders.utils import copytree | ||
|
||
|
||
class TestCopyDependenciesAction(TestCase): | ||
@parameterized.expand( | ||
[ | ||
("single_file",), | ||
("multiple_files",), | ||
("empty_subfolders",), | ||
] | ||
) | ||
def test_copy_dependencies_action(self, source_folder): | ||
curr_dir = Path(__file__).resolve().parent | ||
test_folder = os.path.join(curr_dir, "testdata", source_folder) | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
empty_source = os.path.join(tmpdir, "empty_source") | ||
target = os.path.join(tmpdir, "target") | ||
|
||
os.mkdir(empty_source) | ||
|
||
copy_dependencies_action = CopyDependenciesAction(empty_source, test_folder, target) | ||
copy_dependencies_action.execute() | ||
|
||
self.assertEqual(os.listdir(test_folder), os.listdir(target)) | ||
|
||
def test_must_maintain_symlinks_if_enabled(self): | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
source_dir = os.path.join(tmpdir, "source") | ||
artifact_dir = os.path.join(tmpdir, "artifact") | ||
destination_dir = os.path.join(tmpdir, "destination") | ||
|
||
source_node_modules = os.path.join(source_dir, "node_modules") | ||
os.makedirs(source_node_modules) | ||
os.makedirs(artifact_dir) | ||
os.symlink(source_node_modules, os.path.join(artifact_dir, "node_modules")) | ||
|
||
copy_dependencies_action = CopyDependenciesAction( | ||
source_dir=source_dir, | ||
artifact_dir=artifact_dir, | ||
destination_dir=destination_dir, | ||
maintain_symlinks=True, | ||
) | ||
copy_dependencies_action.execute() | ||
|
||
destination_node_modules = os.path.join(destination_dir, "node_modules") | ||
self.assertTrue(os.path.islink(destination_node_modules)) | ||
destination_node_modules_target = read_link_without_junction_prefix(destination_node_modules) | ||
self.assertEqual(destination_node_modules_target, source_node_modules) | ||
|
||
def test_must_not_maintain_symlinks_by_default(self): | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
source_dir = os.path.join(tmpdir, "source") | ||
artifact_dir = os.path.join(tmpdir, "artifact") | ||
destination_dir = os.path.join(tmpdir, "destination") | ||
|
||
source_node_modules = os.path.join(source_dir, "node_modules") | ||
os.makedirs(os.path.join(source_node_modules, "some_package")) | ||
os.makedirs(artifact_dir) | ||
os.symlink(source_node_modules, os.path.join(artifact_dir, "node_modules")) | ||
|
||
copy_dependencies_action = CopyDependenciesAction( | ||
source_dir=source_dir, artifact_dir=artifact_dir, destination_dir=destination_dir | ||
) | ||
copy_dependencies_action.execute() | ||
|
||
destination_node_modules = os.path.join(destination_dir, "node_modules") | ||
self.assertFalse(os.path.islink(destination_node_modules)) | ||
self.assertEqual(os.listdir(destination_node_modules), os.listdir(source_node_modules)) | ||
|
||
|
||
class TestLinkSinglePathAction(TestCase): | ||
def test_link_directory(self): | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
source_dir = os.path.join(tmpdir, "source") | ||
os.makedirs(source_dir) | ||
dest_dir = os.path.join(tmpdir, "dest") | ||
|
||
link_action = LinkSinglePathAction(source_dir, dest_dir) | ||
link_action.execute() | ||
|
||
self.assertTrue(os.path.islink(dest_dir)) | ||
dest_dir_target = read_link_without_junction_prefix(dest_dir) | ||
self.assertEqual(dest_dir_target, source_dir) | ||
|
||
|
||
class TestMoveDependenciesAction(TestCase): | ||
@parameterized.expand( | ||
[ | ||
("single_file",), | ||
("multiple_files",), | ||
("empty_subfolders",), | ||
] | ||
) | ||
def test_move_dependencies_action(self, source_folder): | ||
curr_dir = Path(__file__).resolve().parent | ||
test_folder = os.path.join(curr_dir, "testdata", source_folder) | ||
with tempfile.TemporaryDirectory() as tmpdir: | ||
test_source = os.path.join(tmpdir, "test_source") | ||
empty_source = os.path.join(tmpdir, "empty_source") | ||
target = os.path.join(tmpdir, "target") | ||
|
||
os.mkdir(test_source) | ||
os.mkdir(empty_source) | ||
|
||
copytree(test_folder, test_source) | ||
|
||
move_dependencies_action = MoveDependenciesAction(empty_source, test_source, target) | ||
move_dependencies_action.execute() | ||
|
||
self.assertEqual(os.listdir(test_folder), os.listdir(target)) | ||
|
||
|
||
def read_link_without_junction_prefix(path: str): | ||
# When our tests run on CI on Windows, it seems to use junctions, which causes symlink targets | ||
# have a prefix. This function reads a symlink and returns the target without the prefix (if any). | ||
target = os.readlink(path) | ||
if target.startswith("\\\\?\\"): # \\?\, with escaped slashes | ||
target = target[4:] | ||
return target |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complete nit: let's use full forms for description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we show this description for the customers any where ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by this @sriram-mv ?
I tried looking for this and actually I don't think so. @moelasmar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dest -> destination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhh I thought you were referring to a form, as in filling out a form, or something. Yeah I can change it (although again I'm not sure what this description is used for as I don't think it's displayed anywhere AFAIK)