(feat) CloudFormation AWS::ApiGateway::Stage Support #1239

viksrivat · 2019-06-24T21:58:32Z

Issue #, if available:

Description of changes:
Design Doc: #1234
AWS:ApiGateway::RestApi pr: #1238

Checklist:

Write Design Document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
Write documentation

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

…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

samcli/commands/local/lib/api_collector.py

jfuss · 2019-07-15T13:32:40Z

samcli/commands/local/lib/cfn_api_provider.py

+        properties = api_resource.get("Properties", {})
+        stage_name = properties.get("StageName")
+        stage_variables = properties.get("Variables")
+        logical_id = properties.get("RestApiId")


What if the logical Id does not equal the RestApi Resource this is being attached too?

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.

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.

samcli/commands/local/lib/cfn_api_provider.py

samcli/commands/local/lib/route_collector.py

jfuss · 2019-07-15T14:26:30Z

tests/unit/commands/local/lib/test_cfn_api_provider.py

+        })
+
+    def test_multi_stage_get_all(self):
+        template = OrderedDict({


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.

jfuss · 2019-07-15T14:28:50Z

tests/unit/commands/local/lib/test_local_api_service.py

        with self.assertRaises(NoApisDefined):
            local_service.start()


-class TestLocalApiService_make_routing_list(TestCase):
-
-    def test_must_return_routing_list_from_apis(self):


Why was this tests removed? Do we have similar coverage if this was refactored out?

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.

jfuss · 2019-07-15T14:29:38Z

tests/unit/commands/local/lib/test_local_api_service.py

@@ -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):


Similar here. Why was this removed? Do we still have this covered in other areas.

Similar reasoning to the other comment

tests/unit/commands/local/lib/test_sam_api_provider.py

jfuss · 2019-07-15T14:33:22Z

tests/unit/commands/local/lib/test_sam_api_provider.py

-                                        },
-                                    }
-                                }
+        template = OrderedDict({


define statically

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.

jfuss · 2019-07-18T21:24:39Z

samcli/commands/local/lib/cfn_api_provider.py

+        properties = api_resource.get("Properties", {})
+        stage_name = properties.get("StageName")
+        stage_variables = properties.get("Variables")
+        logical_id = properties.get("RestApiId")


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.

jfuss · 2019-07-18T21:27:35Z

samcli/commands/local/lib/cfn_base_api_provider.py

+        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:


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.

jfuss · 2019-07-18T21:39:16Z

samcli/commands/local/lib/provider.py

-                yield method.upper()
-        else:
-            yield http_method.upper()
+            return AbstractApiProvider._ANY_HTTP_METHODS


Why did we change to return from yield?

jfuss · 2019-07-18T21:41:26Z

samcli/commands/local/lib/swagger/parser.py

-                          binary_media_types=binary_media_types)
-                result.append(api)
-
+                route = AbstractApiProvider.get_normalized_route(function_name=function_name, path=full_path,


why not just normalize the route method and then create the route here?

jfuss · 2019-07-18T21:42:09Z

samcli/commands/local/lib/provider.py

-                result.append(api._replace(method=normalized_method))
-
-        return result
+        return Route(function_name, path, methods=AbstractApiProvider.normalize_http_methods(method))


This use to return a list of routes that were normalized. Why is this moved to making a singular route?

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.

jfuss · 2019-07-19T15:55:22Z

samcli/commands/local/lib/sam_api_provider.py


+        result = list(merged_route_methods.values())
        return list(result)


You are doing a list of a list here. This can be combined with the line above.

jfuss · 2019-07-19T15:55:34Z

samcli/commands/local/lib/sam_api_provider.py


+        result = list(merged_route_methods.values())


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.

jfuss · 2019-07-24T16:40:18Z

samcli/commands/local/lib/api_provider.py

@@ -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 is not modification in this method. It is just returning the api that is 'collected/parsed'

jfuss · 2019-07-24T17:01:58Z

samcli/commands/local/lib/provider.py

@@ -242,13 +233,6 @@ class AbstractApiProvider(object):
    """
    Abstract base class to return APIs and the functions they route to


jfuss · 2019-07-24T17:03:06Z

samcli/commands/local/lib/sam_api_provider.py

            Instance of the API collector that where we will save the API information
+


Make sure all docstrings are updated. api is not a param and collector should be ApiCollector instead of RouteCollector

jfuss · 2019-07-24T18:02:16Z

samcli/local/apigw/local_apigw_service.py

+
+    def __hash__(self):
+        route_hash = hash(self.function_name) * hash(self.path)
+        for method in self.methods:


order matters here. If we really need hash, we should sort methods.

jfuss · 2019-07-24T18:02:44Z

samcli/commands/local/lib/cfn_base_api_provider.py


 LOG = logging.getLogger(__name__)


 class CfnBaseApiProvider(object):
    RESOURCE_TYPE = "Type"
-


This isn't needed anymore right?

jfuss · 2019-07-24T18:06:24Z

tests/unit/commands/local/lib/test_api_provider.py

 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")


remove print statements.

tests/unit/local/apigw/test_local_apigw_service.py

jfuss

A couple small comments. This looks really good, thanks for keeping up with this.

jfuss

Great work. I know this took some time and lots of back and forth but the end result looks clean! 🎉

viksrivat added 12 commits June 17, 2019 16:26

Initial setup and work

8f6b8fb

Add RestApi tests

f457401

Remove Stage Name/Variable from pr

8112cf4

Run make pr

5050dd8

Add Rest Api integration test

bf5a083

Remove AWS::Stage from current pr

6eb226a

Update integration test to only use CloudFormation types

7c5823d

Re-run Travis Ci

4966513

Update Uri to BodyS3Location

2c38ede

Add docstring to extract_swagger_api

9d3d137

Fix Merge conflicts with dev and branch

b6d9b62

Update AWS::ApiGateway::Stage tests

f8ac623

viksrivat changed the title ~~Feature/cloud formation stage support~~ Feature/CloudFormation stage support Jun 24, 2019

viksrivat and others added 17 commits June 24, 2019 16:15

Test local file in body fail and clean tests

54bda7f

Initial Attempt

9b06696

Refactor Sam API provider

145162e

Cleanup Refactoring

5615163

Merge pull request #1 from viksrivat/feature/cloud_formation_api_prov…

752fba8

…ider Modularize RestApi code

Update AWS::ApiGateway::Stage tests

1157284

Fix merge conflict

5d1dfc7

Remove .vscode

7ed267c

Re-run travis

681ede3

Merge pull request #2 from viksrivat/cleanup/update_common_stage_feat…

a3ca29f

…ures Restructure the information Api has the way it passed through

Update common

0078786

Merge branch 'feature/cloud_formation_stage_support' of github.com:vi…

9879105

…ksrivat/aws-sam-cli into feature/cloud_formation_stage_support

Update AbstractParserProvider documentation

a9117ad

Initial Refactor refactor

997845c

Fix tests and run make pr

de65fd0

Fix merge conflict

df243a7

viksrivat added 3 commits June 28, 2019 15:22

Update tests

4a792bf

Update merge conflictz

644b851

Fix bug with api

238742a

viksrivat mentioned this pull request Jun 29, 2019

(feat) CloudFormation AWS::ApiGateway::Methods and AWS::ApiGateway::Resources support #1247

Merged

6 tasks

viksrivat and others added 6 commits July 8, 2019 08:26

Fixed comments

7662f24

Fix merge with rest_api

cc21095

Remove Invalid Integration Test

b50e644

Run make pr

a01aee2

Merge branch 'develop' into feature/cloud_formation_stage_support

ae97479

Fix merge issues and run make pr

fe706c7

jfuss self-assigned this Jul 15, 2019

jfuss requested changes Jul 15, 2019

View reviewed changes

viksrivat added 3 commits July 15, 2019 15:47

Update Route with comments

9ea9a67

Fix Route collector doc

8353724

Update to run AWS::ApiGateway::Stage Integration tests

8b096d3

jfuss reviewed Jul 19, 2019

View reviewed changes

Update based on comments

fcfc10e

jfuss changed the base branch from develop to start-api/cfn July 23, 2019 18:28

viksrivat added 2 commits July 23, 2019 12:52

Update RouteCollector to collect and create Api

f78cbd0

Cleanup ApiCollector

df8186f

jfuss reviewed Jul 24, 2019

View reviewed changes

jfuss requested changes Jul 24, 2019

View reviewed changes

Update Based on Comments

be44710

jfuss approved these changes Jul 25, 2019

View reviewed changes

viksrivat added 2 commits July 25, 2019 14:33

Update tests

12ce392

Update ApiCollector description

e89cc33

jfuss merged commit aeba546 into aws:start-api/cfn Jul 26, 2019

jfuss pushed a commit that referenced this pull request Aug 7, 2019

feat(start-api): CloudFormation AWS::ApiGateway::Stage Support (#1239)

6e083a9

ramosbugs mentioned this pull request Dec 5, 2020

Set operationName in local API request context #2458

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(feat) CloudFormation AWS::ApiGateway::Stage Support #1239

(feat) CloudFormation AWS::ApiGateway::Stage Support #1239

viksrivat commented Jun 24, 2019 •

edited

Loading

jfuss Jul 15, 2019

viksrivat Jul 15, 2019

jfuss Jul 18, 2019

jfuss Jul 15, 2019

jfuss Jul 15, 2019

viksrivat Jul 15, 2019

jfuss Jul 15, 2019

viksrivat Jul 15, 2019

jfuss Jul 15, 2019

viksrivat Jul 15, 2019 •

edited

Loading

jfuss Jul 18, 2019

jfuss Jul 18, 2019

jfuss Jul 18, 2019

jfuss Jul 18, 2019

jfuss Jul 18, 2019

viksrivat Jul 19, 2019

jfuss Jul 19, 2019

jfuss Jul 19, 2019

jfuss Jul 24, 2019

jfuss Jul 24, 2019

jfuss Jul 24, 2019

jfuss Jul 24, 2019

jfuss Jul 24, 2019

jfuss Jul 24, 2019

jfuss left a comment

jfuss left a comment


		result = list(merged_route_methods.values())
		return list(result)

		@@ -242,13 +233,6 @@ class AbstractApiProvider(object):
		"""
		Abstract base class to return APIs and the functions they route to

		Instance of the API collector that where we will save the API information

(feat) CloudFormation AWS::ApiGateway::Stage Support #1239

(feat) CloudFormation AWS::ApiGateway::Stage Support #1239

Conversation

viksrivat commented Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viksrivat Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss left a comment

Choose a reason for hiding this comment

jfuss left a comment

Choose a reason for hiding this comment

viksrivat commented Jun 24, 2019 •

edited

Loading

viksrivat Jul 15, 2019 •

edited

Loading