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

Call cfn-lint for module fragments #644

Merged
merged 4 commits into from
Dec 14, 2020

Conversation

miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Dec 9, 2020

Issue #, if available: N/A

Description of changes: when calling cfn validate on a module, run cfn-lint on the fragment for extra validations.

(Future improvement: support cfn validate --region so that we can pass the region value to cfn-lint. Related: aws-cloudformation/cfn-lint#607)

Sample output:

MacBook@ ~/Desktop/modules/cfnlint $ cat fragments/sample.json
{
    "AWSTemplateFormatVersion": "2010-09-09",
    "Resources": {
        "S3Bucket": {
            "Type": "AWS::S3::Bucket",
            "DeletionPolicy": "Something",
            "UpdateReplacePolicy": "Retain"
        }
    }
}
MacBook@ ~/Desktop/modules/cfnlint $ cfn validate
Module fragment is probably valid, but there are warnings/errors from cfn-lint (https://github.com/aws-cloudformation/cfn-python-lint):
	DeletionPolicy should be only one of Delete, Retain, Snapshot at Resources/S3Bucket/DeletionPolicy (from rule E3035: Check DeletionPolicy values for Resources)
MacBook@ ~/Desktop/modules/cfnlint $ cat fragments/sample.json
{
    "AWSTemplateFormatVersion": "2010-09-09",
    "Resources": {
        "S3Bucket": {
            "Type": "AWS::S3::Bucket",
            "DeletionPolicy": "Retain",
            "UpdateReplacePolicy": "Retain"
        }
    }
}
MacBook@ ~/Desktop/modules/cfnlint $ cfn validate
Module fragment is valid.

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

setup.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
LOG.warning("Module fragment is valid.")
else:
LOG.warning(
"Module fragment is valid, but there are warnings from cfn-lint "
Copy link
Contributor

@PatMyron PatMyron Dec 9, 2020

Choose a reason for hiding this comment

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

  1. We don't really know the module fragment is valid yet, right?

  2. Worth distinguishing terminology from one of cfn-lints rule levels?

Suggested change
"Module fragment is valid, but there are warnings from cfn-lint "
"Module fragment might be valid, but there are warnings from cfn-lint "

Copy link
Contributor

Choose a reason for hiding this comment

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

will this only appear if the user running validate specifies increased verbosity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PatMyron right. I'll edit both.

@jotompki no, for modules this runs all the time.

@PatMyron
Copy link
Contributor

PatMyron commented Dec 9, 2020

Future improvement: support cfn validate --region so that we can pass the region value to cfn-lint

Other options like --registry-schemas and --include-checks I come to mind as well

README.md Outdated Show resolved Hide resolved
LOG.warning("Module fragment is valid.")
else:
LOG.warning(
"Module fragment is valid, but there are warnings from cfn-lint "
Copy link
Contributor

Choose a reason for hiding this comment

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

will this only appear if the user running validate specifies increased verbosity?

with open(filename, "w") as outfile:
json.dump(raw_fragment, outfile, indent=4)

template = cfnlint.decode.cfn_json.load(filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use the yaml loads() so we don't have to write to a temporary file: https://github.com/aws-cloudformation/cfn-python-lint/blob/master/src/cfnlint/decode/cfn_yaml.py#L183?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that cfnlint.core.run_checks needs a filename to run. I haven't seen an api that validates a template in memory.

Copy link
Contributor

Choose a reason for hiding this comment

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

is filename being used anywhere?

if it's really necessary recommend: https://docs.python.org/3/library/tempfile.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jotompki yep, it's used by cfnlint.core.run_checks. I tried using NamedTemporaryFile, I don't see the benefit, I have to write the file, then read it back, and then manually remove it?

@miparnisari miparnisari requested a review from kddejong December 9, 2020 19:38
@kddejong
Copy link
Contributor

This is great to see. One of the things that may be worth adding as a rule (Warning level) is using parameters in conditions inside your module. Not sure if you would want to add custom rules but I got tripped up on this.

@miparnisari
Copy link
Contributor Author

This is great to see. One of the things that may be worth adding as a rule (Warning level) is using parameters in conditions inside your module. Not sure if you would want to add custom rules but I got tripped up on this.

@kddejong this can be added as an extra validation in RPDK, right? @MalikAtalla-AWS has context on all the validations were are currently doing.

@miparnisari miparnisari merged commit b01caae into aws-cloudformation:master Dec 14, 2020
prerna-p pushed a commit to prerna-p/cloudformation-cli that referenced this pull request Dec 29, 2020
prerna-p pushed a commit to prerna-p/cloudformation-cli that referenced this pull request Dec 29, 2020
@PatMyron PatMyron linked an issue Jan 17, 2021 that may be closed by this pull request
@PatMyron PatMyron mentioned this pull request May 3, 2021
kddejong pushed a commit to kddejong/cloudformation-cli that referenced this pull request Oct 24, 2022
kddejong pushed a commit to kddejong/cloudformation-cli that referenced this pull request Oct 24, 2022
kddejong pushed a commit to kddejong/cloudformation-cli that referenced this pull request Oct 24, 2022
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.

cfn validate is not documented in the readme
5 participants