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

custom_resources: onEvent response disagreement on if PhysicalResourceId is required #29304

Open
mtfurlan opened this issue Feb 28, 2024 · 2 comments
Labels
@aws-cdk/custom-resources Related to AWS CDK Custom Resources bug This issue is a bug. documentation This is a problem with documentation. effort/medium Medium work item – several days of effort p3

Comments

@mtfurlan
Copy link
Contributor

Describe the issue

When implementing a CustomResource, the onEvent handler must return some json.
There is disagreement about if PhysicalResourceId is required.

This repo says it's not required.

The CloudFormation guide says that it is required.

The typescript definitions say it is required, but looks like it was trying to make it only required if it's Update or Delete and then they did all event types.

I can make PRs once I know what it should be.
Though I don't know where the CloudFormation guide source is.

Links

@mtfurlan mtfurlan added documentation This is a problem with documentation. needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@github-actions github-actions bot added the @aws-cdk/custom-resources Related to AWS CDK Custom Resources label Feb 28, 2024
@pahud
Copy link
Contributor

pahud commented Feb 28, 2024

The allocated/assigned physical ID of the resource. If omitted for Create events, the event's RequestId will be used. For Update, the current physical ID will be used. If a different value is returned, CloudFormation will follow with a subsequent Delete for the previous ID (resource replacement). For Delete, it will always return the current physical resource ID, and if the user returns a different one, an error will occur.

I believe it could be omitted in the custom resource provider framework because the framework would generate one for you in some cases.

const physicalResourceId = onEventResult.PhysicalResourceId || defaultPhysicalResourceId(cfnRequest);

@pahud pahud added p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 28, 2024
@mtfurlan
Copy link
Contributor Author

Cool, so it's optional in all cases according to the actual code.

How do you feel about the wording of

The allocated/assigned physical ID of the resource. Default value for Create events is the event's RequestId. Default value for Update and Delete events is the current physical ID. If a different value is returned for an Update, CloudFormation will follow with a subsequent Delete for the previous ID (resource replacement). If a different value for Delete is returned, an error will occur.

Where the source of CloudFormation guide?

Actually, any idea if any of the other fields are required?
The tests seem to just return PhysicalResourceId, Data, and ArbitraryField, and an aws example provider lambda only returns PhysicalResourceId for Update/Delete.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Feb 29, 2024
@ashishdhingra ashishdhingra added the bug This issue is a bug. label Mar 14, 2024
@pahud pahud added p3 and removed p2 labels Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/custom-resources Related to AWS CDK Custom Resources bug This issue is a bug. documentation This is a problem with documentation. effort/medium Medium work item – several days of effort p3
Projects
None yet
Development

No branches or pull requests

3 participants