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

(GH-339) Exit from elevated shell after completion #340

Closed

Conversation

joaocgreis
Copy link

This is my attempt at fixing #339.

I don't have enough experience with Boxstarter to be sure this won't break other use cases, but I confirmed this fixed it for me.

Copy link
Member

@mwrock mwrock left a comment

Choose a reason for hiding this comment

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

I believe there are some scenarios where you would not want the window to close. I think this would be fine in a web launcher scenario because boxstarter prompts ou at the end to "type rnter to exit" and holds the window open. However this is not the case if running Install-BoxstarterPackage. If there was an error or even success you would want to see the errors or notice that the install completed.

Copy link
Member

@flcdrg flcdrg left a comment

Choose a reason for hiding this comment

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

If we do this, I'd prefer it to be optional (eg. enabled by a parameter)

@joaocgreis
Copy link
Author

Thanks for your reviews!

@mwrock this happens when running Install-BoxstarterPackage (in nodejs/node#22645). This only happens when it needs to open a new PowerShell windows with Admin permissions. I removed noexit, but added the Type ENTER to exit message at the end of the command, so errors should be visible.

@flcdrg the current behavior leaves behind an elevated PowerShell window ready for anyone to use or be confused by. If it's worth adding an option for this, doesn't it make more sense for the default to be the new behavior? If there's a case for leaving behind an elevated PowerShell console, the option could be used.

AppVeyor failed, but I'm not sure the failure is related. I also see some failures when running the tests locally (possibly issues with machine setup), but they are the same as master.

@mwrock
Copy link
Member

mwrock commented Sep 2, 2018

Unfortunately I think in the web launcher scenario, you will now have 2 prompts to "type enter". Admittedly the way this works is pretty convoluted but you will want to make sure invoking via the web launcher does not prompt twice.

Copy link
Member

@pauby pauby left a comment

Choose a reason for hiding this comment

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

I agree with @flcdrg in that the existing behaviour should be preserved as that is what people are expecting. I agree the window being open at the end could be abused but given that the use case for Boxstarter I see that 'risk' as been minimised.

We should monitor this to see how well works and could come back to the issue of it being the default behaviour later.

I'd also like to check if there are two windows on the web launcher as @mwrock stated

@joaocgreis
Copy link
Author

I did not find an issue with the web launcher, but I probably didn't test every possibility. I found an issue with BoxStarter.bat though, if anyone was using that for a console it would break (Boxstarter Shell shortcut is not affected).

I'll close this for now, I'll reopen if I revisit. The related issue #339 is still very relevant in my opinion.

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