feat(rds): simpler way to configure parameters for instance and cluster by hoegertn · Pull Request #18126 · aws/aws-cdk

hoegertn · 2021-12-21T23:07:41Z

Fixes #18124

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

…#18124

gitpod-io · 2021-12-21T23:07:44Z

skinny85

Looks good @hoegertn! A few questions.

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

skinny85 · 2021-12-22T00:22:35Z

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

 import * as cdk from '@aws-cdk/core';
 import * as cxapi from '@aws-cdk/cx-api';
 import * as rds from '../lib';
+import { SqlServerEngineVersion } from '../lib';


Nope! Use the qualified import (rds) from above please 🙂.

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

hoegertn · 2021-12-22T08:46:04Z

I also added the cluster now. Currently, there are no README changes as the parameter group is also not documented. How should I proceed here?

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

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

skinny85 · 2021-12-22T18:58:44Z

I also added the cluster now. Currently, there are no README changes as the parameter group is also not documented. How should I proceed here?

Let's add a section in the ReadMe, showing off the two ways of setting a ParameterGroup for an Instance/Cluster, and explaining under which circumstances would you use each of them.

hoegertn · 2022-01-05T19:45:57Z

@skinny85 Is there anything missing from my side?

skinny85 · 2022-01-05T22:41:22Z

@skinny85 Is there anything missing from my side?

Probably nothing beyond the fact that you didn't re-request my approval, so I missed this 😃.

skinny85

Looks good @hoegertn! A few minor comments, mainly around docs.

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

skinny85 · 2022-01-05T22:47:56Z

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

+
+new rds.DatabaseInstance(this, 'Database', {
+  engine: rds.DatabaseInstanceEngine.SQL_SERVER_EE,
+  instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),


This is optional, right? I think there's no need to include it in the example.

skinny85 · 2022-01-05T22:48:05Z

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

+
+new rds.DatabaseInstance(this, 'Database', {
+  engine: rds.DatabaseInstanceEngine.sqlServerEe({ version: rds.SqlServerEngineVersion.VER_12_00_5000_0_V1 }),
+  instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),


Suggested change

instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),

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

skinny85 · 2022-01-05T23:30:28Z

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

+        key: 'value',
+      },
+      instanceProps: {
+        instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),


I think we can get rid of this.

Still not needed.

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

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

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Pull request has been modified.

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

skinny85 · 2022-02-01T20:18:06Z

@hoegertn what's the status of this?

hoegertn · 2022-02-01T23:06:15Z

Sorry, had some busy weeks. I need to write the missing tests and then this is ready. I plan to finish this within the next days.

skinny85 · 2022-02-01T23:21:25Z

Sounds great! No rush, I just wanted to make sure you weren't waiting on my review 🙂.

