WIP: feat(cli): implements 'safe reset' subcommand

The starting point is functional however being my first contribution I’m certain some work remains. I tried to repeat code style already used. Please review the code for any possible improvements.

  • A new CLI subcommand helper, prompt_confirmation, is used for confirmation.
  • Dry run mode is respected.
  • debug! is used.
  • The paths to be deleted is easily editable in code.
  • The unrelated change in node.rs was to work around a panic on my system.

What would you expect to see from tests for this subcommand?

7 Likes

@anon78698497 I’m not familiar with the gitlab UI, and I couldn’t see how to comment on the code there.

Some small things:

  1. 2021 for copyright notice
  2. where we’re removing specific files/dirs, does it make sense to print out the path string for each when in dry run mode? (Does it make sense to print that anyway, in a verbose mode perhaps?)
 if !dry_run {
     match fs::remove_file(&path) {
     ...
    }
} else {
    println!(path)
}
  1. in subcommand/node I think you’ve added a commented line + removed the shorthand for number of nodes: //#[structopt(short = "n", long = "nodes", default_value = "11")], was that intentional?

FYI I havent tested this just yet, but at first scan this looks good :+1: Thanks for the effort so far! I’ll geet the code grabbed and test it locally after lunch :sandwich:

1 Like

Nice job @anon78698497, some other comments (it’d be nice if we could comment in the PR, is there a way to enable that?):

  • Perhaps we can adapt the prompt_user helper to accept an optional arg (e.g. Option<&str>) which specifies what to ask the user to enter, so we have a single helper function covering what prompt_confirmation does too.
  • I see let mut paths: Vec<String> = vec![]; but we only work on a single path which is the ~/.safe, so perhaps we don’t need it, or if we are gonna populate it with several paths then declare it up front as Vec<PathBuf> ?

As a general comment, shouldn’t we try to remove only the data but not the executables? as it stands it’s more like an uninstallation rather than a reset, I was assuming we wanted something which can clear all data stored by nodes, CLI credentials, but not the actual apps/executables ?

@joshuef

Fixed, thanks.

I’d expect dry-run code to do so. That way I can catch if, for example, safe reset would remove a file I hadn’t yet noticed was changed and backed up for later inspection. Or, due to a bug, the working list of paths included something unintended and disastrous to the user’s data.

I think that without enumerating the FS endpoints to be destroyed, dry-mode wouldn’t have much use would it?

I caught the repo in a bad state, that workaround isn’t part of my proposed changes and can be ignored.

@bochaco

At WIP: feat(cli): implements 'safe reset' subcommand (!1) · Merge requests · Safe Network / sn_api · GitLab “Allows commits from members who can merge to the target branch” is enabled. I’d expect that to include anyone at Members · Safe Network / sn_api · GitLab. Perhaps @frabrunelle would add you there and/or to the organization at Group members · Safe Network · GitLab?

They both prompt the user however their logic is different enough that I would keep them separate. I think it’s also more clear to read let result = prompt_confirmation(); than let result = prompt_user(...String::from("confirm"));. That said, it’s your code and I’ll make the change if you decide?

Fixed, thanks.

Agreed. I asked for specific FS endpoints in SAFE CLI subcommand suggestions: reset and uninstall - #7 by anon78698497 however only ~/.safe was mentioned. How would you suggest I put together a better list of items to be deleted?

1 Like

We’ll have to disagree here, it’s not mine, I’m just providing comments like any other could and should do, hopefully with all comments and opinions we converge to a better code.

Perhaps have a list of the apps/files we don’t want to remove and then skip them when walking through the ~/.safe/ dir tree thus not removing them? I’m guessing we wanna keep only:

  • ~/.safe/cli/safe
  • ~/.safe/authd/sn_authd
  • ~/.safe/node/sn_node
2 Likes

@anon78698497 just to also take into account, I just found this, it’s very likely the only place where we currently do this type of confirmation: https://github.com/maidsafe/sn_api/blob/master/sn_cli/subcommands/files_get.rs#L383

2 Likes

@bochaco

Sure, I’ll work on that.

Are you proposing moving prompt_yes_no from subcommands/files_get.rs to subcommands/helpers.rs and reusing it in subcommands/reset.rs obviating prompt_confirmation entirely?

We definitely need files_get and reset to call the same function to do the prompt rather than having two functions which do the same, so we need a single helper in the helpers to be used by both. We can merge anything we need from prompt_yes_no into prompt_confirmation, with whichever we all prefer in general for the confirmation text, i.e. “Y/N”, “confirm”, or anything else.

1 Like

@bochaco

Ok, what do you want the confirmation text to be for the merged helper?

1 Like

@joshuef @StephenC what would you say we use in CLI for prompt confirmation?
@anon78698497 what’s you preference? @mav @Scorch if you are around and wanna share your opinion too? anyone else??

I won’t vote myself this time :slight_smile:

2 Likes

And for context, @bochaco is proposing merging the new helper I’ve created, prompt_confirmation, with prompt_yes_no, and searching for the best wording to the user.

