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

[Fixes #11089] Switch to a new installer approach using a path manipulation helper #12597

Merged
merged 1 commit into from
Dec 23, 2021

Conversation

n1hility
Copy link
Member

@n1hility n1hility commented Dec 14, 2021

What this PR does / why we need it:

Improves the Windows installation experience and fixes a few issues:

Fixes #11089 - cleanup PATH on MSI uninstall
Additionally fixes scenarios where the path can be overwritten by setx
Also removes the console flash, since the helper is built as a silent gui
Helper executable can be rerun by user to repair PATHs broken by other tools
Utilizes executable location instead of passed parameters to remove delicate escaping requirements

How to verify it

  1. Install podman msi
  2. Check registry
reg query hkcu\environment /v path

HKEY_CURRENT_USER\environment
    path    REG_EXPAND_SZ   %USERPROFILE%\AppData\Local\Microsoft\WindowsApps;;C:\Program Files\RedHat\Podman
  1. Uninstall
  2. Recheck registry
reg query hkcu\environment /v path

HKEY_CURRENT_USER\environment
    path    REG_EXPAND_SZ   %USERPROFILE%\AppData\Local\Microsoft\WindowsApps;

Which issue(s) this PR fixes:

Fixes #11089

Special notes for your reviewer:

None

@rhatdan
Copy link
Member

rhatdan commented Dec 14, 2021

@ashley-cui @jwhonce @baude PTAL

@n1hility
Copy link
Member Author

BTW I forgot to mention in case it comes up. The reason this was done as a separate executable as opposed to podman subcommand is to be able to compile it as a GUI application and get rid of the console that splashes and vanishes immediately after during installation. If we decide we do not want to leave the helper executable on the user FS, we can also embed it as a binary element in the installer (config change + switch to path param approach). Although I thought it would be handy to be able to run it outside of installation if needed (fix broken paths, add a path entry for other users, etc)

@TomSweeneyRedHat
Copy link
Member

I've rerun the failed test, looks to be a flake

@rhatdan
Copy link
Member

rhatdan commented Dec 14, 2021

@vrothberg PTAL

@TomSweeneyRedHat
Copy link
Member

LGTM
assuming happy tests

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Sorry, I can't really review. I have no expertise in Windows at all, the code isn't executed in CI and there are barely any code comments.

But I am OK to merge as long as I don't have to main it :^)

@n1hility
Copy link
Member Author

@vrothberg I added some comments and also a little prompt for standalone usage. Let me know if anything looks confusing and needs additional commenting

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Could you rebase on top of the latest HEAD? The e2e had a flake that has been fixed in the meantime.

Fixes containers#11089 - cleanup PATH on MSI uninstall
Additionally fixes scenarios where the path can be overwritten by setx
Also removes the console flash, since the helper is built as a silent gui
Helper executable can be rerun by user to repair PATHs broken by other tools
Utilizes executable location instead of passed parameters to remove delicate escaping requirements

[NO NEW TESTS NEEDED]

Signed-off-by: Jason T. Greene <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Dec 23, 2021

/approve
/lgtm
Thanks @n1hility

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 23, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n1hility, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 23, 2021
@openshift-merge-robot openshift-merge-robot merged commit c8d42d9 into containers:main Dec 23, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cleanup PATH on MSI uninstall
5 participants