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

feat(route53): fromPublicHostedZoneAttributes method with zoneName #19771

Merged
merged 6 commits into from
Apr 11, 2022

Conversation

peterwoodworth
Copy link
Contributor

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

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
Copy link
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
Contributor Author

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
Contributor Author

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

@comcalvi
Copy link
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
Contributor Author

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
Contributor Author

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
Contributor Author

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

@peterwoodworth
Copy link
Contributor Author

@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.

Copy link
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
Contributor Author

Added one @comcalvi 👍🏻

Copy link
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
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
Collaborator

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
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
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

(route53): PublicHostedZone fromPublicHostedZoneAttributes function with zoneName support
3 participants