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

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

Closed
wants to merge 22 commits into from

Conversation

glitchassassin
Copy link
Contributor

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

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jun 6, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team June 6, 2024 12:02
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.

@TheRealAmazonKendra
Copy link
Contributor

Thanks for your contribution. We can't take this just yet because it will conflict with node runtime upgrades (#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 TheRealAmazonKendra added the pr-linter/do-not-close The PR linter will not close this PR while this label is present label Jun 6, 2024
@TheRealAmazonKendra TheRealAmazonKendra self-assigned this Jun 6, 2024
@glitchassassin
Copy link
Contributor Author

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.

@aws-cdk-automation aws-cdk-automation dismissed their stale review June 13, 2024 21:30

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

@glitchassassin
Copy link
Contributor Author

@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

@TheRealAmazonKendra
Copy link
Contributor

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

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

@glitchassassin
Copy link
Contributor Author

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

@glitchassassin
Copy link
Contributor Author

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

@GavinZZ
Copy link
Contributor

GavinZZ commented Aug 21, 2024

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.

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.

@aws-cdk-automation
Copy link
Collaborator

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

AWS CodeBuild CI Report

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

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

@glitchassassin
Copy link
Contributor Author

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

@GavinZZ
Copy link
Contributor

GavinZZ commented Sep 13, 2024

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

@glitchassassin
Copy link
Contributor Author

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

Closing in favor of #31571

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 Sep 26, 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 bug This issue is a bug. effort/small Small work item – less than a day of effort p2 pr-linter/do-not-close The PR linter will not close this PR while this label is present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(custom-resources): Package does not exist
5 participants