fix: only restart if a machine is running

[cdrage]

fix: only restart if a machine is running

What does this PR do?

No matter what, it would try to restart regardless if a machine was
running or not.

This is because the if statement was: if (isRunning !== '') which will
always return true even if isRunning was undefined.

The if statement has now been changes to if (isRunning) meaning it
will only run if isRunning is defined (not '').

Screenshot/screencast of this PR

N/A. The prompt will not appear when trying to restart it if there is no
machine running.

What issues does this PR fix or reference?

Closes #3321

How to test this PR?

Click on Docker Compatibility with a Podman Machine stopped. You should
not get the prompt to restart the podman machine.

Signed-off-by: Charlie Drage charlie@charliedrage.com

[deboer-tim]

findRunningMachine() says it returns a boolean, maybe fix that too?

[cdrage]

Ready for review! 💯 had to figure out what was happening inside the tests and then realized it wasn't being mocked correctly (even for an old test too).

Updated and ready 👍

[dgolovin]

Verified on mac and it works for me now. No requests to restart machine if it does not exist or is not started.

disable() and enable() functions can be improved (optional).

LGTM

[cdrage]

Thank you for the suggestion @dgolovin ! Totally makes the code a lot cleaner. Pushed your suggestion 👍

[cdrage]

@dgolovin

Unfortunately return findRunningMachine().then(machine => this.promptRestart(machine)); does not work compared to:

    const isRunning = await findRunningMachine();
    if (isRunning) {
      await this.promptRestart(isRunning);
    }

    return Promise.resolve();

As even when it returns undefined in findRunningMachine, promptRestart is still ran in the function: return findRunningMachine().then(machine => this.promptRestart(machine));

If you replace the code, you'll see that the tests now fail.

I've updated the code back to the previous change.

[dgolovin]

@cdrage I was suggesting to do this as a fix.

Please remove at least

return Promise.resolve();

it is not needed and result is going to be the same.

[cdrage]

@cdrage I was suggesting to do this as a fix.

Please remove at least

return Promise.resolve();

it is not needed and result is going to be the same.

Hi, I commented on the PR. Bit confused on return Promise.Resolve suggestion, it's added to the tests in order to mock the functionality.

[cdrage]

@dgolovin Ready for another review / I've pushed your changes! :)

[cdrage]

Getting some weird errors with smoke testing @odockal @dgolovin any ideas? Rebased + it works fine with yarn watch, but unsure why it's causing these tests to fail.

[cdrage]

Tests pass now 💯