Skip to content

feat!: Remove AWS SDK V2 Dependency #1180

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

Merged
merged 10 commits into from
Jul 5, 2023

Conversation

texastony
Copy link
Contributor

@texastony texastony commented Jun 30, 2023

Issue #, if available: #1100, #922, #865, #1178, #1177

Description of changes:
In CodeBuild Builds:

  1. Always specify the exact Node version
  2. Name files after the Node Version
  3. Stop testing NodeJS 12 & 14
  4. Start testing NodeJS 20
  5. Use Node 18 for compliance

BREAKING CHANGE:
The AWS Encryption SDK for JavaScript:

  • requires the AWS SDK for JavaScript V3's kms-client (if using the KMS Keyring).
  • no longer requires the AWS SDK V2
  • no longer tests against nor supports NodeJS 12 or 14.

Note: @seebees I had to merge #1177 and #1178 to get CodeBuild to pass.
I think the new package-lock only supports Node >= 16,
so I needed to drop the Node 14 tests.

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

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

BREAKING CHANGE:
The AWS Encryption SDK for JavaScript:
- does not supports the AWS SDK for JavaScript V2
- requires the AWS SDK for JavaScript V3's kms-client
  (if using the KMS Keyring).
BREAKING CHANGE:
This library no longer tests against nor supports NodeJS 12 or 14.
@texastony texastony changed the title Tony drop v2 node 14 feat!: Remove AWS SDK V2 Dependency Jun 30, 2023
@texastony
Copy link
Contributor Author

texastony commented Jun 30, 2023

I do not know why, but the CB testVectorsNodejs20 completes npm run verdaccio-publish and then just hangs until the CB request times out...

It does not run verdaccio-node-decrypt or anything else.

Huh... it's hanging on my local as well...

The question then is if it's learna or verdaccio that is hanging...

Huh... running the commands in util/local_verdaccio_publish with Node20 did not cause a hang...

So something is up with the actual JS in util/local_verdaccio_publish.

Fudge.

Update: I resolved this by setting a time out for the lerna process and having it call process.exit() on close.

@texastony texastony marked this pull request as ready for review July 1, 2023 23:47
@texastony texastony requested a review from a team as a code owner July 1, 2023 23:47
ajewellamz
ajewellamz previously approved these changes Jul 2, 2023
seebees
seebees previously approved these changes Jul 5, 2023
Copy link
Contributor

@seebees seebees left a comment

Choose a reason for hiding this comment

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

LGTM


process.exit()
Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is that this is required because you have added a timeout.
That timeout is likely implemented with setTimeout.
This means when this is executed there is an event in the background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... it was hanging before I added the timeout.
But I guess I could check to see if just exit fixes the hang, with out the time out.

@texastony texastony dismissed stale reviews from seebees and ajewellamz via 6c58ae7 July 5, 2023 22:28
@texastony texastony marked this pull request as draft July 5, 2023 22:36
@texastony texastony marked this pull request as ready for review July 5, 2023 22:39
@texastony texastony merged commit 1d74248 into aws:master Jul 5, 2023
@texastony texastony deleted the tony-drop-v2-node-14 branch July 5, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants