sync foss changes #2359
sync foss changes #2359
Conversation
|
@mingkun2020 builds are failing due to improper formatting. Can you run |
|
@jfuss Just wanna do the reformatting after second reviewer's approvals. If I do reformat now it may hide the actual changes which might confuse the reviewer. |
|
@mingkun2020 Please do the reformatted and I can review. Reformatting through black shouldn't hide changes, or I am miss understanding your concern. I am not comfortable approving or reviewing while the builds do not work. |
| """ | ||
| CloudWatch Client | ||
| """ | ||
| with self._lock: |
There was a problem hiding this comment.
Why do we have a lock? First reaction is this seems odd but don't understand why we would need to do this.
There was a problem hiding this comment.
This client provider may call by multi threads, so we add lock here to prevent concurrency issues.
There was a problem hiding this comment.
I wonder if there is a different way but not critical for this. Thanks for explaining.
| @@ -1,10 +1,10 @@ | |||
| [ | |||
| {"LogicalResourceId": "MyApi", "ResourceType": "AWS::ApiGateway::RestApi"}, | |||
| {"LogicalResourceId": "MyApiDeploymentb30ecf9df1", "ResourceType": "AWS::ApiGateway::Deployment"}, | |||
| {"LogicalResourceId": "MyApiDeployment", "ResourceType": "AWS::ApiGateway::Deployment"}, | |||
There was a problem hiding this comment.
Why are the hashes removed here?
There was a problem hiding this comment.
The LogicalResourceId are generated with random suffix. We only compare the LogicalResourceId without the suffix to see if they match. So we don't need to include the random suffix here.
There was a problem hiding this comment.
So the hashes are their to help guard against changes that accidentally change how SAM hashes those values. When the hashes change, this causes new resources in the stacks and can cause impact to. We had an event where we changed how Layers hashing was done and this caused throttling for customers.
Can we add these back?
|
Approving with the condition we add back the hashing, otherwise our tests are not validating we are not spontaneously changing how the logical id's are generated. This causes unknowing deployments on customers, so the logical ids should always be deterministic. |
Description of changes:
Sync foss public repo with recent changes.
Description of how you validated changes:
Tested in local and success
make prpassesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam initthrough https://github.com/awslabs/aws-sam-cli-app-templates/By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.