feat(route53-targets): aws-apigatewayv2 target

[johanneswuerbach]

Add support for aws-apigatewayv2 DomainName as target.

Closes #8941

---

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

[shivlaks]

thanks for getting this started @johanneswuerbach

I had some questions and some small comments.
I think we should also add an integration test for this target.

[shivlaks]

@johanneswuerbach could you also add an integration test?

[johanneswuerbach]

Sure, could you point me to an example test?

[christophgysin]

@johanneswuerbach Here's an example integration test:
https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-route53-targets/test/integ.api-gateway-domain-name.ts

[gitpod-io]

Pull request has been modified.

[johanneswuerbach]

@christophgysin thanks for the pointer.

@shivlaks integration test added.

[shivlaks]

code looks good, I think we need to tweak the integ test a little bit before we can merge it in

[shivlaks]

integration tests should be deployable (i.e. running npm integ *your test.js* will deploy the stack and automatically write the .expected file)

should this test be creating a certificate and then passing it's ARN? I can't imagine it deploys with the hardcoded certArn value

[christophgysin]

@shivlaks I haven't tried it, but this is exactly how it is done in the existing test: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-route53-targets/test/integ.api-gateway-domain-name.ts#L15

[shivlaks]

@shivlaks I haven't tried it, but this is exactly how it is done in the existing test: https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-route53-targets/test/integ.api-gateway-domain-name.ts#L15

if it's not deployable it's probably also a bug (or there for a reason). I'd love to find out whether we can do better before merging it in though. wdyt?

[christophgysin]

I'm neither the author of this nor the existing test. But I can imagine that the certificate ARN and domain name were changed to an existing cert/domain in the account for testing.

Automating certificate creation is not recommended, because you quickly run into the account limit of 20 certificates per year.

[johanneswuerbach]

Anything I can do to move this forward?

[christophgysin]

@shivlaks Can you comment how to proceed here? Should we automate certificate creation despite the limits? Is there another way to test this? Or this the current approach (not ideal, but consistent with targets.ApiGateway) acceptable in this case?

[shivlaks]

@christophgysin @johanneswuerbach - discussed the outstanding integ test comment with @njlynch - and we agreed that it's probably better to drop it if it's not executable. The rationale is that it doesn't really buy us much protection / validation that cannot be covered through unit tests themselves.

So: let's drop it and ship this PR!!

[johanneswuerbach]

@shivlaks thanks for the update, integration tests removed.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 561656e
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[mergify]

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