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

Add reset command #983

Merged
merged 3 commits into from Jul 22, 2019
Merged

Add reset command #983

merged 3 commits into from Jul 22, 2019

Conversation

pinkisemils
Copy link
Collaborator

@pinkisemils pinkisemils commented Jul 22, 2019

Add CLI command to remove daemon settings, clear the cache and logs.


This change is Reviewable

@pinkisemils pinkisemils requested a review from jvff July 22, 2019 13:12
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @pinkisemils)


mullvad-cli/src/cmds/reset.rs, line 3 at r1 (raw file):

use crate::{new_rpc_client, Command, Result};
use std::io::stdin;
pub struct Reset;

Nit: Could you add a newline to separate the imports from the struct?


mullvad-cli/src/cmds/reset.rs, line 30 at r1 (raw file):

impl Reset {
    fn receive_confirmation() -> bool {
        println!("Are you sure you want to disconnect, log out, delete all settings, logs and cache files for the Mullvad VPN system service? [y/N]");

Minor: Maybe change the input hint so that it's something like [Yes/No (default)]?


mullvad-daemon/src/lib.rs, line 108 at r1 (raw file):

    RemovalError(#[error(cause)] io::Error),

    #[error(display = "Failed to remove directory")]

Missing update to command description.


mullvad-daemon/src/lib.rs, line 947 at r1 (raw file):

    fn on_factory_reset(&mut self, tx: oneshot::Sender<()>) {
        // Disconnect to ensure that log files won't be rotated into the new log directories
        self.set_target_state(TargetState::Unsecured);

Should the code wait for the tunnel to disconnect before starting the factory reset? Not sure if the logs would get written with something while disconnecting.


mullvad-daemon/src/lib.rs, line 969 at r1 (raw file):

            failed = true;
        }
        if !failed {

Nit: Maybe also add a newline here? 🤔


mullvad-types/src/settings.rs, line 29 at r1 (raw file):

    ReadError(String, #[error(cause)] io::Error),

    #[error(display = "Unable to remove settings file")]

Shouldn't the settings file path be printed here?

Copy link
Collaborator Author

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jvff)


mullvad-daemon/src/lib.rs, line 108 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Missing update to command description.

Done.


mullvad-daemon/src/lib.rs, line 947 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Should the code wait for the tunnel to disconnect before starting the factory reset? Not sure if the logs would get written with something while disconnecting.

Nah, I don't believe there's actually any risk. I ran into issues during testing due to the log directories being wholly removed. But I resolved that issue. I'm not sure whether waiting or disconnecting at all here makes any sense.


mullvad-types/src/settings.rs, line 29 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Shouldn't the settings file path be printed here?

Done.

Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@pinkisemils pinkisemils merged commit 6344733 into master Jul 22, 2019
Copy link
Member

@faern faern left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)


mullvad-cli/src/cmds/reset.rs, line 30 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Minor: Maybe change the input hint so that it's something like [Yes/No (default)]?

Using y/N where the capital letter is the default is very standard.


mullvad-daemon/src/lib.rs, line 959 at r2 (raw file):

        }

        if let Err(e) = self.settings.reset() {

Don't we want this on_factory_reset to also notify all frontends that the settings changed?

self.event_listener.notify_settings(self.settings.clone());

Copy link
Collaborator Author

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)


mullvad-daemon/src/lib.rs, line 959 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Don't we want this on_factory_reset to also notify all frontends that the settings changed?

self.event_listener.notify_settings(self.settings.clone());

The daemon is supposed to restart at the end of this - so all listeners will have to resubscribe and reset anyway. But we can publish the new cleared settings, if you feel that will be more correct.

@faern faern deleted the add-reset-command branch August 8, 2019 10:54
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.

None yet

3 participants