aruhier / virt-backup

Backup your kvm guests managed by libvirt
Other
106 stars 23 forks source link

Enable compression level adjustment #17

Closed shunghsiyu closed 5 years ago

shunghsiyu commented 5 years ago

It seems that while the compression_lvl configuration is passed into DomBackup, compression_lvl were not further given to tarfile.open() in get_new_tar(), which mean the default compression level is always used during the actual compression.

This pull request makes sure that the compression_lvl setting in configuration file will take effect, as well as add compression_lvl in the example configuration.

I'm open to changing this PR further (e.g. adding test...though I'm not sure how test for this PR should be written). Thanks for the awesome repo!

aruhier commented 5 years ago

Hi, Thanks a lot for the pull request! It seems that you're breaking some tests, and TarFile doesn't seem to accept compresslevel as parameter in its constructor.

I'll dig into that, thanks ;)

shunghsiyu commented 5 years ago

Thanks for the prompt reply!

The official Python document for tarfile module mentioned (albeit quite vaguely) the use of compresslevel in tarfile.open()

For modes 'w:gz', 'r:gz', 'w:bz2', 'r:bz2', tarfile.open() accepts the keyword argument compresslevel to specify the compression level of the file.

However, you are right that TarFile doesn't accept compresslevel as parameter in its constructor, and I'll see if I can make those tests pass without changing the tests them-self.

shunghsiyu commented 5 years ago

Hmm... after looking into the failed tests it seems that all of them were trying to create a '.tar.xz' archive.

It turns out I should read the official document more carefully as it clearly states that tarfile.open() accept compresslevel argument only when compression algorithm is gz and bz2 😅

For modes 'w:gz', 'r:gz', 'w:bz2', 'r:bz2', tarfile.open() accepts the keyword argument compresslevel to specify the compression level of the file.

xz does its own thing and takes the preset argument instead (not mentioned in the official Python document), so a nested if block is used to deal with that.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.08%) to 78.163% when pulling 849ac3291a9579c5a2840d896dceea6aac1a0f2e on shunghsiyu:enable_compression_lvl into 92de86a3d2067a6b7c95ba28f3ac58408aa21cfd on Anthony25:master.

shunghsiyu commented 5 years ago

Ah... no problem :)

aruhier commented 5 years ago

Thanks a lot!