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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CharlieDigital
Copy link

@CharlieDigital CharlieDigital commented May 9, 2022

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

  • Updated cli.js to check for localhost in the --slave-profile
  • Updated Synchronizer.js to add construction of local client as well as selection of correct configuration for DynamoDB
  • Updated docs

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/cli.js Outdated Show resolved Hide resolved
@CharlieDigital
Copy link
Author

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?

src/cli.js Outdated Show resolved Hide resolved
@echernyavskiy
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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
Copy link

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

@jthomerson
Copy link
Member

@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
Copy link
Author

CharlieDigital commented Jul 6, 2024

@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
Copy link

Coverage Status

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

@jthomerson
Copy link
Member

@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
Copy link

Coverage Status

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

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.

5 participants