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.

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

Change Interface of Return Value From HostedZone.fromHostedZoneId() #6232

Closed
spencerbeggs opened this issue Feb 11, 2020 · 6 comments
Closed
Labels
@aws-cdk/aws-route53 Related to Amazon Route 53 closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@spencerbeggs
Copy link

spencerbeggs commented Feb 11, 2020

Since release 1.19.0, the object returned from HostedZone.fromHostedZoneId() no longer returns the zoneName property, but the function specifies that it will return an IHostedZone, which specifies the zoneName property as required. This is confusing in when using this function as you expect that property as being defined and it takes a bit of spelunking to figure out what is going on.

Is there a particular reason zoneName was removed in the first place? It seems there is no way to look up the zoneName property from a HosedZone id now.

Reproduction Steps

const hostedZone = HostedZone.fromHostedZoneId(this, "MyZone, "XXXXXX");
// hostedZone does not match the IHostedZone interface
// this throws
const  myZoneName = hostedZone.zoneName;

Error Log

This is expect from the code, but not from the interface.

HostedZone.fromHostedZoneId doesn't support "zoneName"

Environment

  • **CLI Version : 1.19.0+
  • **Framework Version: Typescript
  • **OS : macOs
  • **Language : English

Other

I propose we specify a valid interface to be returned from this method.


This is 🐛 Bug Report

@spencerbeggs spencerbeggs added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 11, 2020
@SomayaB SomayaB added the @aws-cdk/aws-route53 Related to Amazon Route 53 label Feb 11, 2020
@richardhboyd
Copy link
Contributor

The [Resource].from[Resource]Id() methods would usually just create a mock object with the resource ID populated so that you could pass it around as a "proper" resource. This had the benefit of not needing to do an SDK call to fetch the remaining values from the cloud. I believe the team is moving towards the fromAttributes() methods for looking up resources.

@spencerbeggs
Copy link
Author

spencerbeggs commented Feb 12, 2020

The object returned from HostedZone.fromHostedZoneId method specifies it returns an IHostedZone, but the return value does not conform to that interface, which is confusing. I ran into this problem when creating a construct.

The HostedZone. fromHostedZoneAttributes method requires both hostedZoneId and zoneName attributes. So, you need to know both to do the look up. So, if you are trying to import a hosted zone from another stack either by ID or zoneName you can only get the value of either zoneName or hostedZoneId, and that value is the one you already have. You can get these values if the same resources are created in the stack, however. Moreover, types that say they implement IHostedZone don't in these cases.

There might be a work around to look up the SOA record if you import the zone, but it seems to me that the zoneName and hostedZoneId are important pieces of information that you want when doing the look up.

If you want to see an example of what I am talking about check out this point in time from the repo for the construct I am working on. To hit the error, just:

yarn install
yarn test

@shivlaks
Copy link
Contributor

@spencerbeggs - I'm trying to see what we changed in 1.19.0 that would contribute to this change in behavior. Currently looking at the PR we shipped in that release

Are you certain it was working with 1.18.0 ?

@shivlaks shivlaks added the p2 label Feb 19, 2020
@spencerbeggs
Copy link
Author

spencerbeggs commented Feb 20, 2020

@shivlaks This breaking change was noted in the release notes from 1.19.0, but looking through the PR, it looks like that code was already in there. I setup a demo repo and went back as far as 1.5.0 and this error is thrown when accessing the property even then. So, perhaps the release notes were just confused.

But let me pitch one more time, it would be quite handy to have the sdk make an api call to lookup the zoneName (and also hostedZoneNameServers). I read in another thread that the fromClassId methods being implemented for the most part just create dumb objects to pass around until their values are filled in later, but like looking up a VPC by ID elsewhere in the library, it seems really worthwhile to actually make the api call to retrieve this info during synthesis as other parts of your stack may need to be responsive to them. It is quite common to separate high level components like VPCs and HostedZone into separate stacks.

Failing that, perhaps we could change the return interface as throwing an error is definitely confusing when you look at the IHostedZone interface definition and tooling can't warn you about it.

Thank you for taking the time to look into this. CDK is a really neat tool that has let me build some cool stuff these past couple months.

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Mar 4, 2020
@shivlaks
Copy link
Contributor

@spencerbeggs I think that makes sense as a feature request. do you. mind updating the issue with the use case and proposed solution? I'm going to drop the bug label from this one and I think we can repurpose it to track the effort.

It'll help us prioritize the issue!

@shivlaks shivlaks added feature-request A feature should be added or improved. and removed bug This issue is a bug. p2 labels Mar 11, 2020
@shivlaks shivlaks added the effort/medium Medium work item – several days of effort label Apr 23, 2020
@shivlaks shivlaks added the p2 label Aug 26, 2020
@NGL321 NGL321 assigned njlynch and unassigned shivlaks Jan 25, 2021
@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-route53 Related to Amazon Route 53 closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

No branches or pull requests

5 participants