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

prune: Add JSON support and separate code #3140

Closed
wants to merge 5 commits into from

Conversation

aawsome
Copy link
Contributor

@aawsome aawsome commented Nov 30, 2020

What does this PR change? What problem does it solve?

Separates the pruning code in three functions:

  • Find out what to do
  • Print statistcs (+ added JSON output here)
  • do the actual pruning work

In a next step, unit tests can be added for all three parts (not included here)

This PR conflicts with #2881 - I'll fix this depending on what PR is tackled first.

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

Refactoring the code: discussion with @fd0
Also closes #3129

Checklist

  • I have read the Contribution Guidelines
  • I have enabled maintainer edits for this PR
  • 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

@aawsome
Copy link
Contributor Author

aawsome commented Dec 23, 2020

rebased after #3148 was merged.

Copy link
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering how the JSON should look like to make it easier to extend it to a streaming output as with the backup command in the future. The most important part is probably to add a message type and a comment that this may change in the future.

restic prune --json still prints a lot of non-JSON text.

cmd/restic/cmd_prune.go Outdated Show resolved Hide resolved
cmd/restic/cmd_prune.go Show resolved Hide resolved
cmd/restic/cmd_prune.go Outdated Show resolved Hide resolved
cmd/restic/cmd_prune.go Outdated Show resolved Hide resolved
cmd/restic/cmd_prune.go Outdated Show resolved Hide resolved
@aawsome aawsome force-pushed the prune-json branch 2 times, most recently from 4e8b88d to 3017a59 Compare December 29, 2020 21:38
@aawsome
Copy link
Contributor Author

aawsome commented Dec 29, 2020

@MichaelEischer I included your proposals and also rebased as #3139 is merged now. The prunePlan struct was quite handy ;-)

restic prune --json still prints a lot of non-JSON text.

Yes, so far --json can be only used in combination with --quiet. I was hoping for #3009 which IMO include the json checks for the various print functions...

@MichaelEischer
Copy link
Member

Yes, so far --json can be only used in combination with --quiet. I was hoping for #3009 which IMO include the json checks for the various print functions...

Unfortunately, it is not possible to let --json imply --quiet. For example the backup command allows for different levels of verbosity for its JSON output.

@aawsome
Copy link
Contributor Author

aawsome commented Jan 30, 2021

rebased.

@MichaelEischer
Copy link
Member

Unfortunately, it is not possible to let --json imply --quiet. For example the backup command allows for different levels of verbosity for its JSON output.

#3009 won't be able to let --json imply --quiet as that would break the backup command output (the format must not force a certain log level). So this has to be handled locally.

@aawsome
Copy link
Contributor Author

aawsome commented Jan 31, 2021

#3009 won't be able to let --json imply --quiet as that would break the backup command output (the format must not force a certain log level). So this has to be handled locally.

Actually I was thinking of something like the last commit I added. IMO this works and doesn't break anything except removing the output for some command that don't support --json if it is given.
Also makes many checks for gopts.JSON obsolete, but this would be an independent follow-up.

I also rebased to current master.

@aawsome
Copy link
Contributor Author

aawsome commented Jan 31, 2021

IMO this works and doesn't break anything

Ok, it breaks find which uses the Print and Printf functions to manually generate JSON output 😩

I'll add some more checks for gopts.JSON and leave this cleanup to #3009 or a separate PR.

@MichaelEischer
Copy link
Member

I've rebased to PR to fix the merge conflicts.

@darkdragon-001
Copy link
Contributor

Thanks for your great work so far!
Are there any updates? I am specifically interested in the JSON output.

@aawsome aawsome closed this Feb 24, 2024
@aawsome aawsome deleted the prune-json branch February 24, 2024 22:06
@darkdragon-001
Copy link
Contributor

@aawsome has this been implemented somewhere else or why was it closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide JSON output on prune
3 participants