-
Notifications
You must be signed in to change notification settings - Fork 163
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
removing duplicate module validation caught by cfn-lint #750
base: master
Are you sure you want to change the base?
Conversation
def __validate_resources(self, raw_fragments): | ||
if "Resources" not in raw_fragments: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
fbf65a6
to
8093414
Compare
The |
That validation should already exist, Maria. I had added that a while ago. |
I had an old version of the CFN CLI |
@MalikAtalla-AWS @PatMyron does this change still apply and/or what rework is needed here? |
My comments are addressed, but the build is failing @mircealam |
looks like that test was removed in #1028 already |
#634
#644
left module-specific validation like restrictions on exports, macros/transforms, and nested stacks