-
Notifications
You must be signed in to change notification settings - Fork 2.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
fix: redeploy api when function name changes #1235
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1235 +/- ##
===========================================
+ Coverage 94.41% 94.42% +<.01%
===========================================
Files 78 78
Lines 4529 4554 +25
Branches 903 912 +9
===========================================
+ Hits 4276 4300 +24
Misses 119 119
- Partials 134 135 +1
Continue to review full report at Codecov.
|
d4184e5
to
8bd6ce0
Compare
@@ -31,6 +31,39 @@ def __init__(self, managed_policy_map, sam_parser, plugins=None): | |||
self.plugins = plugins | |||
self.sam_parser = sam_parser | |||
|
|||
function_names = dict() | |||
|
|||
def _get_function_names(self, resource_dict, parameter_values): |
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.
This function appends all the function names of an API as an API can have multiple functions. If atleast one function name changes, the api will be redeployed.
for item in events: | ||
# If the function event type is `Api`then gets the function name and | ||
# adds to the function_names dict with key as the api_name and value as the function_name | ||
if item.get('Type', "") == 'Api' and item.get('Properties', None) and item.get('Properties', "")\ |
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 don't need the second none/"" argument in 'get', that's the default behavior
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.
Thank you. I have updated the code without ""/None
# Resolving intrinsics and gets the function name | ||
# This gets the function names only for Ref and Fn::Sub intrinsics | ||
elif type(function_name) == dict: | ||
if function_name.get('Ref'): |
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.
This case works for !Ref FunctionParameter
if function_name.get('Ref'): | ||
self.function_names[api_name] = self.function_names.get(api_name, "") + \ | ||
parameter_values.get(function_name.get('Ref')) | ||
if function_name.get('Fn::Sub'): |
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.
This resolves Fn::Sub: FunctionName
to a string
74f7b17
to
0a11eaf
Compare
function_names = redeploy_restapi_parameters.get('function_names') | ||
else: | ||
function_names = None | ||
if function_names and function_names.get(self.logical_id[:-10], None): |
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.
Can you remind me again why self.logical_id[:-10]
is needed here? Maybe add a comment about what you're trying to do?
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.
Sure, I will add a comment. self.logical_id
has the api_name and the word "deployment". I am removing the word "deployment" and just getting the api_name to search for corresponding string of function_name/names
@@ -31,6 +31,36 @@ def __init__(self, managed_policy_map, sam_parser, plugins=None): | |||
self.plugins = plugins | |||
self.sam_parser = sam_parser | |||
|
|||
def _get_function_names(self, resource_dict, parameter_values): |
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.
Is it not possible to use the intrinsic resolver? This looks like a lot of duplicated effort.
Intrinsic resolver does additional things, like resolving "!Sub ${parameter}-name", which this doesn't appear to handle.
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 will check and get back if i can call intrinsics resolver here.
27e73d7
to
8462ead
Compare
8462ead
to
9205f27
Compare
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.
Nice! Thanks for this Shreya!
This is still an issue if you reference the RestApiId via a Ref and have a FunctionName with a Sub in it. The OrderDict object seems to be more specific than needs be and templates can still fall into the else statement under the right conditions. I've made a branch for a change that will resolve the issue. |
Issue #, if available:
#634
Description of changes:
Updated the code to take function name when redeploying api
This PR resolves only
Ref
andFn::Sub
intrinsics for parameter valuesDescription of how you validated changes:
added a test which updates the deployment hash when functionname changes
Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.