-
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(custom-resources): AwsCustomResource copy physicalResourceId from request when omit it in onUpdate #24194
feat(custom-resources): AwsCustomResource copy physicalResourceId from request when omit it in onUpdate #24194
Conversation
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
f248f90
to
f14dfd1
Compare
I'm not sure what "passthrough it from request" means, could you please elaborate?
I don't understand this either. Does it mean that today, it is impossible to call |
Hi @comcalvi, thank you for response.
Lambda backed custom resources recieves
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/crpg-ref-requesttypes-update.html But currently, users of AwsCustomResource construct must specify
No, I intended to allow users to indicate their intention to copy PhysicalResourceId. (I'm sorry if this is a problem caused by my poor English skill.) |
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.
To confirm, leaving the PhysicalResourceId
undefined will tell CFN to use the value that was passed toonCreate
? If so, this makes perfect sense. Please create a new unit test in test/aws-custom-resource
that verifies the updated error checking logic you've added
I added unit tests.
aws-cdk/packages/@aws-cdk/custom-resources/lib/aws-custom-resource/runtime/index.ts Line 153 in 0ebbf58
|
packages/@aws-cdk/custom-resources/test/aws-custom-resource/aws-custom-resource.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/custom-resources/test/aws-custom-resource/aws-custom-resource.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/custom-resources/test/aws-custom-resource/aws-custom-resource.test.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/custom-resources/test/aws-custom-resource/aws-custom-resource.test.ts
Outdated
Show resolved
Hide resolved
I fixed code style and error messages. Could you review again? |
when omit it in onUpdate closes aws#23843
Co-authored-by: Calvin Combs <[email protected]>
cf5dd09
to
1880369
Compare
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). |
…m request when omit it in onUpdate (aws#24194) AwsCustomResource is now able to omit `physicalResourceId` in `onUpdate` to copy it from request. Some `UPDATE` AWS APIs responses with an empty body. When users want to call these APIs using AwsCustomResource, users can't specify physicalResourceId by `PhysicalResourceId.fromResponse()`. Furthermore, when the Create API generates an unpredictable ID and this must be passed to the Update API, this Construct could not be used. For example, following APIs match this situation: - https://docs.aws.amazon.com/athena/latest/APIReference/API_UpdateNotebook.html - https://docs.aws.amazon.com/singlesignon/latest/IdentityStoreAPIReference/API_UpdateUser.html Closes aws#23843. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AwsCustomResource is now able to omit
physicalResourceId
inonUpdate
to copy it from request.Some
UPDATE
AWS APIs responses with an empty body. When users want to call these APIs using AwsCustomResource, users can't specify physicalResourceId byPhysicalResourceId.fromResponse()
. Furthermore, when the Create API generates an unpredictable ID and this must be passed to the Update API, this Construct could not be used. For example, following APIs match this situation:Closes #23843.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license