feat: custom domains in HTTP API #1472

praneetap · 2020-02-27T21:01:09Z

Issue #, if available:

Description of changes:

Description of how you validated changes:
End to end tested
Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected
Add/update example to examples/2016-10-31

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

alexw91 · 2020-02-27T23:19:48Z

Codecov Report

Merging #1472 into develop will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1472      +/-   ##
===========================================
- Coverage    94.39%   94.35%   -0.05%     
===========================================
  Files           78       78              
  Lines         4714     4819     +105     
  Branches       948      969      +21     
===========================================
+ Hits          4450     4547      +97     
- Misses         121      125       +4     
- Partials       143      147       +4

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 4a991a0...6fcacc4. Read the comment docs.

ShreyaGangishetty · 2020-02-28T02:04:55Z

samtranslator/model/api/http_api_generator.py

nit: is the empty "" needed after must be?

just Black being annoying. will fix

samtranslator/model/api/http_api_generator.py

ShreyaGangishetty · 2020-02-28T02:05:05Z

samtranslator/model/api/http_api_generator.py

minor question: I thought if route53.get("IpV6") is True: statement would be enough. Is it needed to add not None check?

What are the possible values? If it's only booleans, then if route53.getI("IpV6"): is sufficient by itself.

samtranslator/model/apigatewayv2.py

ShreyaGangishetty · 2020-02-28T02:19:16Z

tests/translator/input/api_with_basic_custom_domain_intrinsics_http.yaml

Can you update the test by adding condition here?

Sorry, not sure what you mean? Condition is set on the Api and the Function resource.

I wanted to see if conditions work for Domain property

ShreyaGangishetty

Overall looks good to me. Is Domain supported in globals?

praneetap · 2020-02-28T23:08:14Z

Overall looks good to me. Is Domain supported in globlas?

yes. updated a test to use globals instead

ShreyaGangishetty · 2020-02-28T23:14:45Z

Overall looks good to me. Is Domain supported in globlas?

yes. updated a test to use globals instead

Can we add Domain here https://github.com/awslabs/serverless-application-model/blob/develop/docs/globals.rst for HttpApi?

codecov-io · 2020-02-29T02:43:44Z

Codecov Report

Merging #1472 into develop will decrease coverage by 0.03%.
The diff coverage is 92.85%.

@@             Coverage Diff             @@
##           develop    #1472      +/-   ##
===========================================
- Coverage    94.15%   94.11%   -0.04%     
===========================================
  Files           78       78              
  Lines         4789     4897     +108     
  Branches       967      988      +21     
===========================================
+ Hits          4509     4609     +100     
- Misses         130      134       +4     
- Partials       150      154       +4

Impacted Files	Coverage Δ
samtranslator/plugins/globals/globals.py	`99.05% <ø> (ø)`	⬆️
samtranslator/model/sam_resources.py	`94.29% <100%> (+0.09%)`	⬆️
samtranslator/model/apigatewayv2.py	`94.73% <100%> (+0.98%)`	⬆️
samtranslator/model/api/http_api_generator.py	`92.53% <91.75%> (-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 c5221fc...fbd3f80. Read the comment docs.

keetonian

Looks good. Conditions look to be handled correctly, and the translation looks good. Just a few other smaller things to address.

samtranslator/model/api/http_api_generator.py

keetonian · 2020-03-02T17:51:47Z

samtranslator/model/api/http_api_generator.py

        # Assign the OpenApi back to template
        self.definition_body = editor.openapi

+    def _construct_api_domain(self, http_api):


This method is about 100 lines long, and is conceptually difficult to grasp. Could you break the components apart? It looks like this could be split into Domain, BasePath, and RouteSettings methods.

samtranslator/model/api/http_api_generator.py

alexw91 · 2020-03-03T01:40:54Z

Codecov Report

Merging #1472 into develop will decrease coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1472      +/-   ##
===========================================
- Coverage    94.15%   94.10%   -0.06%     
===========================================
  Files           78       78              
  Lines         4789     4900     +111     
  Branches       967      988      +21     
===========================================
+ Hits          4509     4611     +102     
- Misses         130      134       +4     
- Partials       150      155       +5

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 c5221fc...38da4f0. Read the comment docs.

keetonian · 2020-03-03T16:58:42Z

samtranslator/model/api/http_api_generator.py

+            )
+            if "HostedZoneId" in route53:
+                record_set_group.HostedZoneId = route53.get("HostedZoneId")
+            if "HostedZoneName" in route53 and "HostedZoneId" not in route53:


if "HostedZoneName" in route53 and "HostedZoneId" not in route53: -> elif "HostedZoneName" in route53:

praneetap changed the title ~~feat: custom domains in HTTP API~~ feat: custom domains in HTTP API (WIP) Feb 27, 2020

praneetap assigned ShreyaGangishetty and keetonian Feb 27, 2020

praneetap changed the title ~~feat: custom domains in HTTP API (WIP)~~ feat: custom domains in HTTP API Feb 27, 2020

ShreyaGangishetty reviewed Feb 28, 2020

View reviewed changes

samtranslator/model/api/http_api_generator.py Outdated Show resolved Hide resolved

ShreyaGangishetty reviewed Feb 28, 2020

View reviewed changes

samtranslator/model/apigatewayv2.py Outdated Show resolved Hide resolved

ShreyaGangishetty reviewed Feb 28, 2020

View reviewed changes

ShreyaGangishetty suggested changes Feb 28, 2020

View reviewed changes

praneetap added 3 commits February 28, 2020 13:58

feat: custom domains in HTTP API

b7d9b14

code cleanup

fc1c18b

cr comments

5a356dd

praneetap force-pushed the custom_http branch from 6fcacc4 to 5a356dd Compare February 28, 2020 23:02

globals test

e28f4a4

praneetap requested review from ShreyaGangishetty and keetonian February 28, 2020 23:08

ShreyaGangishetty approved these changes Feb 28, 2020

View reviewed changes

cr comment

dfeabd7

keetonian suggested changes Mar 2, 2020

View reviewed changes

cr comments

fbd3f80

keetonian reviewed Mar 3, 2020

View reviewed changes

fixes

0f996c2

keetonian approved these changes Mar 3, 2020

View reviewed changes

make domain name referable

38da4f0

ShreyaGangishetty merged commit 9df2c8a into aws:develop Mar 3, 2020

ShreyaGangishetty pushed a commit to ShreyaGangishetty/serverless-application-model that referenced this pull request Mar 3, 2020

feat: custom domains in HTTP API (aws#1472)

0135a25

ShreyaGangishetty mentioned this pull request Mar 3, 2020

Release tracker for SAM v1.22.0 release #1486

Closed

jamesiri unassigned keetonian Aug 24, 2020

feat: custom domains in HTTP API #1472

feat: custom domains in HTTP API #1472

Uh oh!

Conversation

praneetap commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexw91 commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShreyaGangishetty left a comment • edited by keetonian Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praneetap commented Feb 28, 2020

Uh oh!

ShreyaGangishetty commented Feb 28, 2020

Uh oh!

codecov-io commented Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

keetonian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexw91 commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

praneetap commented Feb 27, 2020 •

edited

Loading

alexw91 commented Feb 27, 2020 •

edited

Loading

ShreyaGangishetty left a comment •

edited by keetonian

Loading

codecov-io commented Feb 29, 2020 •

edited

Loading

alexw91 commented Mar 3, 2020 •

edited

Loading