dump: add --compress flag to compress archives#5054
dump: add --compress flag to compress archives#5054MichaelEischer merged 4 commits intorestic:masterfrom
Conversation
cmd/restic/cmd_dump.go
Outdated
| Target string | ||
| Archive string | ||
| Target string | ||
| CompressZipArchive bool |
There was a problem hiding this comment.
Just Compress should be fine. Considering, we can compress a .tar after archiving.
cmd/restic/cmd_dump.go
Outdated
| initSingleSnapshotFilter(flags, &dumpOptions.SnapshotFilter) | ||
| flags.StringVarP(&dumpOptions.Archive, "archive", "a", "tar", "set archive `format` as \"tar\" or \"zip\"") | ||
| flags.StringVarP(&dumpOptions.Target, "target", "t", "", "write the output to target `path`") | ||
| flags.BoolVar(&dumpOptions.CompressZipArchive, "compress-zip", false, "compress zip archive contents") |
There was a problem hiding this comment.
Can we do a shorthand c as well? You can check if there any conflicts with the global options.
changelog/unreleased/pull-5054
Outdated
| # Describe the problem in the past tense, the new behavior in the present | ||
| # tense. Mention the affected commands, backends, operating systems, etc. | ||
| # Focus on user-facing behavior, not the implementation. | ||
| # Use "Restic now ..." instead of "We have changed ...". |
There was a problem hiding this comment.
These comments can be excluded from the changelog.
There was a problem hiding this comment.
Oh, must have slipped by...
|
@konidev20 It would only be a little more effort to add compression to the tar stream as well, I guess that would be more consistent when renaming the flag to --compress (-c is unused, so we can use that too). Do you agree? because then I would add that? |
|
I've added compression for .tar as well and updated the docs and flag name. I can squash the commits later for better readability |
konidev20
left a comment
There was a problem hiding this comment.
LGTM. I will let Michael do the final review.
MichaelEischer
left a comment
There was a problem hiding this comment.
The changes look good so far, I just have a few small remarks. Also please squash the commits.
77cebb1 to
bad6c54
Compare
cmd/restic/cmd_dump.go
Outdated
| initSingleSnapshotFilter(flags, &dumpOptions.SnapshotFilter) | ||
| flags.StringVarP(&dumpOptions.Archive, "archive", "a", "tar", "set archive `format` as \"tar\" or \"zip\"") | ||
| flags.StringVarP(&dumpOptions.Target, "target", "t", "", "write the output to target `path`") | ||
| flags.BoolVarP(&dumpOptions.Compress, "compress", "c", false, "compress archive contents. When enabled, the DEFLATE algorithm is applied for \"zip\" archives, and the gzip algorithm for \"tar\" archives, resulting in a .tar.gz or .tgz file. (default: false)") |
There was a problem hiding this comment.
This description is IMO way too long and verbose. This inline help text in the CLI should be short and concise, with verbose documentation being in the regular docs.
There was a problem hiding this comment.
Yeah but where can I add something to the docs but not to the CLI, i thought the docs where generated from the cli source? Am I missing something?
There was a problem hiding this comment.
The sources for the docs at restic.readthedocs.io are in the docs/ folder in this repo, see https://github.com/restic/restic/tree/master/doc .
There was a problem hiding this comment.
Yeah, and they are auto-generated, see #5054 (comment)
No changes to the docs itself allowed
There was a problem hiding this comment.
Yes and no.
- The docs at restic.readthedocs.io are indeed auto-generated by RTD, but the sources for them are at https://github.com/restic/restic/tree/master/doc and you are totally fine to edit them to add contents to the official documentation for restic, when it makes sense to do so.
- The documentation for/in the CLI of restic is auto-generated too, as mentioned in the comment you linked to, and those files should not be edited, that is correct. But they are a different thing than the official docs on RTD.
|
@MichaelEischer and I were discussing this PR earlier today and would like to make some changes. Namely:
|
b496dd3 to
a74740d
Compare
a74740d to
1a7fafc
Compare
|
Updated the PR, much simpler now. |
MichaelEischer
left a comment
There was a problem hiding this comment.
LGTM. I've applied the suggestion from @greatroar and minimally extended the test case.
What does this PR change? What problem does it solve?
When using dump and exporting an archive, the archive was not compressed. This PR adds a flag to do so.When using
dumpto export aziparchive, the archive was not compressed. This PR enables compression for these archives.Was the change previously discussed in an issue or on the forum?
No, I don't think so.
Checklist
changelog/unreleased/that describes the changes for our users (see template).gofmton the code in all commits.