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-targets): add AppSync route53 target #31976

Merged
merged 12 commits into from
Nov 29, 2024

Conversation

ScottRobinson03
Copy link
Contributor

Issue # (if applicable)

Closes #26109

Reason for this change

This PR adds support for creating alias records on AppSync's GraphqlApi.

Description of changes

  • Add appsync-target.ts file, with the AppSync class.
  • Add unit tests for this new AppSync target class.
  • Update README.md of aws-cdk-lib/aws-route53-targets

Description of how you validated changes

  • Add unit tests

Checklist


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

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 1, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team November 1, 2024 12:02
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 1, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@ScottRobinson03 ScottRobinson03 changed the title feat(route53-targets): Add AppSync route53 target feat(route53-targets): add AppSync route53 target Nov 1, 2024
@ScottRobinson03
Copy link
Contributor Author

Clarification Request

I've written an integration test, but when trying to run it from aws-cdk/packages/@aws-cdk-testing/framework-integ via yarn integ --update-on-failed integ.appsync-target.js (as per the instructions) I'm getting the following error:
image

Trying to debug I thought maybe I have to build things first, so I ran yarn build, but that then spews out a load of the following:
image

Running yarn install says "Already up-to-date", so I'm not sure what's going on or how I'm meant to do run the added integration test. Assistance is appreciated.

@aws-cdk-automation aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Nov 1, 2024
@paulhcsun
Copy link
Contributor

Hey Scott, I don't see anything incorrect in your integ test file. I would recommend just running a full clean rebuild with

$ git clean -fqdx .
$ yarn install
$ yarn build

and see if that resolves the import error.

@paulhcsun paulhcsun removed the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Nov 1, 2024
@ScottRobinson03
Copy link
Contributor Author

Hey Scott, I don't see anything incorrect in your integ test file. I would recommend just running a full clean rebuild with

$ git clean -fqdx .
$ yarn install
$ yarn build

and see if that resolves the import error.

@paulhcsun Thanks, this did indeed fix the issue and I'm now able run the test. Although, as the comment within the file kinda says (copied from the route53-targets ApiGateway integ test), it's a somewhat redundant test since the stack can never deploy due to the hardcoded certificate & hosted zone arns.

When running (via yarn integ --update-on-failed aws-route53-targets/test/integ.appsync-target.js) I get the following output:
image

I think this means I need a "Exemption Request".

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Nov 5, 2024
@paulhcsun
Copy link
Contributor

paulhcsun commented Nov 6, 2024

Hey Scott, I don't see anything incorrect in your integ test file. I would recommend just running a full clean rebuild with

$ git clean -fqdx .
$ yarn install
$ yarn build

and see if that resolves the import error.

@paulhcsun Thanks, this did indeed fix the issue and I'm now able run the test. Although, as the comment within the file kinda says (copied from the route53-targets ApiGateway integ test), it's a somewhat redundant test since the stack can never deploy due to the hardcoded certificate & hosted zone arns.

When running (via yarn integ --update-on-failed aws-route53-targets/test/integ.appsync-target.js) I get the following output: image

I think this means I need a "Exemption Request".

Awesome! And gotcha, let me just confirm with the team member who added that explanation and if there's no other way to test this then I can add the integ teste exempted label.

Edit:

Actually it seems like there are some integ tests for the specific targets, for example this one for interface-vpc-endpoint-target. Would you be able to reference these tests and write out for AppSync as a target as well?

@ScottRobinson03
Copy link
Contributor Author

Actually it seems like there are some integ tests for the specific targets, for example this one for interface-vpc-endpoint-target. Would you be able to reference these tests and write out for AppSync as a target as well?

@paulhcsun I believe the key difference with API Gateway and AppSync is that they both require a certificate to be created in order to have a custom domain.

The other route53-targets (such as interface-vpc-endpoint-target) don't require this certificate, which is why integ tests are possible.

It's not entirely clear to me why having a certificate makes it impossible to integ test, but that's how the API Gateway integ tests' comment reads to me.

If I'm misunderstanding then please advise accordingly.

@mazyu36
Copy link
Contributor

mazyu36 commented Nov 8, 2024

