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.

Sign up for GitHub

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

Jump to bottom

(feat) CloudFormation AWS::ApiGateway::RestApi support #1238

Merged
merged 30 commits into from
Jul 11, 2019
Merged

(feat) CloudFormation AWS::ApiGateway::RestApi support #1238

merged 30 commits into from
Jul 11, 2019

Conversation

viksrivat
Copy link
Contributor

Issue #, if available:
Related pr: #1234
Description of changes:
Add support for the CloudFormation AWS::ApiGateway::RestApi type for swagger.
Checklist:

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

jfuss
jfuss suggested changes Jun 24, 2019
View reviewed changes
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
tests/integration/testdata/start_api/swagger-rest-api-template.yaml Outdated Show resolved Hide resolved
tests/integration/testdata/start_api/swagger-rest-api-template.yaml Show resolved Hide resolved
tests/integration/testdata/start_api/swagger-rest-api-template.yaml Show resolved Hide resolved
tests/integration/testdata/start_api/swagger-rest-api-template.yaml Outdated
Properties:
CodeUri: "."
Events:
GetApi:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is valid: https://github.com/awslabs/serverless-application-model/blob/master/versions/2016-10-31.md#api

Does this deploy and work as you expect it to in the cloud?

tests/unit/commands/local/lib/test_sam_api_provider.py Outdated Show resolved Hide resolved
tests/unit/commands/local/lib/test_sam_api_provider.py Outdated Show resolved Hide resolved
tests/unit/commands/local/lib/test_sam_api_provider.py Outdated Show resolved Hide resolved
@jfuss jfuss self-assigned this Jun 24, 2019
@viksrivat viksrivat mentioned this pull request Jun 24, 2019
Merged
6 tasks
jfuss
jfuss suggested changes Jun 26, 2019
View reviewed changes
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hard time understanding the design decisions you made here. Might be worth a whiteboard session tomorrow to help me understand reasoning of your decisions. Generally I think it can be simplified.

samcli/commands/local/lib/sam_api_provider.py Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
jfuss
jfuss suggested changes Jun 27, 2019
View reviewed changes
samcli/commands/local/lib/sam_base_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/api_provider.py Show resolved Hide resolved
samcli/commands/local/lib/api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/cf_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/cf_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/cf_base_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/cf_base_api_provider.py Outdated Show resolved Hide resolved
@viksrivat viksrivat changed the title feature/CloudFormation Api Gateway Rest Api (feat) CloudFormation AWS::ApiGateway::RestApi support Jun 27, 2019
jfuss
jfuss suggested changes Jun 28, 2019
View reviewed changes
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple small comments. Mainly around the tests to make sure the integ tests represent a valid template.

tests/integration/local/start_api/test_start_api.py
@@ -288,6 +288,118 @@ def test_binary_response(self):
self.assertEquals(response.content, expected)


class TestStartApiWithSwaggerRestApis(StartApiIntegBaseClass):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed but it would be great if there was an easy to parameterize the whole class. That way we don't have to maintain two of the same tests cases for CFN and SAM. It looks like pytest fixtures might be a solution here but is a much bigger investment than this PR.

tests/integration/testdata/start_api/swagger-rest-api-template.yaml Outdated
type: aws_proxy
uri:
Fn::Sub: arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${EchoBase64EventBodyFunction.Arn}/invocations
"/functionwithnoapievent":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it was copied from the AWS::Serverless::Api Resource tests. I don't think this is needed since a Function with an Event will generate an AWS::Serverless::Api and this is testing the AWS::ApiGateway::RestApi.

tests/integration/testdata/start_api/swagger-rest-api-template.yaml Outdated Show resolved Hide resolved
tests/integration/testdata/start_api/swagger-rest-api-template.yaml Show resolved Hide resolved
tests/unit/commands/local/lib/test_api_provider.py Outdated Show resolved Hide resolved
tests/unit/commands/local/lib/test_api_provider.py Show resolved Hide resolved
tests/unit/commands/local/lib/test_cfn_api_provider.py Outdated Show resolved Hide resolved
tests/unit/commands/local/lib/test_cfn_api_provider.py Show resolved Hide resolved
jfuss
jfuss approved these changes Jul 8, 2019
View reviewed changes
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOOT! Thanks for keeping with it

@jfuss jfuss merged commit 7cb50b0 into aws:develop Jul 11, 2019
sanathkr added a commit that referenced this pull request Jul 22, 2019
@sanathkr
Revert "feat(start-api): CloudFormation AWS::ApiGateway::RestApi supp…
a25cc2d 
…ort (#1238)"

This reverts commit 7cb50b0.
@sanathkr sanathkr mentioned this pull request Jul 22, 2019
Merged
sanathkr added a commit that referenced this pull request Jul 22, 2019
@sanathkr
Revert "feat(start-api): CloudFormation AWS::ApiGateway::RestApi supp…
9306b9c 
…ort (#1238)" (#1282)

This reverts commit 7cb50b0.
jfuss pushed a commit that referenced this pull request Jul 23, 2019
@viksrivat @jfuss
feat(start-api): CloudFormation AWS::ApiGateway::RestApi support (#1238)
49d16b4
jfuss pushed a commit that referenced this pull request Aug 7, 2019
@viksrivat @jfuss
feat(start-api): CloudFormation AWS::ApiGateway::RestApi support (#1238)
2368c33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfuss jfuss jfuss approved these changes

Assignees

@jfuss jfuss

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@viksrivat @jfuss