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

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

Open glitchassassin opened this issue 1 year ago • 5 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 May 07 '24 17:05 glitchassassin

Clarification Request

I am not sure I understand how to run/resolve the integration test snapshots - would appreciate some guidance

glitchassassin avatar May 07 '24 19:05 glitchassassin

...and it seems the pr-linter-exception-labeler action may be broken too - it's failing with an error

glitchassassin avatar May 07 '24 21:05 glitchassassin

@glitchassassin Thanks for drafting the PR. The CodeBuild CI failed because you updated the custom resource handler file, which means the code hash of the handler file changes. This results in all these integration tests that uses this custom resource when comparing the snapshots.

What you can do is to look at the build log and search for !!!!!!!!. You should then see a list of failed integration tests. For each failed tests, you can run yarn integ aws-elasticloadbalancingv2/test/integ.alb.oidc --update-on-failed (replace the path with the actual test).

More can be found in https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md.

GavinZZ avatar May 08 '24 20:05 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 May 13 '24 20:05 aws-cdk-automation

AWS CodeBuild CI Report

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

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

aws-cdk-automation avatar May 13 '24 21:05 aws-cdk-automation

Thanks Gavin. I ran the integration tests like this and then committed the results:

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

I got a SUCCESS message locally for most of them. A few failed with an error similar to this:

The stack named LogGroupDefaultTestDeployAssert353EE07A failed to deploy: CREATE_FAILED (The following resource(s) failed to create: [AwsApiCallCloudWatchLogsdescribeResourcePolicies]. ): Response object is too long.

I pushed up the changes for the ones that passed, but it looks like CodeBuild is still failing all of those integration tests. Have I missed a step?

glitchassassin avatar May 13 '24 21:05 glitchassassin

Prior to running the integ tests, did you run yarn build in npx lerna run build --scope=@aws-cdk/custom-resource-handlers and npx lerna run build --scope=aws-cdk-lib to make sure the correct lambda handler file is used in integ tests?

GavinZZ avatar May 13 '24 21:05 GavinZZ

I'm pretty sure I did, but just in case, I re-ran the build and tried the integration tests again. I'm not seeing any differences.

I noticed that on some of them (e.g. aws-elasticloadbalancingv2/test/integ.alb.oidc.js) the build reports "destructive changes" because I provided a different hosted zone/domain name per framework-integ/README.md. I'm not sure how to resolve this.

glitchassassin avatar May 15 '24 17:05 glitchassassin

For the breaking changes, using integ.alb.oidc test as an example, I saw the template.json and assets.json are modified manually. That could cause breaking changes.

GavinZZ avatar May 15 '24 20:05 GavinZZ

yes, those changes were generated by following these steps to set the hosted zone:

https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/README.md#error-for-this-test-you-must-provide-your-own-hostedzoneidhostedzonenamedomainname

glitchassassin avatar May 15 '24 21:05 glitchassassin

Hi @glitchassassin

I'll open your PR branch later today and run the integ tests from my IDE. You can reach out to me on cdk.dev if you need any discussion around the PR contribution. I am more than happy to help.

pahud avatar May 22 '24 20:05 pahud

Hi @glitchassassin

I have created a PR to your branch to update the integ tests https://github.com/glitchassassin/aws-cdk/pull/2

Can you merge my PR to your branch and this PR should update immediately. Let's see if the CI would pass.

pahud avatar May 23 '24 16:05 pahud

OK looks like your PR got new integ error

