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

refactor restore command #741

Merged
merged 1 commit into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions crates/rustic_core/examples/restore.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//! `restore` example
use rustic_core::{
LocalDestination, Repository, RepositoryOptions, RestoreOpts, TreeStreamerOptions,
};
use simplelog::{Config, LevelFilter, SimpleLogger};

fn main() {
// Display info logs
let _ = SimpleLogger::init(LevelFilter::Info, Config::default());

// Open repository
let repo_opts = RepositoryOptions {
repository: Some("/tmp/repo".to_string()),
password: Some("test".to_string()),
..Default::default()
};

let repo = Repository::new(&repo_opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let repo = Repository::new(&repo_opts)
let repo = Repository::with_options(&repo_opts)

Maybe it's better to make it explicit?

.unwrap()
.open()
.unwrap()
.to_indexed()
.unwrap();

// use latest snapshot without filtering snapshots
let node = repo.node_from_snapshot_path("latest", |_| true).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

These parameters could be a reasonable default. Like latest snapshot without filtering and may be a good candidate for an own method.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO, it is actually vice versa: When you want the latest snapshot, you usually specify extra conditions of w.r.t. what you want the latest snapshot.
On the other hand, if you specify a snapshots by (part of the) ID, you usually don't need any filtering as the snapshot is already defined.

I think the use cases how to get to a certain node which you want to restore will vary a lot depending on the user interface you are using. If you have a CLI, the node_from_snapshot_path is quite handy as it allows to give a string and will automatically resolve ids or latest snapshots from the string.
If you have some kind of GUI, I would prefer just fetching all snapshots using repo.get_matching_snapshots() or even repo.get_all_snapshots() (not present yet but would be repo.get_matching_snapshots() without filtering). Then a user could manually dive into some tree and pick the Node to be used for the restore.

TL;DR: Yes, we need to think about the API, but here I wouldn't do so ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

These notes are also meant as some future reference when we work on the API, by the way. Not really to change here, just so we are on the same boat (hence why I chose the comment instead of changes review feature).

For example, I would prefer both, get_all_snapshots() as a wrapper around (in this case) get_matching_snapshots() and get_latest_snapshot() etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree! 👍


// use list of the snapshot contents using no additional filtering
let recursive = true;
let streamer_opts = TreeStreamerOptions::default();
let ls = repo.ls(&node, &streamer_opts, recursive).unwrap();

let destination = "./restore/"; // restore to this destination dir
let create = true; // create destination dir, if it doesn't exist
let dest = LocalDestination::new(destination, create, !node.is_dir()).unwrap();

let opts = RestoreOpts::default();
let dry_run = false;
// create restore infos. Note: this also already creates needed dirs in the destination
let restore_infos = repo
.prepare_restore(&opts, ls.clone(), &dest, dry_run)
.unwrap();
Comment on lines +28 to +42
Copy link
Contributor

@simonsan simonsan Jul 11, 2023

Choose a reason for hiding this comment

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

I think this is overly verbose for the user-facing API. I think we should either replace all boolean values that are user-facing for these kinds of options with enums here, or wrap the booleans with a new type.

We could also use a DescriptorStruct that we pass to T.restore(), so we are able to have all the settings for the user in one place exposing certain values as reasonable defaults, could even combine that with a builder for the DescriptorStruct:
rust-unofficial/patterns#239 (comment)


repo.restore(restore_infos, &opts, ls, &dest).unwrap();
}
2 changes: 1 addition & 1 deletion crates/rustic_core/src/backend/local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ impl LocalDestination {
Ok(Self { path, is_file })
}

fn path(&self, item: impl AsRef<Path>) -> PathBuf {
pub(crate) fn path(&self, item: impl AsRef<Path>) -> PathBuf {
if self.is_file {
self.path.clone()
} else {
Expand Down
2 changes: 1 addition & 1 deletion crates/rustic_core/src/blob/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ pub struct TreeStreamerOptions {
}

/// [`NodeStreamer`] recursively streams all nodes of a given tree including all subtrees in-order
#[derive(Debug)]
#[derive(Debug, Clone)]
pub struct NodeStreamer<BE>
where
BE: IndexedBackend,
Expand Down
1 change: 1 addition & 0 deletions crates/rustic_core/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ pub mod init;
pub mod key;
pub mod prune;
pub mod repoinfo;
pub mod restore;
pub mod snapshots;
Loading