[ZEPPELIN-1695] Centralize libs, plugin versions in all pom.xml #1668
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1ambda
force-pushed
the
chore/centralize-maven-pom-versions
branch
3 times, most recently
from
a7aed47 to
a5c42ea
Compare
1ambda
changed the title
1ambda
changed the title
bzz
reviewed
Nov 23, 2016
| <dependency> | ||
| <groupId>org.apache.httpcomponents</groupId> | ||
| <artifactId>httpclient</artifactId> | ||
| <version>4.3.6</version> |
Member
There was a problem hiding this comment.
Is it safe to remove versions here, without extracting it to the properties, as in other sub-projects?
Member
Author
There was a problem hiding this comment.
@bzz It's safe since
- we have
dependencyManagementinroot/pom.xmland all dependencies described independencyManagementblock will be inherited and shared by children pom.xml
(ref - https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Management)
<properties>
<httpcomponents.client.version>4.3.6</httpcomponents.client.version>
<dependencyManagement>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<version>${httpcomponents.client.version}</version>
</dependency>
- You can check using effective pom
$ mvn org.apache.maven.plugins:maven-help-plugin:2.2:effective-pom -pl kylin
// you will see
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<version>4.3.6</version>
</dependency>
Member
There was a problem hiding this comment.
great reminder on how dependency mechanism works in Maven, it would be event enough just to point it's existence in parent pom :)
Member
|
Thank you - it's a great effort, a bit long to review so it took a while and have diverged from master. @1ambda could you rebase and also make sure it does not affect automation like https://github.com/apache/zeppelin/blob/master/dev/change_scala_version.sh ? |
1ambda
force-pushed
the
chore/centralize-maven-pom-versions
branch
2 times, most recently
from
40a1709 to
b724218
Compare
Member
Author
Comment Summary@bzz Thanks for review! I didn't know about There are many ways to compare, but i chose a simple and reproduceable way so that others can do again
As you can see, Long Details (sorry, but it's necessary)below are content of |
Member
|
Thank you for double-checking and explanations. Looks like CI is failing right now though |
Member
Author
|
Failed due to not the related test. I will amend the last commit and trigger CI again |
1ambda
force-pushed
the
chore/centralize-maven-pom-versions
branch
from
b724218 to
bc40176
Compare
Member
|
Looks great to me and CI is green now. Merging to master, if there is no further discussion. |
Member
|
@1ambda it looks like rebase is needed after few other |
1ambda
force-pushed
the
chore/centralize-maven-pom-versions
branch
3 times, most recently
from
1f0ba6d to
1527b84
Compare
Member
Author
|
CI failed |
1ambda
force-pushed
the
chore/centralize-maven-pom-versions
branch
3 times, most recently
from
033f0e2 to
2fb4e1c
Compare
Member
Author
|
rebased to keep sync with master (+ to see CI is green as well) |
Member
Author
Member
|
Sounds reasonable. CI is green, merging to master then. |
Member
Author
|
I will resolve conflict with 4ac577f |
Member
|
thanks a lot, @1ambda ! |
1ambda
force-pushed
the
chore/centralize-maven-pom-versions
branch
from
2fb4e1c to
1a5419a
Compare
Member
|
Thank you @1ambda ! Merging to master, if there is no further discussion. |
we already specified in the parent pom.xml - 1.5 commons-codec - 4.3.6 httpclient so it's safe to remove their version fields
1ambda
force-pushed
the
chore/centralize-maven-pom-versions
branch
from
ffc1982 to
5a9c966
Compare
Member
|
Waiting for CI to finish on https://travis-ci.org/1ambda/zeppelin/ Will merge to master right after that, if there is no further discussion. |
Member
|
2 Scala 2.11 profiles failed on IgniteSQL tests - a flaky test tracked under ZEPPELIN-1738 which is not relevant to the change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this PR for?
First of all, this PR doesn't affect on runtime application behaivor and existing build processes. Just abstracting variables in pom.xml
The main goal of this PR is bringing consistency and maintainability in all pom.xml. I referred these 2 projects.
Currently, all libraries and plugins have duplicated version fields since we are not using
dependencyManagement,pluginManagementefficiently in pom.xmlThis results in
These are some examples
// hard coded, non consistent version management <dependency> <groupId>org.apache.thrift</groupId> <artifactId>libthrift</artifactId> <version>${libthrift.version}</version> </dependency> <dependency> <groupId>org.apache.httpcomponents</groupId> <artifactId>httpcore</artifactId> <version>4.3.3</version> </dependency>// a/pom.xml <plugin> <artifactId>maven-enforcer-plugin</artifactId> <version>1.3.1</version> <plugin> <artifactId>maven-dependency-plugin</artifactId> <version>2.8</version> // b/pom.xml <plugin> <artifactId>maven-enforcer-plugin</artifactId> <version>1.3.1</version> <plugin> <artifactId>maven-dependency-plugin</artifactId> <version>2.8</version> // c/pom.xml <plugin> <artifactId>maven-enforcer-plugin</artifactId> <version>1.3.1</version> <plugin> <artifactId>maven-dependency-plugin</artifactId> <version>2.8</version>What type of PR is it?
[Refactoring]
Todos
What is the Jira issue?
ZEPPELIN-1695
How should this be tested?
Since this PR doesn't affect runtime behavior at all, CI test would be enough.
Questions: