fix(cloudformation): custom resource changes property casing (behind feature flag) #7034

chriscoombs · 2020-03-27T07:13:06Z

The CustomResource type is currently uppercasing the first letter in each prop.properties key. There is no requirement in CloudFormation for the first letter to be uppercased. The CloudFormation documentation for Custom Resources shows properties with lowercase keys.

Uppercasing the properties prevents the porting of existing custom resources to CDK. Even when code changes to the backing Lambda function are possible, simple object destructuring is not.

This pull requests disables the uppercasing of Custom Resource properties.

Commit Message

BREAKING CHANGE: Disable Custom Resource properties from being uppercased

Fixes #7035

End Commit Message

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

BREAKING CHANGE: Disable Custom Resource properties from being uppercased

aws-cdk-automation · 2020-03-27T07:19:45Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ad09ebc
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-27T07:58:54Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0d3eba4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

Please update the "commit message" section to actually include a description of the commit and also follow the instructions from the contribution guide on how to describe breaking changes under feature flags in the commit message.

eladb · 2020-03-31T08:47:59Z

packages/@aws-cdk/cx-api/lib/features.ts

+/**
+ * If this is set, properties of a custom resource will not be uppercased.
+ */
+export const DISABLE_CUSTOM_RESOURCE_UPPERCASE_PROPERTIES = '@aws-cdk/aws-cloudformation:disableCustomResourceUppercaseProperties';


Add a _CONTEXT suffix please

eladb

On another thought: I don't think this can be introduced behind a feature flag because it will break any "legacy" custom resource that's used in the application...

Instead, let's just introduce this as a prop in CustomResource. In 1.x we will have to keep the current behavior as the default, but we can change this in 2.x

eladb · 2020-03-31T08:52:57Z

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

