feat: upgrade to AWS SDK v3 #22
Conversation
|
Thanks for this PR, it looks great at first glance! I’ll do a closer review within the next week. |
|
fantastic work, with node16 and therefore SDK v2 AWS EOL in a few weeks too, essential to keep this solution viable |
|
@txase have you had a chance to look at this yet? |
There was a problem hiding this comment.
First, I apologize for not getting to this review as soon as I promised. Thanks for following up with a comment.
Second, after a full review I see how great this PR is. You did an awesome job in keeping the semantics and behavior in place while upgrading the AWS SDK dependency. So good!
I only request one change, which is to re-establish the tests that ensure the RDSDataService constructor is called only once in the first few configuration tests. I don’t want to lose this test check. Since I’m looking for one more change, a fix for the typo in the README would be greatly appreciated as well :).
Thanks again!
|
|
||
| ### Version 2 to 3 | ||
| Version 3 uses verion 3 of the AWS SDK. This SDK is included automatically in the `nodejs18.x` AWS Lambda runtime and above. |
There was a problem hiding this comment.
This has a typo (verion 3). I can fix this post-merge, though.
| } catch (err) { /* istanbul ignore next */ | ||
| throw new Error(`Failed to load aws-sdk rdsdataservice client, did you forget to install it as a dependency? (${err.message})`); | ||
| } | ||
|
|
||
| const https = this.config.connection.sdkConfig && String(this.config.connection.sdkConfig.endpoint).startsWith('http:') | ||
| const isHttp = this.config.connection.sdkConfig && String(this.config.connection.sdkConfig.endpoint).startsWith('http:'); |
There was a problem hiding this comment.
As minor PR feedback, I suggest not renaming existing variables where their semantic meaning is left unchanged. It can make it difficult to understand changes through git history when one is trying to debug problems and needing to read older commits to understand how behavior changes over time. I’m fine with this change here, though. It’s impact is minor.
| connection: { | ||
| database: constants.DATABASE, | ||
| resourceArn: constants.AURORA_CLUSTER_ARN, | ||
| secretArn: constants.SECRET_ARN | ||
| } | ||
| }); | ||
|
|
||
| expect(RDSDataService).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
It’s important to ensure the number of times methods, like the RDSDataService constructor, have been called. I like the simplicity of many of the testing changes you’ve made, but we need to find a way to keep validating this aspect.
For what it’s worth, these checks were added in my testing when I realized that I misunderstood certain knex code paths and found some of my functions were getting called multiple times when they should have been called only once. These checks are important :).
| @@ -37,11 +41,13 @@ class Transaction_AuroraDataMySQL extends Transaction { // eslint-disable-line c | |||
| ...conn.parameters, | |||
| transactionId: conn.transactions[conn.__knexTxId] | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Minor feedback nit: whitespace additions shouldn’t be added in PRs unless they change semantics or behavior. But I’m ok with this stray instance in this PR, it’s minor and not worth fixing up.
| @@ -64,11 +70,13 @@ class Transaction_AuroraDataMySQL extends Transaction { // eslint-disable-line c | |||
| ...conn.parameters, | |||
| transactionId: conn.transactions[conn.__knexTxId] | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Same feedback nit on added whitespace.
This PR removes the legacy v2 AWS SDK and replaces it with the new modular V3 SDK client for RDS. This will add compatibility with the
nodejs18.xruntime on Lambda.