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

Aws sdk v3 #25

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

Aws sdk v3 #25

wants to merge 7 commits into from

Conversation

jeggers-88
Copy link

@jeggers-88 jeggers-88 commented Apr 20, 2024

Followed: #22 @ww-daniel-mora 😃

First of all, I really like your project to migrate dynamo! We are going to use it in production now...

This is my first contribution here and in github in general, could be that I am doing things wrong 🙏

Notes:

  • generally the migration from aws-sdk v2 to v3 was straightforward
  • in order to keep the changes not too big using aws-sdk v3 now, I still used the v2 syntax AWS.Dynamo() (together with import * as AWS from '@aws-sdk/client-dynamodb') and not import { DynamoDBClient } from "@aws-sdk/client-dynamodb";
    • why ? Using the v2 syntax we can still can use createTable(), putItem() on the ddb instance. Alternatively using the v3 syntax we would need to refactor to always use client.send(command). Hope that is ok for you.
  • some functions are not available in v3
    • for example AWS.SharedIniFileCredentials(). Had to use fromIni instead and refactor a bit
    • same for ddb.waitFor() we now have to use waitUntilTableExists()
  • for mocking aws-sdk we have to switch to aws-sdk-client-mock
  • see also blog post
  • unit tests on my local machine are all green, but I did not execute any e2e tests using a hosted dynamodb
    • is this needed before merging ? Should I do that ? What test cases should we cover ?
  • versioning and readme:
    • I think we should increase the major version because the old migrations using the ddb instance from aws-sdk v2 potentially get errors now passing in a ddb instance from v3
    • I assume we need to write something about this in the README.md, who should do this ? I can make a proposal...
    • should we have a beta version and do some e2e testing together on that ?

Big thanks in advance for feedback and a review! And can you please trigger the actions, so that we see that the unit tests are also working in the github pipeline ?

Best regards
Johan

@jeggers-88 jeggers-88 marked this pull request as draft April 20, 2024 17:29
@@ -108,3 +108,5 @@ dist
build

.DS_Store

.vscode/
Copy link
Author

Choose a reason for hiding this comment

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

I was so free to add this to .gitignore as we also ignore '.idea'

@jeggers-88 jeggers-88 marked this pull request as ready for review April 20, 2024 19:42
@jeggers-88
Copy link
Author

Hi @mukund1488 , sorry ! Calling for help here. Is this repo maintained ? Can somebody look over my PR, please?

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.

1 participant