feat(custom-resources): AwsResource - support custom resource type #5248

netroy · 2019-11-28T19:21:53Z

Allow specifying a custom resource type for AwsResources.

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

mergify · 2019-11-28T19:22:31Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-28T19:51:14Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

jogold · 2019-11-28T20:32:36Z

packages/@aws-cdk/custom-resources/lib/aws-custom-resource/runtime/index.ts

+      if (event.RequestType === 'Delete' && call.physicalResourceIdPath) {
+        call.parameters[call.physicalResourceIdPath] = physicalResourceId;
+      }


If I understand it correctly, you want to be able to use the "current" physical id somewhere in you API call for the delete event?

The physicalResourceIdPath works the other way around: extracts the physical id from the response to return it to CloudFormation. You didn't update the documentation for this prop, so only you would know about this behavior here.

How about having some sort of tag ({{PHYSICAL_ID}}?) that could be used in the parameters prop and that would get replaced in the Lambda before the API call?

Can you maybe detail your use case?

@eladb opinion on this?

I'll add an example inline here, and will update the PR tomorrow morning.

When I create a DatasetGroup resource using AwsCustomResource like this:

new AwsCustomResource(this, 'DatasetGroup', { onCreate: { service: 'Personalize', action: 'createDatasetGroup', parameters: { name: 'TestDatasetGroup' }, physicalResourceIdPath: 'datasetGroupArn' }, onDelete: { service: 'Personalize', action: 'deleteDatasetGroup' } })

The resource is created as expected.
But, when I tear down the stack, the call parameters for the deleteDatasetGroup API call don't contain the field datasetGroupArn, the only parameter that's needed for the call.

Same issues exists for other resources on Personalize & Forecast.

Wouldn't the following work here?

const groupName = 'TestDatasetGroup'; new AwsCustomResource(this, 'CreateDatasetGroup', { onCreate: { service: 'Personalize', action: 'createDatasetGroup', parameters: { name: groupName }, physicalResourceId: groupName }, onDelete: { service: 'Personalize', action: 'deleteDatasetGroup', parameters: { // arn:aws:personalize:<region>:<account>:dataset-group/TestDatasetGroup datasetGroupArn: cdk.Stack.of(this).formatArn({ service: 'personalize', resource: 'dataset-group', resourceName: groupName }) } } });

Thanks @jogold, I'll try this out today.
If that works as expected, I can use that for now.

It'd be good to know what @eladb prefers here, before I send out an update on the PR (or close it).

Cheers

I am with @jogold

packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

aws-cdk-automation · 2019-11-29T13:56:40Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

Please revert the change related to physical resource ID in DELETE

mergify · 2019-12-16T12:33:27Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:00:52Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:12Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:38:50Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:24Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:17:31Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-12-17T12:38:40Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-12-18T12:42:05Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-12-18T13:32:54Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-12-18T13:33:27Z

Thank you for contributing! Your pull request is now being automatically merged.

netroy requested a review from eladb as a code owner November 28, 2019 19:21

jogold reviewed Nov 28, 2019

View reviewed changes

netroy force-pushed the custom-resources-patches branch from 8da5a11 to 9ec5d60 Compare November 29, 2019 13:26

SomayaB assigned eladb Nov 29, 2019

eladb suggested changes Dec 12, 2019

View reviewed changes

feat(custom-resource): Allow custom ResourceTypes on CustomResources

f761a78

netroy force-pushed the custom-resources-patches branch from 9ec5d60 to f761a78 Compare December 17, 2019 12:08

netroy requested a review from eladb December 17, 2019 13:12

eladb changed the title ~~feat(custom-resources): two minor changes~~ feat(custom-resources): allow specifying resource type for AwsResource Dec 18, 2019

eladb changed the title ~~feat(custom-resources): allow specifying resource type for AwsResource~~ feat(custom-resources): support custom resource type for AwsResource Dec 18, 2019

eladb changed the title ~~feat(custom-resources): support custom resource type for AwsResource~~ feat(custom-resources): AwsResource - support custom resource type Dec 18, 2019

eladb approved these changes Dec 18, 2019

View reviewed changes

Merge branch 'master' into custom-resources-patches

7503e63

mergify bot merged commit c605ceb into aws:master Dec 18, 2019

netroy deleted the custom-resources-patches branch December 18, 2019 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(custom-resources): AwsResource - support custom resource type #5248

feat(custom-resources): AwsResource - support custom resource type #5248

netroy commented Nov 28, 2019 •

edited by eladb

Loading

mergify bot commented Nov 28, 2019

aws-cdk-automation commented Nov 28, 2019

jogold Nov 28, 2019

netroy Nov 28, 2019 •

edited

Loading

jogold Nov 29, 2019 •

edited

Loading

netroy Nov 29, 2019

eladb Dec 11, 2019

aws-cdk-automation commented Nov 29, 2019

eladb left a comment

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 17, 2019

mergify bot commented Dec 18, 2019

aws-cdk-automation commented Dec 18, 2019

mergify bot commented Dec 18, 2019

feat(custom-resources): AwsResource - support custom resource type #5248

feat(custom-resources): AwsResource - support custom resource type #5248

Conversation

netroy commented Nov 28, 2019 • edited by eladb Loading

mergify bot commented Nov 28, 2019

aws-cdk-automation commented Nov 28, 2019

AWS CodeBuild CI Report

jogold Nov 28, 2019

Choose a reason for hiding this comment

netroy Nov 28, 2019 • edited Loading

Choose a reason for hiding this comment

jogold Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

netroy Nov 29, 2019

Choose a reason for hiding this comment

eladb Dec 11, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 29, 2019

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 17, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 18, 2019

aws-cdk-automation commented Dec 18, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 18, 2019

netroy commented Nov 28, 2019 •

edited by eladb

Loading

netroy Nov 28, 2019 •

edited

Loading

jogold Nov 29, 2019 •

edited

Loading