Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Route53 Resolver Endpoint L2 implementation #38

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: Route53 Resolver Endpoint L2 implementation #38

wants to merge 1 commit into from

Conversation

WinterYukky
Copy link
Contributor

Fixes #

@WinterYukky WinterYukky added the enhancement New feature or request label Jul 3, 2024
hoegertn
hoegertn reviewed Jul 3, 2024
View reviewed changes
src/aws-route53resolver/resolver-endpoint.ts
// Will be replace to Port.DNS_TCP and Port.DNS_UDP
defaultPorts: [Port.tcp(53), Port.udp(53)],
securityGroups: props.securityGroups ?? [
new SecurityGroup(this, 'SecurityGroup', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract the creation of sub-constructs into a factory method that can be overridden in a subclass

src/aws-route53resolver/resolver-endpoint.ts
props.ipAddresses ?? IpAddresses.autoAssign()
).getIpAddresses(props.vpc);

const resource = new CfnResolverEndpoint(this, 'Resource', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract the creation of sub-constructs into a factory method that can be overridden in a subclass

src/aws-route53resolver/resolver-rule.ts
readonly resolverRuleId: string;
constructor(scope: Construct, id: string, props: ResolverRuleBaseProps) {
super(scope, id);
const resource = new CfnResolverRule(this, 'Resource', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract the creation of sub-constructs into a factory method that can be overridden in a subclass

@hoegertn
Copy link
Contributor

@WinterYukky are you still working on this PR?

@WinterYukky
Copy link
Contributor Author

@hoegertn
Thank you very much for your review. I found aws-route53resolver-alpha package in aws-cdk repository so I try create pull request for aws-cdk repository before open this pull request. Of course, when I open pull request for aws-cdk repository, I modify it according to your comment!! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hoegertn hoegertn hoegertn left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@WinterYukky @hoegertn