Skip to content

fix: use resource type instead of resource id for deciding zip method #4485

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
qingchm merged 1 commit into from
Dec 15, 2022
Merged

fix: use resource type instead of resource id for deciding zip method #4485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion samcli/lib/package/packageable_resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ def do_export(self, resource_id, resource_dict, parent_dir):
should_sign_package = self.code_signer.should_sign_package(resource_id)
artifact_extension = "zip" if should_sign_package else None
uploaded_url = upload_local_artifacts(
self.RESOURCE_TYPE,
resource_id,
resource_dict,
self.PROPERTY_NAME,
Expand Down Expand Up @@ -325,7 +326,7 @@ def do_export(self, resource_id, resource_dict, parent_dir):
"""

artifact_s3_url = upload_local_artifacts(
resource_id, resource_dict, self.PROPERTY_NAME, parent_dir, self.uploader
self.RESOURCE_TYPE, resource_id, resource_dict, self.PROPERTY_NAME, parent_dir, self.uploader
)

parsed_url = S3Uploader.parse_s3_url(
Expand Down
4 changes: 3 additions & 1 deletion samcli/lib/package/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ def upload_local_image_artifacts(resource_id, resource_dict, property_name, pare


def upload_local_artifacts(
resource_type: str,
resource_id: str,
resource_dict: Dict,
property_name: str,
Expand All @@ -140,6 +141,7 @@ def upload_local_artifacts(

If path is already a path to S3 object, this method does nothing.

:param resource_type: Type of the CloudFormation resource
:param resource_id: Id of the CloudFormation resource
:param resource_dict: Dictionary containing resource definition
:param property_name: Property name of CloudFormation resource where this
Expand Down Expand Up @@ -174,7 +176,7 @@ def upload_local_artifacts(
local_path,
uploader,
extension,
zip_method=make_zip_with_lambda_permissions if resource_id in LAMBDA_LOCAL_RESOURCES else make_zip,
zip_method=make_zip_with_lambda_permissions if resource_type in LAMBDA_LOCAL_RESOURCES else make_zip,
)

# Path could be pointing to a file. Upload the file
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/sync/test_sync_adl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ def test_sync_watch_code(self):
)
read_until_string(
self.watch_process,
"\x1b[32mFinished syncing Function Layer Reference Sync HelloWorldFunction.\x1b[0m\n",
"\x1b[32mFinished syncing Layer HelloWorldFunction",
Copy link
Contributor

@qingchm qingchm Dec 15, 2022

Choose a reason for hiding this comment

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

Let's change this to function instead of layer, and update L136 to be the same

Copy link
Contributor

@qingchm qingchm Dec 15, 2022

Choose a reason for hiding this comment

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

Sorry nevermind this is actually supposed to be layer, but I'm raising a CR to update the validation on L136 to be the same as this one

timeout=60,
)
self._confirm_lambda_error(lambda_functions[0])
Expand Down
1 change: 1 addition & 0 deletions tests/integration/sync/test_sync_watch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ def _setup_verify_infra(self):
s3_prefix=self.s3_prefix,
kms_key_id=self.kms_key,
tags="integ=true clarity=yes foo_bar=baz",
debug=True,
)
self.watch_process = start_persistent_process(sync_command_list, cwd=self.test_dir)
read_until_string(self.watch_process, "Enter Y to proceed with the command, or enter N to cancel:\n")
Expand Down
2 changes: 1 addition & 1 deletion tests/testing_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ def read_until_string(process: Popen, expected_output: str, timeout: int = 5) ->
"""

def _compare_output(output, _: List[str]) -> bool:
return bool(output == expected_output)
return bool(expected_output in output)

try:
read_until(process, _compare_output, timeout)
Expand Down
82 changes: 66 additions & 16 deletions tests/unit/lib/package/test_artifact_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,22 @@ def _helper_verify_export_resources(
LambdaLayerVersionResource,
):
upload_local_artifacts_mock.assert_called_once_with(
resource_id, resource_dict, test_class.PROPERTY_NAME, parent_dir, s3_uploader_mock
test_class.RESOURCE_TYPE,
resource_id,
resource_dict,
test_class.PROPERTY_NAME,
parent_dir,
s3_uploader_mock,
)
else:
upload_local_artifacts_mock.assert_called_once_with(
resource_id, resource_dict, test_class.PROPERTY_NAME, parent_dir, s3_uploader_mock, None
test_class.RESOURCE_TYPE,
resource_id,
resource_dict,
test_class.PROPERTY_NAME,
parent_dir,
s3_uploader_mock,
None,
)
code_signer_mock.sign_package.assert_not_called()
if "." in test_class.PROPERTY_NAME:
Expand Down Expand Up @@ -272,6 +283,7 @@ def test_upload_local_artifacts_local_file(self, zip_and_upload_mock):
# Verifies that we package local artifacts appropriately
property_name = "property"
resource_id = "resource_id"
resource_type = "resource_type"
expected_s3_url = "s3://foo/bar?versionId=baz"

self.s3_uploader_mock.upload_with_dedup.return_value = expected_s3_url
Expand All @@ -283,7 +295,7 @@ def test_upload_local_artifacts_local_file(self, zip_and_upload_mock):

resource_dict = {property_name: artifact_path}
result = upload_local_artifacts(
resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock
resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock
)
self.assertEqual(result, expected_s3_url)

Expand All @@ -300,6 +312,7 @@ def test_upload_local_artifacts_local_file_abs_path(self, zip_and_upload_mock):
# Verifies that we package local artifacts appropriately
property_name = "property"
resource_id = "resource_id"
resource_type = "resource_type"
expected_s3_url = "s3://foo/bar?versionId=baz"

self.s3_uploader_mock.upload_with_dedup.return_value = expected_s3_url
Expand All @@ -310,7 +323,7 @@ def test_upload_local_artifacts_local_file_abs_path(self, zip_and_upload_mock):

resource_dict = {property_name: artifact_path}
result = upload_local_artifacts(
resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock
resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock
)
self.assertEqual(result, expected_s3_url)

Expand All @@ -321,6 +334,7 @@ def test_upload_local_artifacts_local_file_abs_path(self, zip_and_upload_mock):
def test_upload_local_artifacts_local_folder(self, zip_and_upload_mock):
property_name = "property"
resource_id = "resource_id"
resource_type = "resource_type"
expected_s3_url = "s3://foo/bar?versionId=baz"

zip_and_upload_mock.return_value = expected_s3_url
Expand All @@ -331,7 +345,9 @@ def test_upload_local_artifacts_local_folder(self, zip_and_upload_mock):
parent_dir = tempfile.gettempdir()
resource_dict = {property_name: artifact_path}

result = upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, Mock())
result = upload_local_artifacts(
resource_type, resource_id, resource_dict, property_name, parent_dir, Mock()
)
self.assertEqual(result, expected_s3_url)

