feat(s3-deployment): added property outputObjectKeys for BucketDeployment #31452
feat(s3-deployment): added property outputObjectKeys for BucketDeployment #31452mergify[bot] merged 26 commits intomainfrom
Conversation
Leo10Gama
left a comment
There was a problem hiding this comment.
LGTM aside from a few minor comments. Just want to verify the default behaviour and whether or not we should test when objectOutputKeys: true is set as well.
| Template.fromStack(stack).hasResourceProperties('Custom::CDKBucketDeployment', { | ||
| OutputObjectKeys: false, | ||
| }); |
There was a problem hiding this comment.
Should we also verify that the previous behaviour still exists as well? That is, have a unit test to check that when OutputObjectKeys is true, the object keys are in the output?
There was a problem hiding this comment.
May also be worth having a test to verify the default value of the attribute
There was a problem hiding this comment.
added, check integration test cases: packages/@aws-cdk-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment-big-response.ts && packages/@aws-cdk-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment.ts
Leo10Gama
left a comment
There was a problem hiding this comment.
Changes look good! Just one check regarding one of the integ tests.
...ages/@aws-cdk-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment.ts
Outdated
Show resolved
Hide resolved
Leo10Gama
left a comment
There was a problem hiding this comment.
LGTM. Awesome work, thanks!!
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #28579
Reason for this change
The CR lambda is essentially sending back the same data in the response which is hitting the limit for close to 50 object uploads.
Particularly this is being a limitation when using servicecatalog.ProductStack, if there are local assets beyond a particular number, the Custom::CDKBucketDeployment would fail with the error Response object is too long which is a hard limit of 4096 bytes.
Description of changes
outputObjectKeyshas been set to false by default for the service catalog product so that the error does not occur.Description of how you validated changes
Validated using a sample stack with the property set and confirmed the behavior. Also, the existing deployments would be unaffected.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license