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

backup: print full snapshot id in JSON summary #3993

Merged

Conversation

MichaelEischer
Copy link
Member

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

The JSON output only included the shortened ID of the snapshot and not the full ID. The shortened ID could be ambiguous in rare cases and always has to be expanded first before restic can use it. As the JSON output is intended to be machine readable, there is no use in shortening the ID before printing it.

Strictly speaking, this change is not backwards compatible, but I don't expect much fall-out. In the worst case users have to explicitly truncate the ID down to 8 characters.

Was the change previously discussed in an issue or on the forum?

Fixes #2724

Checklist

  • I have read the contribution guidelines.
  • I have enabled maintainer edits.
  • I have added tests for all code changes.
  • [ ] I have added documentation for relevant changes (in the manual).
  • There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • 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.

Copy link
Contributor

@rawtaz rawtaz left a comment

Choose a reason for hiding this comment

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

Makes sense, no need to shorten it in programmatic/structured output.

@MichaelEischer
Copy link
Member Author

Hmm, the forget and snapshots command print both the full ID and a ShortID. But introducing a LongID here is just horribly inconsistent and not really useful.

@rawtaz
Copy link
Contributor

rawtaz commented Oct 31, 2022

The forget command prints snapshot IDs like this:

"id": "89cefc5d61f95b66dc36e25029d54582fdb5d92d159af95dbb89cd4458f3c4a5",
"short_id": "89cefc5d"

The snapshots command prints snapshot IDs like this:

"id": "ea55667d08c38695cc6d2367ac50f9bc66b7b88a946427dcadde76b806820b07",
"short_id": "ea55667d"

The backup command currently prints snapshot IDs like this:

"snapshot_id": "ea55667d"

We can simply change the backup command to output the snapshot ID as id (long format) and short_id (short format) to make it consistent with the others, while for now keeping snapshot_id as-is (but making a note that it is deprecated in the changelog and the code) and remove that in the future. This should make it all consistent and fully backwards compatible.

@rawtaz
Copy link
Contributor

rawtaz commented Nov 5, 2022

@fd0 Did you read my comment above?

@MichaelEischer
Copy link
Member Author

MichaelEischer commented Nov 5, 2022

I still prefer the current change of just outputting the full snapshot id. In the context of the backup command, it's probably not really obvious, that id would be the snapshot id (it can't really be something else, but still). And replacing the existing field complicates things a lot.

@fd0
Copy link
Member

fd0 commented Nov 12, 2022

I'm with @MichaelEischer, just print the full ID in the JSON field.

@MichaelEischer MichaelEischer merged commit 32ffcd8 into restic:master Nov 12, 2022
@MichaelEischer MichaelEischer deleted the backup-json-full-snapshot-id branch November 12, 2022 19:42
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.

Include full snapshot id in backup json command output
3 participants