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.

Sign up for GitHub

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

Jump to bottom

chore: remove sdk v2 handler and associated tests #29868

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

chore: remove sdk v2 handler and associated tests #29868

wants to merge 37 commits into from
+1,723 −1,298

Conversation

colifran
Copy link
Contributor

@colifran colifran commented Apr 17, 2024 •
edited

Issue # (if applicable)

Closes #29882

Reason for this change

The AWS SDK V2 is being deprecated. We need to remove all uses of it from the CDK.

Description of changes

I removed the v2 handler file and all associated unit tests. Additionally, I refactored the v3 handler and renamed it to index.ts. The refactor I did consolidated logic that was spread across multiple files to be shared between the v2 handler and the v3 handler. Utility related functions are now contained in utils.ts and logic related to loading the SDK package that will be used in making the specific API call is now contained in load-sdk.ts.

Description of how you validated changes

As a result of the handler file name changing and the handler logic being refactored, the asset hash for the v3 handler changed. The asset hash change resulted in integ test failures. All impacted integ tests were updated and deployed with successful results.

Checklist

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

@colifran
remove sdk v2 handler and tests
f880fe0 
Signed-off-by: Francis <colifran@amazon.com>
@github-actions github-actions bot added the p2 label Apr 17, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team April 17, 2024 16:36
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 17, 2024
colifran and others added 12 commits April 17, 2024 11:34
@colifran
custom resource snaps
c84ea57 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
open search snaps
e811716 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
Merge branch 'main' into colifran/remove-v2-handler
390b1e4
@colifran
events targets snaps
bcce8c7 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
logs destinations snaps
6cd51f3 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
route53 snaos
e0b77c8 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
synthetics snaps
a3a6772 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
global accelerator snaps
84073d9 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
elastic search snaps
2280ff7 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
s3 snaps
6d017b8 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
elastic load balancing v2 snaps
fbeb93c 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
elastic load balancing v2 actions snaps
4de2618 
Signed-off-by: Francis <colifran@amazon.com>
@colifran colifran changed the title chore: remove sdk v2 handler and tests chore: remove sdk v2 handler and associated tests Apr 18, 2024
@colifran colifran marked this pull request as ready for review April 18, 2024 01:49
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 18, 2024
@colifran colifran added the pr/do-not-merge This PR should not be merged at this time. label Apr 18, 2024
@github-actions github-actions bot mentioned this pull request Apr 22, 2024
Closed
comcalvi
comcalvi requested changes Apr 23, 2024
View reviewed changes
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Outdated Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Outdated Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Outdated Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Outdated Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Outdated Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Outdated Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Show resolved Hide resolved
.../@aws-cdk/custom-resource-handlers/lib/custom-resources/aws-custom-resource-handler/index.ts Show resolved Hide resolved
@colifran colifran mentioned this pull request Apr 23, 2024
Merged
1 task
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 23, 2024
mergify bot pushed a commit that referenced this pull request Apr 23, 2024
@colifran
chore: add assertions to integ tests using AwsCustomResource (#29929)
1bdd3fa 
Closes #29937 

### Reason for this change

Adding assertions for `AwsCustomResource` will improve our test coverage.

### Description of changes

Added assertions for `AwsCustomResource` to integ tests that were impacted by this [PR](#29868).

### Description of how you validated changes

Assertions were validated by running the integ tests and verifying that it succeeded.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@colifran
give functions return types
f33da94 
Signed-off-by: Francis <colifran@amazon.com>
colifran and others added 8 commits April 23, 2024 16:37
@colifran
refactor
d700235 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
refactor
8f10f53 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
Merge branch 'main' into colifran/remove-v2-handler
0db3f3b
@colifran
custom resource snaps
84164a6 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
open search snaps
580cc60 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
docstring
e9fe02c 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
update docstring
3e1d7f3 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
update docstring
015e535 
Signed-off-by: Francis <colifran@amazon.com>
@SankyRed SankyRed mentioned this pull request Apr 24, 2024
Merged
1 task
comcalvi
comcalvi approved these changes Apr 24, 2024
View reviewed changes
Copy link
Contributor

@comcalvi comcalvi left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work refactoring this thorny code, I think it's in a much better state now.

mergify bot pushed a commit that referenced this pull request Apr 25, 2024
@SankyRed
chore: add assertions to integ tests using AwsCustomResource (#29950)
536d8ff 
Closes #29937 

### Reason for this change

Adding assertions for `AwsCustomResource` will improve our test coverage.

### Description of changes

Added assertions for `AwsCustomResource` to integ tests that were impacted by this [PR](#29868).

### Description of how you validated changes

Assertions were validated by running the integ tests and verifying that it succeeded.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@colifran
merge conflicts
17ca527 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
synthetics snaps
d937159 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
s3 snaps
c3f5987 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
route53 snaps
2294a2c 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
logs destinations snaps
d1dfe89 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
global accelerator snaps
3ad6c89 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
events targets snaps
80f9c5b 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
elastic search snaps
6ad51a5 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
efs snaps
d4217f2 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
elastic load balancing v2 snaps
5d3cc92 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
fix import
7474b09 
Signed-off-by: Francis <colifran@amazon.com>
@colifran
cognito
7d2766d 
Signed-off-by: Francis <colifran@amazon.com>
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@github-actions github-actions bot mentioned this pull request May 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comcalvi comcalvi comcalvi approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS. p2 pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SDKv3-01: Update @aws-cdk/custom-resource-handlers
3 participants
@colifran @aws-cdk-automation @comcalvi