feat: IPv6 support#630
Conversation
- turn Route53::RecordSet into a RecordSetGroup with additional AAAA record - change resource name to avoid type error when updating CloudFormation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughRoute53 custom domain record generation is expanded to emit both IPv4 and IPv6 records. The CloudFormation resource type changes from ChangesRoute53 IPv6 Records
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/resources/Api.ts`:
- Around line 286-298: The RecordSets mapping has lint failures: change the
arrow callback parameter to use parentheses in the map (i.e. use (Type) => ...)
and ensure trailing commas are added for the object literals returned by the map
— add a trailing comma after the AliasTarget object literal and after the mapped
record object so the RecordSets array elements and nested objects end with
commas (references: RecordSets, the map callback Type, domain.name,
domainNameLogicalId, AppSyncDomainName, HostedZoneId, EvaluateTargetHealth).
In `@src/resources/Naming.ts`:
- Line 39: The change of the CloudFormation logical ID from
GraphQlDomainRoute53Record to GraphQlDomainRoute53Records (in the return of
getLogicalId) will force replacement of the Route53 resource (switching from
AWS::Route53::RecordSet to AWS::Route53::RecordSetGroup) and can cause brief DNS
disruption; update the PR description, changelog, and migration guide to clearly
call out this breaking change, list the exact symbols affected
(GraphQlDomainRoute53Record -> GraphQlDomainRoute53Records and the getLogicalId
usage), describe the replacement behavior and expected DNS propagation delay,
and recommend that users plan upgrades during a maintenance window.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f542a86-f8ca-407f-8e96-ef7dbedd4dda
📒 Files selected for processing (2)
src/resources/Api.tssrc/resources/Naming.ts
| RecordSets: ['A', 'AAAA'].map(Type => ({ | ||
| Name: domain.name, | ||
| Type, | ||
| AliasTarget: { | ||
| HostedZoneId: { | ||
| 'Fn::GetAtt': [domainNameLogicalId, 'HostedZoneId'], | ||
| }, | ||
| DNSName: { | ||
| 'Fn::GetAtt': [domainNameLogicalId, 'AppSyncDomainName'], | ||
| }, | ||
| EvaluateTargetHealth: false, | ||
| } | ||
| })) |
There was a problem hiding this comment.
Fix formatting issues to pass linter.
The arrow function and object literals have formatting issues that will cause prettier/linting to fail:
- Line 286: Arrow function parameter needs parentheses
- Lines 297-298: Missing trailing commas
🔧 Proposed fix for formatting
- RecordSets: ['A', 'AAAA'].map(Type => ({
+ RecordSets: ['A', 'AAAA'].map((Type) => ({
Name: domain.name,
Type,
AliasTarget: {
HostedZoneId: {
'Fn::GetAtt': [domainNameLogicalId, 'HostedZoneId'],
},
DNSName: {
'Fn::GetAtt': [domainNameLogicalId, 'AppSyncDomainName'],
},
- EvaluateTargetHealth: false,
- }
- }))
+ EvaluateTargetHealth: false,
+ },
+ })),
},As per coding guidelines, the static analysis hints indicate these formatting violations must be addressed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RecordSets: ['A', 'AAAA'].map(Type => ({ | |
| Name: domain.name, | |
| Type, | |
| AliasTarget: { | |
| HostedZoneId: { | |
| 'Fn::GetAtt': [domainNameLogicalId, 'HostedZoneId'], | |
| }, | |
| DNSName: { | |
| 'Fn::GetAtt': [domainNameLogicalId, 'AppSyncDomainName'], | |
| }, | |
| EvaluateTargetHealth: false, | |
| } | |
| })) | |
| RecordSets: ['A', 'AAAA'].map((Type) => ({ | |
| Name: domain.name, | |
| Type, | |
| AliasTarget: { | |
| HostedZoneId: { | |
| 'Fn::GetAtt': [domainNameLogicalId, 'HostedZoneId'], | |
| }, | |
| DNSName: { | |
| 'Fn::GetAtt': [domainNameLogicalId, 'AppSyncDomainName'], | |
| }, | |
| EvaluateTargetHealth: false, | |
| }, | |
| })), |
🧰 Tools
🪛 ESLint
[error] 286-286: Replace Type with (Type)
(prettier/prettier)
[error] 297-297: Insert ,
(prettier/prettier)
[error] 298-298: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/resources/Api.ts` around lines 286 - 298, The RecordSets mapping has lint
failures: change the arrow callback parameter to use parentheses in the map
(i.e. use (Type) => ...) and ensure trailing commas are added for the object
literals returned by the map — add a trailing comma after the AliasTarget object
literal and after the mapped record object so the RecordSets array elements and
nested objects end with commas (references: RecordSets, the map callback Type,
domain.name, domainNameLogicalId, AppSyncDomainName, HostedZoneId,
EvaluateTargetHealth).
|
|
||
| getDomainReoute53RecordLogicalId() { | ||
| return this.getLogicalId(`GraphQlDomainRoute53Record`); | ||
| return this.getLogicalId(`GraphQlDomainRoute53Records`); |
There was a problem hiding this comment.
Breaking change: Logical ID modification will replace Route53 records.
Changing the CloudFormation logical ID from GraphQlDomainRoute53Record to GraphQlDomainRoute53Records will cause CloudFormation to replace the Route53 resource during stack updates. This means:
- A new RecordSetGroup will be created
- The old RecordSet will be deleted
- Brief DNS propagation delays may occur during the transition
This is intentional to switch from AWS::Route53::RecordSet to AWS::Route53::RecordSetGroup, but users upgrading to this version should be aware of potential brief DNS disruption.
Recommendation: Document this breaking change in the PR description, changelog, or migration guide so users can plan the upgrade during a maintenance window if needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/resources/Naming.ts` at line 39, The change of the CloudFormation logical
ID from GraphQlDomainRoute53Record to GraphQlDomainRoute53Records (in the return
of getLogicalId) will force replacement of the Route53 resource (switching from
AWS::Route53::RecordSet to AWS::Route53::RecordSetGroup) and can cause brief DNS
disruption; update the PR description, changelog, and migration guide to clearly
call out this breaking change, list the exact symbols affected
(GraphQlDomainRoute53Record -> GraphQlDomainRoute53Records and the getLogicalId
usage), describe the replacement behavior and expected DNS propagation delay,
and recommend that users plan upgrades during a maintenance window.
2 participants
Summary by CodeRabbit