absolute_artifact_path = make_abs_path(parent_dir, artifact_path)
Expand All @@ -340,8 +356,9 @@ def test_upload_local_artifacts_local_folder(self, zip_and_upload_mock):

@patch("samcli.lib.package.utils.zip_and_upload")
def test_upload_local_artifacts_local_folder_lambda_resources(self, zip_and_upload_mock):
for resource_id in LAMBDA_LOCAL_RESOURCES:
for resource_type in LAMBDA_LOCAL_RESOURCES:
Copy link
Contributor

@sriram-mv sriram-mv Dec 15, 2022

Choose a reason for hiding this comment

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

Looks like this was full-filling test before then.. Thanks for the fix!

property_name = "property"
resource_id = "resource_id"
expected_s3_url = "s3://foo/bar?versionId=baz"

zip_and_upload_mock.return_value = expected_s3_url
Expand All @@ -351,7 +368,9 @@ def test_upload_local_artifacts_local_folder_lambda_resources(self, zip_and_uplo
parent_dir = tempfile.gettempdir()
resource_dict = {property_name: artifact_path}

result = upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, Mock())
result = upload_local_artifacts(
resource_type, resource_id, resource_dict, property_name, parent_dir, Mock()
)
self.assertEqual(result, expected_s3_url)

absolute_artifact_path = make_abs_path(parent_dir, artifact_path)
Expand All @@ -371,8 +390,9 @@ def test_upload_local_artifacts_local_folder_lambda_resources(self, zip_and_uplo
@patch("samcli.lib.package.utils.zip_and_upload")
def test_upload_local_artifacts_local_folder_non_lambda_resources(self, zip_and_upload_mock):
non_lambda_resources = RESOURCES_WITH_LOCAL_PATHS.keys() - LAMBDA_LOCAL_RESOURCES
for resource_id in non_lambda_resources:
for resource_type in non_lambda_resources:
property_name = "property"
resource_id = "resource_id"
expected_s3_url = "s3://foo/bar?versionId=baz"

zip_and_upload_mock.return_value = expected_s3_url
Expand All @@ -382,7 +402,9 @@ def test_upload_local_artifacts_local_folder_non_lambda_resources(self, zip_and_
parent_dir = tempfile.gettempdir()
resource_dict = {property_name: artifact_path}

result = upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, Mock())
result = upload_local_artifacts(
resource_type, resource_id, resource_dict, property_name, parent_dir, Mock()
)
self.assertEqual(result, expected_s3_url)

absolute_artifact_path = make_abs_path(parent_dir, artifact_path)
Expand All @@ -401,6 +423,7 @@ def test_upload_local_artifacts_local_folder_non_lambda_resources(self, zip_and_
def test_upload_local_artifacts_no_path(self, zip_and_upload_mock):
property_name = "property"
resource_id = "resource_id"
resource_type = "resource_type"
expected_s3_url = "s3://foo/bar?versionId=baz"

zip_and_upload_mock.return_value = expected_s3_url
Expand All @@ -409,7 +432,9 @@ def test_upload_local_artifacts_no_path(self, zip_and_upload_mock):
resource_dict = {}
parent_dir = tempfile.gettempdir()

result = upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock)
result = upload_local_artifacts(
resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock
)
self.assertEqual(result, expected_s3_url)

zip_and_upload_mock.assert_called_once_with(parent_dir, mock.ANY, None, zip_method=make_zip)
Expand All @@ -419,13 +444,16 @@ def test_upload_local_artifacts_no_path(self, zip_and_upload_mock):
def test_upload_local_artifacts_s3_url(self, zip_and_upload_mock):
property_name = "property"
resource_id = "resource_id"
resource_type = "resource_type"
object_s3_url = "s3://foo/bar?versionId=baz"

# If URL is already S3 URL, this will be returned without zip/upload
resource_dict = {property_name: object_s3_url}
parent_dir = tempfile.gettempdir()

result = upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock)
result = upload_local_artifacts(
resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock
)
self.assertEqual(result, object_s3_url)

