-
Notifications
You must be signed in to change notification settings - Fork 522
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
Add Windows support for shell attach #58
Conversation
commands/open-shell-container.ts
Outdated
|
||
const engineTypeShellCommands = { | ||
[DockerEngineType.Linux]: "/bin/sh", | ||
[DockerEngineType.Windows]: "cmd" |
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.
Should this default to Powershell instead? I'd love to hear feedback here.
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.
I keep hearing rumor (I have no inside knowledge one way or another..even though I couldn't share it anyway) that Windows is making the switch themselves, but I question whether that would be accepted in general. I think as long as this was an option for the extension, though, you could default to whatever you want.
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.
here is the blog post: https://blogs.windows.com/windowsexperience/2016/11/17/announcing-windows-10-insider-preview-build-14971-for-pc/#hjIQ7kQcdSPrzb1z.97
Given this, i suggest we default to PowerShell instead of cmd.
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.
@chrisdias Sounds good to me! I'll make this change.
@lostintangent all changes look good, but lets default to PS instead of CMD (see comment above). |
@chrisdias understandable (glad you found the link; I remember hearing something along those lines), but still seems it should be a user option, because:
I am partial to PowerShell myself, but just thinking about what others may want. The default shell for Windows containers is cmd, so users may still want to default that. |
Merging this in and will address the shell configuration in a separate PR. Thanks for the review! |
This change updates the
Docker: Attach Shell
command in order to properly support Windows containers. Previously, this command assumed that it could run/bin/sh
in the selected image, whereas this change will detect a Windows container, and runcmd
instead.Beyond this change, we should allow configuring the default shell use, so that Windows users could toggle PowerShell and Linux users could use Bash/etc.