Hi.
Just for information, to run a test that requires a domain and a certificate, you'll need to set up a domain and configure it in the Environment Variables before running an integ test.

This requirement is noted in the following docs:
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/README.md

I've recently created an integ test that requires a domain:
https://github.com/aws/aws-cdk/pull/30784/files#diff-f22841cd9a989a8be48e18db20b731abb510c127c0c9f7dc07e953b702a7a385

Do you have a domain available for an integ test?
If not, it won't be possible to run the integ test.

@aws-cdk-automation aws-cdk-automation dismissed their stale review November 8, 2024 14:22

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@ScottRobinson03
Copy link
Contributor Author

ScottRobinson03 commented Nov 8, 2024

Thanks for this @mazyu36, I was able to get a passing integ test running: fd783f7

Note that the submitted snapshot has dummy values, since I don't want to leak my domain/certificate/hostedzone.

I think this means I no longer require an exemption, but not sure how to cancel the request.

@ScottRobinson03
Copy link
Contributor Author

The CodeBuild is failing because it's not providing a certificate arn env var:
image

What's the correct way to fix this? I suppose I need to somehow tell CodeBuild to not run this specific test? Not sure...

Copy link
Contributor

@mazyu36 mazyu36 left a comment

Choose a reason for hiding this comment

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

I’m not entirely confident, but I commented based on my experience from when I previously ran the integ test that uses a domain.

@paulhcsun paulhcsun removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Nov 8, 2024
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.42%. Comparing base (8ccdff4) to head (17eb322).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #31976      +/-   ##
==========================================
- Coverage   78.45%   78.42%   -0.03%     
==========================================
  Files         106      106              
  Lines        7208     7208              
  Branches     1323     1323              
==========================================
- Hits         5655     5653       -2     
- Misses       1365     1368       +3     
+ Partials      188      187       -1     
Flag Coverage Δ
suite.unit 78.42% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 78.42% <ø> (-0.03%) ⬇️

Copy link
Contributor

@mazyu36 mazyu36 left a comment

Choose a reason for hiding this comment

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

Thanks!

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 22, 2024
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

Left a nit comment on the AppSync target naming

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 25, 2024
@mergify mergify bot dismissed GavinZZ’s stale review November 27, 2024 10:07

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 27, 2024
GavinZZ
GavinZZ previously approved these changes Nov 28, 2024
Copy link
Contributor

mergify bot commented Nov 28, 2024

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Nov 28, 2024
Copy link
Contributor

mergify bot commented Nov 28, 2024

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

@mergify mergify bot dismissed GavinZZ’s stale review November 28, 2024 21:32

Pull request has been modified.

Copy link
Contributor

mergify bot commented Nov 28, 2024

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

Copy link
Contributor

mergify bot commented Nov 28, 2024

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

@GavinZZ
Copy link
Contributor

GavinZZ commented Nov 29, 2024

@mergify update

Copy link
Contributor

mergify bot commented Nov 29, 2024

update

☑️ Nothing to do

  • #commits-behind > 0 [📌 update requirement]
  • -closed [📌 update requirement]
  • -conflict [📌 update requirement]
  • queue-position = -1 [📌 update requirement]

@ScottRobinson03
Copy link
Contributor Author

If I'm interpreting the checks correctly, it looks like this isn't being merged because the code coverage apparently decreased... I'm not quite sure what to do about this, since my changes do have appropriate tests (as far as I can tell).

@GavinZZ are you able to give any insight on this?

@GavinZZ
Copy link
Contributor

GavinZZ commented Nov 29, 2024

@mergify update

Copy link
Contributor

mergify bot commented Nov 29, 2024

update

✅ Branch has been successfully updated

@GavinZZ
Copy link
Contributor

GavinZZ commented Nov 29, 2024

Code coverage is something we introduced and I believe there's still some issue with it. Will reach out to the team internally to see what's going on and update you once I hear back.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 17eb322
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

mergify bot commented Nov 29, 2024

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

@GavinZZ GavinZZ merged commit dc7574a into aws:main Nov 29, 2024
13 of 15 checks passed
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

route53-targets: Add support for GraphqlApiDomainTarget
5 participants