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.

Sign up for GitHub

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

Jump to bottom

fix(rds): instance identifiers and endpoints of a Cluster are blank #14394

Merged
merged 12 commits into from Apr 29, 2021
Merged

fix(rds): instance identifiers and endpoints of a Cluster are blank #14394

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7ba3f6d
fix(aws-rds): allow accessing instance identifiers and endpoints of a…
Apr 27, 2021
956b771
fix(aws-rds): remove default values from instance identifiers and end…
Apr 28, 2021
bae2b64
fix(aws-rds): refactor destructuring out
Apr 28, 2021
d9d014f
fix(aws-rds): refactor tests to be included into an existing unit test
Apr 28, 2021
50735c3
fix(aws-rds): remove extra empty line
Apr 28, 2021
9a83757
fix(aws-rds): reorder class fields of DatabaseCluster
Apr 29, 2021
5d88e31
fix(aws-rds): make assertion more compact
Apr 29, 2021
b22d926
fix(aws-rds): remove unnecessary empty line
Apr 29, 2021
de8419f
fix(aws-rds): include testing for DatabaseClusterFromSnapshot as well
Apr 29, 2021
2e4c564
fix(aws-rds): fix test
Apr 29, 2021
d8b37f3
Merge branch 'master' into master
mergify[bot] Apr 29, 2021
d46c481
Merge branch 'master' into vilikin_master
skinny85 Apr 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
View file
Open in desktop
Expand Up @@ -290,8 +290,6 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
* Never undefined.
*/
public readonly engine?: IClusterEngine;
public readonly instanceIdentifiers: string[] = [];
public readonly instanceEndpoints: Endpoint[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move these? We keep members in a specific order (public, then protected, then private). Let's keep it this way here.

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 actually moved to the implementing classes, so they no longer exist on the DatabaseClusterNew class. (diff looks almost as if they were just moved a bit down inside same class, but there's actually 200 rows in between)

The reason for moving them was that I could not figure out a way to keep them readonly on the base class if we needed to set them from the implementing classes.


protected readonly newCfnProps: CfnDBClusterProps;
protected readonly securityGroups: ec2.ISecurityGroup[];
Expand Down Expand Up @@ -484,6 +482,9 @@ export class DatabaseCluster extends DatabaseClusterNew {
private readonly singleUserRotationApplication: secretsmanager.SecretRotationApplication;
private readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication;

public readonly instanceIdentifiers: string[] = [];
public readonly instanceEndpoints: Endpoint[] = [];
vilikin marked this conversation as resolved.
Show resolved Hide resolved

vilikin marked this conversation as resolved.
Show resolved Hide resolved
constructor(scope: Construct, id: string, props: DatabaseClusterProps) {
super(scope, id, props);

Expand Down Expand Up @@ -524,7 +525,9 @@ export class DatabaseCluster extends DatabaseClusterNew {
}

setLogRetention(this, props);
createInstances(this, props, this.subnetGroup);
const { instanceIdentifiers, instanceEndpoints } = createInstances(this, props, this.subnetGroup);
this.instanceIdentifiers = instanceIdentifiers;
this.instanceEndpoints = instanceEndpoints;
vilikin marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -594,6 +597,9 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew {
public readonly clusterReadEndpoint: Endpoint;
public readonly connections: ec2.Connections;

vilikin marked this conversation as resolved.
Show resolved Hide resolved
public readonly instanceIdentifiers: string[] = [];
public readonly instanceEndpoints: Endpoint[] = [];
vilikin marked this conversation as resolved.
Show resolved Hide resolved

constructor(scope: Construct, id: string, props: DatabaseClusterFromSnapshotProps) {
super(scope, id, props);

Expand All @@ -616,7 +622,9 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew {
cluster.applyRemovalPolicy(props.removalPolicy ?? RemovalPolicy.SNAPSHOT);

setLogRetention(this, props);
createInstances(this, props, this.subnetGroup);
const { instanceIdentifiers, instanceEndpoints } = createInstances(this, props, this.subnetGroup);
this.instanceIdentifiers = instanceIdentifiers;
this.instanceEndpoints = instanceEndpoints;
vilikin marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
71 changes: 71 additions & 0 deletions packages/@aws-cdk/aws-rds/test/cluster.test.ts
View file
Open in desktop
Expand Up @@ -1902,6 +1902,77 @@ describe('cluster', () => {
DBClusterIdentifier: clusterIdentifier,
});
});

test('allows accessing instanceIdentifiers', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
const cluster = new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.aurora({
version: AuroraEngineVersion.VER_1_22_2,
}),
credentials: {
username: 'admin',
},
instanceProps: {
vpc,
},
instances: 1,
});

// THEN
expect(cluster.instanceIdentifiers).toHaveLength(1);
expect(stack.resolve(cluster.instanceIdentifiers[0])).toEqual({
Ref: 'DatabaseInstance1844F58FD',
});
});

test('allows accessing instanceEndpoints', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
const cluster = new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.aurora({
version: AuroraEngineVersion.VER_1_22_2,
}),
credentials: {
username: 'admin',
},
instanceProps: {
vpc,
},
instances: 1,
});

// THEN
expect(cluster.instanceEndpoints).toHaveLength(1);
expect(stack.resolve(cluster.instanceEndpoints[0])).toEqual({
hostname: {
'Fn::GetAtt': ['DatabaseInstance1844F58FD', 'Endpoint.Address'],
},
port: {
'Fn::GetAtt': ['DatabaseB269D8BB', 'Endpoint.Port'],
},
socketAddress: {
'Fn::Join': [
'',
[
{
'Fn::GetAtt': ['DatabaseInstance1844F58FD', 'Endpoint.Address'],
},
':',
{
'Fn::GetAtt': ['DatabaseB269D8BB', 'Endpoint.Port'],
},
],
],
},
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This is such a small change, I'm not sure it warrants an entirely separate unit test.

How about adding a new assertion to existing tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I now included the expectations to an existing unit test that was creating a cluster with 1 db instance.

But I do feel like the existing unit tests have been pretty specific on testing just a certain functionality, so it didn't fit too naturally into any existing test in my opinion.

});

test.each([
Expand Down