-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
fix(rds): proxy for db cluster fails with model validation error #8896
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for posting a fix. Some comments below.
// Currently(2020-07-04), this property must be set to default. | ||
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbproxytargetgroup.html#TargetGroupName-fn::getatt | ||
proxyTargetGroup.addOverride('Properties.TargetGroupName', 'default'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overrides are set for CloudFormation properties that are not yet supported in the CDK. Looks like this is a CloudFormation return value.
Why is this being set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a difference between the CloudFormation resource document and what CloudFormation actually does.
Currently CloudFormation requires TargetGroupName
in props of ProxyTargetGroup
, but it is not in CfnProxyTargetGroupProps
There is no normal way to set TargetGroupName
, but through addOverride
.
The behavior seems to be changed around DB Proxy's GA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
@@ -408,20 +408,30 @@ export class DatabaseProxy extends cdk.Resource | |||
this.endpoint = this.resource.attrEndpoint; | |||
|
|||
let dbInstanceIdentifiers: string[] | undefined; | |||
if (bindResult.dbClusters) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a unit test for the case that was previously wrong and is fixed in this PR.
// Currently(2020-07-04), this property must be set to default. | ||
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbproxytargetgroup.html#TargetGroupName-fn::getatt | ||
proxyTargetGroup.addOverride('Properties.TargetGroupName', 'default'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
fixes #8885
follows #8476
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license