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
Remove IsAdmin() environment variable override #2229
Conversation
// Note: on Windows, runtimes are not installed to the Admin %PATH%, even if the user has admin | ||
// privileges, so call CleanUserEnv with user scope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't sound right to me. We pass isAdmin
to WriteUserEnv()
, and it's respected there.
If CleanUserEnv()
doesn't handle this input the same as WriteUserEnv()
then we should address that head-on. The current change feels like we're working around the issue from the consuming code, which is error-prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After investigation, it was a bug in ConfigureAvailableShells
. Thanks for suggesting something was amiss.
I checked the other calls to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
Runtimes are not installed to the admin
%PATH%
even if the user has admin privileges (which is the case on CI), so when uninstalling the runtime, the user scope needs to be used. Previously, the environment variable would overrideIsAdmin()
to returnfalse
on CI (and thus uninstall with user scope despite having admin privileges).