aws-cdk icon indicating copy to clipboard operation
aws-cdk copied to clipboard

fix(custom-resource-handlers): better fallback for require failures

Open glitchassassin opened this issue 1 year ago • 11 comments

Issue # (if applicable)

Closes #30067.

Reason for this change

Fallback to existing AWS SDK import misses a rare/flaky edge case where the npm install passes, but the subsequent require fails

Description of changes

Fall back to the pre-existing AWS SDK if requiring the latest version fails

Description of how you validated changes

  • Fixed no-op test "installs the latest SDK"
  • Added test "falls back to installed sdk if require fails"

Checklist


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

glitchassassin avatar Jun 06 '24 12:06 glitchassassin

Thanks for your contribution. We can't take this just yet because it will conflict with node runtime upgrades (https://github.com/aws/aws-cdk/pull/30108) but once that PR is merged we will take a look at this one.

Unfortunately, when that one is merged it will cause merge conflicts for all of your integ tests so I would advise holding off on any additional work here until that time so you're not having to deal with a lot of churn. In the meantime, I'm going to add the do-not-close label to this PR so you don't have to recreate it again if merging that one takes too long (it should not, but I'm just going to be cautious).

TheRealAmazonKendra avatar Jun 06 '24 16:06 TheRealAmazonKendra

I've pushed up the integration test snapshot updates I've gotten through so far. I'm out of the office for the weekend, so if someone else wants to help with those that'd be great! Otherwise, I'll finish them up when I get a chance.

glitchassassin avatar Jun 13 '24 21:06 glitchassassin

@TheRealAmazonKendra I've updated most of the integration tests, but there are a few that appear to be failing due to trying to assume roles without permissions. Anything you can do for the last few tests?

yarn integ --update-on-failed aws-appsync/test/integ.js-resolver.js
yarn integ --update-on-failed aws-certificatemanager/test/integ.dns-validated-certificate.js
yarn integ --update-on-failed aws-chatbot/test/integ.chatbot-logretention.js
yarn integ --update-on-failed aws-cloudfront/test/integ.cloudfront-cross-region-cert.js
yarn integ --update-on-failed aws-cloudfront/test/integ.distribution-function-runtime.js
yarn integ --update-on-failed aws-cloudfront/test/integ.distribution-lambda-cross-region.js

glitchassassin avatar Jun 25 '24 15:06 glitchassassin

Don't make anymore snapshot updates for now. We are continuing to have some problems around them so I'm going to add a do-not-close label to this PR and then probably appear to be ignoring it for another couple weeks. I really appreciate your patience with these delays.

BTW, I did see your message on cdk.dev and that is the right way to get my attention.

TheRealAmazonKendra avatar Jul 03 '24 21:07 TheRealAmazonKendra

Oh, the label is already added. Good. I absolutely plan to come back to this first once I get the other wrinkles ironed out.

TheRealAmazonKendra avatar Jul 03 '24 21:07 TheRealAmazonKendra

sounds good; I'll check back in after two weeks!

glitchassassin avatar Jul 03 '24 21:07 glitchassassin

@TheRealAmazonKendra How are we looking now? Ready to pick this back up?

glitchassassin avatar Aug 08 '24 12:08 glitchassassin

Hello @glitchassassin, the node runtime upgrade is complete and please address the conflicts. I'll take a review on this PR by end of month.

GavinZZ avatar Aug 21 '24 23:08 GavinZZ

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

aws-cdk-automation avatar Aug 28 '24 13:08 aws-cdk-automation

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 5c0960f09f55fc4c3124fb4d8fa2f686f424ab37
  • Result: FAILED
  • Build Logs (available for 30 days)

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

aws-cdk-automation avatar Aug 28 '24 14:08 aws-cdk-automation

@GavinZZ, I re-ran these failing integration tests locally like so:

yarn integ --update-on-failed custom-resources/test/aws-custom-resource/integ.aws-custom-resource.js

For most of these, this appeared to successfully update the snapshot and pass, but when I re-ran yarn integ, all of the tests once again failed.

glitchassassin avatar Aug 28 '24 16:08 glitchassassin

@glitchassassin Sorry for the late response. Because the changes were made in @aws-cdk/custom-resource-handlers module, you need to

  1. navigate to packages/@aws-cdk/custom-resource-handlers/
  2. run yarn build
  3. run npx lerna run build --scope=aws-cdk-lib
  4. then re-run the failed tests.

GavinZZ avatar Sep 13 '24 22:09 GavinZZ

@GavinZZ Thanks. I did re-run those builds, but it's having no effect: when I update the snapshot, the test still fails the next time I run yarn integ.

glitchassassin avatar Sep 16 '24 14:09 glitchassassin

Closing in favor of #31571

glitchassassin avatar Sep 26 '24 12:09 glitchassassin

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[bot] avatar Sep 26 '24 12:09 github-actions[bot]