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.

Sign up for GitHub

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

removing duplicate module validation caught by cfn-lint #750

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

removing duplicate module validation caught by cfn-lint #750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
73df410
removing duplicate module validation caught by cfn-lint
PatMyron May 3, 2021
5520e05
removing no longer needed tests
PatMyron May 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 1 addition & 95 deletions src/rpdk/core/fragment/generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,30 +15,20 @@
from rpdk.core.exceptions import FragmentValidationError

from .lint_warning_printer import print_cfn_lint_warnings
from .module_fragment_reader import get_template_file_size_in_bytes, read_raw_fragments
from .module_fragment_reader import read_raw_fragments

LOG = logging.getLogger(__name__)
FRAGMENT_DIR = "fragments"
SAMPLE_FRAGMENT_OUTPUT = "sample.json"
SCHEMA_NAME = "schema.json"
SAMPLE_FRAGMENT = "../data/examples/module/sample.json"
RESOURCE_LIMIT = 500
OUTPUT_LIMIT = 200
MAPPING_LIMIT = 200
MAPPING_ATTRIBUTE_LIMIT = 200
TEMPLATE_FILE_SIZE_IN_BYTES_LIMIT = 1500000


class TemplateFragment: # pylint: disable=too-many-instance-attributes
def __init__(self, type_name, root=None):
self.root = Path(root) if root else Path.cwd()
self.fragment_dir = self.root / FRAGMENT_DIR
self.type_name = type_name
self.resource_limit = RESOURCE_LIMIT
self.output_limit = OUTPUT_LIMIT
self.mapping_limit = MAPPING_LIMIT
self.mapping_attribute_limit = MAPPING_ATTRIBUTE_LIMIT
self.template_file_size_in_bytes_limit = TEMPLATE_FILE_SIZE_IN_BYTES_LIMIT

LOG.debug("Fragment directory: %s", self.fragment_dir)

