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

feat(gatewayresponses): add support for API Gateway Responses #841

Merged
merged 9 commits into from
Mar 19, 2019
Merged

feat(gatewayresponses): add support for API Gateway Responses #841

merged 9 commits into from
Mar 19, 2019

Conversation

chrisoverzero
Copy link
Contributor

@chrisoverzero chrisoverzero commented Mar 5, 2019

This is a WIP. Remaining:

  1. Documentation
  2. Example
  3. Edge Cases Which Will Be Pointed Out to Me, I'm Sure???

Issue #, if available:

#815

Description of changes:

Added new property GatewayResponses to AWS::Serverless::API. This translates to x-amazon-apigateway-gateway-responses in the generated Swagger with some convenience transformations applied for SAMier syntax.

Description of how you validated changes:

In addition to the present test suite, ran a company project through the translator. It deployed and worked correctly. (Then I reluctantly took it back down.)

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation
  • Create example

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

@chrisoverzero
Copy link
Contributor Author

Documentation is added. I tried to match house style, but lemme know if it's n-not?

I intend to have an example committed today, but Day Job may interfere.

@chrisoverzero
Copy link
Contributor Author

chrisoverzero commented Mar 7, 2019

OK, I think we in it.

I am the furthest thing from a professional Python developer, so please do let me know where I've fallen down.

@chrisoverzero chrisoverzero marked this pull request as ready for review March 7, 2019 15:46
@codecov-io
Copy link

codecov-io commented Mar 7, 2019

Codecov Report

Merging #841 into develop will decrease coverage by 0.04%.
The diff coverage is 92.3%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #841      +/-   ##
===========================================
- Coverage    94.58%   94.54%   -0.05%     
===========================================
  Files           67       67              
  Lines         2865     2933      +68     
  Branches       519      538      +19     
===========================================
+ Hits          2710     2773      +63     
- Misses          83       85       +2     
- Partials        72       75       +3
Impacted Files Coverage Δ
samtranslator/model/sam_resources.py 95.5% <ø> (ø) ⬆️
samtranslator/model/apigateway.py 97.72% <100%> (+0.5%) ⬆️
samtranslator/swagger/swagger.py 97.56% <100%> (+0.09%) ⬆️
samtranslator/plugins/globals/globals.py 100% <100%> (ø) ⬆️
samtranslator/model/api/api_generator.py 94.23% <76.19%> (-2.03%) ⬇️
samtranslator/translator/translator.py 99.09% <0%> (+0.06%) ⬆️

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 972b610...4d18722. Read the comment docs.

Copy link
Contributor

@brettstack brettstack left a comment

Choose a reason for hiding this comment

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

This is really awesome. Thank you so much for doing this!

examples/2016-10-31/api_gateway_responses/template.yaml Outdated Show resolved Hide resolved
examples/2016-10-31/api_gateway_responses/template.yaml Outdated Show resolved Hide resolved
examples/2016-10-31/api_gateway_responses/template.yaml Outdated Show resolved Hide resolved
samtranslator/model/api/api_generator.py Show resolved Hide resolved
samtranslator/model/api/api_generator.py Outdated Show resolved Hide resolved
Copy link
Contributor

@brettstack brettstack left a comment

Choose a reason for hiding this comment

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

Looking good! I haven't yet reviewed the test input/output.

samtranslator/model/apigateway.py Show resolved Hide resolved
samtranslator/swagger/swagger.py Outdated Show resolved Hide resolved
@chrisoverzero
Copy link
Contributor Author

Awesome. Thanks so much. I'm past EoD here in my time zone, but I'll jump on these in the morning.

This is a WIP. Remaining:

1. Documentation
2. Examples
3. Edge Cases Which Will Be Pointed Out to Me, I'm Sure
- Adds New Minimalism Test
- Adds Globals Documentation
- Passes through Values for Gateway Response Types
...Patterned off of Hello World because why not
@chrisoverzero
Copy link
Contributor Author

Apologies for the force pushes -- trying to get the merge conflict resolved.

This is now caught up with current feedback.

Copy link
Contributor

@brettstack brettstack left a comment

Choose a reason for hiding this comment

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

I just have a couple of requests:

  1. The comment about not allowing String for StatusCode. I think API Gateway will accept a String, and if that's the case we shouldn't disallow its use in SAM (i.e we should allow both number and string)
  2. Could you add a test which has multiple GatewayResponses? All tests appear to just have a single UNAUTHORIZED GatewayResponse. You can update one of your existing tests to just include a second one.

samtranslator/model/apigateway.py Outdated Show resolved Hide resolved
- Adds Additional Test Cases
  - ...for Number-Like String StatusCode
  - ...for Multiple Gateway Responses
Copy link
Contributor

@brettstack brettstack left a comment

Choose a reason for hiding this comment

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

Ship!

@keetonian keetonian merged commit b7c6986 into aws:develop Mar 19, 2019
@carlnordenfelt
Copy link
Contributor

Really glad to see this merged! Thanks @chrisoverzero for picking this up!

@brettstack @keetonian do you guys have any idea when this will be released? I'm in dire need of this to avoid having to define the entire API myself at the moment.

@keetonian
Copy link
Contributor

keetonian commented Mar 25, 2019

@carlnordenfelt We're working on it now. We just cut a release branch: https://github.com/awslabs/serverless-application-model/tree/release/v1.11.0

We should be able to release this soon, barring any unforeseen issues with the new features.

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.

None yet

5 participants