fix(route53): warn CrossAccountZoneDelegationRecord delegatedZone Name Server undefined #30602
Conversation
…lidation to warning
github-actions
bot
added
bug
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
samson-keung
changed the title
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Exemption Request Reason: The change affects build time. No changes to deployment behaviour, hence, no integ test update |
aws-cdk-automation
added
the
pr-linter/exemption-requested
comcalvi
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-maintainer-review
| @@ -930,7 +930,8 @@ export class CrossAccountZoneDelegationRecord extends Construct { | |||
| } | |||
|
|
|||
| if (!props.delegatedZone.hostedZoneNameServers) { | |||
| throw Error(`Not able to retrieve Name Servers for ${props.delegatedZone.zoneName} due to it being imported.`); | |||
| Annotations.of(this).addWarningV2('@aws-cdk/aws-route53:importedDelegatedZoneNSValidation', | |||
| `Not able to retrieve Name Servers for \'${props.delegatedZone.zoneName}\' due to it being imported. Use a non-imported one or use AwsCustomResource to retrieve the Name Servers.`); | |||
There was a problem hiding this comment.
how do we know this is imported? Not specified doesn't mean that it's been imported.
aws-cdk-automation
removed
the
pr/needs-maintainer-review
| @@ -930,7 +930,8 @@ export class CrossAccountZoneDelegationRecord extends Construct { | |||
| } | |||
|
|
|||
| if (!props.delegatedZone.hostedZoneNameServers) { | |||
| throw Error(`Not able to retrieve Name Servers for ${props.delegatedZone.zoneName} due to it being imported.`); | |||
There was a problem hiding this comment.
Instead of throwing here we could change this line linked from the original issue:
to read:
ResourceRecords: (DelegatedZoneNameServers ?? []).map(ns => ({ Value: ns })),
but that change would need to be carefully integ tested, we've recently seen issues from passing empty arrays around the custom resource handler framework.
|
Quick FYI here, when you submit a fix, the title should describe the bug, not the fix. |
Issue # (if applicable)
Closes #30600.
Reason for this change
See #30440 (comment)
TLDR: This PR unblocks users who use other ways to retrieve delegated zone name servers
Description of changes
Covert the
throw ErrorintoaddWarningDescription of how you validated changes
Unit test added
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license