-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
(feat) CloudFormation AWS::ApiGateway::Stage Support #1239
(feat) CloudFormation AWS::ApiGateway::Stage Support #1239
Conversation
…ider Modularize RestApi code
The Api object now contains a list of routes and the attributes associated with it. The rest of the code is also managed in terms of routes/endpoints. This will allow for easy extensability if the Apis will grouped into different sections
…ures Restructure the information Api has the way it passed through
…ksrivat/aws-sam-cli into feature/cloud_formation_stage_support
properties = api_resource.get("Properties", {}) | ||
stage_name = properties.get("StageName") | ||
stage_variables = properties.get("Variables") | ||
logical_id = properties.get("RestApiId") |
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.
What if the logical Id does not equal the RestApi Resource this is being attached too?
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.
The only other case to worry about I believe is dealing with intrinsic with Ref, which will be fixed in the other pr. Other than that, I'm not sure if I need to check if the logical_id is in the template and bubble up the error if so.
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.
No. What I mean is, if the logical_id does not match what the RestApi Logical ID is. Then this stage does not belong to that Api but you will still attach the stage to the api, which seems wrong.
}) | ||
|
||
def test_multi_stage_get_all(self): | ||
template = OrderedDict({ |
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 we just statically define this like the other tests? This is much harder to read and understand what the content of template
is. Also remains consistent with all the other tests we have here.
with self.assertRaises(NoApisDefined): | ||
local_service.start() | ||
|
||
|
||
class TestLocalApiService_make_routing_list(TestCase): | ||
|
||
def test_must_return_routing_list_from_apis(self): |
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.
Why was this tests removed? Do we have similar coverage if this was refactored out?
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.
An Api is now defined as a list of routes and it's attributes. All the function was doing was converting a list of Apis -> list of Routes, which isn't needed in the current model as it is directly passed through.
@@ -181,39 +154,3 @@ def test_must_return_none_if_path_not_exists(self, os_mock): | |||
|
|||
result = LocalApiService._make_static_dir_path(cwd, static_dir) | |||
self.assertIsNone(result) | |||
|
|||
|
|||
class TestRoutingList(TestCase): |
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.
Similar here. Why was this removed? Do we still have this covered in other areas.
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.
Similar reasoning to the other comment
}, | ||
} | ||
} | ||
template = OrderedDict({ |
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.
define statically
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.
The problem with defining it statically is that when we parse the template, we use the order of the template to determine which stage to process. In python3 by default, dictionaries have order ,but in python2, there is no guarantee.
properties = api_resource.get("Properties", {}) | ||
stage_name = properties.get("StageName") | ||
stage_variables = properties.get("Variables") | ||
logical_id = properties.get("RestApiId") |
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.
No. What I mean is, if the logical_id does not match what the RestApi Logical ID is. Then this stage does not belong to that Api but you will still attach the stage to the api, which seems wrong.
LOG.debug("Found '%s' APIs in resource '%s'", len(routes), logical_id) | ||
|
||
collector.add_routes(logical_id, routes) | ||
for media_type in parser.get_binary_media_types() + binary_media: |
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't we just leave this how it was? I am not sure I understand the need for this to all change, mainly because this is more complex for the exact same result.
yield method.upper() | ||
else: | ||
yield http_method.upper() | ||
return AbstractApiProvider._ANY_HTTP_METHODS |
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.
Why did we change to return from yield?
binary_media_types=binary_media_types) | ||
result.append(api) | ||
|
||
route = AbstractApiProvider.get_normalized_route(function_name=function_name, path=full_path, |
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.
why not just normalize the route method and then create the route here?
result.append(api._replace(method=normalized_method)) | ||
|
||
return result | ||
return Route(function_name, path, methods=AbstractApiProvider.normalize_http_methods(method)) |
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 use to return a list of routes that were normalized. Why is this moved to making a singular route?
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.
Before, each Api had a single method with the format Api(method="post"), Api(method="get"), which was then simplified later in the pipeline into Route(methods=["post","get"]). In the context of this function, all it's doing is converting ApiGateway specific types like "Any" into a list of methods within a single methods.
|
||
result = list(merged_route_methods.values()) | ||
return list(result) |
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 are doing a list of a list here. This can be combined with the line above.
|
||
result = list(merged_route_methods.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.
Not sure this belongs here. This method is to combine implicit and explicit apis. This changes the scope of the funciton. While this isn't wrong, it doesn't make the code as readable as it could. For example, we already do this function name + path when we print out routes in local_api_service.py. We should find a better how to do this deduping.
@@ -64,12 +63,12 @@ def _extract_apis(self, resources): | |||
The dictionary containing the different resources within the template | |||
Returns | |||
--------- | |||
list of Apis extracted from the resources | |||
The modified api | |||
""" |
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.
there is not modification in this method. It is just returning the api that is 'collected/parsed'
@@ -242,13 +233,6 @@ class AbstractApiProvider(object): | |||
""" | |||
Abstract base class to return APIs and the functions they route to |
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.
property?
Instance of the API collector that where we will save the API information | ||
|
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.
Make sure all docstrings are updated. api is not a param and collector should be ApiCollector instead of RouteCollector
|
||
def __hash__(self): | ||
route_hash = hash(self.function_name) * hash(self.path) | ||
for method in self.methods: |
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.
order matters here. If we really need hash, we should sort methods.
|
||
LOG = logging.getLogger(__name__) | ||
|
||
|
||
class CfnBaseApiProvider(object): | ||
RESOURCE_TYPE = "Type" | ||
|
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 isn't needed anymore right?
from samcli.commands.local.lib.api_provider import ApiProvider | ||
from samcli.commands.local.lib.sam_api_provider import SamApiProvider | ||
from samcli.commands.local.lib.cfn_api_provider import CfnApiProvider | ||
|
||
|
||
class TestApiProvider_init(TestCase): | ||
|
||
@patch.object(ApiProvider, "_extract_apis") |
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.
remove print statements.
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.
A couple small comments. This looks really good, thanks for keeping up with this.
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.
Great work. I know this took some time and lots of back and forth but the end result looks clean! 🎉
Issue #, if available:
Description of changes:
Design Doc: #1234
AWS:ApiGateway::RestApi pr: #1238
Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.