feat(appsync): support custom domain mappings by moofish32 · Pull Request #19368 · aws/aws-cdk

moofish32 · 2022-03-12T18:07:46Z

This adds support for custom domains with AppSync.

gitpod-io · 2022-03-12T18:07:49Z

msambol · 2022-03-12T19:24:05Z

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

+   * The hosted zone and CName must be configured in addition to this setting to
+   * enable custom domain URL
+   *
+   * @default - none a unique name is generated


Can you add a comma or parenthesis after "none" ?

msambol · 2022-03-12T19:32:42Z

packages/@aws-cdk/aws-appsync/README.md

+```ts
+import { Certificate } from '@aws-cdk/aws-certificatemanager';
+
+declare const certificate: Certificate;


Will this work? You're not initializing it to anything.

Will this work? no. Can it compile this way? yes. This is something we are using in our examples for necessary properties that are not essential to the overall example. We don't want to clutter our examples with a new lambda.Function(this, 'myfn', {...}); each time we need one, so it is simpler to write declare const fn: lambda.Function and leave it to the user to provide the exact function they need if they want to use the example.

That being said, I feel like providing a real certificate is necessary to this example, since it is part of the domainName property we are demonstrating. So I do think in this situation we should provide a real certificate.

So I did not invent this pattern I modeled most of the work from the rest api pattern here

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

kaizencc

Thanks @moofish32 for getting this started! Looks good, with a few minor comments.

packages/@aws-cdk/aws-appsync/README.md

kaizencc · 2022-03-15T15:14:35Z

packages/@aws-cdk/aws-appsync/README.md

+```ts
+import { Certificate } from '@aws-cdk/aws-certificatemanager';
+
+declare const certificate: Certificate;


Will this work? no. Can it compile this way? yes. This is something we are using in our examples for necessary properties that are not essential to the overall example. We don't want to clutter our examples with a new lambda.Function(this, 'myfn', {...}); each time we need one, so it is simpler to write declare const fn: lambda.Function and leave it to the user to provide the exact function they need if they want to use the example.

That being said, I feel like providing a real certificate is necessary to this example, since it is part of the domainName property we are demonstrating. So I do think in this situation we should provide a real certificate.

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

+   * The hosted zone and CName must be configured in addition to this setting to
+   * enable custom domain URL
+   *
+   * @default - none a unique name is generated


packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

kaizencc · 2022-03-15T15:20:26Z

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts

    this.schema = props.schema ?? new Schema();
    this.schemaResource = this.schema.bind(this);

+    if (props.domainName) {


In the docs for domainName you write that "The hosted zone and CName must be configured in addition to this setting to enable custom domain URL" but we do not check for that here.

Are we leaving that requirement as a deploy-time check right now? If so, we probably need to think about how we can make it a synth-time check.

Again based on how the rest api works today this is a separate action. I felt like we should tell the user to do this, but if we nested this all in this construct I think it would be doing too much. I assume the rest api developers felt the same way, but of course I do not know.

packages/@aws-cdk/aws-appsync/test/appsync.test.ts

Co-authored-by: Kaizen Conroy <36202692+kaizen3031593@users.noreply.github.com>

Pull request has been modified.

kaizencc

Thanks @moofish32! Added in a few changes (mostly to the readme example, to get it to compile).

mergify · 2022-03-18T00:22:50Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-03-18T12:50:36Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 64c1dd7
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-03-18T12:50:53Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

samdervis · 2022-03-25T15:16:19Z

@moofish32
The code snippet in the README might not work when creating Route53 Record

If you define

const myDomainName = 'api.example.com';

new route53.CnameRecord(this, `CnameApiRecord`, {
  recordName: 'api',
  zone,
  domainName: myDomainName,
});

This is trying to create a DNS record (CNAME) for api.example.com to point to the same address
You need to target the CloudFront URL of the appsync domain.
Like this

new CnameRecord(this, `CnameApiRecord`, {
            recordName: 'api',
            zone,
            domainName: appsyncDomain.attrAppSyncDomainName
        });

See my working sample here #18040 (comment)

sebastiansanio · 2022-07-19T21:27:22Z

Is it possible to make this work with @aws-cdk/aws-appsync-alpha?

feat(aws-appsync): enable support for custom domain mappings

8c82b2a

github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Mar 12, 2022

github-actions bot assigned otaviomacedo Mar 12, 2022

chore: update readme

1419f27

msambol reviewed Mar 12, 2022

View reviewed changes

packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts Show resolved Hide resolved

kaizencc assigned kaizencc and unassigned otaviomacedo Mar 15, 2022

kaizencc added p1 feature-request A feature should be added or improved. labels Mar 15, 2022

kaizencc changed the title ~~feat(aws-appsync): enable support for custom domain mappings~~ feat(appsync): support custom domain mappings Mar 15, 2022

github-actions bot assigned otaviomacedo Mar 15, 2022

aws deleted a comment from msambol Mar 15, 2022

kaizencc previously requested changes Mar 15, 2022

View reviewed changes

kaizencc unassigned otaviomacedo Mar 15, 2022

Apply suggestions from code review

87a87c1

Co-authored-by: Kaizen Conroy <36202692+kaizen3031593@users.noreply.github.com>

cgarvis and others added 7 commits March 17, 2022 13:40

Merge branch 'master' into feat/aws-appsync-custom-domain

9bf500b

Fix linting warnings and errors

60ca733

Fix whitespacing

42c379c

Fix documentation

96ca54c

try to fix readme example

ed9cdb5

another fix

831fcf7

last minor changes

0856ecf

kaizencc approved these changes Mar 17, 2022

View reviewed changes

Merge branch 'master' into feat/aws-appsync-custom-domain

64c1dd7

mergify bot merged commit 8c7a4ac into aws:master Mar 18, 2022

davideicardi mentioned this pull request Mar 24, 2022

AppSyncApi: support custom domain anomalyco/sst#1071

Closed

Conversation

moofish32 commented Mar 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Mar 12, 2022

Uh oh!

msambol Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

msambol Mar 12, 2022

Choose a reason for hiding this comment

Uh oh!

kaizencc Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

moofish32 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaizencc Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

kaizencc Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

moofish32 Mar 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 18, 2022

Uh oh!

aws-cdk-automation commented Mar 18, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 18, 2022

Uh oh!

samdervis commented Mar 25, 2022

Uh oh!

sebastiansanio commented Jul 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

moofish32 commented Mar 12, 2022 •

edited

Loading