feat(aws-route53-targets): add global accelerator target to route53 alias targets#13407
feat(aws-route53-targets): add global accelerator target to route53 alias targets#13407mergify[bot] merged 15 commits intoaws:masterfrom
Conversation
…ndividual classes for consistency with the rest of the codebase. Add documentation to the class.
|
|
github-actions
bot
added
@aws-cdk/aws-globalaccelerator
njlynch
left a comment
njlynch
left a comment
There was a problem hiding this comment.
Looks great! Just one change necessary, due to the fact we can't fully support union types in the CDK.
| * If passing a string value, it must be a valid domain name for an existing Global Accelerator. e.g. xyz.awsglobalaccelerator.com | ||
| * If passing an instance of an accelerator created within CDK, the accelerator.dnsName property will be used as the target. | ||
| */ | ||
| constructor(private readonly accelerator: string | globalaccelerator.IAccelerator) { |
There was a problem hiding this comment.
Unfortunately, we can't use type unions in our public APIs as they cannot be strongly-modeled in all languages the CDK supports.
Solutions:
- Typically we recommend using static methods instead (e.g.,
GlobalAcceleratorTarget.domain('xyz.awsglobalaccelerator.com'),GlobalAcceleratorTarget.accelerator(accelerator)). - Within this package, it looks like maybe just creating a second class like
ApiGateway/ApiGatewayDomainis an existing, alternative pattern. So you could createGlobalAcceleratorTargetandGlobalAcceleratorDomainTarget. - Final alternative would be to just create this
GlobalAcceleratorTargetthat accepts anIAccelerator, and add theGlobalAcceleratorDomainTarget(that takes the domain name/string) later based on feedback from the community.
I think given the existing patterns in this module options 2 or 3 are the principal of least surprise; I'm happy either way.
There was a problem hiding this comment.
Went with option 2. I think it actually makes the code a little cleaner, although it might not be as intuitive to locate and use.
packages/@aws-cdk/aws-route53-targets/test/integ.globalaccelerator-alias-target.ts
Show resolved
Hide resolved
…ate classes for the GlobalAcceleratorTarget
njlynch
left a comment
njlynch
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
|
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). |
4 participants
Closes #12839
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license