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 Spec file and test for properties Allowed Values #259

Closed

Conversation

fatbasstard
Copy link
Contributor

@fatbasstard fatbasstard commented Aug 7, 2018

Issue #50

Introduce new rule E3030 to check allowed values as mentioned in the documentation. Properties sometimes have a list of "allowed values" (e.g. https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-vpc.html#cfn-aws-ec2-vpc-instancetenancy).

These values are not in the Spec files, and probably will never be. This PR will add a new Additional Spec file (AllowedValues.json) which contains these allowed values in the same format as the Spec files itself, and the other Additional Spec files. In this way all Allowed Values are maintained in a single place which makes more transparent, easier to maintain and remove this logic from current (existing) rules.

This is a first version (MVP) that introduces the new Rule and the definition file. The definition file contains a first set of known allowed values. Also "moved" some of the existing rule logic already (including altering/moving the test logic).

It's an MVP rule at the moment that is open for extension/improvement when required. By adding it already we can move move allowed values in the future from rules and more values can be added to the definition.

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

@cmmeyer cmmeyer added the WIP work in progress -- DO NOT MERGE label Sep 4, 2018
@fatbasstard fatbasstard force-pushed the allowed_values branch 4 times, most recently from 33914cc to 6b3051e Compare October 17, 2018 14:59
@fatbasstard fatbasstard force-pushed the allowed_values branch 7 times, most recently from b5e2d4b to 3af4f81 Compare October 23, 2018 18:30
@fatbasstard fatbasstard force-pushed the allowed_values branch 10 times, most recently from 1e61238 to b83d0a4 Compare November 12, 2018 13:16
@kddejong kddejong removed the WIP work in progress -- DO NOT MERGE label Nov 12, 2018
Copy link
Contributor

@kddejong kddejong left a comment

Choose a reason for hiding this comment

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

I love these types of rules as it allows for making new checks a lot easier.

We should remove rule E2531 as well as that should be covered by this.

# the required properties rule (E3003)
if prop in value:

if not isinstance(value[prop], dict):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use the cfn.check_value function to check the value against the supported lists.
With this version Runtime: !If [myCondition, nodejs4.3, nodejs4.4] will not show a failure.

Copy link
Contributor Author

@fatbasstard fatbasstard Nov 14, 2018

Choose a reason for hiding this comment

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

Funny detail BTW. This example breaks the existing Runtime check (E2531 ):

E0002 Unknown exception while processing rule E2531: sequence item 5: expected str instance, int found
test/fixtures/templates/bad/properties_allowedvalues.yaml:1:

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea that is covered by pull request #457


return matches

def match(self, cfn):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are using match instead of the resource and property match functions this isn't providing a failure.

myBucketGood:
    Type: AWS::S3::Bucket
    Properties:
      VersioningConfiguration:
        Status: Bad

As an example.
https://github.com/awslabs/cfn-python-lint/pull/234/files#diff-dd307d557b713eb258c15a2607351227R203

@cmmeyer
Copy link
Contributor

cmmeyer commented Nov 13, 2018

So looking at this and #234 I wonder if we need two separate files? Seems like the only difference is one is for string values and the other is for resource values, unless I missed something?

Would it make sense to consolidate to a single extended spec file, or even patch the existing specs to include value?

Improve the test file with some more exotic use cases
@fatbasstard
Copy link
Contributor Author

Replaced by a new PR that uses the new Spec Patch mechanism

@fatbasstard fatbasstard deleted the allowed_values branch January 2, 2019 09:34
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.

None yet

3 participants