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

feat: add restart button after enabling / disabling mac os compatibility #2841

Merged
merged 1 commit into from Jun 29, 2023

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented Jun 13, 2023

feat: add restart button after enabling / disabling mac os compatibility

What does this PR do?

  • Adds a button that will ask if the user if they'd like to restart the
    podman machine to enable compatibility mode

Screenshot/screencast of this PR

Screen.Recording.2023-06-13.at.2.36.57.PM.mov

What issues does this PR fix or reference?

Fixes #2406

How to test this PR?

  1. Press the compatibility button while on Mac OS
  2. Enable / Disable
  3. At the end it should ask if you'd like to restart your podman machine
  4. Click and it will restart.

@cdrage cdrage requested review from a team and benoitf as code owners June 13, 2023 18:37
@cdrage cdrage requested review from jeffmaury and lstocchi and removed request for a team June 13, 2023 18:37
@cdrage cdrage force-pushed the add-restart-button branch 4 times, most recently from d559c98 to edf78dc Compare June 15, 2023 19:11
@cdrage
Copy link
Contributor Author

cdrage commented Jun 15, 2023

Tests added and ready for review!

@benoitf
Copy link
Collaborator

benoitf commented Jun 16, 2023

Tested but it looks like I'm in an infinite loop on disabling. It always prompts to disable and no dialog about restarting my machine

froKILe9N0.mp4

@cdrage
Copy link
Contributor Author

cdrage commented Jun 19, 2023

@benoitf Can you try again? I've rebased and changed the order of the restart prompt, but I'm unable to reproduce it on my machine (it disables and then prompts right after).

@benoitf
Copy link
Collaborator

benoitf commented Jun 19, 2023

Tried but still in the same state

@benoitf
Copy link
Collaborator

benoitf commented Jun 19, 2023

Also, I was wondering if on macOS it could work with the touchID when I need to disable/enable instead of having to type my password

@cdrage
Copy link
Contributor Author

cdrage commented Jun 19, 2023

Tried but still in the same state

Fixed!

I had:

const defaultMachine: MachineJSON = machines.find(machine => machine?.Name === 'Default');

Instead of:

const defaultMachine: MachineJSON = machines.find(machine => machine?.Default == true');

For finding the default machine.

It should work now 👍

@cdrage
Copy link
Contributor Author

cdrage commented Jun 19, 2023

Also, I was wondering if on macOS it could work with the touchID when I need to disable/enable instead of having to type my password

I only found https://github.com/emilbayes/macos-touchid but it's really old.

The only other thing is perhaps using: https://davidwalsh.name/touch-sudo

@benoitf
Copy link
Collaborator

benoitf commented Jun 19, 2023

trying again, (and yes I'm not using the default machine)

about touch key I know there is a manual way of enabling it (editing pam modules) , was just thinking for 'default users'

@benoitf
Copy link
Collaborator

benoitf commented Jun 19, 2023

Got this time the restart dialog but then clicking on yes:

StatusBarItem.svelte:39 Uncaught (in promise) Error: Error: cannot start VM podman-machine-default. VM foo is currently running or starting: only one VM can be active at a time

(VM that was running before was foo vm)

@benoitf
Copy link
Collaborator

benoitf commented Jun 19, 2023

my usecase:

at least 2 podman machines, the default one and foo and foo is running

then I click on 'disable', it says it is disabled, I click on yes about restarting my machine and then got the message in console (nothing reported in the UI)

@cdrage
Copy link
Contributor Author

cdrage commented Jun 19, 2023

my usecase:

at least 2 podman machines, the default one and foo and foo is running

then I click on 'disable', it says it is disabled, I click on yes about restarting my machine and then got the message in console (nothing reported in the UI)

Ah, it's trying to restart the one that's not default (since I don't specify the machine).

I'll update the PR. Thanks for testing this out. Totally forgot the use case of a machine not-set-as-default.

@cdrage cdrage force-pushed the add-restart-button branch 3 times, most recently from 41e66c8 to e680f0e Compare June 22, 2023 12:47
@benoitf
Copy link
Collaborator

benoitf commented Jun 23, 2023

I tested again but it looks like there is a problem

tooltip say : 'Enable' and when I click it says 'Disable' and before launching Podman Desktop I disabled the compatibility mode.

I would expect that I could enable if it's disabled

yPZjBvHDpL.mp4

@cdrage
Copy link
Contributor Author

cdrage commented Jun 23, 2023

I tested again but it looks like there is a problem

tooltip say : 'Enable' and when I click it says 'Disable' and before launching Podman Desktop I disabled the compatibility mode.

I would expect that I could enable if it's disabled

yPZjBvHDpL.mp4

I was able to replicate this on the main branch, this is a bug not introduced by this PR so I've opened up issue: #2985

@benoitf
Copy link
Collaborator

benoitf commented Jun 23, 2023

I think I'm hitting also #2781 during the restart (two processes want to restart a machine) so probably need to be merged after #2942

@cdrage
Copy link
Contributor Author

cdrage commented Jun 23, 2023

Ah yes you're right. In the console I'm seeing the error appear sometimes. Let's block this PR until #2942 is closed.

@cdrage
Copy link
Contributor Author

cdrage commented Jun 29, 2023

ping @benoitf @lstocchi @jeffmaury ready for another review since #2942 has been merged in

@benoitf
Copy link
Collaborator

benoitf commented Jun 29, 2023

@cdrage do you have rebased the branch ? it seems not so could be easier if you rebase

### What does this PR do?

* Adds a button that will ask if the user if they'd like to restart the
  podman machine to enable compatibility mode

### Screenshot/screencast of this PR

<!-- Please include a screenshot or a screencast explaining what is doing this PR -->

### What issues does this PR fix or reference?

<!-- Please include any related issue from Podman Desktop repository (or from another issue tracker).
-->

Fixes containers#2406

### How to test this PR?

1. Press the compatibility button while on Mac OS
2. Enable / Disable
3. At the end it should ask if you'd like to restart your podman machine
4. Click and it will restart.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Contributor Author

cdrage commented Jun 29, 2023

@cdrage do you have rebased the branch ? it seems not so could be easier if you rebase

Ah you're right. I didn't rebase yet. Just pushed it

@benoitf
Copy link
Collaborator

benoitf commented Jun 29, 2023

JmVBbghExC.mp4

wondering that if I failed to enable it, if I should have the prompt to restart the machine ?

Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

was able to disable/enable or enable/disable with a restart. It is now restarting the correct machine.

was just wondering for the previous comment, if I'm not able to enable or disable, I would expect to not be prompted to restart the machine but could be a follow-up

@cdrage
Copy link
Contributor Author

cdrage commented Jun 29, 2023

I agree, I will assign #2985 to myself so we can fix that.- There's something odd happening with the tooltip / enable / disable.

@cdrage cdrage merged commit fa13be8 into containers:main Jun 29, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.2.0 milestone Jun 29, 2023
mairin pushed a commit to mairin/podman-desktop that referenced this pull request Jul 7, 2023
…ity (containers#2841)

### What does this PR do?

* Adds a button that will ask if the user if they'd like to restart the
  podman machine to enable compatibility mode

### Screenshot/screencast of this PR

<!-- Please include a screenshot or a screencast explaining what is doing this PR -->

### What issues does this PR fix or reference?

<!-- Please include any related issue from Podman Desktop repository (or from another issue tracker).
-->

Fixes containers#2406

### How to test this PR?

1. Press the compatibility button while on Mac OS
2. Enable / Disable
3. At the end it should ask if you'd like to restart your podman machine
4. Click and it will restart.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
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.

podman docker compatability warning/text misleading/confusing
3 participants