fix(docdb): make most attributes of DatabaseClusterAttributes optional by Synohara · Pull Request #19625 · aws/aws-cdk

Synohara · 2022-03-30T10:58:18Z

Test Result

❯ yarn test
yarn run v1.22.17
$ cdk-test
PASS test/endpoint.test.js (17.557 s)
PASS test/parameter-group.test.js (17.601 s)
PASS test/instance.test.js (18.16 s)
PASS test/cluster.test.js (19.233 s)

 

=============================== Coverage summary ===============================
Statements   : 98.76% ( 160/162 )
Branches     : 99.02% ( 102/103 )
Functions    : 100% ( 27/27 )
Lines        : 98.75% ( 158/160 )
================================================================================

 

Test Suites: 4 passed, 4 total
Tests:       51 passed, 51 total
Snapshots:   0 total
Time:        19.946 s
Ran all test suites.
Verifying integ.cluster-rotation.lit.js against integ.cluster-rotation.lit.expected.json ... OK.
Verifying integ.cluster.js against integ.cluster.expected.json ... OK.
Tests successful. Total time (26.2s) | /Users/yoshitaka.koitabashi/Desktop/OSS/tmp/aws-cdk/node_modules/jest/bin/jest.js (23.8s) | cdk-integ-assert (2.4s)
✨  Done in 26.72s.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use cdk-integ to deploy the infrastructure and generate the snapshot (i.e. cdk-integ without --dry-run)?

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

gitpod-io · 2022-03-30T10:58:25Z

mergify · 2022-03-30T10:58:56Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

skinny85

Looks great @Synohara! 2 minor comments.

skinny85 · 2022-03-30T22:16:07Z

packages/@aws-cdk/aws-docdb/lib/cluster.ts

