feat(commands): Add mount command
#973
Conversation
aawsome
force-pushed
the
mount
branch
2 times, most recently
from
e528c44
to
0b938fe
Compare
|
This PR is now ready but we may want to add the feature flag in our build-pipelines on selected targets (and include the dependencies) - in another PR. |
src/commands/mount.rs
Outdated
| let path_template = self | ||
| .path_template | ||
| .clone() | ||
| .unwrap_or_else(|| "[{hostname}]/[{label}]/{time}".to_string()); | ||
| let time_template = self | ||
| .time_template | ||
| .clone() | ||
| .unwrap_or_else(|| "%Y-%m-%d_%H-%M-%S".to_string()); |
There was a problem hiding this comment.
Is that something we want to be able to override from config?
There was a problem hiding this comment.
Is added. I just wonder how we want to use this in our config tests.. Should we add this to full.toml and the check-config fixture?
BTW: We have the same topic with the "webdav" feature, but this we include in all our tests. Here, we have a feature which is definitively not included in some targets, like windows...
Or maybe it is a better idea to compile the config structure unconditionally and just have the command conditionally compiled?
There was a problem hiding this comment.
Hmm, the question is what happens if a config value is given on a system that doesn't support this, e.g. webdav or mount. Is there an error like 'The webdav feature is not supported on your system, it will be ignored in the config.'or something? Because that would be useful I guess and good UX.
There was a problem hiding this comment.
You get an error when parsing the config, e.g. when compiled without the mount feature:
error: rustic-rs fatal error: parse error: unknown field `mount`, expected one of `global`, `repository`, `snapshot-filter`, `backup`, `copy`, `forget`, `webdav` at line 32 column 1
I actually think we should exactly use this behavior. If there is no mount it is simply not wanted in the config.
About tests I think we should add this to the full.toml and show-config tests - as soon as we add the mount option to the tests... (maybe this requires to use special test fixtures for windows...)
There was a problem hiding this comment.
Do we have control over the error message? Because that one is pretty bad and doesn't say anything what is happening and why this is an unknown field in this case. We need to document this error somewhere, so we don't possibly get a lot of issues about this.
Also, I'm not sure about the behaviour itself. Someone could migrate their config to another OS. E.g. from Linux to another Mac or Windows system, where they wouldn't be currently able to use mount. And then suddenly they get this error. Is there a way to check if the error is regarding mount or webdav and if so, spit out a specialized message, in regard to the missing feature on that platform or so?
There was a problem hiding this comment.
Mhh, ok, if we want to support copying tomls over, I agree that shouldn't be an error...
But then I would not even print a warning - if we use another command when having compiled with the mount option, that part of the config is also not used and we don't warn about that..
There was a problem hiding this comment.
I think we need to decide if the profile is OS-dependent or not. I would root for not, because it makes it easier to test as well as writing examples for the whole user base.
Instead of deny_unknown_fields I would propose giving feedback to the user, that certain keys are in the profile and not being used for whatever reason. e.g. mount and webdav if support is not compiled in, or if keys are used, that are not valid in a profile context.
I think that is pretty standard procedure to warn about content in a profile that has no effect, to not leave the user in the dark about it. As they may think, that something is set, and derive the current behaviour from that, although it's not even being considered. Rclone options come to mind and others. So I think it's valuable to warn about that.
Maybe something like: https://crates.io/crates/serde_path_to_error can be helpful here.
There was a problem hiding this comment.
I agree we should just keep the feature flag.
About the warning: In principle I agree about general options, but here we have options specific for a command which cannot even be called when not compiled with the feature...
I wonder how we should be able to print a warning. Imagine a user running rustic backup on a version without the mount feature. Should we warn "you have [mount] in your profile which is not used"? I mean when users call rustic mount with that build, they will already get an error and [mount] is only ever used in the mount command...
And users using a version with the mount feature would call rustic backup without getting a warning about [mount] in the profile - even though their called command does not use any of the options set there...?!?
I have now added a Value field in the case without the feature - that will suppress the error but also don't print any warning...
There was a problem hiding this comment.
I'm thinking more of a println than a warning I think, as an INFO. But independent of the logging. So users become aware of that and can remove it manually from the profile if they want.
There was a problem hiding this comment.
That being said, this is just the way I think it makes sense to do it, for informing the user, about values in the config, that have no effect. We don't need to do it that way. Also, we may need to refactor some things, regarding the config parsing step for this, to be able to spit out better errors/warnings? So probably nothing for this PR here.
simonsan
added
C-enhancement
|
rebased on top of #1329 |
|
Some things that I think we should do to prepare for this PR:
tbc |
This PR adds the
mountcommand to rustic to access snapshot contents as a read-only filesystem when the feature-flagmountis chosen.As for the
webdavcommand, there are following options:rustic mount /mnt 37a63e5b:/my/path.rustic mount /mnt --path-template "[{hostname}]/[{label}]/{time}" --time-template "%Y-%m-%d_%H-%M-%S"(these are also defined as default). Note that for all dirs containing only snapshots, also alatestentry is generated.latestand identical subsequent snapshots are symlinks when usingmount.This PR uses fuse_mt which is not optimal as it introduces some overhead (e.g. needs to save whole Paths in-memory).
Note: Requires rustic-rs/rustic_core#331 to properly show all data of files (without it builds and runs, but files are truncated and return error when reading).
Note: Building with the
mountfeature flag requires special dependencies and is only possible on supported platforms, see https://github.com/cberner/fusercloses #971