@@ -160,11 +161,17 @@ export class CustomResource extends Resource {

    const type = renderResourceType(props.resourceType);
    const providerConfig = props.provider.bind(this);
+
+    // change properties to upper case under a feature flag (since this is a breaking change).
+    const properties = this.node.tryGetContext(cxapi.DISABLE_CUSTOM_RESOURCE_UPPERCASE_PROPERTIES)


Can't use a feature flag for this because it can break "legacy" constructs used in "new" applications.

aws-cdk-automation · 2020-03-31T08:59:38Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0b43c75
Result: FAILED
Build Logs (available for 30 days)

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

chriscoombs · 2020-03-31T09:50:27Z

On another thought: I don't think this can be introduced behind a feature flag because it will break any "legacy" custom resource that's used in the application...

Instead, let's just introduce this as a prop in CustomResource. In 1.x we will have to keep the current behavior as the default, but we can change this in 2.x

@eladb thanks for reviewing the pull request. 😄

Please could I ask you to expand on the "legacy" constructs issue? Wouldn't that statement hold true for any type related bugs (e.g. changing a default value)? I'm guessing this pull request isn't the first to find this sort of mistake and to try and revert it. How were those changes handled?

I'm certainly willing to create another property, but I'd like to fully discount the feature flag approach before we do.

Again, thanks for the time reviewing and commenting on the PR. Much appreciated.

eladb · 2020-03-31T10:07:02Z

Please could I ask you to expand on the "legacy" constructs issue?

Let's say this is released. This means that any new app created with cdk init will have this feature enabled. Now say I use a construct with a custom resource which was designed against an older version of the CDK, and depended on this behavior. Now my construct is broken because the CustomResource class behaves differently in this new app.

How were those changes handled?

We try not to use feature flags for this purpose. They are very risky. As you can see we have very few flags in the system, and we try to keep this to a minimum.

eladb · 2020-04-06T12:30:59Z

Any updates?

…source-uppercase-properties

Pull request has been modified.

aws-cdk-automation · 2020-04-08T03:13:24Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8df89bd
Result: SUCCEEDED
Build Logs (available for 30 days)

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

chriscoombs · 2020-04-08T03:40:53Z

Thanks for your response.

Whilst I still think a feature flag is the way to go, I'll create a seperate branch for creating a new property.

eladb · 2020-04-08T04:29:51Z

Thanks for your response.

Whilst I still think a feature flag is the way to go, I'll create a seperate branch for creating a new property.

Thanks. As I said, a feature flag can break new users that use old custom resources, so it’s not a viable approach.

This commit folds the `CustomResource` and `NestedStack` types from `@aws-cdk/aws-cloudformation` into `@aws-cdk/core` in order to allow code in `core` and other lower layers to use capabilities such as nested stacks and custom resources. This comes at a minor sacrifice to API fidelity: the provider's service token is for custom resources is now passed as a simple `string` instead of a strongly typed `ICustomResourceProvider`. But this is negligible for this type of resource given the high involvement users require to use it anyway. Additionally, the `NestedStack` class accepts a `notificationArns` as a `string[]` instead of an `sns.ITopic[]`. In both cases the API in `@aws-cdk/aws-cloudformation` (which is considered a stable module) remains unchanged with a compatibility layer added. We took this opportunity to change the behavior of custom resources so that it won't pascal-case property names by default. This resolves #4896 and resolves #7035 and supersedes #7034. The API in the aws-cloudformation module are still supported for backwards compatibility but marked as deprecated.

eladb · 2020-05-01T13:09:56Z

Addressed in #7736

This commit folds the `CustomResource` and `NestedStack` types from `@aws-cdk/aws-cloudformation` into `@aws-cdk/core` in order to allow code in `core` and other lower layers to use capabilities such as nested stacks and custom resources. This comes at a minor sacrifice to API fidelity: the provider's service token is for custom resources is now passed as a simple `string` instead of a strongly typed `ICustomResourceProvider`. But this is negligible for this type of resource given the high involvement users require to use it anyway. Additionally, the `NestedStack` class accepts a `notificationArns` as a `string[]` instead of an `sns.ITopic[]`. In both cases the API in `@aws-cdk/aws-cloudformation` (which is considered a stable module) remains unchanged with a compatibility layer added. We took this opportunity to change the behavior of custom resources so that it won't pascal-case property names by default. This resolves #4896 and resolves #7035 and supersedes #7034. The API in the aws-cloudformation module are still supported for backwards compatibility but marked as deprecated.

chriscoombs added 2 commits March 27, 2020 17:30

fix(aws-cloudformation): disable custom resource uppercase properties

0d3eba4

BREAKING CHANGE: Disable Custom Resource properties from being uppercased

Merge branch 'master' into disable-custom-resource-uppercase-properties

ad09ebc

chriscoombs mentioned this pull request Mar 27, 2020

Custom Resource Properties Keys Are Being Uppercased #7035

Closed

SomayaB assigned eladb Mar 30, 2020

restructure code for readability

0b43c75

eladb suggested changes Mar 31, 2020

View reviewed changes

eladb changed the title ~~fix(aws-cloudformation): disable custom resource uppercase properties~~ fix(cloudformation): custom resource changes property casing (under feature flag) Mar 31, 2020

eladb changed the title ~~fix(cloudformation): custom resource changes property casing (under feature flag)~~ fix(cloudformation): custom resource changes property casing (behind feature flag) Mar 31, 2020

eladb suggested changes Mar 31, 2020

View reviewed changes

eladb previously requested changes Mar 31, 2020

View reviewed changes

chriscoombs added 3 commits April 7, 2020 14:16

Merge remote-tracking branch 'upstream/master' into disable-custom-re…

700b73b

…source-uppercase-properties

Merge remote-tracking branch 'upstream/master' into disable-custom-re…

6481110

…source-uppercase-properties

Removing bracket and resolving linting errors

8df89bd

eladb closed this Apr 8, 2020

eladb mentioned this pull request May 1, 2020

feat(core): move all types from "aws-cloudformation" to "core" #7736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudformation): custom resource changes property casing (behind feature flag) #7034

fix(cloudformation): custom resource changes property casing (behind feature flag) #7034

chriscoombs commented Mar 27, 2020 •

edited by eladb

Loading

aws-cdk-automation commented Mar 27, 2020

aws-cdk-automation commented Mar 27, 2020

eladb left a comment

eladb Mar 31, 2020

eladb left a comment

eladb Mar 31, 2020

aws-cdk-automation commented Mar 31, 2020

chriscoombs commented Mar 31, 2020

eladb commented Mar 31, 2020

eladb commented Apr 6, 2020

aws-cdk-automation commented Apr 8, 2020

chriscoombs commented Apr 8, 2020

eladb commented Apr 8, 2020

eladb commented May 1, 2020

fix(cloudformation): custom resource changes property casing (behind feature flag) #7034

fix(cloudformation): custom resource changes property casing (behind feature flag) #7034

Conversation

chriscoombs commented Mar 27, 2020 • edited by eladb Loading

Commit Message

End Commit Message

aws-cdk-automation commented Mar 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 27, 2020

AWS CodeBuild CI Report

eladb left a comment

Choose a reason for hiding this comment

eladb Mar 31, 2020

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

eladb Mar 31, 2020

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 31, 2020

AWS CodeBuild CI Report

chriscoombs commented Mar 31, 2020

eladb commented Mar 31, 2020

eladb commented Apr 6, 2020

aws-cdk-automation commented Apr 8, 2020

AWS CodeBuild CI Report

chriscoombs commented Apr 8, 2020

eladb commented Apr 8, 2020

eladb commented May 1, 2020

chriscoombs commented Mar 27, 2020 •

edited by eladb

Loading