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
Automatically check if zip_file exceeds 4096 chars #537
Conversation
CloudFormation doesn't allow to specify more than 4096 characters in ZipFile. Lets do check locally instead of failing when template is deployed in Amazon.
@@ -38,6 +38,10 @@ def validate(self): | |||
raise ValueError( | |||
"You can't specify both 'S3ObjectVersion' and 'ZipFile'" | |||
) | |||
if zip_file and len(zip_file) > 4096: |
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.
Did you think about how this would work if you write this with a Join? I'll give an example below. I think this check is great if zip_file
is a string, but we shouldn't assume it is.
ZipFile=Join('', [
"var x = '", Ref(InputXParam), "';\n",
"var y = '", Ref(InputYParam), "';\n",
rest_of_code_var,
])
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.
You are correct. In this case there is not way to check it locally as there could be any Ref, or GetAttr etc. So what if we run this check on strings only?
Maybe also on simple cases like Join('', [ ...]) where all elements are strings and we can actually calculate the size. What do you think?
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 think running on strings only is a good solution for this problem. I like the iteration over the join to see if they are strings too.
@themalkolm : I think this is adding great value! I've added a few minor remarks as line comments. |
It is a nice idea as it saves tons of time for me already. I'm trying to fit lambdas into templates all the time to avoid hassle with s3 deployment (there is no s3object or anything similar in CloudFormation, right?). |
@themalkolm @ikben I just pushed a new branch (lambda-zip-file) with some tweaks to this PR. I think it addresses the above code review comments. |
@markpeek aha, thank you for taking this over! Will drop my comments. |
This change and some cleanups were pushed to master. Thanks for the PR! |
CloudFormation doesn't allow to specify more than 4096 characters in ZipFile. Lets
do check locally instead of failing when template is deployed in Amazon.
http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-lambda-function-code.html