fix(rds): unable to create cluster with per-instance parameters (under feature flag) by go-to-k · Pull Request #27040 · aws/aws-cdk

go-to-k · 2023-09-07T06:00:53Z

This PR fixes that DatabaseCluster cannot create multiple readers with each parameters.

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

go-to-k · 2023-09-07T06:08:33Z

Exemption Request because I think this bug can be covered in an unit test.

packages/aws-cdk-lib/aws-rds/lib/aurora-cluster-instance.ts

mrgrain

Thanks @go-to-k for this change!

I think we should just put this behind a feature flag. That makes it the default for new apps and opt-in for existing apps.

mrgrain · 2023-09-19T12:11:39Z

packages/aws-cdk-lib/aws-rds/lib/aurora-cluster-instance.ts

    const instanceParameterGroup = props.parameterGroup ?? (
      props.parameters
-        ? new ParameterGroup(props.cluster, 'InstanceParameterGroup', {
+        ? new ParameterGroup(props.cluster, `${id}InstanceParameterGroup`, {


I reckon this would be better:

Suggested change

? new ParameterGroup(props.cluster, `${id}InstanceParameterGroup`, {

? new ParameterGroup(this, `InstanceParameterGroup`, {

OK, I changed!

I missed to change tests for this changes. I will apply...

go-to-k · 2023-09-19T12:43:14Z

@mrgrain

Thanks for your review!

I think we should just put this behind a feature flag. That makes it the default for new apps and opt-in for existing apps.

I have never had a modification that required a feature flag. Is there anything I should do with this PR?

mrgrain · 2023-09-19T12:49:38Z

I have never had a modification that required a feature flag. Is there anything I should do with this PR?

Exciting! The required steps are described in the contributing guide. LMK if you run into any issues.

mrgrain · 2023-09-19T12:53:04Z

Exemption Request because I think this bug can be covered in an unit test.

@go-to-k I would like to see this deploying though. You should be able to do a small adjustment to an existing integ test.
I can execute the test for you then.

go-to-k · 2023-09-20T14:04:24Z

@mrgrain

I tried to respond to the feature flag and an integ test. Please check if there is anything wrong.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Pull request has been modified.

aws-cdk-automation · 2023-09-22T13:17:48Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 138b3eb
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-09-22T13:18:07Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

fix(rds): Cannot create multiple readers with each parameters

7d7fd72

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Sep 7, 2023

aws-cdk-automation requested a review from a team September 7, 2023 06:01

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Sep 7, 2023

aws-cdk-automation previously requested changes Sep 7, 2023

View reviewed changes

go-to-k marked this pull request as draft September 7, 2023 06:03

go-to-k marked this pull request as ready for review September 7, 2023 06:07

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Sep 7, 2023

test: change an unit test

7beacaa

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 7, 2023

go-to-k commented Sep 7, 2023

View reviewed changes

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

mrgrain previously requested changes Sep 19, 2023

View reviewed changes

mrgrain reviewed Sep 19, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 19, 2023

fix for the review

f83e4d2

mrgrain changed the title ~~fix(rds): Cannot create multiple readers with each parameters~~ fix(rds): unable to use per-instance parameters Sep 19, 2023

mrgrain changed the title ~~fix(rds): unable to use per-instance parameters~~ fix(rds): unable to create cluster with per-instance parameters Sep 19, 2023

mrgrain removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Sep 19, 2023

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Sep 19, 2023

go-to-k changed the title ~~fix(rds): unable to create cluster with per-instance parameters~~ fix(rds): unable to create cluster with per-instance parameters (under feature flag) Sep 20, 2023

go-to-k added 3 commits September 20, 2023 20:47

feature flag without integ tests

ee428c5

change unit test for feature flag

8089170

change integ tests

c973b5f

rix0rrr previously approved these changes Sep 22, 2023

View reviewed changes

Merge branch 'main' into fix/with-multiple-readers-with-each-parameters

138b3eb

rix0rrr approved these changes Sep 22, 2023

View reviewed changes

mergify bot merged commit 1be9894 into aws:main Sep 22, 2023

go-to-k deleted the fix/with-multiple-readers-with-each-parameters branch September 22, 2023 14:47

github-actions bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rds): unable to create cluster with per-instance parameters (under feature flag)#27040

fix(rds): unable to create cluster with per-instance parameters (under feature flag)#27040
mergify[bot] merged 7 commits intoaws:mainfrom
go-to-k:fix/with-multiple-readers-with-each-parameters

go-to-k commented Sep 7, 2023

Uh oh!

aws-cdk-automation left a comment

Uh oh!

go-to-k commented Sep 7, 2023

Uh oh!

Uh oh!

mrgrain left a comment

Uh oh!

mrgrain Sep 19, 2023

Uh oh!

go-to-k Sep 19, 2023

Uh oh!

go-to-k Sep 19, 2023 •

edited

Loading

Uh oh!

go-to-k commented Sep 19, 2023

Uh oh!

mrgrain commented Sep 19, 2023

Uh oh!

mrgrain commented Sep 19, 2023

Uh oh!

go-to-k commented Sep 20, 2023

Uh oh!

aws-cdk-automation commented Sep 22, 2023

Uh oh!

mergify bot commented Sep 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	? new ParameterGroup(props.cluster, `${id}InstanceParameterGroup`, {
	? new ParameterGroup(this, `InstanceParameterGroup`, {

Conversation

go-to-k commented Sep 7, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

go-to-k commented Sep 7, 2023

Uh oh!

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

mrgrain Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

go-to-k Sep 19, 2023

Choose a reason for hiding this comment

Uh oh!

go-to-k Sep 19, 2023 • 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!

go-to-k commented Sep 19, 2023

Uh oh!

mrgrain commented Sep 19, 2023

Uh oh!

mrgrain commented Sep 19, 2023

Uh oh!

go-to-k commented Sep 20, 2023

Uh oh!

aws-cdk-automation commented Sep 22, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Sep 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

go-to-k Sep 19, 2023 •

edited

Loading