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

add rule E3017 to check when a prop is required based on a value #1746

Merged

Conversation

kddejong
Copy link
Contributor

@kddejong kddejong commented Oct 22, 2020

Issue #, if available:
#1744
Description of changes:

  • Add rule E3017 to validate when a property is required based on another property having a certain value

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

@kddejong kddejong force-pushed the fix/1744/requiredbasedonvalue branch 3 times, most recently from b6e36f3 to 830e27a Compare October 22, 2020 17:23
@PatMyron
Copy link
Contributor

PatMyron commented Oct 22, 2020

Think investigating similar rules/requests will be helpful:
CloudWatch, DynamoDB, ECS, EC2, Lambda, RDS, RDS, Route53, Route53, Route53, StepFunctions

Something to note is that quite a few of them are the opposite: a certain property cannot be specified based on the value of another property. Might be useful to support both formats:

# just example possibilities for discussion
not StartingPosition if AWS::Lambda::EventSourceMapping.EventSourceArn == arn:aws*:(kinesis|kafka|dynamodb):*
!StartingPosition if AWS::Lambda::EventSourceMapping.EventSourceArn == arn:aws*:(kinesis|kafka|dynamodb):*

this is also very similar to cfn-guard WHEN checks (aws-cloudformation/cloudformation-guard#52)

src/cfnlint/data/AdditionalSpecs/RequiredBasedOnValue.json Outdated Show resolved Hide resolved
"AWS::Lambda::EventSourceMapping": {
"EventSourceArn": [
{
"Regex": "arn:aws:(kinesis|kafka|dynamodb):*",
Copy link
Contributor

Choose a reason for hiding this comment

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

missing support for other partitions

Suggested change
"Regex": "arn:aws:(kinesis|kafka|dynamodb):*",
"Regex": "arn:aws*:(kinesis|kafka|dynamodb):*",

Copy link
Contributor

@PatMyron PatMyron Oct 30, 2020

Choose a reason for hiding this comment

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

@kddejong sorry, got confused by that trailing wildcard. Should be something like this instead:

+                    "Regex": "arn:aws.*:(kinesis|kafka|dynamodb):",
+                    "Regex": "arn:aws.*:(kinesis|kafka|dynamodb):.*",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Resolved it here #1759

@kddejong kddejong force-pushed the fix/1744/requiredbasedonvalue branch 4 times, most recently from 13d4fc6 to 22b1412 Compare October 22, 2020 20:48
@kddejong kddejong requested a review from PatMyron October 26, 2020 17:49
@kddejong kddejong force-pushed the fix/1744/requiredbasedonvalue branch from c27aae5 to 911f0f3 Compare October 26, 2020 18:32
@kddejong
Copy link
Contributor Author

@PatMyron I'm going to look at another rule for doing the opposite check because I'm finding use cases where it could be valuable.

Co-authored-by: Pat Myron <PatMyron@users.noreply.github.com>
@kddejong kddejong merged commit f48b141 into aws-cloudformation:master Oct 29, 2020
@kddejong kddejong deleted the fix/1744/requiredbasedonvalue branch October 29, 2020 21:33
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.

2 participants