fix(rds): instance engine can be (wrongly) specified as engine when defining a cluster #8514
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
nija-at
changed the title
There was a problem hiding this comment.
So much easier to review when the changes are separated. Thanks for taking the effort!
I've updated the title of this PR to talk about what problem is being solved, rather than how it is solved. For future reference, this goes into our changelog under 'Bug Fixes'.
Customers who read this should see what got fixed.
skinny85
force-pushed
the
feat/rds-engines-split
branch
from
6da44bd to
456b137
Compare
|
@nija-at this is ready for another round of reviews! |
skinny85
added
the
pr-linter/exempt-test
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| */ | ||
| public readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication; | ||
|
|
||
| protected readonly parameterGroupFamilies?: ParameterGroupFamily[]; |
There was a problem hiding this comment.
This, and the 4th parameter on the constructor, are only needed on the DatabaseClusterEngine class.
There was a problem hiding this comment.
Yes, but they will be used in later PRs for engines, so I'd rather not remove them.
There was a problem hiding this comment.
Isn't this a case where we're leaving the state of the module in a half baked state, if/when the other PR takes a different turn or plans change?
Would prefer that this PR do the correct thing now and adjust it later as part of the future PR.
There was a problem hiding this comment.
Isn't this a case where we're leaving the state of the module in a half baked state, if/when the other PR takes a different turn or plans change?
No. This field is not public in DatabaseInstanceEngine, so it doesn't leave the module in any different shape than it is now.
nija-at
changed the title
skinny85
force-pushed
the
feat/rds-engines-split
branch
from
456b137 to
c0a0e90
Compare
|
@nija-at included your comments, this is ready for another review! |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
… separate types Currently, InstanceEngine extended ClusterEngine, which meant you could pass InstanceEngine when creating a Cluster, which would fail at deploy time. This change splits the two classes into distinct types, without a subtyping relationship between the two. BREAKING CHANGE: DatabaseClusterEngine now takes props in its constructor instead of positional arguments * **rds**: DatabaseInstanceEngine now takes props in its constructor instead of positional arguments
skinny85
force-pushed
the
feat/rds-engines-split
branch
from
c0a0e90 to
40d8622
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| * Base properties for all types of engines | ||
| * (cluster and instances). | ||
| */ | ||
| export interface BaseEngineProps { |
There was a problem hiding this comment.
No, it has to be exported (otherwise JSII fails).
| */ | ||
| public readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication; | ||
|
|
||
| protected readonly parameterGroupFamilies?: ParameterGroupFamily[]; |
There was a problem hiding this comment.
Isn't this a case where we're leaving the state of the module in a half baked state, if/when the other PR takes a different turn or plans change?
Would prefer that this PR do the correct thing now and adjust it later as part of the future PR.
| @@ -83,18 +83,20 @@ export = { | |||
|
|
|||
| 'parameter group family'(test: Test) { | |||
There was a problem hiding this comment.
(Missed this in the last round)
Extract all tests around engines into test.engine.ts
|
Superseded by #8686 . |
Currently, InstanceEngine extended ClusterEngine,
which meant you could pass InstanceEngine when creating a Cluster,
which would fail at deploy time.
This change splits the two classes into distinct types,
without a subtyping relationship between the two.
BREAKING CHANGE: DatabaseClusterEngine now takes props in its constructor instead of positional arguments
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license