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

Updating lambda functions to nodejs 20 #222

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

mettke
Copy link
Contributor

@mettke mettke commented Dec 24, 2023

Fixes: #221

This PR switches NodeJS Version from v16 to v20.
Two additional changes came from me using an Editor which adds a newline on save.

@alexcasalboni
Copy link
Owner

Amazing, thanks 🙏 🎉

This looks great and all the unit tests run smoothly as expected 👌 Don't worry about the failing integration tests for now, it's just an auth issue.

I'll do some more testing in the next few days and then merge!

@mettke
Copy link
Contributor Author

mettke commented Dec 28, 2023

It looks like lambda's starting with nodejs 18 do not ship aws sdk v2 1. Currently the code still uses const AWS = require('aws-sdk'); so we may have to do a bit more then this to get it up and running 2. This should have been an issue with the nodejs 18 build already, but I guess the build does not explicitly check that.

I suggest that we add a test which tries to run the code on a given aws lambda container 3. This way we should be able to detect whether its going to work in given nodejs environment.

@mettke mettke force-pushed the master branch 2 times, most recently from c0e64f7 to ff90a49 Compare December 28, 2023 13:18
@mettke
Copy link
Contributor Author

mettke commented Dec 28, 2023

Ok I think I should be done now. What I changed:

  • I migrated aws calls to a new file called aws.js. It automatically routes between v3 and v2 depending on whats available with v3 having the preference.
  • I added mocking for aws sdk v3
  • I added a new workflow which will run the tests inside the official aws nodejs docker container for lambda.

This should do it. I guess it was a bit more difficult then changing 16 to 20 😆

@alexcasalboni
Copy link
Owner

Hey @mettke 👋 apologies for the long wait :)

I don't fully understand why we need all of this, since we can control the available SDK with the ad-hoc layer. Wouldn't that work with Node v20?

@mettke
Copy link
Contributor Author

mettke commented Jan 5, 2024

It would work yes. But aws sdk v2 will enter maintaince mode "soon":

We are formalizing our plans to make the Maintenance Announcement (Phase 2) for AWS SDK for JavaScript v2 in early 2024. Please refer to the AWS SDKs and Tools maintenance policy for further details. [1]

So the question is, do we want to use the layer now and once sdk v2 is deprecated we migrate to v3, or do we want to use this code now and remove the v2 stuff once its deprecated.

Decision is up to you and I'm happy to revert. Of course we can also turn the question around. Instead of using the layer for v2 we could also use the layer for v3 and get nodejs 16 to support sdk v3. This way we can migrate already without having to support both.

[1] https://github.com/aws/aws-sdk-js

@alexcasalboni
Copy link
Owner

I see, you're right.

Then I would:

  1. keep this PR focused on Node v20
  2. open a new PR to migrate to SDKv3
  3. use the existing layer for SDKv3

The reason is that I don't think supporting both SDKs will be that useful in the long term and since you've already mapped all the API calls, it makes sense to just migrate and forget about SDKv2 (big thanks for that, I had been thinking about SDKv3 migration for a while!).

@mettke
Copy link
Contributor Author

mettke commented Jan 5, 2024

Sounds reasonable. I removed the last two commits and now only the migration to nodejs20 is in. I will also create a new PR regarding moving to sdkv3 removing sdkv2.

@mettke mettke mentioned this pull request Jan 5, 2024
@alexcasalboni alexcasalboni merged commit 56cc874 into alexcasalboni:master Jan 8, 2024
13 of 15 checks passed
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.

Upgrade to Node 20
2 participants