fix(neptune-alpha): multiple cloudwatchLogsExports cannot be set when configuring log retention#28643
Conversation
cloudwatchLogsExports cannot be setcloudwatchLogsExports cannot be set when configuring log retention.
| vpc, | ||
| vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }, | ||
| instanceType: InstanceType.R5_LARGE, | ||
| engineVersion: EngineVersion.V1_2_1_0, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I plan to address the support for the v1.3 in a separate issue.
There was a problem hiding this comment.
Sounds good, feel free to ping me for a review when you get around to that.
There was a problem hiding this comment.
@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?
cloudwatchLogsExports cannot be set when configuring log retention.cloudwatchLogsExports cannot be set when configuring log retention
cloudwatchLogsExports cannot be set when configuring log retentioncloudwatchLogsExports cannot be set when configuring log retention
cloudwatchLogsExports cannot be set when configuring log retentioncloudwatchLogsExports cannot be set when configuring log retention (under feature flag)
0e2d052 to
a3eaf7f
Compare
This reverts commit b1218a8.
Pull request has been modified.
There was a problem hiding this comment.
@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.
|
@paulhcsun Thank you for confirming. I have updated the description. Could you please check it again? |
Pull request has been modified.
paulhcsun
left a comment
There was a problem hiding this comment.
The breaking change callout looks good to me. Nice work @badmintoncryer!
| vpc, | ||
| vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }, | ||
| instanceType: InstanceType.R5_LARGE, | ||
| engineVersion: EngineVersion.V1_2_1_0, |
There was a problem hiding this comment.
Sounds good, feel free to ping me for a review when you get around to that.
|
Why is the CodeBuild job in a pending state..?? |
|
@paulhcsun Could you please re-run the CodeBuild job? |
Pull request has been modified.
|
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). |
|
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.
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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*
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
cloudwatchLogsExportswhen configuring log retention for aDatabaseCluster.The root cause of the problem was that the LogRetention creation included
[object object]in the construct's ID. With the current fix, the givenlogTypecan now be correctly included in theLogRetention.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