feat(rds): Aurora clusters from snapshots by jumic · Pull Request #17759 · aws/aws-cdk

jumic · 2021-11-29T20:13:48Z

Add DatabaseClusterFromSnapshot to support creating Aurora clusters from snapshots.

Closes #10936.

The logic is implemented similar to PR #10130 where the same feature was implemented for database clusters.

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

gitpod-io · 2021-11-29T20:13:50Z

skinny85 · 2021-11-30T04:42:20Z

Thanks for the contribution @jumic!

@jogold do you mind taking a first pass on this one?

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

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

skinny85 · 2021-12-09T16:19:41Z

Thanks @jogold!

jumic

deleted

jumic · 2021-12-09T19:31:11Z

Thanks for your review, @jogold. I implemented the suggested changes.

I also took over the test cases from rds Instance where more test cases were available than in rds cluster. Should I also manually deploy the combinations that used in the unit tests? I deployed a Aurora cluster from a snapshot before submitting this PR. However, I have not yet tested all combinations from the unit tests.

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

jumic · 2021-12-09T20:06:32Z

@skinny85 PR is ready for review again. (I can't request your review in GitHub in the current state.)

skinny85

Thanks for the contribution @jumic! The API looks good, we just need to reduce the duplication a little bit before we merge this in.

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

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

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

skinny85 · 2021-12-15T00:22:21Z

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

+    this.connections = new ec2.Connections({
+      securityGroups: this.securityGroups,
+      defaultPort: ec2.Port.tcp(this.clusterEndpoint.port),
+    });


Can we move this up to ServerlessClusterNew? I believe this is duplicated between ServerlessClusterFromSnapshot and ServerlessCluster.

skinny85 · 2021-12-15T00:22:53Z

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

+    if (secret) {
+      this.secret = secret.attach(this);
+    }
+
+    cluster.applyRemovalPolicy(props.removalPolicy ?? RemovalPolicy.SNAPSHOT);


Same here - can we move this up to ServerlessClusterNew?

I tried to do so.

secret is a readonly property. We can assign it only in the constructer. So I think we can't move it up to ServerlessClusterNew because the source value isn't available yet.

cluster.applyRemovalPolicy could be moved to a new method renderApplyRemovalPolicy(cluster: CfnCluster, removalPolicy?: RemovalPolicy). Then, we could call this method in ServerlessClusterFromSnapshot and ServerlessCluster. Should I implement it this way? I'm not sure if this is really a good optimisation.

In instance.ts those lines are also duplicated. Maybe because there is no better option. Or do you have other ideas? If so, please let me know.

packages/@aws-cdk/aws-rds/README.md

skinny85 · 2021-12-15T00:26:38Z

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

-  public static fromServerlessClusterAttributes(scope: Construct, id: string,
-    attrs: ServerlessClusterAttributes): IServerlessCluster {
+  protected readonly newCfnProps: CfnDBClusterProps;
+  protected readonly subnetGroup: ISubnetGroup;


Why is this a field, and protected? I don't see it used anywhere outside of this class' constructor?

I copied this line from cluster.ts. You're right. I was able to remove field subnetGroup.

Pull request has been modified.

skinny85

Looks great @jumic! One more tiny naming change please before we merge this in.

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

skinny85 · 2021-12-17T22:16:53Z

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

+  public readonly clusterIdentifier: string;
+  public readonly clusterEndpoint: Endpoint;
+  public readonly clusterReadEndpoint: Endpoint;
+  protected enableDataApi?: boolean;


Can we move this up to ServerlessClusterNew?

enableDataApi --> done

If I move the readonly fields to ServerlessClusterNew, it shows message Property 'clusterIdentifier' has no initializer and is not definitely assigned in the constructor.. I could declare them as abstract. However, then I still have to create those fields in the sub classes.

skinny85 · 2021-12-17T22:17:12Z

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

+
+  public readonly secret?: secretsmanager.ISecret;
+
+  protected enableDataApi?: boolean;


Can we move this up to ServerlessClusterNew?

enableDataApi --> done

readonly field: see comment above.

Pull request has been modified.

skinny85

Looks great @jumic, thanks for the contribution!

mergify · 2021-12-17T23:27:23Z

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 · 2021-12-18T00:12:45Z

AWS CodeBuild CI Report

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

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

mergify · 2021-12-18T00:13:10Z

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

Add `DatabaseClusterFromSnapshot` to support creating Aurora clusters from snapshots. Closes aws#10936. The logic is implemented similar to PR aws#10130 where the same feature was implemented for database clusters. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(rds): Aurora clusters from snapshots

8443330

github-actions bot assigned skinny85 Nov 29, 2021

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

fix readme

cbb5487

jogold reviewed Dec 9, 2021

View reviewed changes

jumic added 2 commits December 9, 2021 18:42

Merge branch 'master' into aurora-clusters-from-snapshots

4150278

change secret to credentials

1b2b113

jumic commented Dec 9, 2021

View reviewed changes

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

skinny85 previously requested changes Dec 15, 2021

View reviewed changes

jumic added 3 commits December 17, 2021 21:52

refactoring

2d8b057

refactor Connections

58bde9f

Merge branch 'master' into aurora-clusters-from-snapshots

2a72e3e

update imports

3993294

jumic requested a review from skinny85 December 17, 2021 21:35

skinny85 previously requested changes Dec 17, 2021

View reviewed changes

rename interface, move enableDataApi

2e031ac

jumic requested a review from skinny85 December 17, 2021 22:47

skinny85 approved these changes Dec 17, 2021

View reviewed changes

Merge branch 'master' into aurora-clusters-from-snapshots

f88fff7

mergify bot merged commit e5259ee into aws:master Dec 18, 2021


		public readonly secret?: secretsmanager.ISecret;

		protected enableDataApi?: boolean;

Conversation

jumic commented Nov 29, 2021

Uh oh!

gitpod-io bot commented Nov 29, 2021

Uh oh!

skinny85 commented Nov 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Dec 9, 2021

Uh oh!

jumic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jumic commented Dec 9, 2021

Uh oh!

Uh oh!

jumic commented Dec 9, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

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!

mergify bot commented Dec 17, 2021

Uh oh!

aws-cdk-automation commented Dec 18, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Dec 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jumic left a comment •

edited

Loading