You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by GitBox <gi...@apache.org> on 2022/07/22 09:51:58 UTC
[GitHub] [zeppelin] JoWonHyeung opened a new pull request, #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration c…
JoWonHyeung opened a new pull request, #4419:
URL: https://github.com/apache/zeppelin/pull/4419
What is this PR for?
Even though the user's operating system is windows, 'isWindowsPath' function returns False and enters the else.
Therefore, I think that I need to change isWindowsPath function.
What type of PR is it?
Bug Fix
Todos
I replace isWindowsPath Function with isWindowsCheck function. isWindowCheck function uses SystemUtils.IS_OS_WINDOWS
to determine whether the OS is Windows or not. If the user's operating system is windows, It returns true.
What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-5773
How should this be tested?
CI
Questions:
Does the licenses files need to update? No
Is there breaking changes for older versions? Yes
Does this needs documentation? No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927494204
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +633,12 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean isWindowsCheck(){
Review Comment:
I'm not sure but how about using `isWindows` or `checkWindows` as a method name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] yaini commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
yaini commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r928093043
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java:
##########
@@ -69,18 +69,18 @@ public void getAllowedOriginsNoneTest() throws MalformedURLException {
}
@Test
- public void isWindowsPathTestTrue() {
+ public void checkWindowsTestTrue() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("c:\\test\\file.txt");
+ Boolean isIt = conf.checkWindows();
Assert.assertTrue(isIt);
}
@Test
- public void isWindowsPathTestFalse() {
+ public void checkWindowsTestFalse() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("~/test/file.xml");
+ Boolean isIt = conf.checkWindows();
Assert.assertFalse(isIt);
}
Review Comment:
Previously written tests were checked using file paths, but as this function becomes system-dependent, the meaning of the previous tests seems to have changed.
If you write a test, it would be better to mock `checkWindows` and test the result value of a function that uses `checkWindows`, such as `getAbsoluteDir` and `getKeyStorePath` .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] JoWonHyeung commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
JoWonHyeung commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927499221
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +633,12 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean isWindowsCheck(){
+ Boolean wc = SystemUtils.IS_OS_WINDOWS;
+ if(Objects.isNull(wc)) {
Review Comment:
OK, Thank you. I will edit and re-upload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] JoWonHyeung commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
JoWonHyeung commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927675104
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java:
##########
@@ -69,18 +69,18 @@ public void getAllowedOriginsNoneTest() throws MalformedURLException {
}
@Test
- public void isWindowsPathTestTrue() {
+ public void checkWindowsTestTrue() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("c:\\test\\file.txt");
+ Boolean isIt = conf.checkWindows();
Assert.assertTrue(isIt);
}
@Test
- public void isWindowsPathTestFalse() {
+ public void checkWindowsTestFalse() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("~/test/file.xml");
+ Boolean isIt = conf.checkWindows();
Assert.assertFalse(isIt);
}
Review Comment:
@jongyoul
I will question it.
I only changed the function name in the previous version for the time being. The internal code is the same as before.
_SystemUtils.IS_OS_WINDOWS_ will have different results for each operating system, so how can I create test code?
Do you have any good ideas?
Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] yaini commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
yaini commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r928092112
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +633,12 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean isWindowsCheck(){
Review Comment:
nitpick) How about `isWindowsOS`? It would be better to specify what to check.
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +633,12 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean isWindowsCheck(){
Review Comment:
nitpick) How about `isWindowsOS`? It would be better to specify what to check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927575453
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +637,9 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean checkWindows(){
+ Boolean wc = SystemUtils.IS_OS_WINDOWS;
+ return wc;
Review Comment:
How about?
```suggestion
return SystemUtils.IS_OS_WINDOWS;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] yaini commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
yaini commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r928092112
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +633,12 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean isWindowsCheck(){
Review Comment:
nitpick) How about `isOSWindows`? It would be better to specify what to check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] yaini commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
yaini commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r928092112
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +633,12 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean isWindowsCheck(){
Review Comment:
nitpick) How about `isOSWindows` or `isWindowsOS`? It would be better to specify what to check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] JoWonHyeung commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
JoWonHyeung commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927495429
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +633,12 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean isWindowsCheck(){
Review Comment:
OK, I will do what you say.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r928224464
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java:
##########
@@ -69,18 +69,18 @@ public void getAllowedOriginsNoneTest() throws MalformedURLException {
}
@Test
- public void isWindowsPathTestTrue() {
+ public void checkWindowsTestTrue() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("c:\\test\\file.txt");
+ Boolean isIt = conf.checkWindows();
Assert.assertTrue(isIt);
}
@Test
- public void isWindowsPathTestFalse() {
+ public void checkWindowsTestFalse() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("~/test/file.xml");
+ Boolean isIt = conf.checkWindows();
Assert.assertFalse(isIt);
}
Review Comment:
We can define a new CI env like `CI_LINUX=TRUE` in Github action and use it when testing like
```
if(System.getenv("CI_LINUX").equals("TRUE")) {
// Github action
Assert.assertFalse(conf.checkWindows());
} else if (System.getenv("CI_WINDOW").equals("TRUE")) {
// Appveyor
Assert.assertTrue(conf.checkWindows());
} else {
// Local
Assert.assertFalse(conf.checkWindows());
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
Reamer commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927592543
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +637,9 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean checkWindows(){
+ Boolean wc = SystemUtils.IS_OS_WINDOWS;
+ return wc;
Review Comment:
much better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927495792
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -636,8 +633,12 @@ public String getInterpreterPortRange() {
return getString(ConfVars.ZEPPELIN_INTERPRETER_RPC_PORTRANGE);
}
- public boolean isWindowsPath(String path){
- return path.matches("^[A-Za-z]:\\\\.*");
+ public boolean isWindowsCheck(){
+ Boolean wc = SystemUtils.IS_OS_WINDOWS;
+ if(Objects.isNull(wc)) {
Review Comment:
It returns a primitive type. I guess you don't have to check if it's null or not.
https://commons.apache.org/proper/commons-lang/javadocs/api-release/org/apache/commons/lang3/SystemUtils.html#IS_OS_WINDOWS
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927492092
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -25,11 +25,7 @@
import java.net.URISyntaxException;
import java.time.Duration;
import java.time.format.DateTimeParseException;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
Review Comment:
I think you might change your IDE setting not to use `*`. You can change the value to `200` for instance. :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] JoWonHyeung commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
JoWonHyeung commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927675104
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java:
##########
@@ -69,18 +69,18 @@ public void getAllowedOriginsNoneTest() throws MalformedURLException {
}
@Test
- public void isWindowsPathTestTrue() {
+ public void checkWindowsTestTrue() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("c:\\test\\file.txt");
+ Boolean isIt = conf.checkWindows();
Assert.assertTrue(isIt);
}
@Test
- public void isWindowsPathTestFalse() {
+ public void checkWindowsTestFalse() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("~/test/file.xml");
+ Boolean isIt = conf.checkWindows();
Assert.assertFalse(isIt);
}
Review Comment:
I will question it.
I only changed the function name in the previous version for the time being. The internal code is the same as before.
_SystemUtils.IS_OS_WINDOWS_ will have different results for each operating system, so how can I create test code?
Do you have any good ideas?
Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4419: [ZEPPELIN-5773] Fix isWindowsPath Function in ZeppelinConfiguration class
Posted by GitBox <gi...@apache.org>.
jongyoul commented on code in PR #4419:
URL: https://github.com/apache/zeppelin/pull/4419#discussion_r927577081
##########
zeppelin-zengine/src/test/java/org/apache/zeppelin/conf/ZeppelinConfigurationTest.java:
##########
@@ -69,18 +69,18 @@ public void getAllowedOriginsNoneTest() throws MalformedURLException {
}
@Test
- public void isWindowsPathTestTrue() {
+ public void checkWindowsTestTrue() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("c:\\test\\file.txt");
+ Boolean isIt = conf.checkWindows();
Assert.assertTrue(isIt);
}
@Test
- public void isWindowsPathTestFalse() {
+ public void checkWindowsTestFalse() {
ZeppelinConfiguration conf = ZeppelinConfiguration.create("zeppelin-test-site.xml");
- Boolean isIt = conf.isWindowsPath("~/test/file.xml");
+ Boolean isIt = conf.checkWindows();
Assert.assertFalse(isIt);
}
Review Comment:
How do the tests work?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org