prompt_user was also suggested as a possibility for merging however its logic is different.

To help select the most appropriate language, here’s a list of the uses:

2 Likes

This is my opinion, so take it for what it is.

From a usability standpoint though, I don’t think we need to reinvent the wheel in terms of what the behavior is for removing a bunch of files. I.e. do we prompt for y/n or confirm or something else. Sometimes clarity & usability are more related to user expectations than to the exact semantics.

CLI users can be reasonably expected to have some knowledge of the command-line-interface in general, since they’re running sn_cli in a terminal after all. What if we just copied an existing behavior that feels familiar to those users, like the ubiquitous rm command (Linux man page is here)? Or really anything else that feels familiar for “removing some files” (maybe looking towards some of the git subcommands).

In the case of something like rm, it would mean replacing dry-run, and only prompt for confirmation with the -i or -I options. Users who are afraid of deleting everything can always alias the command. What do you think?

4 Likes

I’ve always liked these apt-get flags
-y | --yes | --assume-yes (man page entry)
and
--assume-no (man page entry just below --assume-yes)
with default being a prompt
Do you want to continue? [Y/n]:
so pressing enter with blank value means yes
or typing y or Y means yes
or typing n or N means no
or pressing Ctrl^C exits and is equivalent to a no
anything else prompts again
but this is not exactly the simplest option to implement

Also agree with @Scorch that rm is probably a good place to take inspiration in the specific case of safe reset.

3 Likes

I think it looks to me like we can drop prompt_confirmation in favour of prompt_yes_no and use that with some tweaked wording (and add a default as @mav suggests).


I think one reason to avoid rm style, no-conf is that these operations will have a monetary cost. dry-run is pretty clear, and also it’s place in well used CLIs (aws being one that springs to mind). As long as we can enable it and or negate prompts I think we’re good.

But this is probably a bigger convo about the style of the CLI in general…

1 Like

That’s the opposite of prompt_confirmation, which fails for anything other than the user entering “confirm”.

It could be that the new function should remain.

  • prompt_yes_no prompts for, and defaults to, affirmative.
  • prompt_confirmation prompts for “confirm” (potentially configurable) and defaults to negative.
  • prompt_user prompts for input.

Thoughts everyone?

1 Like

@bochaco

How could I obtain permission to add a dependency for ~/.safe traversal?

1 Like

My impression is that it’s not that formal of a process unless the dependency is really big or un-needed (somebody correct me if I’m wrong). Probably easiest to make your code do what you want first. Then you can always see about getting feedback, pairing down dependencies, and making things smaller/faster.

On a related note, I don’t know what you need to do with it other than walk the directory, but you can always implement this yourself with std::fs::read_dir. In the docs there’s a straight-forward example that’s like ~10 lines that shows how to recursively walk a directory. Might be easier than worrying about dependencies/breakage in the future :man_shrugging: .

I don’t see anything wrong with this implementation from an API perspective, so seems fine to me for now. But I would say, in terms of future usage I agree with @joshuef that one is still a specialization of the other. It still makes sense to me to include a generic driver function, because they do duplicate an awful lot of code, even though they’re different.

I don’t want to tell you what to do, but I tend to think in terms of code so hopefully this expresses what I’m getting at. Here’s an example function signature idea that could drive both.

fn prompt_binary<T: AsRef<str>>(
    prompt_msg: &str,
    affirmative_strs: Iterator<Item=T>,
    negative_strs: Iterator<Item=T>,
) -> Option<bool> {
    //...
    // call prompt_user() and do some pattern matching here
    //...
}

Now you use "" as the “default” by just adding it into affirmative_strs or negative_strs. Then it returns Some(true) if the input was in the affirmative_strs, Some(false) if the input was in the negative_strs and None if it was unkown.

Thusly, prompt_yes_no just becomes the following. You can also add stuff like default_yes as an argument and tweak it in generaly a lot easier than before.

fn prompt_yes_no(msg: &str) -> bool {

    let yes_strs = ["yes", "y", ""].iter();
    let no_strs = ["no", "n"].iter();

    while let maybe_resp = prompt_binary(msg, yes_strs, no_strs) {
        match maybe_resp {
            Some(resp) => return resp;
            None => println!("Unrecognized input, please try again.");
        }
    }
}

And prompt_confirmation is just

fn prompt_confirmation(msg: &str) -> bool {
    prompt_binary(
        msg,
        ["confirm"].iter(),
        std::iter::empty()
    )
    .unwrap_or(false)
}
1 Like

Seems to me like prompt confirmation is just prompt Y/n, but y/N, ie not defaulting to accepting the prompt.

Seems like prompt_binary is a neat way to handle all these cases.


re: deps, there’s no strict policy atm @anon78698497. Assessment would form part of review steps I suppose. But a general rule could be to avoid adding one if we can do so easily.

2 Likes

Unfortunately I’m not available to do anything further with the code. Its latest state is available to anyone to modify as-needed, if desired.