Re-request my review (there's a button in the top-right of the PR window, next to my avatar) once you're ready for another round of reviews!

skinny85

Looks great @hoegertn! Minor stylistic comments before we merge this in.

skinny85 · 2022-01-06T18:53:22Z

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

+declare const vpc: ec2.Vpc;
+
+new rds.DatabaseInstance(this, 'Database', {
+  engine: rds.DatabaseInstanceEngine.sqlServerEe({ version: rds.SqlServerEngineVersion.VER_12_00_5000_0_V1 }),


Ok. Can we make the above example use a versioned engine too then?

skinny85 · 2022-02-14T20:57:13Z

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

+declare const vpc: ec2.Vpc;
+
+new rds.DatabaseInstance(this, 'Database', {
+  engine: rds.DatabaseInstanceEngine.sqlServerEe({ version: rds.SqlServerEngineVersion.VER_12_00_5000_0_V1 }),


Can we use rds.SqlServerEngineVersion.VER_11 here too, like we do above?

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

skinny85 · 2022-02-14T21:00:59Z

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

+      ...props.parameters && {
+        dbParameterGroupName: new ParameterGroup(this, 'ParameterGroup', {
+          engine: props.engine,
+          parameters: props.parameters,
+        }).bindToInstance({}).parameterGroupName,
+      },


Can we extract this all of this to a parameterGroupName: string | undefined local variable? This code is way, way too clever for my liking.

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

skinny85 · 2022-02-14T21:13:21Z

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

+        credentials: {
+          username: 'admin',
+        },


This is optional, I would remove it from this test.

skinny85 · 2022-02-14T21:13:34Z

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

+    // WHEN
+    new rds.DatabaseInstance(stack, 'Database', {
+      engine: rds.DatabaseInstanceEngine.sqlServerEe({ version: rds.SqlServerEngineVersion.VER_12_00_5000_0_V1 }),
+      instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),


This is optional, I would remove it from this test.

skinny85 · 2022-02-14T21:14:01Z

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

+      engine: rds.DatabaseInstanceEngine.sqlServerEe({
+        version: rds.SqlServerEngineVersion.VER_11,
+      }),
+      description: 'desc',


This is optional, I would remove it from this test.

skinny85 · 2022-02-14T21:14:15Z

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

+    expect(() => {
+      new rds.DatabaseInstance(stack, 'Database', {
+        engine: rds.DatabaseInstanceEngine.sqlServerEe({ version: rds.SqlServerEngineVersion.VER_12_00_5000_0_V1 }),
+        instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),


This is optional, I would remove it from this test.

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

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Pull request has been modified.

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

skinny85

👍

mergify · 2022-02-14T23:18:58Z

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-02-15T00:00:54Z

AWS CodeBuild CI Report

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

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

mergify · 2022-02-15T00:01:16Z

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

Since aws/aws-cdk#18126, this exact logic is handled by aws-cdk.

…er (aws#18126) Fixes aws#18124 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Since aws/aws-cdk#18126, this exact logic is handled by aws-cdk.

feat(rds): simpler way to configure parameters for instance or cluster …

0761972

…#18124

hoegertn marked this pull request as draft December 21, 2021 23:07

github-actions bot assigned skinny85 Dec 21, 2021

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

skinny85 reviewed Dec 22, 2021

View reviewed changes

hoegertn added 2 commits December 22, 2021 07:50

review

925526f

cluster

5e7010e

hoegertn marked this pull request as ready for review December 22, 2021 08:46

jogold reviewed Dec 22, 2021

View reviewed changes

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

jogold reviewed Dec 22, 2021

View reviewed changes

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

jogold reviewed Dec 22, 2021

View reviewed changes

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

hoegertn added 2 commits December 25, 2021 14:21

readme

482621e

fix rosetta code

5a9dc03

skinny85 previously requested changes Jan 5, 2022

View reviewed changes

hoegertn and others added 2 commits January 6, 2022 12:03

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

b8d8ebd

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

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

781208c

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

hoegertn and others added 8 commits January 6, 2022 12:04

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

4407e85

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>

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

3043565

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>

Update packages/@aws-cdk/aws-rds/lib/instance.ts

41322eb

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>

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

03ecbe9

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

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

1866029

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

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

699fbd0

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

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

a6626b1

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

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

d1a5195

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Merge branch 'master' into hoegertn-18124

eca4cee

hoegertn added 2 commits February 14, 2022 14:29

Merge branch 'master' into hoegertn-18124

b04a55f

review feedback

d1eb5ae

hoegertn requested a review from skinny85 February 14, 2022 15:28

skinny85 changed the title ~~feat(rds): simpler way to configure parameters for instance or cluster~~ feat(rds): simpler way to configure parameters for instance and cluster Feb 14, 2022

skinny85 previously requested changes Feb 14, 2022

View reviewed changes

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

e3987fa

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

hoegertn and others added 6 commits February 14, 2022 22:19

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

6638fb3

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

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

ec02d7e

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

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

1be8163

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Update packages/@aws-cdk/aws-rds/lib/instance.ts

f5c568a

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Update packages/@aws-cdk/aws-rds/lib/props.ts

688f13e

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

review

1818410

hoegertn requested a review from skinny85 February 14, 2022 21:51

skinny85 approved these changes Feb 14, 2022

View reviewed changes

Merge branch 'master' into hoegertn-18124

20c8925

mergify bot merged commit 3ba9088 into aws:master Feb 15, 2022

akash1810 added a commit to guardian/cdk that referenced this pull request Feb 21, 2022

refactor: Remove RDS parameter group code

118f712

Since aws/aws-cdk#18126, this exact logic is handled by aws-cdk.

akash1810 added a commit to guardian/cdk that referenced this pull request Feb 22, 2022

refactor: Remove RDS parameter group code

77585a2

Since aws/aws-cdk#18126, this exact logic is handled by aws-cdk.

hoegertn deleted the hoegertn-18124 branch April 10, 2022 17:38

Conversation

hoegertn commented Dec 21, 2021

Uh oh!

gitpod-io bot commented Dec 21, 2021

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!

Uh oh!

hoegertn commented Dec 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoegertn commented Jan 5, 2022

Uh oh!

skinny85 commented Jan 5, 2022

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Feb 1, 2022

Uh oh!

hoegertn commented Feb 1, 2022

Uh oh!

skinny85 commented Feb 1, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Feb 14, 2022

Uh oh!

aws-cdk-automation commented Feb 15, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Feb 15, 2022

Uh oh!

Reviewers

skinny85 commented Dec 22, 2021 •

edited

Loading