Skip to content
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

Merged

Conversation

konokenj
Copy link
Contributor

@konokenj konokenj commented Feb 16, 2023

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:

Closes #23843.


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

@gitpod-io
Copy link

gitpod-io bot commented Feb 16, 2023

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Feb 16, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team February 16, 2023 02:43
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Feb 16, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@aws-cdk-automation aws-cdk-automation dismissed their stale review February 16, 2023 02:58

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@konokenj konokenj force-pushed the konokenj/passthrough-physical-resource-id branch from f248f90 to f14dfd1 Compare February 16, 2023 02:59
@comcalvi
Copy link
Contributor

AwsCustomResource is now able to omit physicalResourceId in onUpdate with passthrough it from request. This is useful when users want to call an update API without resoure replacing.

I'm not sure what "passthrough it from request" means, could you please elaborate?

call an update API without resoure replacing.

I don't understand this either. Does it mean that today, it is impossible to call onUpdate without a replacement of the custom resource?

@konokenj konokenj changed the title feat(custom-resources): AwsCustomResource passthrough physicalResourceId when omit it in onUpdate feat(custom-resources): AwsCustomResource copy physicalResourceId from request when omit it in onUpdate Feb 17, 2023
@konokenj
Copy link
Contributor Author

Hi @comcalvi, thank you for response.

I'm not sure what "passthrough it from request" means, could you please elaborate?

Lambda backed custom resources recieves physicalResourceId in request event on update. The handler function has responsibility to return same physicalResourceId when the resource is not replaced.

The value returned for a PhysicalResourceId can change custom resource update operations. If the value returned is the same, it is considered a normal update. If the value returned is different, AWS CloudFormation recognizes the update as a replacement and sends a delete request to the old resource.

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/crpg-ref-requesttypes-update.html

But currently, users of AwsCustomResource construct must specify physicalResourceId by hand (fromResponse() or .of()). There is no way to copy from request, although there are AWS APIs that response with empty bodies. This PR adds ability to copy that.

call an update API without resoure replacing.

I don't understand this either. Does it mean that today, it is impossible to call onUpdate without a replacement of the custom resource?

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.)

Copy link
Contributor

@comcalvi comcalvi left a 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

@mergify mergify bot dismissed comcalvi’s stale review March 2, 2023 05:39

Pull request has been modified.

@konokenj
Copy link
Contributor Author

konokenj commented Mar 2, 2023

I added unit tests.

To confirm, leaving the PhysicalResourceId undefined will tell CFN to use the value that was passed to onCreate?

PhysicalResourceId is not rendered in CFn template when omit in onUpdate. When handler function doesn't recieve PhysicalResourceId from request, it uses previous value.

physicalResourceId = event.ResourceProperties[event.RequestType]?.physicalResourceId?.id ?? event.PhysicalResourceId;

@mergify mergify bot dismissed comcalvi’s stale review March 6, 2023 09:08

Pull request has been modified.

@konokenj
Copy link
Contributor Author

konokenj commented Mar 6, 2023

I fixed code style and error messages. Could you review again?

@konokenj konokenj force-pushed the konokenj/passthrough-physical-resource-id branch from cf5dd09 to 1880369 Compare March 6, 2023 13:15
@mergify
Copy link
Contributor

mergify bot commented Mar 8, 2023

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 7627dad
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 21ad7a7 into aws:main Mar 8, 2023
5 checks passed
@mergify
Copy link
Contributor

mergify bot commented Mar 8, 2023

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).

homakk pushed a commit to homakk/aws-cdk that referenced this pull request Mar 28, 2023
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom-resource: Passthrough physicalResourceId when its not specified in onUpdate at AwsCustomResource
3 participants