Skip to content

feat(route53): fromPublicHostedZoneAttributes method with zoneName#19771

Merged
mergify[bot] merged 6 commits intomasterfrom
peterwoodworth/publicZoneImportFeat
Apr 11, 2022
Merged

feat(route53): fromPublicHostedZoneAttributes method with zoneName#19771
mergify[bot] merged 6 commits intomasterfrom
peterwoodworth/publicZoneImportFeat

Conversation

@peterwoodworth
Copy link
Copy Markdown
Contributor

@peterwoodworth peterwoodworth commented Apr 5, 2022

fixes #18700

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use cdk-integ to deploy the infrastructure and generate the snapshot (i.e. cdk-integ without --dry-run)?

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

@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Apr 5, 2022

@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Apr 5, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team April 5, 2022 21:03
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 5, 2022
comcalvi
comcalvi approved these changes Apr 5, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

Conditional approval on adding the README changes.

@comcalvi comcalvi added the pr/do-not-merge This PR should not be merged at this time. label Apr 5, 2022
@peterwoodworth peterwoodworth added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 5, 2022
@peterwoodworth
Copy link
Copy Markdown
Contributor Author

peterwoodworth commented Apr 5, 2022

Wasn't expecting an approval so fast haha, was gonna add that soon!

I also want to add testing for this, but I'm not sure what the best way to test this is - the other import methods aren't tested. If they were, we'd need to either test it in route53 and add elasticloadbalancingV2 as a dependency or vice versa. This is because the only construct which uses an IPublicHostedZone that I can find that uses zoneName prop is VpcEndpointServiceDomainName which requires you to create a NLB

Will that as simple as just adding the dependency to the package.json file in the respective package? I tried that at first but ran into some weird errors about not being able to export Cfn properties

@peterwoodworth
Copy link
Copy Markdown
Contributor Author

peterwoodworth commented Apr 5, 2022

@comcalvi in case this doesn't end up in your queue otherwise 😛

@comcalvi
Copy link
Copy Markdown
Contributor

comcalvi commented Apr 6, 2022

Yep, you should be able to add @aws-cdk/aws-elasticloadbalancingv2 as a dependency of route53 by adding it to the dependencies section of the package.json and running yarn install --frozen-lockfile. It would be awesome if you added testing for these methods.

@comcalvi comcalvi self-assigned this Apr 6, 2022
@peterwoodworth
Copy link
Copy Markdown
Contributor Author

peterwoodworth commented Apr 6, 2022

Added testing in elbv2 package due to what I assume is creating a circular dependency when I add the elbv2 package to route53

1 similar comment
@peterwoodworth
Copy link
Copy Markdown
Contributor Author

peterwoodworth commented Apr 6, 2022

Added testing in elbv2 package due to what I assume is creating a circular dependency when I add the elbv2 package to route53

@peterwoodworth peterwoodworth changed the title feat(route53): fromPubicHostedZoneAttributes method with zoneName feat(route53): fromPublicHostedZoneAttributes method with zoneName Apr 6, 2022
@peterwoodworth
Copy link
Copy Markdown
Contributor Author

peterwoodworth commented Apr 6, 2022

... don't look at what the title used to be 🤦🏻

@peterwoodworth
Copy link
Copy Markdown
Contributor Author

peterwoodworth commented Apr 8, 2022

@comcalvi can you take a quick look at this at some point to make sure it's all good with the recent additions? I don't want to just remove the label and let this go through without you taking another look.

comcalvi
comcalvi previously requested changes Apr 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

This looks almost perfect, but can you add one test that verifies the behavior of fromPublicHostedZoneId()? I know that method already existed, but we should have had tests for it before.

@peterwoodworth
Copy link
Copy Markdown
Contributor Author

peterwoodworth commented Apr 9, 2022

Added one @comcalvi 👍🏻

comcalvi
comcalvi approved these changes Apr 11, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

Looks great!

@peterwoodworth peterwoodworth removed the pr/do-not-merge This PR should not be merged at this time. label Apr 11, 2022
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Apr 11, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Apr 11, 2022

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: a499edb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 7867dc4 into master Apr 11, 2022
@mergify mergify bot deleted the peterwoodworth/publicZoneImportFeat branch April 11, 2022 20:28
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Apr 11, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

StevePotter pushed a commit to StevePotter/aws-cdk that referenced this pull request Apr 27, 2022
@peterwoodworth
feat(route53): fromPublicHostedZoneAttributes method with zoneName (a…
e4d0dcc 
…ws#19771)

fixes aws#18700

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@kwagodev kwagodev mentioned this pull request Jul 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@comcalvi comcalvi comcalvi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@comcalvi comcalvi

Labels

contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

(route53): PublicHostedZone fromPublicHostedZoneAttributes function with zoneName support

4 participants

@peterwoodworth @comcalvi @aws-cdk-automation @woodwoop-ospo