Skip to content
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

Merged
merged 8 commits into from
Dec 27, 2019

Conversation

ShreyaGangishetty
Copy link

Issue #, if available:
#634
Description of changes:
Updated the code to take function name when redeploying api
This PR resolves only Ref and Fn::Sub intrinsics for parameter values

Description of how you validated changes:
added a test which updates the deployment hash when functionname changes

Checklist:

  • Write/update tests
  • make pr passes
  • Verify transformed template deploys and application functions as expected

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

@ShreyaGangishetty ShreyaGangishetty changed the title Redeploy api hash when function name changes fix: redeploy api hash when function name changes Nov 7, 2019
@codecov-io
Copy link

codecov-io commented Nov 7, 2019

Codecov Report

Merging #1235 into develop will increase coverage by <.01%.
The diff coverage is 96.77%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ
samtranslator/model/apigateway.py 97.95% <100%> (+0.05%) ⬆️
samtranslator/model/api/api_generator.py 95.13% <100%> (ø) ⬆️
samtranslator/model/sam_resources.py 93.99% <100%> (+0.01%) ⬆️
samtranslator/translator/translator.py 98.37% <94.73%> (-0.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba4ac42...a417034. Read the comment docs.

@ShreyaGangishetty ShreyaGangishetty changed the title fix: redeploy api hash when function name changes fix: redeploy api when function name changes Nov 7, 2019
@praneetap praneetap self-assigned this Nov 8, 2019
@@ -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):
Copy link
Author

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', "")\
Copy link
Contributor

@praneetap praneetap Nov 12, 2019

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

Copy link
Author

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'):
Copy link
Author

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'):
Copy link
Author

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

function_names = redeploy_restapi_parameters.get('function_names')
else:
function_names = None
if function_names and function_names.get(self.logical_id[:-10], None):
Copy link
Contributor

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?

Copy link
Author

@ShreyaGangishetty ShreyaGangishetty Dec 9, 2019

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):
Copy link
Contributor

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.

Copy link
Author

@ShreyaGangishetty ShreyaGangishetty Dec 9, 2019

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.

Copy link
Contributor

@praneetap praneetap left a 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!

@praneetap praneetap merged commit 40d2d79 into aws:develop Dec 27, 2019
@patrickgreenwell
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants