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

vilikin · 2021-04-27T10:22:52Z

Previously instanceIdentifiers and instanceEndpoints were readonly properties of DatabaseClusterNew, defaulting to empty arrays. However, they they were never set to the correct values after instances were added to the cluster. Those properties are now moved on to the DatabaseCluster and DatabaseClusterFromSnapshot classes where they can now actually be set to correct values.

fixes #14377

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

… cluster Previously instanceIdentifiers and instanceEndpoints were readonly properties of DatabaseClusterNew, defaulting to empty arrays. However, they they were never set to the correct values after instances were added to the cluster. Those properties are now moved on to the DatabaseCluster and DatabaseClusterFromSnapshot classes where they can now actually be set to correct values. fixes aws#14377

gitpod-io · 2021-04-27T10:22:54Z

skinny85

Thanks for the contribution @vilikin! It looks great. However, since I'm a picky asshole, I've managed to find a few places I would do things differently. I hope you agree with my suggestions!

skinny85 · 2021-04-27T19:13:59Z

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

@@ -290,8 +290,6 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
   * Never undefined.
   */
  public readonly engine?: IClusterEngine;
-  public readonly instanceIdentifiers: string[] = [];
-  public readonly instanceEndpoints: Endpoint[] = [];


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

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.

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

skinny85 · 2021-04-27T19:23:26Z

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

+        ],
+      },
+    });
+  });


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?

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.

…points

Pull request has been modified.

vilikin · 2021-04-28T08:19:12Z

Thanks for the contribution @vilikin! It looks great. However, since I'm a picky asshole, I've managed to find a few places I would do things differently. I hope you agree with my suggestions!

Thanks for the thorough review, I do appreciate even the nitpicks :)

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

skinny85 · 2021-04-28T17:15:45Z

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

@@ -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[];


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.

Ah, right. I missed that. Sounds good!

Can you please move these to the block of the public fields above? (The one that ends with public readonly connections: ec2.Connections;).

Oops, forgot to address that part of the comment. Now it should be ok :)

Pull request has been modified.

skinny85

2 small things are still not perfect - any chance of getting these in @vilikin?

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

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

skinny85

Awesome contribution. Thanks for enduring my pickiness @vilikin 🙂.

skinny85 · 2021-04-29T18:35:29Z

The build fails with

@aws-cdk/aws-rds: FAIL test/cluster.test.js (14.981 s)
@aws-cdk/aws-rds:   â—� cluster â€º create a cluster from a snapshot
@aws-cdk/aws-rds:     expect(received).toContain(expected) // indexOf
@aws-cdk/aws-rds:     Expected value:  {"Ref": "DatabaseInstance1844F58FD"}
@aws-cdk/aws-rds:     Received object: {"Ref": "DatabaseInstance1844F58FD"}

I think this

    expect(stack.resolve(cluster.instanceIdentifiers[0])).toContain({
      Ref: 'DatabaseInstance1844F58FD',
    });

should be

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

As in the test for DatabaseCluster.

Pull request has been modified.

vilikin · 2021-04-29T18:38:01Z

Awesome contribution. Thanks for enduring my pickiness @vilikin 🙂.

Thanks for all the comments, they were all very much valid. I was just a bit careless due to lack of time. (which I obviously then spent more in the end... :D)

Is this now just going to be auto-merged at some point or is there still something to do?

vilikin · 2021-04-29T18:39:22Z

The build fails with

@aws-cdk/aws-rds: FAIL test/cluster.test.js (14.981 s)
@aws-cdk/aws-rds:   â—� cluster â€º create a cluster from a snapshot
@aws-cdk/aws-rds:     expect(received).toContain(expected) // indexOf
@aws-cdk/aws-rds:     Expected value:  {"Ref": "DatabaseInstance1844F58FD"}
@aws-cdk/aws-rds:     Received object: {"Ref": "DatabaseInstance1844F58FD"}

I think this

    expect(stack.resolve(cluster.instanceIdentifiers[0])).toContain({
      Ref: 'DatabaseInstance1844F58FD',
    });

should be

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

As in the test for DatabaseCluster.

Ooops, yep. That's now fixed.

skinny85

The build fails with

@aws-cdk/aws-rds: FAIL test/cluster.test.js (14.981 s)
@aws-cdk/aws-rds:   â—� cluster â€º create a cluster from a snapshot
@aws-cdk/aws-rds:     expect(received).toContain(expected) // indexOf
@aws-cdk/aws-rds:     Expected value:  {"Ref": "DatabaseInstance1844F58FD"}
@aws-cdk/aws-rds:     Received object: {"Ref": "DatabaseInstance1844F58FD"}

I think this

    expect(stack.resolve(cluster.instanceIdentifiers[0])).toContain({
      Ref: 'DatabaseInstance1844F58FD',
    });

should be

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

As in the test for DatabaseCluster.

Ooops, yep. That's now fixed.

Awesome, thanks so much!

mergify · 2021-04-29T19:03:00Z

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).

skinny85 · 2021-04-29T19:04:28Z

@vilikin looks like there's a merge conflict with the current master. Do you have time to resolve it?

vilikin · 2021-04-29T19:06:14Z

@vilikin looks like there's a merge conflict with the current master. Do you have time to resolve it?

