-
Notifications
You must be signed in to change notification settings - Fork 2k
Conversation
Why not switch to a |
That works. Thx |
@nathanleclaire updated PTAL |
f89763f
to
f2593ad
Compare
Cool. I was thinking something more along the lines of this for the template (to help with readability):
(then the newlines would just be taken out of the suffix) |
On second thought, I think we can cut most of the verbosity in the switches down by having a template which does stuff like so:
Does it sound good to you? It's a little more verbose in the actual template itself but I think it will be a lot more readable / easier to understand to anyone looking at the code. |
Fixing test. The problem is the windows shells are very, very picky about whitespace. For example, if there is a line starting with a blank in powershell it dies. Edit: as well as newlines. |
@ehazlett I see. That shouldn't be a problem if we're spitting out only the powershell-specific template when we are in powershell, wouldn't it? The example in that last comment would only output the relevant bits based on the shell. (the |
|
||
func detectShell() (string, error) { | ||
// attempt to get the SHELL env var | ||
shell := filepath.Base(os.Getenv("SHELL")) |
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.
Is this wrapped in filepath.Base
so that we get the value we expect in case SHELL contains something like /usr/bin/zsh
? Just making sure I understand correctly.
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.
Correct -- this is the current behavior today.
Don't forget zsh homies! 😎 |
@blacktop i believe zsh behaves the same as bash -- that's what we do today :) |
Tests updated. @nathanleclaire PTAL |
I think this would be better using some split |
@ehazlett, you right, you right. 😉 I could have sworn that I used to be able to set the env like with I see now that I am supposed to type: |
@nathanleclaire that sound good to you? |
@ehazlett Yes, definitely- can you please add some docs? At least a small paragraph and example explaining that |
Also, should we maybe not be hardcoding the |
@nathanleclaire updated to remove the hardcoded binary. |
@ehazlett Nice, thanks - I'm getting some issues when using the command hint on Powershell. WDYT? EDIT: Ah, the hint is slightly off. We should be including |
Yeah, it works flawlessly if you do: $ docker-machine env --shell powershell dev | Invoke-Expression |
Other than that, testing on Powershell and cmd.exe looks good on my end. |
Tests fixed. Usage hint updated for powershell. ping @nathanleclaire |
LGTM let's do this 👍 Created separate issue to track documenting this : #1054 |
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
7779187
to
9aa861e
Compare
/cc @ahmetalpbalkan would you mind taking a look to make sure this seems sane? |
shell := filepath.Base(os.Getenv("SHELL")) | ||
// none detected; check for windows env | ||
if shell == "." && os.Getenv("windir") != "" { | ||
log.Printf("On Windows, please specify either cmd or powershell with the --shell flag.\n\n") |
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.
might be nice to put cmd, powershell strings in single quotes for readability.
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.
good point -- thx!
@ehazlett if they're working fine, LGTM It's sort of hard to trace from the code by eye. So maybe if can you please paste the outputs/screenshots from cmd/powershell (without piping to invoke-expression) I can double check it for you. |
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
Could the docs be updated to show how this can be used (as opposed to msysgit)? |
@nathanleclaire Great, thanks for updating the docs. One small thing for the cmd.exe is that the Here is a small proposal how it could look like for cmd.exe with $ docker-machine.exe env --shell cmd dev
set DOCKER_TLS_VERIFY=1
set DOCKER_HOST=tcp://192.168.99.101:2376
set DOCKER_CERT_PATH=C:\Users\captain\.docker\machine\machines\dev
set DOCKER_MACHINE_NAME=dev
rem Run this command to configure your shell:
rem for /f "tokens=*" %i in ('docker-machine.exe env --shell=cmd dev') do %i And in the powershell sample there is missing the is missing the machine name # Run this command to configure your shell:
# docker-machine.exe env --shell=powershell dev | Invoke-Expression |
@StefanScherer cmd.exe output of env cmd was designed for copy-paste. Your proposal looks interesting. Care to put that in a proposal/pull request? |
@ahmetalpbalkan Have a look at #1493 ;-) |
@StefanScherer Re: missing machine name in the powershell output; that is odd, because I'm using 0.3.0 and it works correctly for me:
Note how my machine name, |
@masaeedu I have seen that the sourcecode is correct. It is only missing in the docs. |
This adds PowerShell support and shell selection support (to force selection for
bash
,fish
, etc) for theenv
command.Refs #906