fix(rds): do not allow aurora engines when using DatabaseInstance #5367

jogold · 2019-12-11T09:00:51Z

Aurora instances can only be created inside a cluster and should use the
DatabaseCluster construct.

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

Aurora instances can only be created inside a cluster and should use the `DatabaseCluster` construct. Closes aws#5345

mergify · 2019-12-11T09:01:31Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-12-11T09:33:00Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-12-16T12:32:53Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:00:39Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:52:58Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:38:39Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:03Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:17:05Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

skinny85

One suggestion / question.

skinny85 · 2019-12-17T18:09:54Z

packages/@aws-cdk/aws-rds/lib/instance.ts

@@ -670,6 +670,10 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
  constructor(scope: Construct, id: string, props: DatabaseInstanceSourceProps) {
    super(scope, id, props);

+    if (/aurora/.test(props.engine.name)) {


I'm not sure I love this test...

How about a method on Engine, something like get allowedOutsideCluster(): boolean (name TBD of course)? Then, this code could be:

if (!props.engine.allowedOutsideCluster) { throw new Error(`${props.engine.name} instances can only be created inside a cluster. Please use the DatabaseCluster class instead`); }

What do you think?

Yes, it's cleaner.

Actually just a public readonly isDatabaseInstanceEngine = true; in DatabaseInstanceEngine is enough no? Because then the typing system will not allow to use a DatabaseInstanceCluster for a DatabaseInstance and we don't need to check anymore?

Yep. This works:

export class DatabaseInstanceEngine extends DatabaseClusterEngine { // ... /** To make it a compile-time error to pass a DatabaseClusterEngine where a DatabaseInstanceEngine is expected. */ public readonly isDatabaseInstanceEngine = true; }

Having it be a compile-time error is even better than adding runtime validation.

aws-cdk-automation · 2019-12-18T09:52:35Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-24T09:16:45Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-12-24T09:21:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85

Thanks @jogold !

mergify · 2019-12-26T17:45:11Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-12-26T18:17:57Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-12-26T18:18:24Z

Thank you for contributing! Your pull request is now being automatically merged.

fix(rds): do not allow aurora engines when using DatabaseInstance

409a958

Aurora instances can only be created inside a cluster and should use the `DatabaseCluster` construct. Closes aws#5345

jogold requested a review from skinny85 as a code owner December 11, 2019 09:00

SomayaB assigned skinny85 Dec 11, 2019

skinny85 reviewed Dec 17, 2019

View reviewed changes

Merge branch 'master' into rds-aurora-engine

2ff7e12

jogold added 3 commits December 24, 2019 09:43

Merge branch 'master' into rds-aurora-engine

dd00cf1

compile-time error with isDatabaseInstanceEngine

c95c497

remove test on engine name

9d4415e

skinny85 approved these changes Dec 26, 2019

View reviewed changes

Merge branch 'master' into rds-aurora-engine

3d0bb7a

mergify bot merged commit 03b3b7a into aws:master Dec 26, 2019

jogold deleted the rds-aurora-engine branch January 24, 2020 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rds): do not allow aurora engines when using DatabaseInstance #5367

fix(rds): do not allow aurora engines when using DatabaseInstance #5367

jogold commented Dec 11, 2019

mergify bot commented Dec 11, 2019

aws-cdk-automation commented Dec 11, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

skinny85 left a comment

skinny85 Dec 17, 2019 •

edited

Loading

jogold Dec 18, 2019

jogold Dec 18, 2019

skinny85 Dec 23, 2019

aws-cdk-automation commented Dec 18, 2019

aws-cdk-automation commented Dec 24, 2019

aws-cdk-automation commented Dec 24, 2019

skinny85 left a comment

mergify bot commented Dec 26, 2019

aws-cdk-automation commented Dec 26, 2019

mergify bot commented Dec 26, 2019

fix(rds): do not allow aurora engines when using DatabaseInstance #5367

fix(rds): do not allow aurora engines when using DatabaseInstance #5367

Conversation

jogold commented Dec 11, 2019

mergify bot commented Dec 11, 2019

aws-cdk-automation commented Dec 11, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

jogold Dec 18, 2019

Choose a reason for hiding this comment

jogold Dec 18, 2019

Choose a reason for hiding this comment

skinny85 Dec 23, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Dec 18, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 24, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 24, 2019

AWS CodeBuild CI Report

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 26, 2019

aws-cdk-automation commented Dec 26, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 26, 2019

skinny85 Dec 17, 2019 •

edited

Loading