fix: Lambda timeout from parameter (#925)

[setrofim]

Issue #, if available:
#925

Description of changes:

This fixes the specific case used as the example in the issue of using parameters to specify the Timeout for Lambdas.

As per CloudFormation documentation (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/parameters-section-structure.html) the fact that the the Type of the parameter is ignored and the value is passed as a string appears to be expected behaviour. Therefore, I'm doing the numeric conversion just before passing the value to the Function.

Checklist:

• Write Design Document (Do I need to write a design document?)
• Write unit tests
• Write/update functional tests
• Write/update integration tests
• make pr passes
• Write documentation

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

[sriram-mv]

are you not able pass timeout as parameter override with the current model? and thats you are evaluating Timeout?

Edit: checked with local invoke, this broke. Created a new timeout parameter, and ref'd it.

sam local invoke --parameter-overrides='ParameterKey=MyTimeOut,ParameterValue=20'

[setrofim]

Please could you provide details of what broke? I tried testing sam local invoke on my setup, but it seems to be working fine for me.

[sriram-mv]

@setrofim as in I was testing ref on a timeout parameter without applying this PR patch.

[setrofim]

Ah I see. Sorry, I misunderstood.

[sriram-mv]

Can we add an integ test for this as well, happy to merge it after its done.

[setrofim]

I have added an integration test for this (and confirmed that it fails without the fix).

[sriram-mv]

👍

[jfuss]

Why ast here instead of doing an int() conversation?

In either case if the string can't be casted into a int, ast.literal_eval will throw SyntaxError and int(timeout) will throw ValueError. We should be catching and handling them gracefully here.

[setrofim]

I did an ast.literal_eval here in case the value is a float (as the parameter type is Number). I shall catch and re-raise as ValueError -- thanks!

[setrofim]

I have made the change. It looks like now appveyor tests are failing, however, looking a the errors, I don't believe it's related to my change? (Seems to be a permissions issue.)

[setrofim]

@thesriram @jfuss is there anything else required from me for this?

[sriram-mv]

No, I dont think so, I re-kicked the build now.

[jfuss]

This will raise a ValueError up the chain and wont be caught. This will show up as a command failure with a stack track. We should raise a custom error/exception here to avoid that and provide a better customer experience.

[setrofim]

Is there an existing exception used for this? Is there an existing convention for how this is handled in the codeline?

[jfuss]

@setrofim You can follow what we are currently doing for Invalid Layer ARNs.

We throw there and the exception is defined here

There is currently an exception for InvalidSamTemplateException. This might be sufficient here with a good error message (what you currently have is fine). You can define a more specific to improve code readability but either works.

When defining exceptions, make sure the inherit from UserException. This will make sure the correct information is added to the exception so Click will work as expected (no stack trace and printing the error message to console). Only other this is to just update your current unit test to expect the new exception instead of ValueError.

[setrofim]

Sorry for the delayed response. I have now made the change -- I stuck with InvalidSamTemplateException as that see to cover this case.

[setrofim]

Are there any outstanding issues with this?