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

Finalize the design of ParameterGroup in RDS #8932

Closed
skinny85 opened this issue Jul 8, 2020 · 0 comments · Fixed by #8959
Closed

Finalize the design of ParameterGroup in RDS #8932

skinny85 opened this issue Jul 8, 2020 · 0 comments · Fixed by #8959
Assignees
@skinny85
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database effort/small Small work item – less than a day of effort feature-request A feature should be added or improved.
Milestone
[DevPreview] @aws...

Comments

@skinny85
Copy link
Contributor

skinny85 commented Jul 8, 2020

The current ParameterGroup class in RDS leaves a lot to be desired.

  1. The naming is a little weird - right now, we have 2 classes, ParameterGroup and ClusterParameterGroup, that implement the IParameterGroup interface. The first one should probably be called InstanceParameterGroup instead.
  2. It doesn't seem to be a great experience to have 2 basically identical classes there at all. It's been modeled that because there are 2 separate L1s, CfnDBParameterGroup and CfnDBClusterParameterGroup. But we can probly have one class instead, and it will create the correct L1 based on where it's used (when can add bindToCluster() and bindToInstance() methods to ParameterGroup, which will do the right thing).

See these comments for more discussion on this topic:
@skinny85 skinny85 self-assigned this Jul 8, 2020
@skinny85 skinny85 added @aws-cdk/aws-rds Related to Amazon Relational Database effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Jul 8, 2020
@skinny85 skinny85 added this to the RDS to 'developer preview' milestone Jul 8, 2020
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jul 9, 2020
@skinny85
feat(rds): unify ParameterGroup and ClusterParameterGroup
40c3e40 
The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes aws#8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead
@skinny85 skinny85 mentioned this issue Jul 9, 2020
Merged
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jul 9, 2020
@skinny85
feat(rds): unify ParameterGroup and ClusterParameterGroup
b5f9906 
The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes aws#8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jul 9, 2020
@skinny85
feat(rds): unify ParameterGroup and ClusterParameterGroup
2cb2fa5 
The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes aws#8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jul 14, 2020
@skinny85
feat(rds): unify ParameterGroup and ClusterParameterGroup
3c3a4e4 
The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes aws#8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead
skinny85 added a commit to skinny85/aws-cdk that referenced this issue Jul 16, 2020
@skinny85
feat(rds): unify ParameterGroup and ClusterParameterGroup
aa1361f 
The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes aws#8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead
@mergify mergify bot closed this as completed in #8959 Jul 16, 2020
mergify bot pushed a commit that referenced this issue Jul 16, 2020
@skinny85
feat(rds): unify ParameterGroup and ClusterParameterGroup (#8959)
17b690b 
The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes #8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this issue Aug 11, 2020
@skinny85
feat(rds): unify ParameterGroup and ClusterParameterGroup (aws#8959)
75b7425 
The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes aws#8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@skinny85 skinny85

Labels
@aws-cdk/aws-rds Related to Amazon Relational Database effort/small Small work item – less than a day of effort feature-request A feature should be added or improved.
Projects
None yet
Milestone
[DevPreview] @aws-cdk/aws-rds
Development

Successfully merging a pull request may close this issue.

feat(rds): unify ParameterGroup and ClusterParameterGroup skinny85/aws-cdk
1 participant
@skinny85