Actually, this time I really don't have time anymore. We are expecting a baby and the water just broke, so we are off to the hospital soon - so if you'd be able to resolve it, I would highly appreciate it. :D

skinny85 · 2021-04-29T19:06:55Z

@vilikin looks like there's a merge conflict with the current master. Do you have time to resolve it?

Actually, this time I really don't have time anymore. We are expecting a baby and the water just broke, so we are off to the hospital soon - so if you'd be able to resolve it, I would highly appreciate it. :D

That is a great excuse for not resolving merge conflicts. I'll handle this one. Best of luck with the baby!

# Conflicts: # packages/@aws-cdk/aws-rds/test/cluster.test.ts

Pull request has been modified.

aws-cdk-automation · 2021-04-29T19:39:06Z

AWS CodeBuild CI Report

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

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

mergify · 2021-04-29T19:39:29Z

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).

…ws#14394) Previously instanceIdentifiers and instanceEndpoints were readonly properties of DatabaseClusterNew, defaulting to empty arrays. However, they they were never set to the correct values after instances were added to the cluster. Those properties are now moved on to the DatabaseCluster and DatabaseClusterFromSnapshot classes where they can now actually be set to correct values. fixes aws#14377 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot assigned skinny85 Apr 27, 2021

github-actions bot added the @aws-cdk/aws-rds Related to Amazon Relational Database label Apr 27, 2021

skinny85 previously requested changes Apr 27, 2021

View reviewed changes

Vili Kinnunen added 3 commits April 28, 2021 10:57

fix(aws-rds): remove default values from instance identifiers and end…

956b771

…points

fix(aws-rds): refactor destructuring out

bae2b64

fix(aws-rds): refactor tests to be included into an existing unit test

d9d014f

fix(aws-rds): remove extra empty line

50735c3

skinny85 previously requested changes Apr 28, 2021

View reviewed changes

Vili Kinnunen added 2 commits April 29, 2021 07:54

fix(aws-rds): reorder class fields of DatabaseCluster

9a83757

fix(aws-rds): make assertion more compact

5d88e31

vilikin requested a review from skinny85 April 29, 2021 04:58

skinny85 reviewed Apr 29, 2021

View reviewed changes

packages/@aws-cdk/aws-rds/lib/cluster.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-rds/test/cluster.test.ts Show resolved Hide resolved

Vili Kinnunen added 2 commits April 29, 2021 21:00

fix(aws-rds): remove unnecessary empty line

b22d926

fix(aws-rds): include testing for DatabaseClusterFromSnapshot as well

de8419f

vilikin requested a review from skinny85 April 29, 2021 18:11

skinny85 previously approved these changes Apr 29, 2021

View reviewed changes

fix(aws-rds): fix test

2e4c564

skinny85 changed the title ~~fix(aws-rds): allow accessing instance identifiers and endpoints of a cluster~~ fix(rds): instance identifiers and endpoints of a Cluster are unset Apr 29, 2021

skinny85 previously approved these changes Apr 29, 2021

View reviewed changes

Merge branch 'master' into master

d8b37f3

Merge branch 'master' into vilikin_master

d46c481

# Conflicts: # packages/@aws-cdk/aws-rds/test/cluster.test.ts

skinny85 approved these changes Apr 29, 2021

View reviewed changes

skinny85 changed the title ~~fix(rds): instance identifiers and endpoints of a Cluster are unset~~ fix(rds): instance identifiers and endpoints of a Cluster are blank Apr 29, 2021

mergify bot merged commit 9597d97 into aws:master Apr 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vilikin commented Apr 27, 2021

gitpod-io bot commented Apr 27, 2021 •

edited

skinny85 left a comment

skinny85 Apr 27, 2021

vilikin Apr 28, 2021

skinny85 Apr 27, 2021

vilikin Apr 28, 2021

vilikin commented Apr 28, 2021

skinny85 Apr 28, 2021

vilikin Apr 29, 2021

skinny85 left a comment

skinny85 left a comment

skinny85 commented Apr 29, 2021

vilikin commented Apr 29, 2021

vilikin commented Apr 29, 2021

skinny85 left a comment

mergify bot commented Apr 29, 2021

skinny85 commented Apr 29, 2021

vilikin commented Apr 29, 2021

skinny85 commented Apr 29, 2021

aws-cdk-automation commented Apr 29, 2021

mergify bot commented Apr 29, 2021

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

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

Conversation

vilikin commented Apr 27, 2021

gitpod-io bot commented Apr 27, 2021 • edited

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Apr 27, 2021

Choose a reason for hiding this comment

vilikin Apr 28, 2021

Choose a reason for hiding this comment

skinny85 Apr 27, 2021

Choose a reason for hiding this comment

vilikin Apr 28, 2021

Choose a reason for hiding this comment

vilikin commented Apr 28, 2021

skinny85 Apr 28, 2021

Choose a reason for hiding this comment

vilikin Apr 29, 2021

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 commented Apr 29, 2021

vilikin commented Apr 29, 2021

vilikin commented Apr 29, 2021

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 29, 2021

skinny85 commented Apr 29, 2021

vilikin commented Apr 29, 2021

skinny85 commented Apr 29, 2021

aws-cdk-automation commented Apr 29, 2021

AWS CodeBuild CI Report

mergify bot commented Apr 29, 2021

gitpod-io bot commented Apr 27, 2021 •

edited