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

Add billingMode option #90

Closed
wants to merge 9 commits into from
Closed

Conversation

bashou
Copy link

@bashou bashou commented Sep 7, 2021

Hi,

As some use PAY_BY_REQUEST billing mode, we need to be able to choose our current setup for selected table to restore (not necessary for backups).

This PR resolve this point.

Copy link
Owner

@bchew bchew left a comment

Choose a reason for hiding this comment

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

Thanks for the PR but have noted issues with it that should be addressed. Also would be good if you could add tests to cover this new functionality if you can, thanks.

dynamodump/dynamodump.py Outdated Show resolved Hide resolved
dynamodump/dynamodump.py Outdated Show resolved Hide resolved
dynamodump/dynamodump.py Outdated Show resolved Hide resolved
dynamodump/dynamodump.py Outdated Show resolved Hide resolved
dynamodump/dynamodump.py Outdated Show resolved Hide resolved
@bashou
Copy link
Author

bashou commented Sep 14, 2021

Let me check for tests

@bashou
Copy link
Author

bashou commented Sep 14, 2021

@bchew is it satisfying ?
May be unit test must be improved in another way but i think it should be another specific PR, what do you think ?

Copy link
Owner

@bchew bchew left a comment

Choose a reason for hiding this comment

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

There are changes on streams which are unrelated to the original PR which should be removed

dynamodump/dynamodump.py Outdated Show resolved Hide resolved
dynamodump/dynamodump.py Outdated Show resolved Hide resolved
@@ -9,6 +9,14 @@ python dynamodump/dynamodump.py -m backup -r local -s testRestoredTable --host l
--accessKey a --secretKey a
python tests/test.py

# Test wildcard restore and backup with PAY_BY_REQUEST BillingMode
Copy link
Owner

Choose a reason for hiding this comment

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

This test does not actually work - it's PAY_PER_REQUEST.

Currently this test script is for local manual testing, thus it did not get run on CI (https://github.com/bchew/dynamodump/blob/master/.github/workflows/test.yml). I'll probably refactor this at some point so local and CI testing shares the same tests.

@asyschikov
Copy link

asyschikov commented Nov 24, 2021

+1 This is a very needed feature!

@bchew
Copy link
Owner

bchew commented Dec 31, 2021

@bashou sorry that I've not been able to work on the tests refactor yet, but I've moved your changes into #119 so I can get it merged. Thanks again for the work you've done for this!

@bchew bchew closed this Dec 31, 2021
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.

None yet

3 participants