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 compatibility button for Linux using flatpak-spawn #1925

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented Apr 3, 2023

feat: add compatibility button for Linux using flatpak-spawn

What does this PR do?

  • Uses flatpak-spawn
  • Adds a compatibility button that will enable / disable the systemd
    podman.socket that allows Docker compatibility
  • Refactors the runSudoHelperCommand function so that both macOS and
    Linux implementations can use it

Screenshot/screencast of this PR

What issues does this PR fix or reference?

Closes the rest of #1447

How to test this PR?

  1. Use Linux
  2. yarn watch
  3. Click on the compatibility mode and follow the steps to enable /
    disable.

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

@cdrage cdrage requested review from a team and benoitf as code owners April 3, 2023 15:04
@cdrage cdrage requested review from jeffmaury and lstocchi and removed request for a team April 3, 2023 15:04
@cdrage cdrage marked this pull request as draft April 3, 2023 15:07
@cdrage cdrage force-pushed the linux-compat branch 3 times, most recently from b2791f6 to 2d87d7a Compare April 3, 2023 20:07
@cdrage cdrage marked this pull request as ready for review April 3, 2023 20:16
@cdrage
Copy link
Contributor Author

cdrage commented Apr 3, 2023

Ready for review!

Works on Fedora 39 / Ubuntu but not on Debian.

Will ask you for a password when enabling / disabling as it's using systemctl to enable and disable the socket.

The warning will NOT go away due to this issue: #1928 on Linux.

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Tried on fedora 37 but nothing happens apparently. Not sure if i'm expecting a different behavior or it doesn't work. I click on the compatibility button, it asks me to enable it. I authenticate. Everything seems went fine. Then if i re-click i would expect the disable workflow but it repeat the enablement.
enable

@cdrage cdrage force-pushed the linux-compat branch 8 times, most recently from d0c111a to a686e81 Compare April 5, 2023 12:44
@cdrage cdrage marked this pull request as draft April 5, 2023 12:57
@cdrage
Copy link
Contributor Author

cdrage commented Apr 5, 2023

Tried on fedora 37 but nothing happens apparently. Not sure if i'm expecting a different behavior or it doesn't work. I click on the compatibility button, it asks me to enable it. I authenticate. Everything seems went fine. Then if i re-click i would expect the disable workflow but it repeat the enablement. enable enable

That's an oversight on my part, there are some issues with running yarn watch vs the binary and I've set this PR back to draft so I can fix them. The sudo-prompt isn't working correctly. Thanks for pointing this out, I was testing it via the binary only.

@cdrage cdrage force-pushed the linux-compat branch 6 times, most recently from 3bc8bc7 to 6b52398 Compare April 5, 2023 15:34
@cdrage
Copy link
Contributor Author

cdrage commented Apr 5, 2023

Done, ready for another review.

Using polkit a password prompt will automatically show when running systemctl enable

Keep in mind, that the warning will ALWAYS show due to bug: #1928

See video below:

Screen.Recording.2023-04-05.at.11.09.47.AM.mov

@cdrage cdrage marked this pull request as ready for review April 5, 2023 15:35
@cdrage cdrage force-pushed the linux-compat branch 4 times, most recently from ea0adfd to 7e1de3c Compare April 5, 2023 20:11
@lstocchi lstocchi self-requested a review April 6, 2023 12:45
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Tried but i still don't see the disable part.

When i click on the action, the enablement (systemctl enable...) works fine but then i read in the dialog i have to execute the command ln -s ... . Can't this be performed automatically somehow?
If i click ok i don't see it anymore and i don't have any way to retrieve it. Even changing the icon on the bottom with a mark if the docker socket does not exists to remind me to do the linking would be helpful. Or atleast we could add a button copy command in the dialog so it is more clear i have to do something. Even though i'd prefer to have it working without doing anything manually.

The other problem i have is that i cannot never disable it. If i click on the compatibility action it keeps enabling it. It would be great if it disable/delete the linking/socket

@cdrage
Copy link
Contributor Author

cdrage commented Apr 6, 2023

Tried but i still don't see the disable part.

When i click on the action, the enablement (systemctl enable...) works fine but then i read in the dialog i have to execute the command ln -s ... . Can't this be performed automatically somehow? If i click ok i don't see it anymore and i don't have any way to retrieve it. Even changing the icon on the bottom with a mark if the docker socket does not exists to remind me to do the linking would be helpful. Or atleast we could add a button copy command in the dialog so it is more clear i have to do something. Even though i'd prefer to have it working without doing anything manually.

The other problem i have is that i cannot never disable it. If i click on the compatibility action it keeps enabling it. It would be great if it disable/delete the linking/socket

Unfortunately it's because of "Keep in mind, that the warning will ALWAYS show due to bug: #1928"

I did automate the sudo ln -s but unfortunately there was ANOTHER bug with polkit not asking for authentication under flatpak-spawn --host sudo, but it works with flatpak-spawn --host systemctl

@cdrage cdrage force-pushed the linux-compat branch 4 times, most recently from ab48e66 to 7bc5353 Compare April 12, 2023 15:10
@cdrage
Copy link
Contributor Author

cdrage commented Apr 12, 2023

@lstocchi Figured it out! Was able to use pkexec successfully and added the ability to create a ln -s command that will run.

NOTE: Still wont get rid of the warning due to #1928 but at least compatibility mode will work with CLI tools

Ready for another review @lstocchi and @benoitf

Screen.Recording.2023-04-12.at.11.11.36.AM.mov

@lstocchi lstocchi self-requested a review April 17, 2023 10:26
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Tested. LGTM. GJ!! 🚀

I can see the docker.sock created correctly and it works as expected.

My only concern is that we should fix #1928 before release with this patch otherwise it is confusing.

@cdrage cdrage requested a review from lstocchi April 20, 2023 16:56
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

Reapproving as i use the gh filter to see which PRs i need to review 😆

@cdrage
Copy link
Contributor Author

cdrage commented Jun 1, 2023

This was requested in the Discord recently with regards to the status of it.

I"ve gone ahead and rebased it. Could use with another review / possible merge!

### What does this PR do?

* Uses flatpak-spawn
* Adds a compatibility button that will enable / disable the systemd
  podman.socket that allows Docker compatibility
* Refactors the runSudoHelperCommand function so that both macOS and
  Linux implementations can use it

### 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).
-->

Closes the rest of podman-desktop#1447

### How to test this PR?

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

1. Use Linux
2. `yarn watch`
3. Click on the compatibility mode and follow the steps to enable /
   disable.

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

checked still working on macOS

@cdrage
Copy link
Contributor Author

cdrage commented Jun 5, 2023

Merging as we've got one approval from @lstocchi who tested on Linux, and @benoitf made sure that functionality still works on macOS with the other changes added.

Thanks! 💯

@cdrage cdrage merged commit 8b7ed6b into podman-desktop:main Jun 5, 2023
@podman-desktop-bot podman-desktop-bot added this to the 1.1.0 milestone Jun 5, 2023
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.

4 participants