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

BREAKING CHANGE: remove resource attribute types for string attributes #712

Merged
merged 16 commits into from
Sep 19, 2018

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Sep 13, 2018

Implementation of #695 for resource attributes that return string.

Removed the code generation of attribute types for all resource attributes which return a string primitive and use stringified tokens to represent these attribute as strings.

Instead of .ref, we now generate a property based on the resource's RefKind which returns a string with a token that resolves to Ref (so instead of ref: QueueArn it is now queueArn: string). This is a much better experience because this way people don't need to guess what "ref" means (or use an artificial type).

This change also removes the cdk.Arn type (the utility functions are now available under cdk.ArnUtils.parse(s) and cdk.ArnUtils.format(components): string. The parse utility will discover if the ARN contains tokens and will not attempt to actually parse the string, but will rather use CFN intrinsics to split the ARN to it's components.

Remaining work:

  • Fix all build breaks (effectively removing all tokens from the world).
  • Verify all tests pass

Further ideas:

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

Elad Ben-Israel added 2 commits September 17, 2018 13:26
* Converted all attribute types to strings
* Remove a stale example from aws-cloudformation
* Add `--no-bail` to `build.sh` so it won't stop on the first error
* Reduce some verbosity from cdk-build and cfn2ts
@eladb eladb changed the title [WIP] BREAKING CHANGE: remove resource attribute types for string attributes BREAKING CHANGE: remove resource attribute types for string attributes Sep 17, 2018
@@ -1,104 +0,0 @@
import lambda = require('@aws-cdk/aws-lambda');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't want these files here, please move them instead of deleting them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are not compiling and became stale and unusable. What do you recommend we do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move them to examples/typescript-examples ?

packages/@aws-cdk/aws-iam/lib/role.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-s3/test/notification-dests.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/cdk/lib/cloudformation/arn.ts Outdated Show resolved Hide resolved
@@ -102,11 +102,16 @@ export class Token {
}

/**
* Returns true if obj is a token (i.e. has the resolve() method)
* Returns true if obj is a token (i.e. has the resolve() method or is a string
* that includes token markers).
* @param obj The object to test.
*/
export function isToken(obj: any): obj is Token {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't hasToken() or containsToken() be more appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to unresolved, what do you think? (should it be isUnresolved)?


const refType = new ClassDeclaration(refClass, baseClass);
return new Attribute('ref', refType, constructorArguments);
const refType = new ClassDeclaration(CodeName.forPrimitive('string'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the name ClassDeclaration is now wrong again, because we're never going to declare that class anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still have non-primitive attributes that we are emitting as strong types. Renamed to AttributeTypeDeclaration for now.

tools/cfn2ts/lib/index.ts Outdated Show resolved Hide resolved
Elad Ben-Israel added 5 commits September 19, 2018 14:42
* Fix typo "managedPolicieArns" => "managedPolicyArns"
* Remove unneeded `.toString`
* Rename `isToken(x)` to `unresolved(x)`
* Rename cfn2ts `ClassDeclaration` to `AttributeTypeDeclaration`
* Revert log "generated code up-to-date" from cfn2ts
* Restore custom resource example
* Change `resource.getAtt` to return a token
@eladb eladb merged commit 6508f78 into master Sep 19, 2018
eladb pushed a commit that referenced this pull request Sep 20, 2018
__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
eladb pushed a commit that referenced this pull request Sep 20, 2018
* v0.9.2

__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
@eladb eladb deleted the benisrae/remove-attribute-types branch November 19, 2018 12:31
eladb pushed a commit that referenced this pull request Jan 8, 2019
Now that we can represent attributes as native strings and string lists,
this covers the majority of resource attributes in CloudFormation.

This change:

* String attributes are represented as `string` (like before).
* String list attribute are now represented as `string[]`.
* Any other attribute types are represented as `cdk.Token`.

Attributes that are represented as Tokens as of this change:

* amazonmq has a bunch of `Integer` attributes (will be solved by #1455)
* iot1click has a bunch of `Boolean` attributes
* cloudformation has a JSON attribute
* That's all.

A few improvements to cfn2ts:

* Auto-detect cfn2ts scope from package.json so it is more self-contained
  and doesn't rely on cdk-build-tools to run.
* Added a "cfn2ts" npm script to all modules so it is now possible
  to regenerate all L1 via "lerna run cfn2ts".
* Removed the premature optimization for avoiding code regeneration
  (it saved about 0.5ms).

Fixes #1406

BREAKING CHANGE: any `CfnXxx` resource attributes that represented a list of strings are now typed as `string[]`s (via #1144). Attributes that represent strings, are still typed as `string` (#712) and all other attribute types are represented as `cdk.Token`.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants