feat(validation): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue #32841

iankhou · 2025-01-10T19:06:59Z

Issue #32840

Reason for this change

When initializing a new RDS DB Cluster, the current implementation fails on the first validation error, making it possible for the user to encounter another failure after fixing known validation issues.

The same holds true for new SQS Queue props validation.

Description of changes

Implemented a validation function that collects all validation errors and presents them to the user.

Describe any new or updated permissions being added

No permissions changes.

Description of how you validated changes

Added unit tests and modified existing unit tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

iankhou · 2025-01-10T19:10:22Z

packages/aws-cdk-lib/aws-rds/test/cluster.test.ts

@@ -2093,30 +2172,6 @@ describe('cluster', () => {
      });
    });

-    test('throws if performanceInsightRetention is set but performance insights is disabled', () => {


These two tests did the same thing (as implemented), removed them here and moved to line 567.

iankhou · 2025-01-10T19:11:05Z

packages/aws-cdk-lib/aws-rds/test/cluster.test.ts

-        }).toThrow('Aurora Limitless Database requires I/O optimized storage type, got: aurora');
-      });
-
-      test.each([[], undefined])('throw error for invalid cloudwatch log exports', (cloudwatchLogsExports) => {


Tested at line 584.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

codecov · 2025-01-10T19:37:45Z

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.45%. Comparing base (9d8a7e2) to head (098a5b2).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #32841      +/-   ##
==========================================
- Coverage   81.48%   81.45%   -0.04%     
==========================================
  Files         226      227       +1     
  Lines       13768    13776       +8     
  Branches     2416     2417       +1     
==========================================
+ Hits        11219    11221       +2     
- Misses       2271     2277       +6     
  Partials      278      278

Flag	Coverage Δ
suite.unit	`81.45% <25.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.89% <ø> (ø)`
packages/aws-cdk-lib/core	`82.03% <25.00%> (-0.07%)`	⬇️

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

iankhou · 2025-01-13T15:44:59Z

Exemption Request: unit test changes provide coverage for single and multiple errors returned, no README changes are needed as the effects of this change will be intuitive to users.

mrgrain

Two pieces of feedback:

Can you please change this to use ValidationError? We are currently starting a migration of all error types to the new ValidationError. You could not have known that. But would be good to get started on it.
This is great progress towards a more generic validation system. Good stuff! However I think we already should be aiming much higher and make this a generic (private) feature in core, so that all our constructs can you use this.

iankhou · 2025-01-13T16:08:04Z

@mrgrain

Two pieces of feedback:

Can you please change this to use ValidationError? We are currently starting a migration of all error types to the new ValidationError. You could not have known that. But would be good to get started on it.

This is great progress towards a more generic validation system. Good stuff! However I think we already should be aiming much higher and make this a generic (private) feature in core, so that all our constructs can you use this.

First one is an easy change and I will implement this.

For your second point, are you suggesting to generalize validateDatabaseClusterProps to validateProps, and to take in a set of validation rules? In my current PR's implementation, the rules are generated using another function createDatabaseClusterValidationRules, but I suppose this could be made into createValidationRules and take in a set of conditions + messages?

But I worry this might be too abstracted. We have different rules for standard and limitless database initializations, and offering flexibility in an abstraction might make a generalized function more difficult to use. Happy to hear your thoughts on this.

mrgrain · 2025-01-13T21:04:54Z

For your second point, are you suggesting to generalize validateDatabaseClusterProps to validateProps, and to take in a set of validation rules? In my current PR's implementation, the rules are generated using another function createDatabaseClusterValidationRules, but I suppose this could be made into createValidationRules and take in a set of conditions + messages?

If I understand the PR correctly, you are introducing a feature that allows a construct to declare a number if rules that can be validated independently from each other. If any of the rules fails, the construct will throw a validation error with all failing rules.

I think this a feature that would benefit other constructs as well and is worth generalizing.

But I worry this might be too abstracted. We have different rules for standard and limitless database initializations, and offering flexibility in an abstraction might make a generalized function more difficult to use. Happy to hear your thoughts on this.

That's a good concern, and we will have to make sure the API is easy enough to understand. I agree, this PR feels complex because you are starting from the list of all possible rules and then run some filters to only pick the applicable ones. That part I think can be kept local to the RDS DB Cluster.

…is interface for SQS and RDS props validation.

aws-cdk-automation · 2025-01-16T19:54:17Z

The pull request linter fails with the following errors:

❌ Features must contain a change to a README file.
❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

✅ A exemption request has been requested. Please wait for a maintainer's review.

aws-cdk-automation · 2025-01-16T19:55:17Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 2d0a107
Result: FAILED
Build Logs (available for 30 days)

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

iankhou self-assigned this Jan 10, 2025

aws-cdk-automation requested a review from a team January 10, 2025 19:07

github-actions bot added feature-request A feature should be added or improved. p2 labels Jan 10, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 10, 2025

aws-cdk-automation previously requested changes Jan 10, 2025

View reviewed changes

iankhou commented Jan 10, 2025

View reviewed changes

iankhou changed the title ~~test refactor to notify user of all validation issues when initializi…~~ feature(rds): test refactor to notify user of all validation issues when initializing a new RDS DB Cluster Jan 10, 2025

iankhou changed the title ~~feature(rds): test refactor to notify user of all validation issues when initializing a new RDS DB Cluster~~ test(rds): test refactor to notify user of all validation issues when initializing a new RDS DB Cluster Jan 10, 2025

iankhou marked this pull request as ready for review January 11, 2025 01:37

github-actions bot added the effort/small Small work item – less than a day of effort label Jan 11, 2025

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 11, 2025

iankhou changed the title ~~test(rds): test refactor to notify user of all validation issues when initializing a new RDS DB Cluster~~ test(rds): refactor to notify user of all validation issues when initializing a new RDS DB Cluster Jan 11, 2025

iankhou changed the title ~~test(rds): refactor to notify user of all validation issues when initializing a new RDS DB Cluster~~ feat(rds): refactor to notify user of all validation issues when initializing a new RDS DB Cluster Jan 13, 2025

aws-cdk-automation requested changes Jan 13, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 13, 2025

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Jan 13, 2025

mrgrain requested changes Jan 13, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 13, 2025

Create a shared internal interface for props validation. Implement th…

16787f0

…is interface for SQS and RDS props validation.

iankhou force-pushed the iankhou-rds-test-refactor branch from 8aef9e4 to 16787f0 Compare January 16, 2025 19:13

iankhou requested a review from a team as a code owner January 16, 2025 19:13

iankhou changed the title ~~feat(rds): refactor to notify user of all validation issues when initializing a new RDS DB Cluster~~ feat(rds + sqs): refactor to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue Jan 16, 2025

iankhou changed the title ~~feat(rds + sqs): refactor to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue~~ feat(rds,sqs): refactor to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue Jan 16, 2025

iankhou changed the title ~~feat(rds,sqs): refactor to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue~~ feat(validation): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue Jan 16, 2025

iankhou added 2 commits January 16, 2025 14:24

fix import order

098a5b2

Use ValidationError instead of Error

2d0a107

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(validation): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue #32841

feat(validation): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue #32841

iankhou commented Jan 10, 2025 •

edited

Loading

aws-cdk-automation left a comment

iankhou Jan 10, 2025 •

edited

Loading

iankhou Jan 10, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

aws-cdk-automation left a comment

iankhou commented Jan 13, 2025

mrgrain left a comment

iankhou commented Jan 13, 2025

mrgrain commented Jan 13, 2025

aws-cdk-automation commented Jan 16, 2025

aws-cdk-automation commented Jan 16, 2025

feat(validation): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue #32841

Are you sure you want to change the base?

feat(validation): shared interface to notify user of all validation issues when initializing a new RDS DB Cluster or SQS Queue #32841

Conversation

iankhou commented Jan 10, 2025 • edited Loading

Issue #32840

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

iankhou Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

iankhou Jan 10, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

aws-cdk-automation left a comment

Choose a reason for hiding this comment

iankhou commented Jan 13, 2025

mrgrain left a comment

Choose a reason for hiding this comment

iankhou commented Jan 13, 2025

mrgrain commented Jan 13, 2025

aws-cdk-automation commented Jan 16, 2025

aws-cdk-automation commented Jan 16, 2025

AWS CodeBuild CI Report

iankhou commented Jan 10, 2025 •

edited

Loading

iankhou Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading