fix(route53): return plain hosted zone id without /hostedzone/ prefix #5230

AlexZeitler · 2019-11-27T20:57:02Z

When creating a HostedZone, the hostedZoneId is the ID of this hosted zone, such as "Z23ABC4XYZL05B".

Until how, when importing a HostedZone using route53.HostedZone.fromLookup, it did return the hostedZoneId in this format: "/hostedzone/Z23ABC4XYZL05B".

This commit makes the value of hostedZoneId consistent between creating a new HostedZone, using HostedZone.fromLookup and fromHostedZoneAttributes.

BREAKING CHANGE: value of hostedZoneId is now be different compared to previous versions of CDK when using HostedZone.fromLookup or fromHostedZoneAttributes

fixes #4744

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

When creating a HostedZone, the `hostedZoneId` is the ID of this hosted zone, such as "Z23ABC4XYZL05B". Until how, when importing a HostedZone using route53.HostedZone.fromLookup, it did return the `hostedZoneId` in this format: /hostedzone/Z23ABC4XYZL05B. This commit makes the value of `hostedZoneId` consistent between creating a new `HostedZone`, using `HostedZone.fromLookup` and `fromHostedZoneAttributes`. BREAKING CHANGE: value of `hostedZoneId` is now be different compared to previous versions of CDK when using `HostedZone.fromLookup` or `fromHostedZoneAttributes`

mergify · 2019-11-27T20:57:40Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-27T21:19:20Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-28T09:09:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-28T17:07:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb · 2019-11-29T07:17:52Z

packages/@aws-cdk/aws-route53/test/test.hosted-zone-provider.ts

@@ -15,8 +15,9 @@ export = {
      const missing = SynthUtils.synthesize(stack).assembly.manifest.missing!;
      test.ok(missing && missing.length === 1);

+      const fakeZoneId = '11111111111111';
      const fakeZone = {


Please add a test which verifies that if users pass a hosted zone without the “/hostedzone” prefix, we still get the desired effect.

Done, please see 667f65f

Pull request has been modified.

aws-cdk-automation · 2019-11-29T09:16:54Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-29T09:33:16Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-29T16:00:42Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-29T23:00:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-12-01T15:35:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

add hostedZoneArn attribute & tests.

aws-cdk-automation · 2019-12-08T02:38:43Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

eladb · 2019-12-12T07:58:06Z

packages/@aws-cdk/aws-route53/lib/hosted-zone-ref.ts

@@ -16,6 +16,13 @@ export interface IHostedZone extends IResource {
   */
  readonly zoneName: string;

+  /**
+   * ARN of this hosted zone, such as arn:${Partition}:route53:::hostedzone/${Id}


change arn:${partition}... to a concrete example of an ARN

eladb · 2019-12-12T07:59:24Z

packages/@aws-cdk/aws-route53/lib/hosted-zone.ts

@@ -94,6 +103,8 @@ export class HostedZone extends Resource implements IHostedZone {
      response.Name = response.Name.substring(0, response.Name.length - 1);
    }

+    response.Id = response.Id.replace('/hostedzone/', '');


Change to regex so it matches only the beginning of the string: replace(/^\/hostedzone\//, '')

eladb · 2019-12-12T08:00:28Z

packages/@aws-cdk/aws-route53/test/test.hosted-zone-provider.ts

-      const stack = new cdk.Stack(undefined, 'TestStack', { env: { account: '12345', region: 'us-east-1' } });
-      const filter = {domainName: 'test.com'};
+      const stack = new cdk.Stack(undefined, 'TestStack', {
+        env: { account: '12345', region: 'us-east-1' }
+      });
+      const filter = { domainName: 'test.com' };


To improve code reviewer experience, try to avoid formatting changes (it's hard to ensure that there are no functional changes here)

eladb · 2019-12-12T08:01:24Z

packages/@aws-cdk/aws-route53/test/test.hosted-zone-provider.ts

-        Id: "/hostedzone/11111111111111",
-        Name: "example.com.",
-        CallerReference: "TestLates-PublicZo-OESZPDFV7G6A",
+        Id: `/hostedzone/${fakeZoneId}`,
+        Name: 'example.com.',
+        CallerReference: 'TestLates-PublicZo-OESZPDFV7G6A',


I'd prefer it if this code was completely reverted to demonstrate there is no regression.

…aws#5230) When creating a HostedZone, the `hostedZoneId` is the ID of this hosted zone, such as "Z23ABC4SAMPLE". Until how, when importing a HostedZone using route53.HostedZone.fromLookup, it did return the `hostedZoneId` in this format: /hostedzone/Z23ABC4SAMPLE. This commit makes the value of `hostedZoneId` consistent between creating a new `HostedZone`, using `HostedZone.fromLookup` and `fromHostedZoneAttributes`. BREAKING CHANGE: the value of `hostedZoneId` will no longer include `/hostedzone/` prefix and only includes the hostedZoneId when using `HostedZone.fromLookup` or `fromHostedZoneAttributes`

AlexZeitler requested a review from shivlaks as a code owner November 27, 2019 20:57

SomayaB assigned shivlaks Nov 28, 2019

Merge branch 'master' into plain-hostedzone-id-from-lookup

add5072

Merge branch 'master' into plain-hostedzone-id-from-lookup

ac14a78

eladb previously requested changes Nov 29, 2019

View reviewed changes

Merge branch 'master' into plain-hostedzone-id-from-lookup

c8f39b4

test: ensure zone is resolved correctly when using no /hostedzone prefix

667f65f

Merge branch 'master' into plain-hostedzone-id-from-lookup

b3f4294

Merge branch 'master' into plain-hostedzone-id-from-lookup

849a128

Merge branch 'master' into plain-hostedzone-id-from-lookup

7659d1b

modified fix into fromValue instead.

6b14122

add hostedZoneArn attribute & tests.

AlexZeitler requested a review from SoManyHs as a code owner December 8, 2019 02:08

eladb reviewed Dec 12, 2019

View reviewed changes

shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Dec 12, 2019

shivlaks approved these changes Dec 12, 2019

View reviewed changes

shivlaks merged commit 5e08753 into aws:master Dec 12, 2019

AlexZeitler deleted the plain-hostedzone-id-from-lookup branch December 12, 2019 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(route53): return plain hosted zone id without /hostedzone/ prefix #5230

fix(route53): return plain hosted zone id without /hostedzone/ prefix #5230

AlexZeitler commented Nov 27, 2019 •

edited

Loading

mergify bot commented Nov 27, 2019

aws-cdk-automation commented Nov 27, 2019

aws-cdk-automation commented Nov 28, 2019

aws-cdk-automation commented Nov 28, 2019

eladb Nov 29, 2019

AlexZeitler Nov 29, 2019

aws-cdk-automation commented Nov 29, 2019

aws-cdk-automation commented Nov 29, 2019

aws-cdk-automation commented Nov 29, 2019

aws-cdk-automation commented Nov 29, 2019

aws-cdk-automation commented Dec 1, 2019

aws-cdk-automation commented Dec 8, 2019

eladb Dec 12, 2019

eladb Dec 12, 2019

eladb Dec 12, 2019

eladb Dec 12, 2019

fix(route53): return plain hosted zone id without /hostedzone/ prefix #5230

fix(route53): return plain hosted zone id without /hostedzone/ prefix #5230

Conversation

AlexZeitler commented Nov 27, 2019 • edited Loading

mergify bot commented Nov 27, 2019

aws-cdk-automation commented Nov 27, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 28, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 28, 2019

AWS CodeBuild CI Report

eladb Nov 29, 2019

Choose a reason for hiding this comment

AlexZeitler Nov 29, 2019

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 1, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 8, 2019

AWS CodeBuild CI Report

eladb Dec 12, 2019

Choose a reason for hiding this comment

eladb Dec 12, 2019

Choose a reason for hiding this comment

eladb Dec 12, 2019

Choose a reason for hiding this comment

eladb Dec 12, 2019

Choose a reason for hiding this comment

AlexZeitler commented Nov 27, 2019 •

edited

Loading