feat(rds): add secret rotation to DatabaseClusterFromSnapshot#20020
Conversation
|
|
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
I would say that we do definitely want to move addRotationSingleUser and addRotationMultiUser into DatabaseClusterNew rather than rewriting them again since the functions and added fields are identical in both places. I'd encourage you to go ahead and give it a shot!
In fact, a bit more of the duplicated functionality could be moved into DatabaseClusterNew but I won't ask for that refactor since you're not making changes to those pieces of code. Still, feel free to give that a shot as well, if you'd like.
Thank you for the reply and comments @TheRealAmazonKendra! I tried putting the methods up into the parent class, and it is turning out to be a bit stickier than expected -- it is a significant lift for me to attempt a refactor given I am not so familiar with the internals here, but I will keep trying... |
refactor: pull duplicative code to parent class
Pull request has been modified.
|
Should be good to go for another look @TheRealAmazonKendra 🙏 |
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Great work on this. Just a couple more things and this should be good to go.
| expect(() => cluster.addRotationSingleUser()).toThrow(/A single user rotation was already added to this cluster/); | ||
| }); | ||
|
|
||
| test('create a cluster from a snapshot with multi user secret rotation', () => { |
There was a problem hiding this comment.
Let's do an error case for a missing secret on multi user.
There was a problem hiding this comment.
It looks like right now a secret is required in RotationMultiUserOptions, unlike in RotationSingleUserOptions, so a similar test as in addRotationSingleUser cannot be constructed for addRotationMultiUser. Can you advise a bit further? Thank you!
There was a problem hiding this comment.
Nope, I was wrong here. Your error cases are fine.
Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>
Pull request has been modified.
Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>
TheRealAmazonKendra
left a comment
There was a problem hiding this comment.
Great work! Thanks you for your contribution!
|
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). |
Thank you! |
…20020) Bring RDS `DatabaseClusterFromSnapshot` API to parity with `DatabaseCluster` in being able to add Secrets Manager credential rotation with `addRotationSingleUser` or `addRotationMultiUser`. My first PR here! There may be some potential to DRY up this approach by moving up the method to the parent `DatabaseClusterNew` class as for now the code is duplicative between the classes, but I am frankly not comfortable doing it myself. Any input and suggestions very welcome -- thanks in advance! closes aws#12877 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: N/A ### New Features N/A *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Bring RDS
DatabaseClusterFromSnapshotAPI to parity withDatabaseClusterin being able to add Secrets Manager credential rotation withaddRotationSingleUseroraddRotationMultiUser.My first PR here! There may be some potential to DRY up this approach by moving up the method to the parent
DatabaseClusterNewclass as for now the code is duplicative between the classes, but I am frankly not comfortable doing it myself. Any input and suggestions very welcome -- thanks in advance!closes #12877
All Submissions:
Adding new Unconventional Dependencies:
N/A
New Features
N/A
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license