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 preflight checks for windows #369

Merged
merged 9 commits into from Aug 8, 2022

Conversation

evidolob
Copy link
Contributor

@evidolob evidolob commented Aug 3, 2022

What does this PR do?

Add preflight check which executes before podman installation.

Screenshot/screencast of this PR

WSL2 not installed:

2022-08-03.16-04-11_Trim.mp4

All checks passed:

2022-08-03.16-23-23_Trim.mp4

What issues does this PR fix or reference?

Resolve #326

How to test this PR?

You need windows machine without podman, hyper-v, wsl2 installed. Run podman-desktop, on podman welcome page click on Install button.

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.

I'll test it on a Windows VM (may be delayed until tomorrow)

if (this.ARCH === currentArch) {
return this.createSuccessfulResult();
} else {
return this.createFailureResult('WSL2 works only on x64 OS.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if it would be possible to add links/pointers to the user explaining it with more details (could be another iteration)

like showing https://docs.microsoft.com/en-us/windows/wsl/install or stuff like that

Here what is unclear to me is that it seems WL 2 is working with ARM64 based on https://docs.microsoft.com/en-us/windows/wsl/install-manual#step-2---check-requirements-for-running-wsl-2

so here probably it doesn't work for Podman, but not for WSL2 (need to change the failure message ?)

return { successful: true };
} else {
return this.createFailureResult(
'To be able to run WSL2 you need Windows 10 Build 18362 or later. Please update your OS.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

extensions/podman/src/podman-install.ts Outdated Show resolved Hide resolved
extensions/podman/src/podman-install.ts Outdated Show resolved Hide resolved
@benoitf
Copy link
Collaborator

benoitf commented Aug 4, 2022

I tried with Windows 11 development VM without the nested virtualization enabled
I received:
image

but WSL2 is pre-installed on this image so it looks like we're giving a wrong indication to the users saying WSL2 is not installed

and the hint: do "wsl --install" in terminal is not working

image

status command indicates I have wsl2

image

@evidolob
Copy link
Contributor Author

evidolob commented Aug 4, 2022

@benoitf As for doc links, I add links, but they rendered as plain text.
I could add them as links, but we need API which allows to open new tabs on system browser, it can be done with https://www.electronjs.org/docs/latest/api/shell module, but it is available on main process

@benoitf
Copy link
Collaborator

benoitf commented Aug 4, 2022

@evidolob there is already openExternal exposed in window/UI side using shell:openExternal

contextBridge.exposeInMainWorld('openExternal', async (link: string): Promise<void> => {
return ipcInvoke('shell:openExternal', link);
});

@benoitf
Copy link
Collaborator

benoitf commented Aug 5, 2022

I'll try again when PR has produced the windows artifact

@benoitf
Copy link
Collaborator

benoitf commented Aug 5, 2022

Trying the latest commit

Everything looks good (all checks are green)
image

But I don't have nested virtualization so I can't initialize a podman machine
image

Probably the pre-check should warn me about that so I don't install something that is not possible to run at the end?

@benoitf
Copy link
Collaborator

benoitf commented Aug 5, 2022

Saying no on Install I always see the pre-flight checks as well there is no way to hide them

b4OQmVisdK.mp4

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.

I'll address some enhancements in upcoming PR

good work @evidolob

evidolob and others added 9 commits August 8, 2022 14:11
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Signed-off-by: Yevhen Vydolob <yvydolob@redhat.com>
Change-Id: Ic4104e57b3e7fdea073f099bfc4fc36e6a36304e
Signed-off-by: Florent BENOIT <fbenoit@redhat.com>
Change-Id: I13335889c9a6da7b7f6a4d158273a3cdcb45c5c5
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@benoitf
Copy link
Collaborator

benoitf commented Aug 8, 2022

I've added the hide of checks at the end of the install step

@benoitf benoitf merged commit df13dfb into containers:main Aug 8, 2022
@podman-desktop-bot podman-desktop-bot added this to the 0.0.6 milestone Aug 8, 2022
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.

Implement preflight checks for podman on Windows
3 participants