-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(appsync): support custom domain mappings #19368
feat(appsync): support custom domain mappings #19368
Conversation
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comma or parenthesis after "none" ?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
```ts | ||
import { Certificate } from '@aws-cdk/aws-certificatemanager'; | ||
|
||
declare const certificate: Certificate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work? You're not initializing it to anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I did not invent this pattern I modeled most of the work from the rest api pattern here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @moofish32 for getting this started! Looks good, with a few minor comments.
```ts | ||
import { Certificate } from '@aws-cdk/aws-certificatemanager'; | ||
|
||
declare const certificate: Certificate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* 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 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@@ -472,6 +495,18 @@ export class GraphqlApi extends GraphqlApiBase { | |||
this.schema = props.schema ?? new Schema(); | |||
this.schemaResource = this.schema.bind(this); | |||
|
|||
if (props.domainName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Kaizen Conroy <36202692+kaizen3031593@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @moofish32! Added in a few changes (mostly to the readme example, to get it to compile).
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
@moofish32 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 new CnameRecord(this, `CnameApiRecord`, {
recordName: 'api',
zone,
domainName: appsyncDomain.attrAppSyncDomainName
}); See my working sample here #18040 (comment) |
Is it possible to make this work with @aws-cdk/aws-appsync-alpha? |
fixes #18040
This adds support for custom domains with AppSync.