+/**
+ * Represents an imported database cluster.
+ */
+class ImportedDatabaseCluster extends DatabaseClusterBase implements IDatabaseCluster {


Out of curiosity - why did you move this class from where it was defined?

Let's maybe leave it where it is - this way, the diff will be significantly smaller?

Thanks all for the lighting fast response!
I have made changes based on the following code.

aws-cdk/packages/@aws-cdk/aws-rds/lib/cluster.ts

Lines 427 to 487 in 4071d9b

/**

* Represents an imported database cluster.

*/

class ImportedDatabaseCluster extends DatabaseClusterBase implements IDatabaseCluster {

public readonly clusterIdentifier: string;

public readonly connections: ec2.Connections;

public readonly engine?: IClusterEngine;

private readonly _clusterEndpoint?: Endpoint;

private readonly _clusterReadEndpoint?: Endpoint;

private readonly _instanceIdentifiers?: string[];

private readonly _instanceEndpoints?: Endpoint[];

constructor(scope: Construct, id: string, attrs: DatabaseClusterAttributes) {

super(scope, id);

this.clusterIdentifier = attrs.clusterIdentifier;

const defaultPort = attrs.port ? ec2.Port.tcp(attrs.port) : undefined;

this.connections = new ec2.Connections({

securityGroups: attrs.securityGroups,

defaultPort,

});

this.engine = attrs.engine;

this._clusterEndpoint = (attrs.clusterEndpointAddress && attrs.port) ? new Endpoint(attrs.clusterEndpointAddress, attrs.port) : undefined;

this._clusterReadEndpoint = (attrs.readerEndpointAddress && attrs.port) ? new Endpoint(attrs.readerEndpointAddress, attrs.port) : undefined;

this._instanceIdentifiers = attrs.instanceIdentifiers;

this._instanceEndpoints = (attrs.instanceEndpointAddresses && attrs.port)

? attrs.instanceEndpointAddresses.map(addr => new Endpoint(addr, attrs.port!))

: undefined;

}

public get clusterEndpoint() {

if (!this._clusterEndpoint) {

throw new Error('Cannot access `clusterEndpoint` of an imported cluster without an endpoint address and port');

}

return this._clusterEndpoint;

}

public get clusterReadEndpoint() {

if (!this._clusterReadEndpoint) {

throw new Error('Cannot access `clusterReadEndpoint` of an imported cluster without a readerEndpointAddress and port');

}

return this._clusterReadEndpoint;

}

public get instanceIdentifiers() {

if (!this._instanceIdentifiers) {

throw new Error('Cannot access `instanceIdentifiers` of an imported cluster without provided instanceIdentifiers');

}

return this._instanceIdentifiers;

}

public get instanceEndpoints() {

if (!this._instanceEndpoints) {

throw new Error('Cannot access `instanceEndpoints` of an imported cluster without instanceEndpointAddresses and port');

}

return this._instanceEndpoints;

}

}

but I agree with your suggestion. So I left it as is and made fix.

skinny85 · 2022-03-30T22:18:14Z

packages/@aws-cdk/aws-docdb/test/cluster.test.ts

+  test('minimal imported cluster throws on accessing attributes for unprovided parameters', () => {
+    const stack = testStack();
+
+    const cluster = DatabaseCluster.fromDatabaseClusterAttributes(stack, 'Database', {
+      clusterIdentifier: 'identifier',
+    });
+
+    expect(() => cluster.clusterEndpoint).toThrow(/Cannot access `clusterEndpoint` of an imported cluster/);
+    expect(() => cluster.clusterReadEndpoint).toThrow(/Cannot access `clusterReadEndpoint` of an imported cluster/);
+    expect(() => cluster.instanceIdentifiers).toThrow(/Cannot access `instanceIdentifiers` of an imported cluster/);
+    expect(() => cluster.instanceEndpoints).toThrow(/Cannot access `instanceEndpoints` of an imported cluster/);
+    expect(() => cluster.securityGroupId).toThrow(/Cannot access `securityGroupId` of an imported cluster/);
+  });


Let's maybe merge these 2 into one test?

Pull request has been modified.

skinny85

Looks great @Synohara! A few last stylistic changes before we merge this in.

skinny85 · 2022-04-01T18:49:01Z

packages/@aws-cdk/aws-docdb/lib/cluster.ts

+      private readonly _instanceEndpoints?: Endpoint[];
+      private readonly _securityGroupId?: string;
+
+      constructor(_scope: Construct, _id: string, _attrs: DatabaseClusterAttributes) {


Do we need this constructor? Can't we just assign the fields like _clusterEndpoint and _clusterReadEndpoint directly, like we do for clusterIdentifier and defaultPort?

skinny85 · 2022-04-01T18:51:45Z

packages/@aws-cdk/aws-docdb/lib/cluster.ts

+        this._clusterEndpoint = (_attrs.clusterEndpointAddress && _attrs.port) ? new Endpoint(_attrs.clusterEndpointAddress, _attrs.port) : undefined;
+        this._clusterReadEndpoint = (_attrs.readerEndpointAddress && _attrs.port)
+          ? new Endpoint(_attrs.readerEndpointAddress, _attrs.port)
+          : undefined;
+        this._instanceIdentifiers = _attrs.instanceIdentifiers;
+        this._instanceEndpoints = (_attrs.instanceEndpointAddresses && _attrs.port)


We don't need the parenthesis in the ternary operator, let's get rid of them.

skinny85 · 2022-04-01T18:59:44Z

packages/@aws-cdk/aws-docdb/lib/cluster.ts

  public static fromDatabaseClusterAttributes(scope: Construct, id: string, attrs: DatabaseClusterAttributes): IDatabaseCluster {
    class Import extends DatabaseClusterBase implements IDatabaseCluster {
-      public readonly defaultPort = ec2.Port.tcp(attrs.port);
+      public readonly clusterIdentifier = attrs.clusterIdentifier;


Can we keep the fields in the same order they are now? This way, the diff will be easier to to read.

skinny85 · 2022-04-01T19:03:20Z

packages/@aws-cdk/aws-docdb/lib/cluster.ts

    class Import extends DatabaseClusterBase implements IDatabaseCluster {
-      public readonly defaultPort = ec2.Port.tcp(attrs.port);
+      public readonly clusterIdentifier = attrs.clusterIdentifier;
+      public readonly defaultPort = attrs.port ? ec2.Port.tcp(attrs.port) : undefined;


Remember that 0 is falsy in JavaScript/TypeScript. You probably want to compare against undefined here.

skinny85 · 2022-04-01T19:03:46Z

packages/@aws-cdk/aws-docdb/lib/cluster.ts

+      public readonly defaultPort = attrs.port ? ec2.Port.tcp(attrs.port) : undefined;
      public readonly connections = new ec2.Connections({
-        securityGroups: [attrs.securityGroup],
+        securityGroups: !!attrs.securityGroup ? [attrs.securityGroup] : undefined,


Why the double !! here, all of a sudden? Removing it will not change this code.

skinny85 · 2022-04-01T19:04:28Z

packages/@aws-cdk/aws-docdb/lib/cluster.ts

+        return this._instanceEndpoints;
+      }
+
+      public get securityGroupId() {


Can you specify the return types for all of these getters?

Pull request has been modified.

skinny85

Looks almost perfect @Synohara!

skinny85 · 2022-04-04T18:52:42Z

packages/@aws-cdk/aws-docdb/lib/cluster.ts

-      public readonly clusterReadEndpoint = new Endpoint(attrs.readerEndpointAddress, attrs.port);
-      public readonly instanceEndpoints = attrs.instanceEndpointAddresses.map(a => new Endpoint(a, attrs.port));
-      public readonly securityGroupId = attrs.securityGroup.securityGroupId;
+      private readonly _instanceIdentifiers?= attrs.instanceIdentifiers;


Why the ?=? I think this should just be a =. Also, missing space:

Suggested change

private readonly _instanceIdentifiers?= attrs.instanceIdentifiers;

private readonly _instanceIdentifiers = attrs.instanceIdentifiers;

Same comment in the other fields added in the PR.

Thank you for suggestion! I fixed the use of optional properties.

Pull request has been modified.

Co-authored-by: sandship <sandship.blancard@gmail.com> Co-authored-by: yoshitaka KOITABASHI <Yoshiitaka@users.noreply.github.com>

directory assign fields add return types in getter

…ined

skinny85

Thanks for the contribution @Synohara!

Pull request has been modified.

mergify · 2022-04-07T01:29:38Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-04-07T02:12:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 989dbcb
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-04-07T02:12:46Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws#19625) Fixes aws#14492 - Test Result ``` ❯ yarn test yarn run v1.22.17 $ cdk-test PASS test/endpoint.test.js (17.557 s) PASS test/parameter-group.test.js (17.601 s) PASS test/instance.test.js (18.16 s) PASS test/cluster.test.js (19.233 s) =============================== Coverage summary =============================== Statements : 98.76% ( 160/162 ) Branches : 99.02% ( 102/103 ) Functions : 100% ( 27/27 ) Lines : 98.75% ( 158/160 ) ================================================================================ Test Suites: 4 passed, 4 total Tests: 51 passed, 51 total Snapshots: 0 total Time: 19.946 s Ran all test suites. Verifying integ.cluster-rotation.lit.js against integ.cluster-rotation.lit.expected.json ... OK. Verifying integ.cluster.js against integ.cluster.expected.json ... OK. Tests successful. Total time (26.2s) | /Users/yoshitaka.koitabashi/Desktop/OSS/tmp/aws-cdk/node_modules/jest/bin/jest.js (23.8s) | cdk-integ-assert (2.4s) ✨ Done in 26.72s. ``` ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Mar 30, 2022

Synohara force-pushed the fix-documentdb-database-cluster-attributes branch from 0f0fc84 to 1b9ee78 Compare March 30, 2022 11:00

Synohara marked this pull request as ready for review March 30, 2022 11:01

Synohara changed the title ~~fix documentdb database cluster attributes~~ fix(aws-docdb): documentdb database cluster attributes Mar 30, 2022

github-actions bot added the @aws-cdk/aws-docdb Related to Amazon DocumentDB label Mar 30, 2022

github-actions bot assigned skinny85 Mar 30, 2022

skinny85 previously requested changes Mar 30, 2022

View reviewed changes

skinny85 changed the title ~~fix(aws-docdb): documentdb database cluster attributes~~ fix(docdb): make most attributes of DatabaseClusterAttributes optional Mar 30, 2022

Synohara force-pushed the fix-documentdb-database-cluster-attributes branch from 1b9ee78 to 981e56b Compare April 1, 2022 09:51

skinny85 previously requested changes Apr 1, 2022

View reviewed changes

Synohara force-pushed the fix-documentdb-database-cluster-attributes branch from 8157fe3 to 1ace74d Compare April 4, 2022 07:09

skinny85 previously requested changes Apr 4, 2022

View reviewed changes

mk-shinohara and others added 7 commits April 5, 2022 09:41

fix documentdb database cluster attributes

80002e3

Co-authored-by: sandship <sandship.blancard@gmail.com> Co-authored-by: yoshitaka KOITABASHI <Yoshiitaka@users.noreply.github.com>

combine two tests into one test

da0f35f

reverted position of the class definition.

49ce65f

make a few stylistic changes

0bead41

directory assign fields add return types in getter

remove optional property

25fa595

fix(docdb): use typeof and exact equal operator to compare with undef…

d133ee9

…ined

use typeof and exact equal operator

dcccdc2

Synohara force-pushed the fix-documentdb-database-cluster-attributes branch from 99f66fc to dcccdc2 Compare April 5, 2022 00:41

skinny85 previously approved these changes Apr 5, 2022

View reviewed changes

Merge branch 'master' into fix-documentdb-database-cluster-attributes

a2a8189

Synohara requested a review from skinny85 April 7, 2022 00:31

skinny85 approved these changes Apr 7, 2022

View reviewed changes

Merge branch 'master' into fix-documentdb-database-cluster-attributes

989dbcb

mergify bot merged commit 5f6d20c into aws:master Apr 7, 2022

	/**
	* Represents an imported database cluster.
	*/
	class ImportedDatabaseCluster extends DatabaseClusterBase implements IDatabaseCluster {
	public readonly clusterIdentifier: string;
	public readonly connections: ec2.Connections;
	public readonly engine?: IClusterEngine;

	private readonly _clusterEndpoint?: Endpoint;
	private readonly _clusterReadEndpoint?: Endpoint;
	private readonly _instanceIdentifiers?: string[];
	private readonly _instanceEndpoints?: Endpoint[];

	constructor(scope: Construct, id: string, attrs: DatabaseClusterAttributes) {
	super(scope, id);

	this.clusterIdentifier = attrs.clusterIdentifier;

	const defaultPort = attrs.port ? ec2.Port.tcp(attrs.port) : undefined;
	this.connections = new ec2.Connections({
	securityGroups: attrs.securityGroups,
	defaultPort,
	});
	this.engine = attrs.engine;

	this._clusterEndpoint = (attrs.clusterEndpointAddress && attrs.port) ? new Endpoint(attrs.clusterEndpointAddress, attrs.port) : undefined;
	this._clusterReadEndpoint = (attrs.readerEndpointAddress && attrs.port) ? new Endpoint(attrs.readerEndpointAddress, attrs.port) : undefined;
	this._instanceIdentifiers = attrs.instanceIdentifiers;
	this._instanceEndpoints = (attrs.instanceEndpointAddresses && attrs.port)
	? attrs.instanceEndpointAddresses.map(addr => new Endpoint(addr, attrs.port!))
	: undefined;
	}

	public get clusterEndpoint() {
	if (!this._clusterEndpoint) {
	throw new Error('Cannot access `clusterEndpoint` of an imported cluster without an endpoint address and port');
	}
	return this._clusterEndpoint;
	}

	public get clusterReadEndpoint() {
	if (!this._clusterReadEndpoint) {
	throw new Error('Cannot access `clusterReadEndpoint` of an imported cluster without a readerEndpointAddress and port');
	}
	return this._clusterReadEndpoint;
	}

	public get instanceIdentifiers() {
	if (!this._instanceIdentifiers) {
	throw new Error('Cannot access `instanceIdentifiers` of an imported cluster without provided instanceIdentifiers');
	}
	return this._instanceIdentifiers;
	}

	public get instanceEndpoints() {
	if (!this._instanceEndpoints) {
	throw new Error('Cannot access `instanceEndpoints` of an imported cluster without instanceEndpointAddresses and port');
	}
	return this._instanceEndpoints;
	}
	}

	private readonly _instanceIdentifiers?= attrs.instanceIdentifiers;
	private readonly _instanceIdentifiers = attrs.instanceIdentifiers;

Conversation

Synohara commented Mar 30, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Mar 30, 2022

Uh oh!

mergify bot commented Mar 30, 2022

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Apr 7, 2022

Uh oh!

aws-cdk-automation commented Apr 7, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Apr 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants