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

fix(aws-dynamodb): not emit empty GSI and LSI properties #909

Merged
merged 1 commit into from
Oct 15, 2018
Merged

fix(aws-dynamodb): not emit empty GSI and LSI properties #909

merged 1 commit into from
Oct 15, 2018

Conversation

jungseoklee
Copy link
Contributor

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

Summary

This patch deletes GSI and LSI properties at validation phase when they are empty because it is pointless and potentially a source of confusion, which addresses a comment from @eladb in #882.

Test

OK: 53 assertions (905ms)

=============================== Coverage summary ===============================
Statements   : 97.93% ( 142/145 )
Branches     : 96.84% ( 92/95 )
Functions    : 100% ( 31/31 )
Lines        : 97.89% ( 139/142 )
================================================================================
Verifying integ.dynamodb.js against integ.dynamodb.expected.json... OK.

Notes

  • I updated integ.dynamodb.expected.json through npm run integ command.

@@ -396,6 +396,14 @@ export class Table extends Construct {
errors.push('a sort key of the table must be specified to add local secondary indexes');
}

// reshape GSI and LSI since they can be added in a lazy manner
if (this.globalSecondaryIndexes.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather you change the location where we pass these to the L1 into something like this:

globalSecondaryIndexes: new cdk.Token(() => this.globalSecondaryIndexes.length > 0 ? this.globalSecondaryIndexes : undefined),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for a valuable comment! Let me update to use cdk.Token.

This patch uses cdk.Token not to emit GSI and LSI properties when they
are empty because it is pointless and potentially a source of confusion.
@rix0rrr rix0rrr merged commit 841975a into aws:master Oct 15, 2018
eladb pushed a commit that referenced this pull request Oct 19, 2018
### Highlights

 - __A new construct library for AWS Step Functions__
   ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-stepfunctions/README.md)).
   The library provides rich APIs for modeling state machines by exposing a
   programmatic interface for [Amazon State
   Language](https://docs.aws.amazon.com/step-functions/latest/dg/concepts-amazon-states-language.html).
 - __A new construct library for Amazon S3 bucket deployments__
   ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3-deployment/README.md)).
   You can use now automatically populate an S3 Bucket from a .zip file or a
   local directory. This is a building block for end-to-end support for static
   websites in the AWS CDK.

### Bug Fixes

* **aws-apigateway:** make LambdaRestApi proxy by default ([#963](#963)) ([a5f5e2c](a5f5e2c)), closes [#959](#959)
* **aws-cdk:** Allow use of assumed roles behind a proxy ([#898](#898)) ([f2b1048](f2b1048))
* **aws-cdk:** Auto-delete stacks that failed creating before new attempt ([#917](#917)) ([2af8309](2af8309))
* **aws-cloudfront:** expose distributionId ([#938](#938)) ([f58d98c](f58d98c))
* **aws-dynamodb:** don't emit empty array properties ([#909](#909)) ([841975a](841975a))
* **docs:** use ..code to display file structure in "writing constructs" ([#935](#935)) ([b743362](b743362))

### Features

* **assets:** isZipArchive indicates if this is a zip asset ([#944](#944)) ([65190f9](65190f9))
* **aws-cdk:** deploy supports CloudFormation Role ([#940](#940)) ([393be6f](393be6f)), closes [#735](#735)
* **aws-cloudformation:** allow specifying custom resource type ([#943](#943)) ([9de3a84](9de3a84))
* **aws-cloudformation:** correctly handle the templateConfiguration property in the CreateUpdateStack Pipeline Action. ([#923](#923)) ([d251a46](d251a46))
* **aws-cloudfront:** add support for "webAclId" ([#969](#969)) ([3ec9d76](3ec9d76))
* **aws-codedeploy:** add auto rollback configuration to server Deployment Group. ([#925](#925)) ([7ee91cf](7ee91cf))
* **aws-codedeploy:** add instance tag filter support for server Deployment Groups. ([#824](#824)) ([e6e8c51](e6e8c51))
* **aws-codedeploy:** add support for setting CloudWatch alarms on a server Deployment Group. ([#926](#926)) ([27b26b1](27b26b1))
* add support for Step Functions ([#827](#827)) ([81b533c](81b533c))
* **aws-lambda:** add grantInvoke() method ([#962](#962)) ([1ee8135](1ee8135)), closes [#961](#961)
* **aws-lambda:** improvements to the code and runtime APIs ([#945](#945)) ([36f29b6](36f29b6)), closes [#902](#902) [#188](#188) [#947](#947) [#947](#947) [#664](#664)
* **aws-logs:** extractMetric() returns Metric object ([#939](#939)) ([5558fff](5558fff)), closes [#850](#850)
* **aws-s3:** initial support for website hosting ([#946](#946)) ([2d3661c](2d3661c))
* **aws-s3-deployment:** bucket deployments ([#971](#971)) ([84d6876](84d6876)), closes [#952](#952) [#953](#953) [#954](#954)
* **docs:** added link to CloudFormation concepts ([#934](#934)) ([666bbba](666bbba))

### BREAKING CHANGES

* **aws-apigateway:** specifying a path no longer works. If you used to
provide a '/', remove it. Otherwise, you will have to supply `proxy: false`
and construct more complex resource paths yourself.
* **aws-lambda:** The construct `lambda.InlineJavaScriptLambda` is no longer supported. Use `lambda.Code.inline` instead; `lambda.Runtime.NodeJS43Edge` runtime is removed. CloudFront docs [stipulate](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/lambda-requirements-limits.html#lambda-requirements-lambda-function-configuration) that you should use node6.10 or node8.10. It is always possible to use any value by instantiating a `lambda.Runtime` object.
@eladb eladb mentioned this pull request Oct 19, 2018
eladb pushed a commit that referenced this pull request Oct 19, 2018
### Highlights

 - __A new construct library for AWS Step Functions__
   ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-stepfunctions/README.md)).
   The library provides rich APIs for modeling state machines by exposing a
   programmatic interface for [Amazon State
   Language](https://docs.aws.amazon.com/step-functions/latest/dg/concepts-amazon-states-language.html).
 - __A new construct library for Amazon S3 bucket deployments__
   ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3-deployment/README.md)).
   You can use now automatically populate an S3 Bucket from a .zip file or a
   local directory. This is a building block for end-to-end support for static
   websites in the AWS CDK.

### Bug Fixes

* **aws-apigateway:** make LambdaRestApi proxy by default ([#963](#963)) ([a5f5e2c](a5f5e2c)), closes [#959](#959)
* **aws-cdk:** Allow use of assumed roles behind a proxy ([#898](#898)) ([f2b1048](f2b1048))
* **aws-cdk:** Auto-delete stacks that failed creating before new attempt ([#917](#917)) ([2af8309](2af8309))
* **aws-cloudfront:** expose distributionId ([#938](#938)) ([f58d98c](f58d98c))
* **aws-dynamodb:** don't emit empty array properties ([#909](#909)) ([841975a](841975a))
* **docs:** use ..code to display file structure in "writing constructs" ([#935](#935)) ([b743362](b743362))

### Features

* **assets:** isZipArchive indicates if this is a zip asset ([#944](#944)) ([65190f9](65190f9))
* **aws-cdk:** deploy supports CloudFormation Role ([#940](#940)) ([393be6f](393be6f)), closes [#735](#735)
* **aws-cloudformation:** allow specifying custom resource type ([#943](#943)) ([9de3a84](9de3a84))
* **aws-cloudformation:** correctly handle the templateConfiguration property in the CreateUpdateStack Pipeline Action. ([#923](#923)) ([d251a46](d251a46))
* **aws-cloudfront:** add support for "webAclId" ([#969](#969)) ([3ec9d76](3ec9d76))
* **aws-codedeploy:** add auto rollback configuration to server Deployment Group. ([#925](#925)) ([7ee91cf](7ee91cf))
* **aws-codedeploy:** add instance tag filter support for server Deployment Groups. ([#824](#824)) ([e6e8c51](e6e8c51))
* **aws-codedeploy:** add support for setting CloudWatch alarms on a server Deployment Group. ([#926](#926)) ([27b26b1](27b26b1))
* add support for Step Functions ([#827](#827)) ([81b533c](81b533c))
* **aws-lambda:** add grantInvoke() method ([#962](#962)) ([1ee8135](1ee8135)), closes [#961](#961)
* **aws-lambda:** improvements to the code and runtime APIs ([#945](#945)) ([36f29b6](36f29b6)), closes [#902](#902) [#188](#188) [#947](#947) [#947](#947) [#664](#664)
* **aws-logs:** extractMetric() returns Metric object ([#939](#939)) ([5558fff](5558fff)), closes [#850](#850)
* **aws-s3:** initial support for website hosting ([#946](#946)) ([2d3661c](2d3661c))
* **aws-s3-deployment:** bucket deployments ([#971](#971)) ([84d6876](84d6876)), closes [#952](#952) [#953](#953) [#954](#954)
* **docs:** added link to CloudFormation concepts ([#934](#934)) ([666bbba](666bbba))

### BREAKING CHANGES

* **aws-apigateway:** specifying a path no longer works. If you used to
provide a '/', remove it. Otherwise, you will have to supply `proxy: false`
and construct more complex resource paths yourself.
* **aws-lambda:** The construct `lambda.InlineJavaScriptLambda` is no longer supported. Use `lambda.Code.inline` instead; `lambda.Runtime.NodeJS43Edge` runtime is removed. CloudFront docs [stipulate](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/lambda-requirements-limits.html#lambda-requirements-lambda-function-configuration) that you should use node6.10 or node8.10. It is always possible to use any value by instantiating a `lambda.Runtime` object.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants