-
Notifications
You must be signed in to change notification settings - Fork 31
Migrate to AWS SDK V3 - Part 1 #65
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
Conversation
lib/configuration/variables/sources/instance-dependent/get-aws.js
Outdated
Show resolved
Hide resolved
dc02d82
to
79e9aa0
Compare
lib/configuration/variables/sources/instance-dependent/get-aws.js
Outdated
Show resolved
Hide resolved
ListAttachedRolePoliciesCommand, | ||
CreateRoleCommand, | ||
AttachRolePolicyCommand, | ||
} = require('@aws-sdk/client-iam'); | ||
const AWSClientFactory = require('../../../../../aws/client-factory'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these files the custom resource handlers?
If yes, they shouldn't be impacted by the changes in the CLI. And they wouldn't be able to require files from the CLI package (because they would run in Lambda).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this was a fix to make sure they are re-using the same client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the changes and did some basic tests, things seem to work with the v3 SDK. I also cleaned up the diff.
The last thing we need to figure out for me are the custom resources: I don't see why they are impacted here. I'd revert them entirely but I'm not sure if I'm missing something, so I'll wait for your take on this.
The changes to those are not strictly necessary, but they should improve performance by re-using the TPC connections. |
@GrahamCampbell but wait I don't understand at all. Custom resource handlers are deployed in AWS lambda in users accounts. They don't have the whole codebase of serverless framework, nor its dependencies. I don't see how it's possible that a custom resource handler requires from the rest of the codebase, that makes no sense to me. https://github.com/oss-serverless/serverless/blob/1f3039673266a4bd88188ef4c8aa728c8cfeb6c8/lib/plugins/aws/custom-resources/resources/README.md#L4-L3 |
OK, sure. We can revert those changes. |
This PR adds AWS SDK v3 support for everything. Opt in to v3 with
SLS_AWS_SDK_V3=1
. I am running the CI on both versions in the Node 22 job... however mostly things are mocked, so this is not testing much. A notable difference is that the v2 SDK has memoization that we expose withuseCache
. We are not doing that for v3. Do we want that?In a follow-up PR we could flip the default (Part 2), and in another follow-up, remove v2 support (Part 3).
Smashing through this with Claude Code. 🤖