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

Json forget #2185

Merged
merged 4 commits into from Apr 13, 2019

Conversation

Projects
None yet
4 participants
@d3zd3z
Copy link
Contributor

commented Feb 23, 2019

What is the purpose of this change? What does it change?

This makes the --json option work with the forget command.

Was the change discussed in an issue or in the forum before?

I created issue #2184 to describe this.

Closes #2184

Checklist

  • I have read the Contribution Guidelines
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
    (it could be argued that this is already described in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

d3zd3z added some commits Feb 23, 2019

Add test for `--json` support for `forget` command
This adds a test of the json output of the forget command, by running it
once, asking it to keep one snapshot, and verifying that the output has
the right number of snapshots listed in the Keep and Remove fields of
the result.
@codecov-io

This comment has been minimized.

Copy link

commented Feb 23, 2019

Codecov Report

Merging #2185 into master will decrease coverage by 3.99%.
The diff coverage is 54.83%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #2185    +/-   ##
========================================
- Coverage   50.76%   46.77%    -4%     
========================================
  Files         176      176            
  Lines       14251    14276    +25     
========================================
- Hits         7234     6677   -557     
- Misses       5964     6582   +618     
+ Partials     1053     1017    -36
Impacted Files Coverage Δ
cmd/restic/cmd_forget.go 56.32% <54.83%> (+13.47%) ⬆️
internal/backend/b2/b2.go 0% <0%> (-80.69%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-78.45%) ⬇️
internal/backend/gs/gs.go 0% <0%> (-74%) ⬇️
internal/backend/azure/azure.go 0% <0%> (-69.46%) ⬇️
internal/backend/swift/config.go 36.95% <0%> (-54.35%) ⬇️
internal/backend/s3/s3.go 59.39% <0%> (-0.76%) ⬇️

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 7519c73...d19a29f. Read the comment docs.

@fd0
Copy link
Member

left a comment

Looks great already, thanks for your contribution! It'd be nice if you could also integrate the reasons each snapshot is kept (which is part of the normal output, but not included in the JSON output yet). Would you like to do that in this PR? Or open a new PR later on?

Show resolved Hide resolved changelog/unreleased/issue-2184 Outdated

d3zd3z added some commits Feb 23, 2019

Include reasons in json output of forget
This dumps the reasons as well as the list of keeps and removes with the
output from the forget command.

@d3zd3z d3zd3z force-pushed the d3zd3z:json-forget branch from 22a7cc5 to d19a29f Feb 23, 2019

@d3zd3z

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2019

This includes the reasons, although this is kind of simplistic in that it just dumps the reasons out. Does it seem useful this way, or should I try to correlate each reason with each keep and remove?

@fd0

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Awesome, thanks! looks good!

@fd0

fd0 approved these changes Apr 13, 2019

@fd0 fd0 merged commit d19a29f into restic:master Apr 13, 2019

2 checks passed

Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

fd0 added a commit that referenced this pull request Apr 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.