feat(rds): S3 import and export for DatabaseInstances #10370
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change introduces S3 import/export for DatabaseInstances, the same as what currently exists today for DatabaseClusters. This change was heavily influenced by #10132 (the work to introduce feature names for DatabaseCluster), and steals patterns and names heavily from it. **Implementation Notes:** * Unlike for clsuters, for instances, the feature names are required; if the feature name doesn't exist, we shouldn't be creating the role. * For both Oracle and SQL Server, all current/active versions support the same feature names. This simplified the implementation quite a bit. * I opted **not** to support features for the deprecated Oracle versions. * I moved the `setupS3ImportExport` helper function into a utils class. One quirk of the SQL Server requirement is that you must create an OptionGroup with only one role (for both import & export). Oracle, likewise, has a single feature for both import and export. So I opted to default to creating a single role (if necessary) for both import and export. Open to challenges on this. * The `OptionGroup` class needed some rework to be able to make the list of configurations dynamic. I then had to do some light tweaking to ensure backwards compatibility with the connections property. fixes #4419
skinny85
suggested changes
Sep 17, 2020
Co-authored-by: Adam Ruka <adamruka@amazon.com>
skinny85
approved these changes
Sep 18, 2020
There was a problem hiding this comment.
I love (almost) everything about this. The only thing that I'm not a huge fan of is adding the description property to IInstanceEngine - I feel like a utility function might be better, especially that we use description only in a single spot (unless I missed something) in the entire codebase, and it can be considered backwards-incompatible.
I'll leave it to your judgement though 🙂.
skinny85
reviewed
Sep 18, 2020
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
This was referenced Aug 18, 2021
mergify bot
pushed a commit
that referenced
this pull request
Aug 25, 2021
As mentioned in #14546, Postgres database instances did not support `s3export` when most of the `s3 import/export` features were added in #10370. This PR fills in that gap now that `s3export` is [available](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/postgresql-s3-export.html) for Postgres. The supported versions are documented [here](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/postgresql-s3-export.html) and I manually verified via AWS CLI that Postgres 13+ supports `s3export` as well. Looking into the test suite, I think it makes more sense to modify the existing test than to create an entirely new one, similar to how other database instances test s3 import/export at the same time. But I can also move it to its own test if necessary. I also fixed a very minor doc typo in `cluster-instance.ts`. Closes #14546. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd
pushed a commit
to hollanddd/aws-cdk
that referenced
this pull request
Aug 26, 2021
) As mentioned in aws#14546, Postgres database instances did not support `s3export` when most of the `s3 import/export` features were added in aws#10370. This PR fills in that gap now that `s3export` is [available](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/postgresql-s3-export.html) for Postgres. The supported versions are documented [here](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/postgresql-s3-export.html) and I manually verified via AWS CLI that Postgres 13+ supports `s3export` as well. Looking into the test suite, I think it makes more sense to modify the existing test than to create an entirely new one, similar to how other database instances test s3 import/export at the same time. But I can also move it to its own test if necessary. I also fixed a very minor doc typo in `cluster-instance.ts`. Closes aws#14546. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO
pushed a commit
to TikiTDO/aws-cdk
that referenced
this pull request
Sep 6, 2021
) As mentioned in aws#14546, Postgres database instances did not support `s3export` when most of the `s3 import/export` features were added in aws#10370. This PR fills in that gap now that `s3export` is [available](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/postgresql-s3-export.html) for Postgres. The supported versions are documented [here](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/postgresql-s3-export.html) and I manually verified via AWS CLI that Postgres 13+ supports `s3export` as well. Looking into the test suite, I think it makes more sense to modify the existing test than to create an entirely new one, similar to how other database instances test s3 import/export at the same time. But I can also move it to its own test if necessary. I also fixed a very minor doc typo in `cluster-instance.ts`. Closes aws#14546. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
david-doyle-as24
pushed a commit
to david-doyle-as24/aws-cdk
that referenced
this pull request
Sep 7, 2021
) As mentioned in aws#14546, Postgres database instances did not support `s3export` when most of the `s3 import/export` features were added in aws#10370. This PR fills in that gap now that `s3export` is [available](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/postgresql-s3-export.html) for Postgres. The supported versions are documented [here](https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/postgresql-s3-export.html) and I manually verified via AWS CLI that Postgres 13+ supports `s3export` as well. Looking into the test suite, I think it makes more sense to modify the existing test than to create an entirely new one, similar to how other database instances test s3 import/export at the same time. But I can also move it to its own test if necessary. I also fixed a very minor doc typo in `cluster-instance.ts`. Closes aws#14546. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change introduces S3 import/export for DatabaseInstances, the same as what
currently exists today for DatabaseClusters. This change was heavily influenced
by #10132 (the work to introduce feature
names for DatabaseCluster), and steals patterns and names heavily from it.
Implementation Notes:
feature name doesn't exist, we shouldn't be creating the role.
feature names. This simplified the implementation quite a bit.
setupS3ImportExporthelper function into a utils class. Onequirk of the SQL Server requirement is that you must create an OptionGroup
with only one role (for both import & export). Oracle, likewise, has a single
feature for both import and export. So I opted to default to creating a single
role (if necessary) for both import and export. Open to challenges on this.
OptionGroupclass needed some rework to be able to make the list ofconfigurations dynamic. I then had to do some light tweaking to ensure
backwards compatibility with the connections property.
fixes #4419
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license