Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat (Add a check for props for all constructs) #177

Merged
merged 6 commits into from
May 14, 2021
Merged

feat (Add a check for props for all constructs) #177

merged 6 commits into from
May 14, 2021

Conversation

biffgaut
Copy link
Contributor

Issue #, if available: 176

Description of changes:
Currently every construct performs its own validation of inputs. This validation is usually consistent across constructs but is inconsistent across services. For instance, if you send an existing VPC and VPC Props to a construct it will throw an error, but if you send an existing Lambda function and Lambda function props to a construct then the construct will ignore the props silently.

We need to have a consistent way to check inputs that ensures consistency across constructs as well as across services. In the process we will settle on explicitly throwing an error when incompatible props are submitted (eg - props and an existing resource).

(this is internal issues T131 and T137)

Use Case
Proposed Solution
This will be accomplished with a single prop validation routine that is called at the beginning of every construct.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@biffgaut biffgaut requested a review from hnishar as a code owner May 11, 2021 17:15
@biffgaut biffgaut self-assigned this May 11, 2021
@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: aa28f33
  • Result: FAILED
  • Build Logs (available for 30 days)

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

import * as secretsmanager from "@aws-cdk/aws-secretsmanager";

export interface VerifiedProps {
dynamoTableProps?: dynamodb.TableProps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark all the variables readonly ?

let errorFound = false;

if (propsObject.dynamoTableProps && propsObject.existingTableObj) {
errorMessages += 'Cannot specify an existing DDB table AND DDB table props\n';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the error message more user friendly by saying "Either provide existingTableObj or dynamoTableProps, not both", similar feedback for the rest of error messages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grumble grumble...OK

existingBucketObj?: s3.Bucket,
bucketProps?: s3.BucketProps,

// topicsProps is an incorrect attribute used in event-rule-sns that
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create an issue to track this change and make the update before we flip the switch for this pattern to stable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought is we leave it for now, and when we rename the construct aws-eventsrule-sns in 2.0 we also fix this.

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: 9b18dc1
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: 71541d7
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: 4927f57
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: 27f7233
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-solutions-constructs-team
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: codebuildgithubautobuildPro-fkVQbXRiQi6A
  • Commit ID: bd5c830
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor Author

@biffgaut biffgaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things to update, mostly naming.

@hnishar hnishar linked an issue May 14, 2021 that may be closed by this pull request
2 tasks
@biffgaut biffgaut merged commit 33a9dd7 into main May 14, 2021
@biffgaut biffgaut deleted the T137 branch May 14, 2021 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistent Handling of Property Validation/Flagging Invalid Inputs
3 participants