Skip to content

Commit

Permalink
chore(rds): improve error messages for providing a vpc to cluster (#2…
Browse files Browse the repository at this point in the history
…9706)

Just an idea/I was curious.

Personally, when I stumbled upon this error I got a bit confused because I wasn't sure what instanceProps was. I think the message  'Provide either vpc or instanceProps.vpc, but not both" provides the same explanation for both cases?

Or we rephrase the error message to not use "If instanceProps was provided", because the code can figure whether instanceProps was provided with another if condition Then the message would just be "VPC must be provided". Hence, someone who is unaware of instanceProps, doesn't need to be notified of it. Just an idea though and it's nitpicking! Thoughts?

### Reason for this change

Better error message when running into this error

### Description of changes

Consolidated logs statements

### Description of how you validated changes

I have not validated anything because this was just an idea/I was curious

### Checklist
- [X ] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
prasanthlouis authored Apr 17, 2024
1 parent 213fffc commit 21804fd
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 4 deletions.
4 changes: 1 addition & 3 deletions packages/aws-cdk-lib/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -595,10 +595,8 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
constructor(scope: Construct, id: string, props: DatabaseClusterBaseProps) {
super(scope, id);

if ((props.vpc && props.instanceProps?.vpc)) {
if ((props.vpc && props.instanceProps?.vpc) || (!props.vpc && !props.instanceProps?.vpc)) {
throw new Error('Provide either vpc or instanceProps.vpc, but not both');
} else if (!props.vpc && !props.instanceProps?.vpc) {
throw new Error('If instanceProps is not provided then `vpc` must be provided.');
}
if ((props.vpcSubnets && props.instanceProps?.vpcSubnets)) {
throw new Error('Provide either vpcSubnets or instanceProps.vpcSubnets, but not both');
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/aws-rds/test/cluster.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ describe('cluster new api', () => {
iamAuthentication: true,
});
// THEN
}).toThrow(/If instanceProps is not provided then `vpc` must be provided./);
}).toThrow(/Provide either vpc or instanceProps.vpc, but not both/);
});

test('when both vpc and instanceProps.vpc are provided', () => {
Expand Down

0 comments on commit 21804fd

Please sign in to comment.