Fix issue in updating iam role arn for deployments #363
Conversation
Previously the update_function took no account for updates to configuration with respect to IAM role arns. Now this value are plumbed through to the update_function method call.
The SAM packaging logic did not respect the case where the user supplies their own iam role.
This parallels the retry used in the create_function method that retries the creation of the Lambda function until Lambda can use the provided role.
Codecov Report
@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 90.69% 90.73% +0.03%
==========================================
Files 18 18
Lines 2139 2148 +9
Branches 274 276 +2
==========================================
+ Hits 1940 1949 +9
Misses 146 146
Partials 53 53
Continue to review full report at Codecov.
|
chalice/awsclient.py
Outdated
| @@ -90,22 +90,27 @@ def create_function(self, | |||
| kwargs['Timeout'] = timeout | |||
| if memory_size is not None: | |||
| kwargs['MemorySize'] = memory_size | |||
| return self._create_or_update_function_with_retries( | |||
| 'create_function', kwargs)['FunctionArn'] | |||
There was a problem hiding this comment.
- You can technically pass any client method here so the name seems too restrictive.
- We don't need getattr logic, it's always going to be a method we know statically. I'd prefer to update this to just pass the method obj directly. (
self._client('lambda').create_function)
|
Looks good, just had a comment about |
chalice/awsclient.py
Outdated
| self._client('lambda').create_function, kwargs)['FunctionArn'] | ||
|
|
||
| def _call_client_method_with_retries(self, method, kwargs): | ||
| # type: (Callable[..., Any], Dict[str, Any]) -> Dict[str, Any] |
There was a problem hiding this comment.
Can we tighten up the signature here? Callable that only takes kwargs and returns a dict of str, Any maybe? http://mypy.readthedocs.io/en/latest/kinds_of_types.html#extended-callable-types
There was a problem hiding this comment.
Right. Clients will always return a dict.
|
@jamesls I updated to make stricter type checking on the client method. Decided not to use the KwArg types as that would require pulling in mypy_extensions as a dependency. |
chalice/awsclient.py
Outdated
| self._create_or_update_function_with_retries( | ||
| 'update_function_configuration', kwargs) | ||
| self._call_client_method_with_retries( | ||
| self._client('lambda').update_function_configuration, kwargs) |
There was a problem hiding this comment.
You already create a lambda_client in the first line of this function, you shouldn't need to create it again here.
|
Updated to use already instantiated client. Travis passed as well. Merging. |
This fixes two issues related to providing an iam role:
iam_role_arnis added after the first deployment and is not used in the update.iam_role_arnwas straight up not even accounted for when it is a valid value that can be passed to the SAM template via the Role property.