-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 Fixes issue with Windows, paths can't be escaped #3501
🐛 Fixes issue with Windows, paths can't be escaped #3501
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
Welcome @genehynson! |
Hi @genehynson. Thanks for your PR. I'm waiting for a kubernetes-sigs 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. |
if runtime.GOOS == "windows" { | ||
path = strings.TrimPrefix(path, "/") | ||
path = filepath.FromSlash(path) | ||
} else { | ||
// Windows 10 update 1803 stops decoding file paths | ||
// https://support.microsoft.com/en-us/help/4467268/url-encoded-unc-paths-not-url-decoded-in-windows-10-version-1803-later |
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.
ideally, the note about Windows should be in the Windows branch above, indicating that escaping is not required for older versions and not supported in 1803 and later, linking to the page about the UNC change.
the "in case of windows..." note should be moved in the Windows branch as well.
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.
Moved both comments within if
block
} else { | ||
// Windows 10 update 1803 stops decoding file paths | ||
// https://support.microsoft.com/en-us/help/4467268/url-encoded-unc-paths-not-url-decoded-in-windows-10-version-1803-later | ||
path = url.EscapedPath() |
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.
there could be a comment here on why the path is escaped for other OSes.
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.
Honestly...we could probably remove url.EscapedPath()
altogether. Looking at the blame, it was introduced in this commit for adding Windows support: 737e369
I don't believe escaping file paths is a requirement for other OSes. Thoughts?
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.
it was already present here before that commit:
737e369#diff-88fbf51cc189eba2daf622ffc4a215d0L134
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.
/cc @fabriziopandini
do you recall why EscapedPath() was required when preparing "basepath"?
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.
Ah, yup. Introduced here: b5b9f79#diff-88bc45b90b0324f78a6bea9e5c59aa26R138
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.
TBH I don't recall.
I'm fine in removing it if tests report green signal on both OSes (unfortunately there is no CI signal for windows :-))
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.
Ok, I removed it. Manually tested on Windows and it works.
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.
thanks for the PR
/ok-to-test
given the OS is detected on runtime the IsAbs logic could be extracted to a helper function that accepts the OS as a parameter and this could be unit tested. currently the unit tests don't seem to handle Windows paths. potentially a separate PR, though. |
Authored-by: Gene Hynson <ghynson@vmware.com>
a1c2488
to
ca50ea9
Compare
thanks for the update. /kind cleanup bug |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
/lgtm
Authored-by: Gene Hynson ghynson@vmware.com
What this PR does / why we need it:
url.EscapedPath()
step from Windows local repositories. On Windows 10 versions >= 1803, Windows will no longer decode file paths. This causes a failure when clusterctl tries to read the providers if your Windows username has a space in it (e.g.C:\Users\Gene Hynson\...
):Which issue(s) this PR fixes:
Fixes #3487
Testing
I'm not exactly sure what's the best way to add automated tests for this change, since it would require setting
runtime.GOOS
. If anyone has suggestions, I'm all ears!In terms of manual tests, I tested this on the latest version of Windows and on Mac - it works fine.