Skip to content

Conversation

mbarneyjr
Copy link
Contributor

@mbarneyjr mbarneyjr commented Jan 22, 2020

Issue #, if available:

Issue #1402

Description of changes:

Adds support for inclusion of HostedZoneName instead of HostedZoneId in the Domain section of an AWS::Serverless::Api. This allows users to not need to worry about looking up the hosted zone ID, and letting CloudFormation handle it for them via the AWS::Route53::RecordSetGroup resource.

HostedZoneName is forwarded to the RecordSetGroup resource just as HostedZoneId is.

The following should now be valid SAM:

Globals:
  Api:
    Domain:
      DomainName: www.my-domain.com
      CertificateArn: my-valid-cert-arn
      EndpointConfiguration: EDGE
      Route53:
        HostedZoneName: my-domain.com.

Description of how you validated changes:

Not 100% sure how to validate the changes, didn't immediately see any documentation around it

Checklist:

  • Write/update tests
  • make pr passes (It doesn't have any more failures than what is on master)
  • 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.

Copy link
Contributor

@praneetap praneetap left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Could you also change/add a unit test here. I think you can just change one of the existing custom domain tests in https://github.com/awslabs/serverless-application-model/tree/develop/tests/translator/input, and you will have to change the corresponding expected outputs in aws, aws-cn and aws-us-gov

@praneetap praneetap self-assigned this Jan 22, 2020
@mbarneyjr
Copy link
Contributor Author

@praneetap I added the test cases and fixed a bug in the LogicalID generation thatI found because of it, thanks for the guidance!

Copy link
Contributor

@praneetap praneetap left a comment

Choose a reason for hiding this comment

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

Nicely done and thank you!

@praneetap praneetap assigned keetonian and unassigned praneetap Jan 23, 2020
@praneetap
Copy link
Contributor

@mbarneyjr please run make black, it will auto format for you. That should fix the build.

@keetonian
Copy link
Contributor

Looks like it's just a black formatting error. I submitted #1419 to update the black version that our tests use, this PR should be good to go once that is merged. I don't see any other test issues.

@keetonian keetonian closed this Jan 27, 2020
@keetonian keetonian reopened this Jan 27, 2020
@keetonian
Copy link
Contributor

Closed/re-opened PR to kick off a new travis run. The black formatting issues should now be resolved.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (develop@e018454). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1408   +/-   ##
==========================================
  Coverage           ?   94.42%           
==========================================
  Files              ?       78           
  Lines              ?     4666           
  Branches           ?      932           
==========================================
  Hits               ?     4406           
  Misses             ?      121           
  Partials           ?      139
Impacted Files Coverage Δ
samtranslator/model/api/api_generator.py 95.02% <100%> (ø)
samtranslator/model/sam_resources.py 94.06% <100%> (ø)

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 e018454...71d3112. Read the comment docs.

@keetonian keetonian merged commit 3ea32ea into aws:develop Jan 31, 2020
@keetonian
Copy link
Contributor

@mbarneyjr Thank you for this contribution!

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.

5 participants