silvermine / dynamodb-table-sync

MIT License
45 stars 15 forks source link

feat: Add support for synchronizing to a local slave instance #26

Open CharlieDigital opened 2 years ago

CharlieDigital commented 2 years ago

Purpose

This PR adds support for synchronizing from a remote DynamoDB to a local instance by specifying a localhost URL for the parameter --slave-profile.

See #27

Key Changes

CharlieDigital commented 2 years ago

Hi @echernyavskiy I checked the CI error and it is because of the commit messages (sorry, I did not read the rules before I made the initial commits!).

What would you like to do on that front?

echernyavskiy commented 2 years ago

Hi @echernyavskiy I checked the CI error and it is because of the commit messages (sorry, I did not read the rules before I made the initial commits!).

What would you like to do on that front?

Looks like Travis jobs failed because commitlint wasn't happy. If I understand correctly, commit messages must start with a type in order to satisfy this check, so you just need to push a valid commit here. I'm not a maintainer of this project, just guessing here.

jthomerson commented 2 years ago

@echernyavskiy @CharlieDigital please just rebase and squash all these commits down into one commit, with an appropriate message. More info at: https://github.com/silvermine/silvermine-info/blob/master/commit-history.md

CharlieDigital commented 2 years ago

I've had to do a small refactor to deal with the lint rules. All lint rules pass now and all tests pass.

image

However, one of the CI pipelines is broken:

Screen Shot 2022-05-18 at 4 11 00 PM

Let me know your thoughts.

CharlieDigital commented 2 years ago

Just checking in to see if there's anything else I can help with here. It seems that the root cause of the failing check is a configuration of the CI environment.

jthomerson commented 2 years ago

Sorry, I’m the main developer on this and am on extended holiday. Be back in a couple weeks and hope to look at it then.

ned-kelly commented 5 months ago

HI @jthomerson - any way we can get this merged into master and added to the latest NPM release?

jthomerson commented 5 months ago

@CharlieDigital can you please rebase to merge the two commits so that the build will pass? Alternatively, you can click they button that will allow me to push to your branch so I can update this PR.

git fetch --all
git rebase -i 80edaf9a1b5b5119fc241a336c3f59cf461bf07f
# in your editor, change "pick" to "fix" on the second commit
git push --force

Or:

git reset HEAD^
git add --all
git commit --amend
git push --force
CharlieDigital commented 5 months ago

@jthomerson added you to the repo. Just want to make sure this gets merged in correctly so other folks looking for similar features have it available :)

coveralls commented 5 months ago

Coverage Status

coverage: 0.0%. remained the same when pulling 7ef89f21cc42250aaac3fd13c5d721efc3677315 on CharlieDigital:master into 80edaf9a1b5b5119fc241a336c3f59cf461bf07f on silvermine:master.

jthomerson commented 5 months ago

@CharlieDigital @echernyavskiy I just rewrote this PR to (1) not overload the --slave-profile parameter, and (2) thereby reduce complexity of the code so that there were less places that needed to know about the local endpoint (only where we're creating a DDB SDK instance).

I've done a basic test of syncing a remote table to a local table. I have not been able to do a test of using remote masters and slaves, especially in separate accounts. Is that something one of you could test? Just a smoke test of overall master/slave credentials management?

If someone can verify that things are still working as they were before, I'll merge and cut a release. Thanks!

coveralls commented 5 months ago

Coverage Status

coverage: 0.0%. remained the same when pulling 95d4595de375bb191eaa638649bd243de6813e1f on CharlieDigital:master into 80edaf9a1b5b5119fc241a336c3f59cf461bf07f on silvermine:master.