Expand Down Expand Up @@ -69,17 +59,13 @@ def validate_fragments(self):
since it can occur anywhere in the template.
"""
raw_fragments = read_raw_fragments(self.fragment_dir)
self.__validate_file_size_limit()
self.__validate_resources(raw_fragments)
self.__validate_parameters(raw_fragments)
self.__validate_no_transforms_present(raw_fragments)
self.__validate_outputs(raw_fragments)
self.__validate_mappings(raw_fragments)
print_cfn_lint_warnings(self.fragment_dir)

def __validate_outputs(self, raw_fragments):
self.__validate_no_exports_present(raw_fragments)
self.__validate_output_limit(raw_fragments)

@staticmethod
def __validate_no_exports_present(raw_fragments):
Expand All @@ -91,24 +77,7 @@ def __validate_no_exports_present(raw_fragments):
"Found an Export statement in Output: " + _output_name
)

def __validate_output_limit(self, raw_fragments):
if "Outputs" in raw_fragments:
output_count = len(raw_fragments["Outputs"].items())
if output_count > self.output_limit:
raise FragmentValidationError(
"The Module template fragment has "
+ str(output_count)
+ " outputs but must not exceed the limit of "
+ str(self.output_limit)
+ " outputs"
)

def __validate_resources(self, raw_fragments):
if "Resources" not in raw_fragments:
Copy link
Contributor

Choose a reason for hiding this comment

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

Validations like this have been added to avoid parsing errors that are impossible to understand by the user. I'm not sure if we're regressing on the user experience here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cfn-lint will throw E1001 Missing top level template section Resources for this validation

Copy link
Contributor

Choose a reason for hiding this comment

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

@PatMyron For all the validations that you removed in this PR can you show us what it looks like right now and what it will look like after? Wanna make sure that the error messages in cfn-lint are on par with the ones in the CLI

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that cfn lint is executed after these validations and that some validations require others. For example this line checks that the Resources section exists, but there are other validations further down which assume that this validation has already happened. So, I suspect that in this case the user would only see some kind of null pointer exception instead of a useful message. (which is why Maria asked for these cases to be tested)

raise FragmentValidationError(
"A Module template fragment must have a Resources section"
)
self.__validate_resource_limit(raw_fragments)
for _resource_name, resource in raw_fragments["Resources"].items():
if "Type" in resource:
self.__validate_no_nested_stacks(resource)
Expand All @@ -118,10 +87,6 @@ def __validate_resources(self, raw_fragments):
raise FragmentValidationError(
"Resource '" + _resource_name + "' is invalid"
)
else:
raise FragmentValidationError(
"Resource '" + _resource_name + "' has neither Type nor Name"
)

@staticmethod
def __validate_no_include(resource):
Expand All @@ -142,26 +107,6 @@ def __validate_no_nested_stacks(resource):
"Template fragment can't contain nested stack."
)

def __validate_resource_limit(self, raw_fragments):
resource_count = len(raw_fragments["Resources"].items())
if resource_count > self.resource_limit:
raise FragmentValidationError(
"The Module template fragment has "
+ str(resource_count)
+ " resources but must not exceed the limit of "
+ str(self.resource_limit)
+ " resources"
)

@staticmethod
def __validate_parameters(raw_fragments):
if "Parameters" in raw_fragments:
for _parameter_name, parameter in raw_fragments["Parameters"].items():
if "Type" not in parameter:
raise FragmentValidationError(
"Parameter '" + _parameter_name + "' must have a Type"
)

@staticmethod
def __validate_no_transforms_present(raw_fragments):
if "transform" in raw_fragments or "Transform" in raw_fragments:
Expand All @@ -173,45 +118,6 @@ def __validate_no_transforms_present(raw_fragments):
"Template fragment can't contain any transform."
)

def __validate_mappings(self, raw_fragments):
self.__validate_mapping_limit(raw_fragments)
self.__validate_mapping_attribute_limit(raw_fragments)

def __validate_mapping_limit(self, raw_fragments):
if "Mappings" in raw_fragments:
mapping_count = len(raw_fragments["Mappings"].items())
if mapping_count > self.mapping_limit:
raise FragmentValidationError(
"The Module template fragment has "
+ str(mapping_count)
+ " mappings but must not exceed the limit of "
+ str(self.output_limit)
+ " mappings"
)

def __validate_mapping_attribute_limit(self, raw_fragments):
if "Mappings" in raw_fragments:
for _mapping_name, mapping in raw_fragments["Mappings"].items():
attribute_count = len(mapping.items())
if attribute_count > self.mapping_attribute_limit:
raise FragmentValidationError(
"The mapping "
+ _mapping_name
+ " has "
+ str(attribute_count)
+ " attributes but must not exceed the limit of "
+ str(self.output_limit)
+ " mapping attributes"
)

def __validate_file_size_limit(self):
total_size = get_template_file_size_in_bytes(self.fragment_dir)
if total_size > self.template_file_size_in_bytes_limit:
raise FragmentValidationError(
"The total file size of the template"
" fragments exceeds the CloudFormation Template size limit"
)

@staticmethod
def __build_resources(raw_fragments):
raw_resources = {}
Expand Down
4 changes: 0 additions & 4 deletions src/rpdk/core/fragment/module_fragment_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ def read_raw_fragments(fragment_dir):
return _load_fragment(_get_fragment_file(fragment_dir))


def get_template_file_size_in_bytes(fragment_dir):
return os.stat(_get_fragment_file(fragment_dir)).st_size


def _load_fragment(fragment_file):
try:
with open(fragment_file, "r", encoding="utf-8") as f:
Expand Down
21 changes: 0 additions & 21 deletions tests/data/sample_fragments/fragment_mapping_with_three_attributes.json
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions tests/data/sample_fragments/fragment_three_mappings.json
View file
Open in desktop

This file was deleted.

19 changes: 0 additions & 19 deletions tests/data/sample_fragments/fragment_three_outputs.json
View file
Open in desktop

This file was deleted.

14 changes: 0 additions & 14 deletions tests/data/sample_fragments/fragment_three_resources.json
View file
Open in desktop

This file was deleted.

10 changes: 0 additions & 10 deletions tests/data/sample_fragments/noresources.json
View file
Open in desktop

This file was deleted.

23 changes: 0 additions & 23 deletions tests/data/sample_fragments/parameter_without_type.json
View file
Open in desktop

This file was deleted.

13 changes: 0 additions & 13 deletions tests/data/sample_fragments/resource_without_type_or_name.json
View file
Open in desktop

This file was deleted.

Loading