feat: add symlink support in actions #461
Conversation
torresxb1
changed the title
torresxb1
changed the title
| @@ -1,59 +0,0 @@ | |||
| import os | |||
There was a problem hiding this comment.
moved to tests/functional/test_actions.py and added a few more tests. Since our integration tests are not counted towards code coverage, if I added tests here it would fail the 94% check. Anyway, I think these probably better suited, or at least equally suited, as functional tests.
| @@ -0,0 +1,127 @@ | |||
| import os | |||
There was a problem hiding this comment.
I added lines 34-91, and 121-127. The rest were just moved from it's previous location (tests/integration/test_actions.py)
| @@ -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.
Should we be prescriptive on the necessary settings? ex: set x
There was a problem hiding this comment.
I'm not sure that we can pinpoint exactly why it failed or how to resolve it. It might vary by OS and version.
aws_lambda_builders/actions.py
Outdated
| class LinkSinglePathAction(BaseAction): | ||
| NAME = "LinkSource" | ||
|
|
||
| DESCRIPTION = "Creates symbolic link at dest, pointing to source" |
There was a problem hiding this comment.
Complete nit: let's use full forms for description.
There was a problem hiding this comment.
Do we show this description for the customers any where ?
There was a problem hiding this comment.
Complete nit: let's use full forms for description.
What do you mean by this @sriram-mv ?
Do we show this description for the customers any where ?
I tried looking for this and actually I don't think so. @moelasmar
There was a problem hiding this comment.
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)
| 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.
create_symlink_or_copy(linkto, new_destination)
shutil.copystat(dependencies_source, new_destination, follow_symlinks=False)
We are symlinking and copying?
There was a problem hiding this comment.
same question here ... may be I am missing something, but will the copy ovwerwrite the created symlink ?
There was a problem hiding this comment.
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.
|
Thanks for the reviews! |
Issue #, if available:
Description of changes:
Add symbolic link support in actions for copying code, copying dependencies, and cleaning directories. Also adds an action to support creating a symbolic link from a source path to a destination path.
Currently not used for anything, but will be used for the node_npm workflow in a subsequent PR (would be kinda big if I combined everything into one)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.