fix(neptune-alpha): multiple `cloudwatchLogsExports` cannot be set when configuring log retention #28643

badmintoncryer · 2024-01-10T06:10:51Z

This PR includes a breaking change to the aws-neptune-alpha module.

I have resolved an issue where it was not possible to set multiple cloudwatchLogsExports when configuring log retention for a DatabaseCluster.

The root cause of the problem was that the LogRetention creation included [object object] in the construct's ID. With the current fix, the given logType can now be correctly included in the LogRetention.

- new logs.LogRetention(this, `${logType}LogRetention`, {..} // "DatabaseobjectObjectLogRetentionA247AF0C"
+ new logs.LogRetention(this, `${logType.value}LogRetention`, {..} // "DatabaseauditLogRetention8A29F5CC"

BREAKING CHANGE: Corrected LogRetention IDs for DatabaseCluster. Previously, regardless of the log type, the string ‘objectObject’ was always included, but after the correction, the log type is now included.

Closes #26295.

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

badmintoncryer · 2024-01-10T06:31:26Z

packages/@aws-cdk/aws-neptune-alpha/test/integ.cluster.ts

@@ -37,6 +38,7 @@ const cluster = new DatabaseCluster(stack, 'Database', {
 vpc,
 vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS },
 instanceType: InstanceType.R5_LARGE,
+ engineVersion: EngineVersion.V1_2_1_0,


If engineVersion is not specified, CFN will create the cluster using the latest engine version. Currently, the latest engine is the v1.3 series, which the existing CDK code does not support.
(If the parameter group is also not set to v1.3, the deployment will fail.)

Therefore, in this integration test, the engineVersion and parameter group family have been explicitly set to v1.2.

I plan to address the support for the v1.3 in a separate issue.

Sounds good, feel free to ping me for a review when you get around to that.

@paulhcsun
This problem is resolved by #28647.
Do I need to conduct this integration test with version 1.3, or is it okay to proceed as it is?

This reverts commit 59f0466, reversing changes made to b1218a8.

This reverts commit b1218a8.

Pull request has been modified.

paulhcsun

@badmintoncryer You're right, sorry I misread the section in the Contributing guide on allowing breaking changes. Codewise this looks good to me, could you just update your PR description so that the BREAKING CHANGE: callout includes an explanation of the behaviour before and the behaviour now? I'm good to approve after that's been updated.

badmintoncryer · 2024-01-26T05:24:25Z

@paulhcsun Thank you for confirming. I have updated the description. Could you please check it again?

Pull request has been modified.

paulhcsun

The breaking change callout looks good to me. Nice work @badmintoncryer!

paulhcsun · 2024-01-26T22:54:07Z

packages/@aws-cdk/aws-neptune-alpha/test/integ.cluster.ts

@@ -37,6 +38,7 @@ const cluster = new DatabaseCluster(stack, 'Database', {
 vpc,
 vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS },
 instanceType: InstanceType.R5_LARGE,
+ engineVersion: EngineVersion.V1_2_1_0,


Sounds good, feel free to ping me for a review when you get around to that.

badmintoncryer · 2024-01-27T17:36:35Z

Why is the CodeBuild job in a pending state..??

badmintoncryer · 2024-01-29T16:52:08Z

@paulhcsun Could you please re-run the CodeBuild job?

Pull request has been modified.

mergify · 2024-01-29T23:48:29Z

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

mergify · 2024-01-29T23:51:44Z

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

Pull request has been modified.

aws-cdk-automation · 2024-01-30T00:37:29Z

AWS CodeBuild CI Report

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

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

mergify · 2024-01-30T00:37:44Z

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

…en configuring log retention (#28643) This PR includes a breaking change to the aws-neptune-alpha module. I have resolved an issue where it was not possible to set multiple `cloudwatchLogsExports` when configuring log retention for a `DatabaseCluster`. The root cause of the problem was that the LogRetention creation included `[object object]` in the construct's ID. With the current fix, the given `logType` can now be correctly included in the `LogRetention`. ```diff - new logs.LogRetention(this, `${logType}LogRetention`, {..} // "DatabaseobjectObjectLogRetentionA247AF0C" + new logs.LogRetention(this, `${logType.value}LogRetention`, {..} // "DatabaseauditLogRetention8A29F5CC" ``` BREAKING CHANGE: Corrected LogRetention IDs for DatabaseCluster. Previously, regardless of the log type, the string ‘objectObject’ was always included, but after the correction, the log type is now included. Closes #26295. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the p2 label Jan 10, 2024

aws-cdk-automation requested a review from a team January 10, 2024 06:11

github-actions bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Jan 10, 2024

badmintoncryer changed the title ~~fix(neptune-alpha): Multiple cloudwatchLogsExports cannot be set~~ fix(neptune-alpha): Multiple cloudwatchLogsExports cannot be set when configuring log retention. Jan 10, 2024

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 and removed p2 labels Jan 10, 2024

badmintoncryer commented Jan 10, 2024

View reviewed changes

badmintoncryer marked this pull request as ready for review January 10, 2024 06:33

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 10, 2024

badmintoncryer changed the title ~~fix(neptune-alpha): Multiple cloudwatchLogsExports cannot be set when configuring log retention.~~ fix(neptune-alpha): Multiple cloudwatchLogsExports cannot be set when configuring log retention Jan 10, 2024

paulhcsun changed the title ~~fix(neptune-alpha): Multiple cloudwatchLogsExports cannot be set when configuring log retention~~ fix(neptune-alpha): multiple cloudwatchLogsExports cannot be set when configuring log retention Jan 11, 2024

badmintoncryer marked this pull request as draft January 18, 2024 17:06

badmintoncryer changed the title ~~fix(neptune-alpha): multiple cloudwatchLogsExports cannot be set when configuring log retention~~ fix(neptune-alpha): multiple cloudwatchLogsExports cannot be set when configuring log retention (under feature flag) Jan 18, 2024

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 18, 2024

badmintoncryer added 3 commits January 19, 2024 03:30

fix: logName

da91441

fix: remove irrevant update

a6ba118

test: integ test

a3eaf7f

badmintoncryer force-pushed the 26295-logExport branch from 0e2d052 to a3eaf7f Compare January 18, 2024 18:30

badmintoncryer added 4 commits January 19, 2024 04:51

feat: add feature flag

811d19a

feat: feature flag

a7daf85

fix: integ test

78e3a6f

Merge branch 'main' into 26295-logExport

015a758

badmintoncryer marked this pull request as ready for review January 18, 2024 21:30

fix: integ test

b1218a8

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 19, 2024

badmintoncryer added 3 commits January 19, 2024 14:07

Merge branch 'main' into 26295-logExport

59f0466

Revert "Merge branch 'main' into 26295-logExport"

bfc21d6

This reverts commit 59f0466, reversing changes made to b1218a8.

Revert "fix: integ test"

a1e707e

This reverts commit b1218a8.

Merge branch 'main' into 26295-logExport

0b0c0b7

paulhcsun previously requested changes Jan 25, 2024

View reviewed changes

Merge branch 'main' into 26295-logExport

68f4c5c

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 26, 2024

Merge branch 'main' into 26295-logExport

9a3cbb0

paulhcsun previously approved these changes Jan 26, 2024

View reviewed changes

Merge branch 'main' into 26295-logExport

cd32bd5

Merge branch 'main' into 26295-logExport

cef5ad3

paulhcsun previously approved these changes Jan 29, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 29, 2024

Merge branch 'main' into 26295-logExport

9844c1d

paulhcsun previously approved these changes Jan 30, 2024

View reviewed changes

Merge branch 'main' into 26295-logExport

3703619

paulhcsun approved these changes Jan 30, 2024

View reviewed changes

mergify bot merged commit 56794fc into aws:main Jan 30, 2024
11 checks passed

badmintoncryer deleted the 26295-logExport branch January 30, 2024 03:36

github-actions bot mentioned this pull request Feb 1, 2024

Monthly PR metrics report - Jan 2024 #28952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(neptune-alpha): multiple `cloudwatchLogsExports` cannot be set when configuring log retention #28643

fix(neptune-alpha): multiple `cloudwatchLogsExports` cannot be set when configuring log retention #28643

badmintoncryer commented Jan 10, 2024 •

edited by paulhcsun

Loading

badmintoncryer Jan 10, 2024 •

edited

Loading

badmintoncryer Jan 10, 2024

paulhcsun Jan 26, 2024

badmintoncryer Jan 27, 2024 •

edited

Loading

paulhcsun left a comment •

edited

Loading

badmintoncryer commented Jan 26, 2024

paulhcsun left a comment

paulhcsun Jan 26, 2024

badmintoncryer commented Jan 27, 2024

badmintoncryer commented Jan 29, 2024

mergify bot commented Jan 29, 2024

mergify bot commented Jan 29, 2024

aws-cdk-automation commented Jan 30, 2024

mergify bot commented Jan 30, 2024

fix(neptune-alpha): multiple cloudwatchLogsExports cannot be set when configuring log retention #28643

fix(neptune-alpha): multiple cloudwatchLogsExports cannot be set when configuring log retention #28643

Conversation

badmintoncryer commented Jan 10, 2024 • edited by paulhcsun Loading

badmintoncryer Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

badmintoncryer Jan 10, 2024

Choose a reason for hiding this comment

paulhcsun Jan 26, 2024

Choose a reason for hiding this comment

badmintoncryer Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

paulhcsun left a comment • edited Loading

Choose a reason for hiding this comment

badmintoncryer commented Jan 26, 2024

paulhcsun left a comment

Choose a reason for hiding this comment

paulhcsun Jan 26, 2024

Choose a reason for hiding this comment

badmintoncryer commented Jan 27, 2024

badmintoncryer commented Jan 29, 2024

mergify bot commented Jan 29, 2024

mergify bot commented Jan 29, 2024

aws-cdk-automation commented Jan 30, 2024

AWS CodeBuild CI Report

mergify bot commented Jan 30, 2024

fix(neptune-alpha): multiple `cloudwatchLogsExports` cannot be set when configuring log retention #28643

fix(neptune-alpha): multiple `cloudwatchLogsExports` cannot be set when configuring log retention #28643

badmintoncryer commented Jan 10, 2024 •

edited by paulhcsun

Loading

badmintoncryer Jan 10, 2024 •

edited

Loading

badmintoncryer Jan 27, 2024 •

edited

Loading

paulhcsun left a comment •

edited

Loading