-
Notifications
You must be signed in to change notification settings - Fork 2
feature: Add package for creating/managing Route53 hosted zone and associated optional resources #1575
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
base: main
Are you sure you want to change the base?
feature: Add package for creating/managing Route53 hosted zone and associated optional resources #1575
Conversation
Create hosted zones, SSL certs and DNS records
7b6a877 to
b2719f4
Compare
TheOrangePuff
left a comment
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.
Looks good! Thanks for contributing 🙂 Just some very minor points to address
| } = props; | ||
|
|
||
| let hostedZone; | ||
| const createCert = createCertificate || subDomains.length > 0; |
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.
I'm unclear why the subDomain length is relevant here? 🤔 Maybe add a comment?
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.
Not entirely necessary but just helps in case the user wants to add subdomains to their certificate but forgot to say createCertificate I've also gone and updated it to certificateSubDomains to make it a bit more explicit as that's all their used for.
| | { type: "SRV"; name: string; value: SrvRecordValue[] }; | ||
|
|
||
| export interface DomainHostingProps { | ||
| domainName: string; |
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 some basic JS doc to these variables? We're trying to be more consistent with this 😅
See this as an example: https://github.com/aligent/cdk-constructs/blob/main/packages/static-hosting/lib/static-hosting.ts
| }); | ||
| break; | ||
| default: | ||
| break; |
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.
Perhaps default should throw an error to say type not implemented?
| Make sure to install compatible versions of these packages in your CDK application. | ||
|
|
||
| ## Usage | ||
| ### `domainName`(string) |
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.
I'd just move these from the readme into the JS doc
Description of the proposed changes
Package to handle hosted zones for setting up new projects or managing existing ones. Works with Route53 domains and external providers (with NS records) and provides ways to adjust resources easily.
CNAME,A,AAAA,MX, andSRVrecords in said zoneTried to keep it quite optional with the only requirement being a domain name, I've tested all the different ways to set it up but may have missed one, can add tests if needed
I needed this and thought these constructs might also find a use for it
Notes to reviewers
🛈 When you've finished leaving feedback, please add a final comment to the PR tagging the author, letting them know that you have finished leaving feedback