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

Add GroupBy option to snapshots command #2087

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ArcticXWolf
Copy link

ArcticXWolf commented Nov 14, 2018

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

This commit adds a --group-by option to the snapshots command, which
behaves similar to the --group-by option of forget. Valid option values
are "host, paths, tags". If this option is given, the output of
snapshots will be divided into multiple tables, according to the value
given (i.e. "host" will create a table of snapshots for each host, that
has a snapshot in the list). Also the JSON output will be grouped.

The default behavior (when --group-by is not given) has not changed.

More to this discussion can be found in issue #2037.

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

Yes, it was discussed in issue #2037.

Closes #2037

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)
  • 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
@ArcticXWolf

This comment has been minimized.

Copy link
Author

ArcticXWolf commented Nov 14, 2018

Hey, I want to work on the group-by option discussed in #2037. The main functionality is already done, whats missing are tests, documentation and changelogs.

As this is my first contribution to restic and also my first time coding in Go, I have some questions:

  1. What's used to do the testing? (As I'm coming from Ruby, I know that there are many testing frameworks/DSLs)
  2. Is the style of my changes okay? I'm not sure, if I violated any naming conventions or else.
  3. What shall I do concerning the error at Appveyor? It seems like it cannot install go1.11, is that my error?

I hope my contribution helps!

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 25, 2018

Codecov Report

Merging #2087 into master will decrease coverage by 4.34%.
The diff coverage is 14.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2087      +/-   ##
==========================================
- Coverage   50.69%   46.35%   -4.35%     
==========================================
  Files         176      177       +1     
  Lines       14167    14215      +48     
==========================================
- Hits         7182     6589     -593     
- Misses       5938     6632     +694     
+ Partials     1047      994      -53
Impacted Files Coverage Δ
internal/restic/snapshot_group.go 0% <0%> (ø)
cmd/restic/cmd_snapshots.go 20.33% <24.63%> (-4.67%) ⬇️
cmd/restic/cmd_forget.go 41.75% <9.52%> (-1.1%) ⬇️
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/rclone/backend.go 66.08% <0%> (ø) ⬆️
... and 2 more

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 0dd8054...ef3f554. Read the comment docs.

@fd0
Copy link
Member

fd0 left a comment

Hey, I've had a look at the code, thanks for your contribution (especially as a first-time contributor new to Go)!

I've got a few things that you could improve in the code, but it looks great already.

  • The Tests on Travis fail because the file internal/archiver/testing_test.go is not formatted properly with gofmt (the Go formatting standard is an implementation, rather than a document), which has changed a bit recently in the Go 1.11 release. In order to pass the tests, you need to format the code with gofmt on Go 1.11.
  • There are modifications to files in vendor/, these should not be there. Did you maybe run gofmt on all files, including vendor/? Please remove the modifications from the PR.

For testing, we're using the builtin testing framework, you can easily run all tests with go test ./... in the repo, or chdir to some directory and just run go test.

Unfortunately, we don't have tests for user-interface interactions like this in place (at least not yet).

Let me know if you need any help!

Show resolved Hide resolved cmd/restic/cmd_snapshots.go Outdated
Show resolved Hide resolved cmd/restic/cmd_snapshots.go Outdated
Show resolved Hide resolved cmd/restic/cmd_snapshots.go Outdated
Show resolved Hide resolved cmd/restic/cmd_snapshots.go Outdated
Show resolved Hide resolved cmd/restic/cmd_snapshots.go Outdated
Add GroupBy option to snapshots command
This commit adds a --group-by option to the snapshots command, which
behaves similar to the --group-by option of forget. Valid option values
are "host, paths, tags". If this option is given, the output of
snapshots will be divided into multiple tables, according to the value
given (i.e. "host" will create a table of snapshots for each host, that
has a snapshot in the list). Also the JSON output will be grouped.

The default behavior (when --group-by is not given) has not changed.

More to this discussion can be found in issue #2037.

@ArcticXWolf ArcticXWolf force-pushed the ArcticXWolf:add_group_by_option_for_snapshots branch from 0943b3a to 39df8a7 Nov 30, 2018

Fix json tags for grouped snapshot output
This commit will add json tags to the structs for json output, so all
json variables of the snapshot command output are lowercase and
snake-case.

Furthermore it adds some internal code changes based on the feedback in
the pull request #2087.
Move snapshot grouping code into own function to deduplicate code
This commit moves the code which is used to group snapshots in the
snapshots command into an own function to deduplicate code shared by the
snapshots command and forget command.

@ArcticXWolf ArcticXWolf force-pushed the ArcticXWolf:add_group_by_option_for_snapshots branch from a41856e to 3de3584 Nov 30, 2018

@ArcticXWolf

This comment has been minimized.

Copy link
Author

ArcticXWolf commented Nov 30, 2018

Okay, this is nearly complete, I just have some last questions:

For testing, we're using the builtin testing framework, you can easily run all tests with go test ./... in the repo, or chdir to some directory and just run go test.

Unfortunately, we don't have tests for user-interface interactions like this in place (at least not yet).

The tests succeed with my changes, but shall I add new ones? I have no idea how I should design such UI tests. Maybe just save the expected output into a fixture?

Furthermore I added a changelog file, whats missing now is the modification of the documentation. Where shall I mention it? Maybe at the working with repos section?

Refactor duplicate code for grouping snapshots
This commit is a followup to the addition of the --group-by flag for the
snapshots command. Adding the grouping code there introduced duplicated
code (the forget command also does grouping). This commit refactors
boths sides to only use shared code.

@ArcticXWolf ArcticXWolf force-pushed the ArcticXWolf:add_group_by_option_for_snapshots branch from 6f79eae to 9bae836 Jan 4, 2019

@ArcticXWolf

This comment has been minimized.

Copy link
Author

ArcticXWolf commented Jan 4, 2019

Okay, I refactored the forget command to use the shared code and added the documentation. Tests are green for me, just waiting for the CI to confirm this. As I do not know how to do the interface tests (and we did not have them before), I believe this is ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment