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

Progress interval cli option #730

Closed
wants to merge 5 commits into from

Conversation

trbs
Copy link
Contributor

@trbs trbs commented Jan 18, 2017

This PR will allow a user to optionally specify the update interval for restic.NewProgress.

@codecov-io
Copy link

codecov-io commented Jan 19, 2017

Codecov Report

Merging #730 into master will decrease coverage by -0.03%.
The diff coverage is 26.31%.

@@            Coverage Diff             @@
##           master     #730      +/-   ##
==========================================
- Coverage   53.48%   53.46%   -0.03%     
==========================================
  Files          88       88              
  Lines        7533     7538       +5     
==========================================
+ Hits         4029     4030       +1     
- Misses       2913     2917       +4     
  Partials      591      591
Impacted Files Coverage Δ
src/cmds/restic/cmd_check.go 45.97% <ø> (ø)
src/restic/progress.go 8.57% <ø> (-0.34%)
src/cmds/restic/cmd_backup.go 28.74% <ø> (ø)
src/cmds/restic/global.go 23.93% <100%> (+0.4%)
src/cmds/restic/cmd_prune.go 67.8% <80%> (ø)

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 4a51ddf...e18fc43. Read the comment docs.

@fd0
Copy link
Member

fd0 commented Jan 29, 2017

Thanks for taking a stab at this! I must apologize: I haven't left any feedback for you because I needed to think about what to do with this feature first. I must admit that I do not like restic printing progress information when run as a cron job at all, so I planned to remove this periodic progress output for non-terminal runs completely.

What do you think? Do you need this feature? Or did you implemented this option to disable progress reporting for non-terminals?

@fd0 fd0 added the state: need feedback waiting for feedback, e.g. from the submitter label Jan 29, 2017
@trbs
Copy link
Contributor Author

trbs commented Jan 29, 2017

My use case is that the backups are schedules and run via Jenkins. Some are fairly large so it's nice to have some output logged but then every 30s or 60s is good enough. Think the same would go for cron jobs for having the output of the current run (or using logrotate).

I find this useful to check progress if needed when Restic is running on the server or for looking at the logs later to keep an eye on how backups are doing.

Restic already have a -q option right for when people want it to be silent (except for errors) during cron jobs ?

@zcalusic
Copy link
Member

I share the opinion with @fd0, that progress output for unattended runs is completely unneeded. Post-mortem progress just provides no useful information, and actually hides the warnings 'cause it's interspersed in the output. That doesn't mean setting interval is useless, just provide an option that completely disables progress logging, for cases where it's not useful.

Option -q is not the solution, it completely hides the output, and I wan't a few lines in the email to check that backup has run properly, how long it took, any warnings, someday how much data it added to the repo, etc...

@trbs
Copy link
Contributor Author

trbs commented Jan 29, 2017

If we want I can add a PR (or add to this PR) for --disable-progress.

@fd0
Copy link
Member

fd0 commented Jan 29, 2017

Let me think about this for a bit. The problem with all command-line switches is that we cannot easily take them back, because that will break users' scripts. That's a bad thing and I'd like to prevent it if possible :)

* master: (121 commits)
  Add section to the manual about same directory names
  Add News section
  Add `--prune` switch to `forget`
  Update Appveyor
  Update Travis
  Correct archiver behavior in case of errors
  Correct error check for ENOTSUP, add errors.Wrap()
  Continue if extended attribute cannot be read
  Add support for extended attributes on FreeBSD
  Use github.com/pkg/xattr for extended attributes
  Add support for extended attributes (e.g. ACL)
  restore: Make sure buffer is large enough
  Fix correct number of arguments for key command
  Fix wrong description of rebuild-index command in help text
  Add long description of list command in help text
  Consistently refer to 'the' instead of 'a' repository in help text
  Use lowercase consistently in help text
  Adds JSON support for the snapshots command
  Manual: Add section about hard links for fuse
  Correct hardlinks for fuse directories
  ...
* master: (28 commits)
  Fix layout issue in cmd_snapshot "ascii art"
  Fix SamePaths() and make it into a receiver function
  Documentation fixes
  Add design and user documentation for the `restic tag` command
  Snapshot: Add Original ID
  Fix type of ID field in `cmd_snapshots` type Snapshot
  Snapshot: Add `AddTags()` and `RemoveTags()`
  Add `tag`: Manipulate tags on existing snapshots
  Add hint for other backend URI formats
  Make columns for host and tags size width dynamicly on their content.
  Add integration test to make sure cmd_backup adds tags when required.
  Changed cmd_snapshots to be testable (no more using os.Stdout)
  English typo: rewriten > rewritten.
  Add progressbar to repack and blob remove phases of prune cmd.
  Display the proper amount of bytes we will be pruning from the repo.
  Update github.com/pkg/xattr
  Fix Minio Server URL
  Really use absolute pathnames, not all systems use /.
  Fix unit test, we need to check for absolute paths now.
  Display absolute paths when displaying the output of ls and find.
  ...
@trbs
Copy link
Contributor Author

trbs commented Mar 7, 2017

Turns out Restic will already slow down the progress interval to every 10s if it does not run on a terminal.

For situations where you want to force this behaviour or where a terminal is "faked" one can use:

$ restic backup ... | cat

For that reason I'm closing this PR.

@trbs trbs closed this Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: need feedback waiting for feedback, e.g. from the submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants