-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: redeploy api when function name changes #1235
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
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
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.
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
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
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
| 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
RefandFn::Subintrinsics for parameter valuesDescription of how you validated changes:
added a test which updates the deployment hash when functionname changes
Checklist:
make prpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.