zip_and_upload_mock.assert_not_called()
Expand All @@ -435,17 +463,22 @@ def test_upload_local_artifacts_s3_url(self, zip_and_upload_mock):
def test_upload_local_artifacts_invalid_value(self, zip_and_upload_mock):
property_name = "property"
resource_id = "resource_id"
resource_type = "resource_type"
parent_dir = tempfile.gettempdir()

with self.assertRaises(exceptions.InvalidLocalPathError):
non_existent_file = "some_random_filename"
resource_dict = {property_name: non_existent_file}
upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock)
upload_local_artifacts(
resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock
)

with self.assertRaises(exceptions.InvalidLocalPathError):
non_existent_file = ["invalid datatype"]
resource_dict = {property_name: non_existent_file}
upload_local_artifacts(resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock)
upload_local_artifacts(
resource_type, resource_id, resource_dict, property_name, parent_dir, self.s3_uploader_mock
)

zip_and_upload_mock.assert_not_called()
self.s3_uploader_mock.upload_with_dedup.assert_not_called()
Expand Down Expand Up @@ -481,7 +514,13 @@ class MockResource(ResourceZip):
resource.export(resource_id, resource_dict, parent_dir)

upload_local_artifacts_mock.assert_called_once_with(
resource_id, resource_dict, resource.PROPERTY_NAME, parent_dir, self.s3_uploader_mock, None
resource.RESOURCE_TYPE,
resource_id,
resource_dict,
resource.PROPERTY_NAME,
parent_dir,
self.s3_uploader_mock,
None,
)

self.assertEqual(resource_dict[resource.PROPERTY_NAME], s3_url)
Expand Down Expand Up @@ -756,7 +795,13 @@ class MockResource(ResourceZip):
resource_dict = {}
resource.export(resource_id, resource_dict, parent_dir)
upload_local_artifacts_mock.assert_called_once_with(
resource_id, resource_dict, resource.PROPERTY_NAME, parent_dir, self.s3_uploader_mock, None
resource.RESOURCE_TYPE,
resource_id,
resource_dict,
resource.PROPERTY_NAME,
parent_dir,
self.s3_uploader_mock,
None,
)
self.code_signer_mock.should_sign_package.assert_called_once_with(resource_id)
self.code_signer_mock.sign_package.assert_not_called()
Expand Down Expand Up @@ -851,7 +896,12 @@ class MockResource(ResourceWithS3UrlDict):
resource.export(resource_id, resource_dict, parent_dir)

upload_local_artifacts_mock.assert_called_once_with(
resource_id, resource_dict, resource.PROPERTY_NAME, parent_dir, self.s3_uploader_mock
resource.RESOURCE_TYPE,
resource_id,
resource_dict,
resource.PROPERTY_NAME,
parent_dir,
self.s3_uploader_mock,
)

self.assertEqual(
Expand Down