-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(rds): add support for update and backup properties to Cluster instances #10324
feat(rds): add support for update and backup properties to Cluster instances #10324
Conversation
If this policy is acceptable, I can also add these parameters to DatabaseInstance. |
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 a lot for the contribution @hixi-hyi ! Some small comments, and one question.
/** | ||
* Whether to allow upgrade of major version for the DB instance. | ||
* | ||
* @default - true |
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.
is this really true
by default? The documentation says:
Constraints: Major version upgrades must be allowed when specifying a value for the EngineVersion parameter that is a different major version than the DB instance's current version.
I guess that suggests that the default is, in fact, false
...?
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.
Surely the default value for AllowMajorVersionUpgrade
may be false.
Reading the documentation, this parameter may not be used routinely and is only needed when changing instance types, but I don't understand exactly what it is. (There was no value for AllowMajorVersionUpgrade
in the results of aws rds describe-instances
)
The @default
value is currently written from the Cloudformation documentation.
However, I haven't programmatically set it, and its behavior is dependent on Cloudformation.
Should such a parameter be written as @default None
? Or should I explicitly set true or false in the cdk?
I'd like to hear your opinion.
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.
If not setting it and setting it to false
provide the same result (as I believe it does in this case), it should be @default false
.
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.
I understood. Thank you for your reply.
In fact, AllowMajorUpgrade
is default false, and it seems to be necessary to add this parameter when upgrading the major version.
Pull request has been modified.
Hey @hixi-hyi , looks like some trailing spaces snuck into the file:
Would you mind updating the PR (and also inclding the change discussed in #10324 (comment) )? Thanks! |
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.
Putting in "Request changes" awaiting @hixi-hyi 's new iteration to remove this from my ToDo list.
Pull request has been modified.
@skinny85 |
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.
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). |
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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thanks for the merge and the fix testcode :D |
My pleasure, thanks for the contribution! |
fixes #9926
Added the following parameters to DatabaseCluster.
#10092 as a reference, only defined simple parameters.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license