Skip to content

feat(rds): allow DatabaseClusterFromSnapshot to set copyTagsToSnapshot property #19932

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

Merged
merged 13 commits into from
Apr 25, 2022
Merged

feat(rds): allow DatabaseClusterFromSnapshot to set copyTagsToSnapshot property #19932

merged 13 commits into from
Apr 25, 2022

Conversation

AnuragMohapatra
Copy link
Contributor

@AnuragMohapatra AnuragMohapatra commented Apr 15, 2022

This change will now allow users to set copyTagsToSnapshot attribute for the DatabaseClusterFromSnapshot.

This will now be consistent with the way the database instance works.

Integration test may not be possible since this requires a valid snapshot to pre-exist before building the CDK stack to generate the CDK local snapshot.

It might be possible to add a snapshot first by creating a new DB Cluster taking a manual snapshot then using that in DatabaseClusterFromSnapshot but it will become a hassle for anyone else in future to maintain or update since they will always need to proceed in the same manner to get the exact snapshot.

An integration test for copyTagsToSnapshot already exists for DatabaseCluster in integ.cluster.js which is passing successfully on executing the tests.

Closes #19884


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@github-actions github-actions bot added the p2 label Apr 15, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team April 15, 2022 12:22
@gitpod-io
Copy link

gitpod-io bot commented Apr 15, 2022

Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @AnuragMohapatra! A few small comments.

@@ -425,6 +431,8 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
// Encryption
kmsKeyId: props.storageEncryptionKey?.keyArn,
storageEncrypted: props.storageEncryptionKey ? true : props.storageEncrypted,
//Tag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//Tag
// Tags

@skinny85 skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label Apr 23, 2022
@skinny85 skinny85 changed the title feat(rds): allow database cluster from snapshot to set copytagtosnapshot property feat(rds): allow DatabaseClusterFromSnapshot to set copyTagsToSnapshot property Apr 23, 2022
@skinny85 skinny85 added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 23, 2022
@mergify mergify bot dismissed skinny85’s stale review April 23, 2022 05:58

Pull request has been modified.

@AnuragMohapatra
Copy link
Contributor Author

Thanks @skinny85 for reviewing, I have updated the changes as per suggestions.

@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Apr 25, 2022
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @AnuragMohapatra!

@mergify
Copy link
Contributor

mergify bot commented Apr 25, 2022

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

@mergify
Copy link
Contributor

mergify bot commented Apr 25, 2022

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-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 1881318
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 40a6ceb into aws:master Apr 25, 2022
@mergify
Copy link
Contributor

mergify bot commented Apr 25, 2022

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

@AnuragMohapatra AnuragMohapatra deleted the feat(aws-rds)-MoveCopyTagsToSnapshotToBaseProps branch April 26, 2022 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RDS: DatabaseClusterFromSnapshot should allow setting copyTagsToSnapshot
3 participants