Skip to content

Conversation

@mndeveci
Copy link
Contributor

@mndeveci mndeveci commented Dec 14, 2022

Which issue(s) does this change fix?

N/A

Why is this change necessary?

With this PR #4462, we've changed some permissions of the files in ZIP package if the file is used for serverless function resources. But that is checking if logical id of the resource is type.

How does it address the issue?

With this change, we are checking the resource type instead of resource logical id.

What side effects does this change have?

N/A

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@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

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!

@mndeveci mndeveci marked this pull request as ready for review December 15, 2022 00:43
@mndeveci mndeveci requested a review from a team as a code owner December 15, 2022 00:43
@qingchm qingchm merged commit 3adc94f into aws:develop Dec 15, 2022
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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants