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
Fix editor launch with Windows cmd.exe when KUBE_EDITOR has spaces in path #112104
base: master
Are you sure you want to change the base?
Conversation
Welcome @oldium! |
Hi @oldium. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
@marosset @jsturtevant Can you PTAL / find someone to kick the tires of this change? |
/sig windows |
I can try it out |
Mark said that it worked for him, so LGTM. /lgtm |
LGTM label has been added. Git tree hash: 3e075b7ef78bf57a9ce59944620d001b4291f5fd
|
/triage accepted |
/priority important-soon |
/ok-to-test |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oldium The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ff3ca06
to
0cad8d7
Compare
Fixed CI findings, rebased. /retest |
Sorry - this feel off my radar - i'll take a look |
0cad8d7
to
df71672
Compare
I tried this again in powershell and hit an issue when the path had a space in it.
(the client should be from head of PR branch) I copied
|
it i set
I think we should try and handle setting the ENV VAR not wrapped in single quotes if possible |
This is expected. PowerShell removes the double-quotes, so you actually have KUBE_EDITOR set without double-quotes. The following does the trick in PowerShell: $env:KUBE_EDITOR="`"C:\temp\has space\notepad.exe`""
$env:KUBE_EDITOR='"C:\temp\has space\notepad.exe"' |
Please see #117781 (comment) |
If you would like to interpret |
Single-quotes in your example just ensured that double-quotes are part of Without spaces (groups are functionally equivalent, each 2 lines represent equivalent values): # Value: C:\temp\nospace\notepad++.exe
# cmd.exe: set KUBE_EDITOR=C:\temp\nospace\notepad++.exe
$env:KUBE_EDITOR='C:\temp\nospace\notepad++.exe'
$env:KUBE_EDITOR="C:\temp\nospace\notepad++.exe"
# Value: "C:\temp\nospace\notepad++.exe"
# cmd.exe: set KUBE_EDITOR="C:\temp\nospace\notepad++.exe"
$env:KUBE_EDITOR='"C:\temp\nospace\notepad++.exe"'
$env:KUBE_EDITOR="`"C:\temp\nospace\notepad++.exe`""
# Value: C:\temp\nospace\notepad++.exe -multiInst -notabbar -nosession
# cmd.exe: set KUBE_EDITOR=C:\temp\nospace\notepad++.exe -multiInst -notabbar -nosession
$env:KUBE_EDITOR='C:\temp\nospace\notepad++.exe -multiInst -notabbar -nosession'
$env:KUBE_EDITOR="C:\temp\nospace\notepad++.exe -multiInst -notabbar -nosession"
# Value: "C:\temp\nospace\notepad++.exe" -multiInst -notabbar -nosession
# cmd.exe: set KUBE_EDITOR="C:\temp\nospace\notepad++.exe" -multiInst -notabbar -nosession
$env:KUBE_EDITOR='"C:\temp\nospace\notepad++.exe" -multiInst -notabbar -nosession'
$env:KUBE_EDITOR="`"C:\temp\nospace\notepad++.exe`" -multiInst -notabbar -nosession" With spaces: # Value: "C:\temp\has space\notepad++.exe"
# cmd.exe: set KUBE_EDITOR="C:\temp\has space\notepad++.exe"
$env:KUBE_EDITOR='"C:\temp\has space\notepad++.exe"'
$env:KUBE_EDITOR="`"C:\temp\has space\notepad++.exe`""
# Value: "C:\temp\has space\notepad++.exe" -multiInst -notabbar -nosession'
# cmd.exe: set KUBE_EDITOR="C:\temp\has space\notepad++.exe" -multiInst -notabbar -nosession
$env:KUBE_EDITOR='"C:\temp\has space\notepad++.exe" -multiInst -notabbar -nosession'
$env:KUBE_EDITOR="`"C:\temp\has space\notepad++.exe`" -multiInst -notabbar -nosession" |
Updated previous comment with all variants of setting the KUBE_EDITOR value in |
Thanks for the links to the comments. /lgtm thanks @oldium |
LGTM label has been added. Git tree hash: 236c7c8f62edfb262e3ed31cca2f7207b6cffcd8
|
What type of PR is this?
/kind bug
What this PR does / why we need it:
When on Windows and cmd.exe and we are not able to parse the EDITOR/KUBE_EDITOR environmental variables into arguments, the Go language documentation mentions that when executing cmd.exe, special care needs to be taken regarding arguments - pass whole command line as a string and not as separate arguments with
SysProcAttr.CmdLine
.Moreover, when doing this, the whole string after
cmd /C
needs to be double-quoted.Which issue(s) this PR fixes:
Fixes #72734
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: