-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Validate the template against aws before stack creation #485
Validate the template against aws before stack creation #485
Conversation
Friendly ping - Hoping to get a review on this PR. |
(literal_eval(e.error_message)['Error']['Message'],)) | ||
print("Exiting...") | ||
sys.exit(1) | ||
print("Template Validated") |
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.
What happens if both template
and url
are None
? Wont this return "Template Validated"? This does not seem like a desired result.
What about returning True
or False
? It would make the function more flexible. The code that calls it could then decide what to do in either case.
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.
I like these ideas, I'll update this and get it back to you.
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.
Anything that validates returns a boto.cloudformation.template.Template
object. If it does not validate it will throw an error:
BotoServerError: BotoServerError: 400 Bad Request
<ErrorResponse xmlns="http://cloudformation.amazonaws.com/doc/2010-05-15/">
<Error>
<Type>Sender</Type>
<Code>ValidationError</Code>
<Message>Template format error: unsupported type or structure. (line 2, column 5)</Message>
</Error>
<RequestId>aacaaf5b-2442-11e6-96bc-4793db5d2621</RequestId>
</ErrorResponse>
The style of this script is that any error causes a fault. We could cause this method to return True
or False
, but it steps away from the style of the other methods. Your call, I'm happy to implement either.
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.
Also, I just updated to cause an error if neither url
or template
are provided.
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.
Good point about not wanting to loose the error message.
Out of scope for this PR, but there is something wrong with the way the logging
feature and print
is used in this script over all, but I'm not sure what it is.
These changes LGTM. 👍 Please note, that I did not run the script to test it. Just looked at the changes. |
Fantastic. I can't merge this so I'll leave it up to your team to do so. Thanks for the review! |
Thanks for the PR! @rcuza thanks for the code review feedback |
Working with @markfalk discovered its helpful to have AWS validate the template before stack creation. This change includes these updates while maintaining backwards compatibility: