fix(redshift): single-node clusters fail with node count error #9961
Conversation
Single-node clusters defaulted to a `numberOfNodes` of 1; while this makes sense, CloudFormation actually throws if any number of nodes is defined for a single-node cluster. This change fixes this to default to `undefined` and extends the validation for multi-node clusters. fixes #9856
| @@ -537,4 +533,20 @@ export class Cluster extends ClusterBase { | |||
| target: this, | |||
| }); | |||
| } | |||
|
|
|||
| private validateNodeCount(clusterType: ClusterType, numberOfNodes?: number): number | undefined { | |||
There was a problem hiding this comment.
rename to parseNodeCode because it returns a value. Usually "validateXxxx" methods return void.
There was a problem hiding this comment.
validateXxxx methods usually return void, and parseXxxx methods usually parse something. Neither really fits here IMO. I personally prefer "validate" here as describing more of the "main" intent rather than "parse", which doesn't describe the method at all.
That being said, I see only a few "validate" methods with return types (one of which was also my fault), and lots of "parse" methods that don't actually parse anything, so I guess that's the defined convention at this point. Would something like validateAndReturnXxx or validateOrDefault or something like that be any better?
|
added do-not-merge label |
|
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). |
Single-node clusters defaulted to a
numberOfNodesof 1; while this makessense, CloudFormation actually throws if any number of nodes is defined for a
single-node cluster. This change fixes this to default to
undefinedandextends the validation for multi-node clusters.
fixes #9856
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license