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 2021/09/30 09:40:21 UTC
[GitHub] [zeppelin] huage1994 opened a new pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
huage1994 opened a new pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239
### What is this PR for?
Support multiple maven repo in config item zeppelin.interpreter.dep.mvnRepo
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-5545
### How should this be tested?
CI pass
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* 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] zjffdu commented on pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#issuecomment-942401056
LGTM, thanks @huage1994 will merge if no more comment
--
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] zjffdu commented on a change in pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
zjffdu commented on a change in pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#discussion_r719337695
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/dep/Booter.java
##########
@@ -78,20 +82,32 @@ static String resolveLocalRepoPath(String localRepoPath) {
return Paths.get(home).resolve(localRepoPath).toAbsolutePath().toString();
}
- public static RemoteRepository newCentralRepository(Proxy proxy) {
- String mvnRepo = System.getenv("ZEPPELIN_INTERPRETER_DEP_MVNREPO");
- if (mvnRepo == null) {
- mvnRepo = ZeppelinConfiguration.create().getString(
+ public static List<RemoteRepository> newCentralRepository(Proxy proxy) {
Review comment:
rename it to `newCentralRepositories`
--
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] huage1994 commented on a change in pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
huage1994 commented on a change in pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#discussion_r720660927
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/dep/Booter.java
##########
@@ -78,20 +82,32 @@ static String resolveLocalRepoPath(String localRepoPath) {
return Paths.get(home).resolve(localRepoPath).toAbsolutePath().toString();
}
- public static RemoteRepository newCentralRepository(Proxy proxy) {
- String mvnRepo = System.getenv("ZEPPELIN_INTERPRETER_DEP_MVNREPO");
- if (mvnRepo == null) {
- mvnRepo = ZeppelinConfiguration.create().getString(
+ public static List<RemoteRepository> newCentralRepository(Proxy proxy) {
Review comment:
> I think we could add a test to check proper input/output. Could you please add it?
thanks for review @jongyoul, I would add test for it.
--
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] huage1994 commented on pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
huage1994 commented on pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#issuecomment-932767901
> Thanks for the contribution, @huage1994 left one comment, and document needs to update as well https://github.com/apache/zeppelin/blob/master/docs/setup/operation/configuration.md#zeppelin_interpreter_dep_mvnrepo
ok. finished
--
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 change in pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
jongyoul commented on a change in pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#discussion_r719878328
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/dep/Booter.java
##########
@@ -78,20 +82,32 @@ static String resolveLocalRepoPath(String localRepoPath) {
return Paths.get(home).resolve(localRepoPath).toAbsolutePath().toString();
}
- public static RemoteRepository newCentralRepository(Proxy proxy) {
- String mvnRepo = System.getenv("ZEPPELIN_INTERPRETER_DEP_MVNREPO");
- if (mvnRepo == null) {
- mvnRepo = ZeppelinConfiguration.create().getString(
+ public static List<RemoteRepository> newCentralRepository(Proxy proxy) {
Review comment:
I think we could add a test to check proper input/output. Could you please add it?
--
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] zjffdu commented on pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#issuecomment-931260562
Thanks for the contribution, @huage1994 left one comment, and document needs to update as well https://github.com/apache/zeppelin/blob/master/docs/setup/operation/configuration.md#zeppelin_interpreter_dep_mvnrepo
--
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] asfgit closed pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239
--
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] huage1994 commented on a change in pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
huage1994 commented on a change in pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#discussion_r720660525
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/dep/Booter.java
##########
@@ -78,20 +82,32 @@ static String resolveLocalRepoPath(String localRepoPath) {
return Paths.get(home).resolve(localRepoPath).toAbsolutePath().toString();
}
- public static RemoteRepository newCentralRepository(Proxy proxy) {
- String mvnRepo = System.getenv("ZEPPELIN_INTERPRETER_DEP_MVNREPO");
- if (mvnRepo == null) {
- mvnRepo = ZeppelinConfiguration.create().getString(
+ public static List<RemoteRepository> newCentralRepository(Proxy proxy) {
Review comment:
> rename it to `newCentralRepositories`
thanks for review @zjffdu , I've renamed it.
--
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] zjffdu commented on pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
zjffdu commented on pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#issuecomment-932768992
Thanks @huage1994 Let's wait for CI
--
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] huage1994 commented on a change in pull request #4239: [ZEPPELIN-5545] Support multiple maven repo in zeppelin.interpreter.d…
Posted by GitBox <gi...@apache.org>.
huage1994 commented on a change in pull request #4239:
URL: https://github.com/apache/zeppelin/pull/4239#discussion_r720689871
##########
File path: zeppelin-interpreter/src/main/java/org/apache/zeppelin/dep/Booter.java
##########
@@ -78,20 +82,32 @@ static String resolveLocalRepoPath(String localRepoPath) {
return Paths.get(home).resolve(localRepoPath).toAbsolutePath().toString();
}
- public static RemoteRepository newCentralRepository(Proxy proxy) {
- String mvnRepo = System.getenv("ZEPPELIN_INTERPRETER_DEP_MVNREPO");
- if (mvnRepo == null) {
- mvnRepo = ZeppelinConfiguration.create().getString(
+ public static List<RemoteRepository> newCentralRepository(Proxy proxy) {
Review comment:
> > I think we could add a test to check proper input/output. Could you please add it?
>
> thanks for review @jongyoul, I would add test for it.
done
--
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