@aws-cdk-testing/framework-integ:   CHANGED    custom-resources/test/aws-custom-resource/integ.aws-custom-resource-dynamodb 1.258s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ: [~] AWS::Lambda::Function AWS679f53fac002430cb0da5b7982bd22872D164C4C 
@aws-cdk-testing/framework-integ:  └─ [~] Code
@aws-cdk-testing/framework-integ:      └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:          ├─ [-] 4c349b026f9dadd9a6737d4682c2da3e53101244798442ccec89ee0a0bd96f26.zip
@aws-cdk-testing/framework-integ:          └─ [+] 107e4ea7fe26b0049a79003e5710556207812d452795845039064ba20e7b7d56.zip
@aws-cdk-testing/framework-integ:   CHANGED    custom-resources/test/aws-custom-resource/integ.aws-custom-resource-athena 1.381s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ: [~] AWS::Lambda::Function AWS679f53fac002430cb0da5b7982bd22872D164C4C 
@aws-cdk-testing/framework-integ:  └─ [~] Code
@aws-cdk-testing/framework-integ:      └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:          ├─ [-] 4c349b026f9dadd9a6737d4682c2da3e53101244798442ccec89ee0a0bd96f26.zip
@aws-cdk-testing/framework-integ:          └─ [+] 107e4ea7fe26b0049a79003e5710556207812d452795845039064ba20e7b7d56.zip
@aws-cdk-testing/framework-integ:   UNCHANGED  custom-resources/test/aws-custom-resource/integ.aws-custom-resource-ssm 1.368s
@aws-cdk-testing/framework-integ:   UNCHANGED  custom-resources/test/aws-custom-resource/integ.invoke-function-payload 1.276s
@aws-cdk-testing/framework-integ:   UNCHANGED  custom-resources/test/aws-custom-resource/integ.cross-account-assumeRole 1.506s
@aws-cdk-testing/framework-integ:   CHANGED    custom-resources/test/aws-custom-resource/integ.aws-custom-resource 1.662s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ: [~] AWS::Lambda::Function AWS679f53fac002430cb0da5b7982bd22872D164C4C 
@aws-cdk-testing/framework-integ:  └─ [~] Code
@aws-cdk-testing/framework-integ:      └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:          ├─ [-] 4c349b026f9dadd9a6737d4682c2da3e53101244798442ccec89ee0a0bd96f26.zip
@aws-cdk-testing/framework-integ:          └─ [+] 107e4ea7fe26b0049a79003e5710556207812d452795845039064ba20e7b7d56.zip

and

@aws-cdk-testing/framework-integ:   CHANGED    aws-synthetics/test/integ.canary-auto-delete-lambda 4.905s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ: [~] AWS::Lambda::Function AWS679f53fac002430cb0da5b7982bd22872D164C4C 
@aws-cdk-testing/framework-integ:  └─ [~] Code
@aws-cdk-testing/framework-integ:      └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:          ├─ [-] 746da84b10e215c552e68b6d2061024e4429f0386f43a35ef5e4d2940655692e.zip
@aws-cdk-testing/framework-integ:          └─ [+] 107e4ea7fe26b0049a79003e5710556207812d452795845039064ba20e7b7d56.zip
@aws-cdk-testing/framework-integ:   UNCHANGED  pipelines/test/integ.pipeline-with-stack-outputs-in-custom-step 1.233s
@aws-cdk-testing/framework-integ:   CHANGED    custom-resources/test/aws-custom-resource/integ.aws-custom-resource-vpc 3.48s
@aws-cdk-testing/framework-integ:       Resources
@aws-cdk-testing/framework-integ: [~] AWS::Lambda::Function AWS679f53fac002430cb0da5b7982bd22872D164C4C 
@aws-cdk-testing/framework-integ:  └─ [~] Code
@aws-cdk-testing/framework-integ:      └─ [~] .S3Key:
@aws-cdk-testing/framework-integ:          ├─ [-] 4c349b026f9dadd9a6737d4682c2da3e53101244798442ccec89ee0a0bd96f26.zip
@aws-cdk-testing/framework-integ:          └─ [+] 107e4ea7fe26b0049a79003e5710556207812d452795845039064ba20e7b7d56.zip

You'll need to re-run all the CHANGED integ tests and update all their snapshots.

I can't run all of them for you. Please reach out to me on cdk.dev slack if you need any help I can show you how to do that with a video.

pahud avatar May 24 '24 16:05 pahud

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation avatar May 29 '24 00:05 aws-cdk-automation

not abandoned; still working through integration test issues

glitchassassin avatar May 29 '24 00:05 glitchassassin

AWS CodeBuild CI Report

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

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

aws-cdk-automation avatar May 29 '24 20:05 aws-cdk-automation

Cross-posting from the Slack discussion:

I'm not sure there's a good way to test these changes in an integration test, as it's the handler source code that is changing. There is a unit test for the change, but I'm not sure how to force the error in an integration test scenario. Any suggestions, or should I request an exemption?

glitchassassin avatar Jun 05 '24 20:06 glitchassassin

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

aws-cdk-automation avatar Jun 06 '24 00:06 aws-cdk-automation

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 Jun 06 '24 00:06 aws-cdk-automation

⚠️ The sha of the head commit of this PR conflicts with #30469. Mergify cannot evaluate rules on this PR. ⚠️

mergify[bot] avatar Jun 06 '24 12:06 mergify[bot]

Reopening here: https://github.com/aws/aws-cdk/pull/30469

glitchassassin avatar Jun 06 '24 12:06 glitchassassin