Skip to content

Add support for dump CLI extra parameters #23

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

Merged
merged 2 commits into from
Jun 7, 2019

Conversation

JoaRiski
Copy link
Contributor

@JoaRiski JoaRiski commented Jun 6, 2019

Add support for defining extra parameters to pass to the dump CLI tool, for
both the pg_dump and mysqldump

Resolves #22

@JoaRiski JoaRiski requested a review from suutari-ai June 6, 2019 13:18
@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #23 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines         446    474   +28     
  Branches       91     97    +6     
=====================================
+ Hits          446    474   +28
Impacted Files Coverage Δ
database_sanitizer/dump/postgres.py 100% <100%> (ø) ⬆️
database_sanitizer/dump/mysql.py 100% <100%> (ø) ⬆️
database_sanitizer/config.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df9a399...c9fb408. Read the comment docs.

Add support for defining extra parameters to pass to the dump CLI tool, for
both the pg_dump and mysqldump

Resolves #22
@JoaRiski JoaRiski force-pushed the add-extra-params-support branch from 7e9308b to 37be155 Compare June 6, 2019 13:24
@suutari-ai
Copy link
Member

Why don't you just make --single-transaction a default parameter for MySQL? Then the support for custom parameters wouldn't be needed at all, right? (Would result in less code to maintain and less special cases to test)

@JoaRiski
Copy link
Contributor Author

JoaRiski commented Jun 7, 2019

@suutari-ai there might be cases where you don't want to do that, e.g. with MyISAM engine based databases. Additionally I think this is more future proof in case there's other flags that might need to be set.

@JoaRiski
Copy link
Contributor Author

JoaRiski commented Jun 7, 2019

Added the --single-transaction as a default in the configured extra parameters, which this still allows for it to be removed if the extra parameters are manually defined in the .sanitizerconfig file.

Include the --single-transaction mysqldump parameter by default in the configuration
object, but allow it to be removed if the extra parameters are defined on the configuration
file explicitly.

Related to #22
@JoaRiski JoaRiski force-pushed the add-extra-params-support branch from 4d137bc to c9fb408 Compare June 7, 2019 07:51
Copy link
Member

@suutari-ai suutari-ai left a comment

Choose a reason for hiding this comment

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

Well, I guess it's good that this can be configured, after all. As long as the defaults are good too, as they are now.

Nice work! 👍

@JoaRiski JoaRiski merged commit 861976f into master Jun 7, 2019
@JoaRiski JoaRiski deleted the add-extra-params-support branch June 7, 2019 10:52
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.

Add --single-transaction support for mysql dump
2 participants