fix: fix the copy terraform script to maintain the files permissions while extracting the ZIP files - #4640
Conversation
… library modules imports
github-actions
Bot
added
pr/internal
area/local/invoke
| try: | ||
| # this case will work only while executing the copy terraform command from the makefile | ||
| from zip import unzip # type: ignore | ||
| except ImportError: | ||
| # this way to fix the hidden import unit testing | ||
| from samcli.local.lambdafn.zip import unzip |
There was a problem hiding this comment.
why do we use something different for unit tests than for the actual execution? Doesn't this remove the confidence we can have in our unit tests?
There was a problem hiding this comment.
The issue is the script will run in a different way than we do in our unit testing (the hidden import test case) .. let me check with Wing how can we remove it from this test case.
There was a problem hiding this comment.
Which unit test does it break?
There was a problem hiding this comment.
my instinct is we should fix the test
There was a problem hiding this comment.
I skipped the samcli.hook_packages.terraform.copy_terraform_built_artifacts from the test_import_should_succeed_for_a_defined_hidden_package test case, as this script will be only executed as a standalone script and does not depend on any non standard modules
torresxb1
left a comment
torresxb1
left a comment
There was a problem hiding this comment.
left comments above, but will change to approve so I don't block
|
|
||
| # Forcefully set the permissions to 700 on files and directories. This is to ensure the owner | ||
| # of the files is the only one that can read, write, or execute the files. | ||
| unzip(layer_zip_path, unzip_output_dir, permission=0o700) |
There was a problem hiding this comment.
we want to be very careful with directly setting permissions anywhere, whatever we do should be additive.
There was a problem hiding this comment.
the unzipped files will be used only for local testing, and will not be packaged or deployed using sam cli, so I think there is no risk
…raform # Conflicts: # samcli/local/lambdafn/zip.py # samcli/local/layers/layer_downloader.py
| samcli_root_path = Path(os.path.dirname(__file__)).parent.parent.parent.parent | ||
|
|
||
| # copy zip.py script into output directory | ||
| ZIP_UTILS_MODULE_script_path = os.path.join(samcli_root_path, "local", "lambdafn", ZIP_UTILS_MODULE) |
There was a problem hiding this comment.
nit: use all lowercase for variable name - https://peps.python.org/pep-0008/#function-and-variable-names
4 participants
Which issue(s) does this change fix?
#4605
Why is this change necessary?
The current copy terraform built artifacts script loses all the file permissions while extracting ZIP files, and this cause some issues to the customers while invoking the lambda functions locally.
How does it address the issue?
The current script is using the standard ZIP library to unzip which has this bug and switch to use our ZIP module.
I had to split our ZIP module into two modules as it contains some functions to read a file from a URL, and require some libraries that does not exist in the standard library, and so it could not be used with the copy terraform built artifacts script.
I tested generating the PyInstaller package, and tested that I can execute the sam commands on a terraform project.
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.