feat(rds): Add InstanceParameterGroup #2075

aereal · 2019-03-22T07:23:02Z

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

sam-goodwin · 2019-03-22T23:17:02Z

packages/@aws-cdk/aws-rds/lib/instance-parameter-group-ref.ts

+/**
+ * A instance parameter group
+ */
+export abstract class InstanceParameterGroupRef extends Construct {


We stopped using the Ref suffix an instead use interfaces. Lambda is a good example:

https://github.com/awslabs/aws-cdk/blob/0de2206f8ba324fbfe0a3685eb5a406d17f8a422/packages/%40aws-cdk/aws-lambda/lib/function-base.ts#L14

https://github.com/awslabs/aws-cdk/blob/0de2206f8ba324fbfe0a3685eb5a406d17f8a422/packages/%40aws-cdk/aws-lambda/lib/function-base.ts#L126

https://github.com/awslabs/aws-cdk/blob/0de2206f8ba324fbfe0a3685eb5a406d17f8a422/packages/%40aws-cdk/aws-lambda/lib/function.ts#L214

https://github.com/awslabs/aws-cdk/blob/0de2206f8ba324fbfe0a3685eb5a406d17f8a422/packages/%40aws-cdk/aws-lambda/lib/function.ts#L564

These are imported but not used yet, so just removed them. fixed db1c8f3

sam-goodwin · 2019-03-22T23:17:59Z

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

+   */
+  protected validate(): string[] {
+    if (Object.keys(this.parameters).length === 0) {
+      return ['At least one parameter required, call setParameter().'];


Add unit tests for this case.

fixed 9c77fbf

sam-goodwin · 2019-03-22T23:18:33Z

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

+  /**
+   * The parameters in this parameter group
+   */
+  parameters?: Parameters;


Why optional?

Is there a @default?

No reason, just followed ClusterParameterGroup. I have no own opinion of this and change this to be required.

fixed ad3d753

sam-goodwin · 2019-03-22T23:19:31Z

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

+ */
+export interface InstanceParameterGroupProps {
+  /**
+   * Database family of this parameter group


Is there more detail? Perhaps a link to some documentation describing what a family is?

fixed 8435164

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

sam-goodwin · 2019-03-22T23:20:53Z

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

+      this.setParameter(key, value);
+    }
+
+    this.parameterGroupName = resource.ref;


There should be a safe ref on the Resource, e.g. resource.parameterGroupName.

fixed 8a22d7c

we already introduced `ImportedInstanceParameterGroup`

…ily`

aereal · 2019-03-24T15:29:53Z

@sam-goodwin

Thank you for review. I pushed some changes, so could you review them?

sam-goodwin · 2019-03-25T17:31:31Z

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

+    });
+
+    for (const [key, value] of Object.entries(parameters || {})) {
+      this.setParameter(key, value);


Are you worried about using the passed in parameters object? Because you could do the following instead:

this.parameters = props.parameters || {};

sam-goodwin · 2019-03-25T17:34:46Z

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

+   */
+  public setParameter(key: string, value: string | undefined) {
+    if (value === undefined && key in this.parameters) {
+      delete this.parameters[key];


Don't overload the setParameter function with delete behavior. We don't support union types in public APIs because they don't translate well to other languages like Java, and it is also a confusing API to call set to delete something.

setParameter is for setting parameters and removeParameter is for removing them, keep it simple.

@sam-goodwin I think this PR is based on the existing implementation of the cluster parameter group (https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-rds/lib/cluster-parameter-group.ts implemented here #729) which proposes the same approach.

Does you comment mean that the cluster parameter group should also be refactored?

I'm currently working on a PR to support db instances and would like to have your opinion on this.

On a second pass, I realize my comment about this union type was incorrect - string | undefined will work fine because it means optional. My mistake.

That said, it does seem confusing to me that we use setParameter to delete a parameter. I wouldn't expect to (for example) use map.put(key, null) to delete a key from a Map in Java. I was unaware of the precedent set in cluster parameter groups. What is the use case for this overloaded behavior?

I don't know, it seems to come from this discussion #729 (comment).

For me it doesn't make sense to have setParameter and removeParameter methods, parameters should be defined directly in the construction props.

I agree. @rix0rrr can you chime in?

I think it might make sense to build the RDS parameters object incrementally, since it can be frking huge (there are many parameters to set!), so I think I might want some function to do:

lessDurabilityMorePerformance(paramGroup);

Or something. But I don't feel super strongly about this, I'm okay with removing the mutators as well.

sam-goodwin · 2019-03-25T17:36:11Z

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

+   * Remove a previously-set parameter from this parameter group
+   */
+  public removeParameter(key: string) {
+    this.setParameter(key, undefined);


... do the delete work here.

sam-goodwin · 2019-03-25T17:36:58Z

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

+    });
+
+    // THEN
+    expect(stack).to(haveResource('AWS::RDS::DBInstance', {


Should you also verify the value of the parameter instance group?

sam-goodwin · 2019-03-25T17:38:28Z

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

+
+    // THEN
+    test.deepEqual(stack.node.resolve(exported), { parameterGroupName: { 'Fn::ImportValue': 'Stack:ParamsParameterGroupNameA6B808D7' } });
+    test.deepEqual(stack.node.resolve(imported.parameterGroupName), { 'Fn::ImportValue': 'Stack:ParamsParameterGroupNameA6B808D7' });


Validate the stacks. expect(stack).to(haveResource(..)).

sam-goodwin · 2019-03-25T17:39:57Z

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

+    new InstanceParameterGroup(stack, 'Params', {
+      family: 'hello',
+      description: 'desc',
+      parameters: {},


I don't think we should allow you to create an invalid instance in the first place.

jogold · 2019-04-08T19:15:39Z

See #2187 where parameter groups are refactored and the changes from this PR could be integrated?

Specific classes are exposed for a source database instance (`DatabaseInstance`), an instance created from a snapshot (`DatabaseInstanceFromSnapshot`) and a read replica instance (`DatabaseInstanceReadReplica`). Add construct for option groups and refactor parameter groups. Add basic support for instance event rules. Integration with Secrets Manager and secret rotation. Closes #2075 Closes #1693

aereal added 3 commits March 22, 2019 16:20

feat(rds): Add InstanceParameterGroup

327dee3

feat(rds): pass InstanceParameterGroup to CfnDBInstance

a59d9ad

feat(rds): add tests for InstanceParameterGroup

46b7558

aereal requested a review from a team as a code owner March 22, 2019 07:23

aereal mentioned this pull request Mar 22, 2019

Unable to set instance parameter group on instances in rds.DatabaseCluster #1693

Closed

sam-goodwin reviewed Mar 22, 2019

View reviewed changes

aereal added 5 commits March 24, 2019 23:43

fix(rds): Make parameters required

ad3d753

fix(rds): Use dbParameterGroupName instead of ref

8a22d7c

fix(rds): removed InstanceParameterGroupRef

db1c8f3

we already introduced `ImportedInstanceParameterGroup`

fix(rds): add test for InstanceParameterGroup#validate

9c77fbf

fix(rds): add more detailed docs for `InstanceParameterGroupProps.fam…

8435164

…ily`

sam-goodwin added the @aws-cdk/aws-rds Related to Amazon Relational Database label Mar 25, 2019

sam-goodwin suggested changes Mar 25, 2019

View reviewed changes

jogold mentioned this pull request May 30, 2019

feat(rds): add support for database instances #2187

Merged

4 tasks

rix0rrr closed this in #2187 May 31, 2019

This was referenced Jan 20, 2020

[Snyk] Upgrade @aws-cdk/aws-route53 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#20

Closed

[Snyk] Upgrade @aws-cdk/aws-s3 from 0.22.0 to 0.39.0 MechanicalRock/tech-radar#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): Add InstanceParameterGroup #2075

feat(rds): Add InstanceParameterGroup #2075

aereal commented Mar 22, 2019 •

edited

Loading

sam-goodwin Mar 22, 2019

aereal Mar 24, 2019

sam-goodwin Mar 22, 2019

aereal Mar 24, 2019

sam-goodwin Mar 22, 2019

aereal Mar 24, 2019

aereal Mar 24, 2019

sam-goodwin Mar 22, 2019

aereal Mar 24, 2019

sam-goodwin Mar 22, 2019

aereal Mar 24, 2019

aereal commented Mar 24, 2019

sam-goodwin Mar 25, 2019

sam-goodwin Mar 25, 2019

jogold Mar 26, 2019

sam-goodwin Mar 26, 2019

jogold Mar 27, 2019

sam-goodwin Apr 2, 2019

rix0rrr Apr 2, 2019

sam-goodwin Mar 25, 2019

sam-goodwin Mar 25, 2019

sam-goodwin Mar 25, 2019

sam-goodwin Mar 25, 2019

jogold commented Apr 8, 2019

feat(rds): Add InstanceParameterGroup #2075

feat(rds): Add InstanceParameterGroup #2075

Conversation

aereal commented Mar 22, 2019 • edited Loading

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aereal commented Mar 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold commented Apr 8, 2019

aereal commented Mar 22, 2019 •

edited

Loading