-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
feat(s3-deployment): added property outputObjectKeys for BucketDeployment #31452
base: main
Are you sure you want to change the base?
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* | ||
* See https://github.com/aws/aws-cdk/issues/28579 | ||
* | ||
* @default true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be missing it, but is there somewhere in the code we are defaulting this value to true? The closest thing I can find is ../product-stack-synthesizer.ts
, but that sets it to false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its value is set to true here
Line 67 in e610d05
output_object_keys = props.get('OutputObjectKeys', 'true') == 'true' |
Template.fromStack(stack).hasResourceProperties('Custom::CDKBucketDeployment', { | ||
OutputObjectKeys: false, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May also be worth having a test to verify the default value of the attribute
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
outputObjectKeys
has 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