Skip to content

[plugin cli] Fix file:/// paths on Windows#10083

Merged
jbudz merged 3 commits intoelastic:masterfrom
jbudz:fix/windows-local-plugin
Feb 16, 2017
Merged

[plugin cli] Fix file:/// paths on Windows#10083
jbudz merged 3 commits intoelastic:masterfrom
jbudz:fix/windows-local-plugin

Conversation

@jbudz
Copy link
Copy Markdown
Contributor

@jbudz jbudz commented Jan 26, 2017

Parsing an absolute file path using file:/// on Windows keeps the leading slash and causes the path to not resolve correctly. This also adds a deprecation warning for paths on Windows with two slashes.

Closes #9301
Closes #8933
Closes #7133

@jbudz
Copy link
Copy Markdown
Contributor Author

jbudz commented Jan 26, 2017

jenkins, test this

@jbudz jbudz added the Team:Operations Kibana-Operations Team label Jan 26, 2017
@epixa
Copy link
Copy Markdown
Contributor

epixa commented Jan 26, 2017

By "break file:// paths", do you mean that using 2 slashes on windows will no longer work?

@jbudz
Copy link
Copy Markdown
Contributor Author

jbudz commented Jan 26, 2017

Yeah. I can make the regex more specific to only remove the leading / if it's followed by a drive letter if we want to keep supporting file://.

@epixa
Copy link
Copy Markdown
Contributor

epixa commented Jan 26, 2017

We probably should preserve the existing functionality for 5.x and remove the support for // in 6.0.

@mayyamus
Copy link
Copy Markdown

@jbudz I think it is not strict, must check string format,like follow :
0. path.length >=3
1.path.charCodeAt(0) is slash('/')
2.path.charCodeAt(1) is windows drive letters
3.path.charCodeAt(2) is ':'
if all passed,then is local file.so should strip the leading slash('/')

@LeeDr
Copy link
Copy Markdown

LeeDr commented Jan 27, 2017

I haven't tested this PR, but just wondering if we can just add a catch that does the path slash manipulation only if the file is not found?

@epixa
Copy link
Copy Markdown
Contributor

epixa commented Jan 27, 2017

@LeeDr When would a file be found on windows using 3 slashes?

@LeeDr
Copy link
Copy Markdown

LeeDr commented Jan 27, 2017

@epixa I was suggesting that if the user gives this incorrect syntax for Windows

file://c:/path/to/x-pack-5.0.0.zip but it currently works, we leave that as-is.

And if the user gives this correct syntax for Windows

file:///c:/path/to/x-pack-5.0.0.zip and it fails to open the file we catch that and replace /// with // and try again.

@jbudz jbudz force-pushed the fix/windows-local-plugin branch from 934ac50 to d798c0f Compare January 27, 2017 16:07
@mayyamus
Copy link
Copy Markdown

mayyamus commented Jan 28, 2017

@epixa @jbudz @LeeDr for incorrect syntax(file://),we should give tips or warnings. a plan not support incorrect syntax should be scheduled.if not,wrong will be right. right is right,wrong is wrong.

@jbudz jbudz removed the v5.3.0 label Feb 9, 2017
@jbudz jbudz force-pushed the fix/windows-local-plugin branch from d798c0f to 051fcb1 Compare February 14, 2017 19:49
@jbudz jbudz added the v5.4.0 label Feb 15, 2017
Copy link
Copy Markdown
Member

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

LGTM - verified I am getting the deprecation warning for file://

@tylersmalley tylersmalley removed their assignment Feb 15, 2017
@kobelb
Copy link
Copy Markdown
Contributor

kobelb commented Feb 16, 2017

LGTM -- tested on Windows

@jbudz jbudz merged commit 9120d17 into elastic:master Feb 16, 2017
elastic-jasper added a commit that referenced this pull request Feb 16, 2017
Backports PR #10083

**Commit 1:**
[plugin cli] Fix file:/// paths on Windows

* Original sha: f47bc1e
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-26T17:02:24Z

**Commit 2:**
[plugin cli] Stricter path checking, keeps support for file://

* Original sha: 6bb8801
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-27T16:04:38Z

**Commit 3:**
[plugin cli] Add deprecation warning for file://

* Original sha: 051fcb1
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-02-14T19:49:19Z
jbudz pushed a commit that referenced this pull request Feb 16, 2017
Backports PR #10083

**Commit 1:**
[plugin cli] Fix file:/// paths on Windows

* Original sha: f47bc1e
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-26T17:02:24Z

**Commit 2:**
[plugin cli] Stricter path checking, keeps support for file://

* Original sha: 6bb8801
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-27T16:04:38Z

**Commit 3:**
[plugin cli] Add deprecation warning for file://

* Original sha: 051fcb1
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-02-14T19:49:19Z
elastic-jasper added a commit that referenced this pull request Feb 23, 2017
Backports PR #10083

**Commit 1:**
[plugin cli] Fix file:/// paths on Windows

* Original sha: f47bc1e
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-26T17:02:24Z

**Commit 2:**
[plugin cli] Stricter path checking, keeps support for file://

* Original sha: 6bb8801
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-27T16:04:38Z

**Commit 3:**
[plugin cli] Add deprecation warning for file://

* Original sha: 051fcb1
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-02-14T19:49:19Z
elastic-jasper added a commit that referenced this pull request Feb 23, 2017
Backports PR #10083

**Commit 1:**
[plugin cli] Fix file:/// paths on Windows

* Original sha: f47bc1e
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-26T17:02:24Z

**Commit 2:**
[plugin cli] Stricter path checking, keeps support for file://

* Original sha: 6bb8801
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-27T16:04:38Z

**Commit 3:**
[plugin cli] Add deprecation warning for file://

* Original sha: 051fcb1
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-02-14T19:49:19Z
jbudz pushed a commit that referenced this pull request Feb 23, 2017
Backports PR #10083

**Commit 1:**
[plugin cli] Fix file:/// paths on Windows

* Original sha: f47bc1e
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-26T17:02:24Z

**Commit 2:**
[plugin cli] Stricter path checking, keeps support for file://

* Original sha: 6bb8801
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-01-27T16:04:38Z

**Commit 3:**
[plugin cli] Add deprecation warning for file://

* Original sha: 051fcb1
* Authored by Jonathan Budzenski <jon@jbudz.me> on 2017-02-14T19:49:19Z
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

6 participants