Skip to content
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

chore: unify the dependencies versions of the entire project #478

Merged
merged 1 commit into from
Jun 18, 2023

Conversation

conghuhu
Copy link
Contributor

@conghuhu conghuhu commented Jun 9, 2023

close #458

@github-actions github-actions bot added client hugegraph-client hubble hugegraph-hubble loader hugegraph-loader tools hugegraph-tools labels Jun 9, 2023
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #478 (5aab946) into master (f0cbdbc) will not change coverage.
The diff coverage is n/a.

❗ Current head 5aab946 differs from pull request most recent head dc89b26. Consider uploading reports for the commit dc89b26 to get more accurate results

@@            Coverage Diff            @@
##             master     #478   +/-   ##
=========================================
  Coverage     62.48%   62.48%           
  Complexity     1868     1868           
=========================================
  Files           260      260           
  Lines          9432     9432           
  Branches        875      875           
=========================================
  Hits           5894     5894           
  Misses         3154     3154           
  Partials        384      384           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@imbajin
Copy link
Member

imbajin commented Jun 9, 2023

Thanks & need check the CI problem

@conghuhu conghuhu force-pushed the pom branch 3 times, most recently from c2d25b9 to 4cc540d Compare June 9, 2023 08:18
@conghuhu
Copy link
Contributor Author

conghuhu commented Jun 9, 2023

@imbajin PTAL

@imbajin imbajin added the dependencies Pull requests that update a dependency file label Jun 9, 2023
zookeeper-3.6.2.jar
zookeeper-jute-3.5.6.jar
zookeeper-jute-3.6.2.jar
Copy link
Member

Choose a reason for hiding this comment

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

These dependencies seems to have been duplicated, can we remove one version of them?

@conghuhu conghuhu force-pushed the pom branch 2 times, most recently from caa5b79 to 53270a0 Compare June 9, 2023 14:37
@imbajin
Copy link
Member

imbajin commented Jun 11, 2023

check it:
image

pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
Comment on lines -49 to -59
<dependencyManagement>
<dependencies>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>${junit.version}</version>
<scope>test</scope>
</dependency>
</dependencies>
</dependencyManagement>

Copy link
Member

Choose a reason for hiding this comment

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

nice catch

hugegraph-loader/pom.xml Show resolved Hide resolved
Comment on lines +57 to +63
<dependency>
<groupId>com.fasterxml.jackson</groupId>
<artifactId>jackson-bom</artifactId>
<version>${jackson.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

why we need add jackson here? (seems client has included it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is introduced to unify the dependencies related to jackson denpendencies of all submodules

Copy link
Member

Choose a reason for hiding this comment

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

This is introduced to unify the dependencies related to jackson denpendencies of all submodules

OK, just ensure the need

<guava.version>30.0-jre</guava.version>
<lz4.version>1.4.0</lz4.version>
<jcommand-version>1.72</jcommand-version>
<jackson.version>2.12.3</jackson.version>
Copy link
Member

@imbajin imbajin Jun 12, 2023

Choose a reason for hiding this comment

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

and be careful, the jackson version may influence or have conflicts in some cases (like in spark/flink env, if we change the default version in loader, it's better to test it later, add a WARN comment for it ⚠️ )

imbajin
imbajin previously approved these changes Jun 13, 2023
Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

LGTM, and it's good to keep this PR clean

@conghuhu
Copy link
Contributor Author

image So I update guava to 32.0.1-jre

Comment on lines 258 to 275
netty-3.10.6.Final.jar
netty-all-4.1.68.Final.jar
netty-buffer-4.1.39.Final.jar
netty-buffer-4.1.50.Final.jar
netty-codec-4.1.39.Final.jar
netty-codec-4.1.50.Final.jar
netty-common-4.1.39.Final.jar
nimbus-jose-jwt-4.41.1.jar
netty-common-4.1.50.Final.jar
netty-handler-4.1.39.Final.jar
netty-handler-4.1.50.Final.jar
netty-resolver-4.1.39.Final.jar
netty-resolver-4.1.50.Final.jar
netty-transport-4.1.39.Final.jar
netty-transport-4.1.50.Final.jar
netty-transport-native-epoll-4.1.39.Final.jar
netty-transport-native-epoll-4.1.50.Final.jar
netty-transport-native-unix-common-4.1.39.Final.jar
netty-transport-native-unix-common-4.1.50.Final.jar
Copy link
Member

@imbajin imbajin Jun 15, 2023

Choose a reason for hiding this comment

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

why a string of (& duplicate) netty jars appear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of them form zookeeper in hugegraph-tools.
image

Copy link
Member

Choose a reason for hiding this comment

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

seems a little strange, each netty dependency have 2 version now? ZK will not include 2 version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have identified this issue and implemented version consistency by introducing netty-bom

Copy link
Member

Choose a reason for hiding this comment

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

I have identified this issue and implemented version consistency by introducing netty-bom

fine & no need to force-push commit so that we could review the latest change easily

the multi commit will be rebased together

fix: unify zk version

chore: update guava to 32.0.0-jre
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client hugegraph-client dependencies Pull requests that update a dependency file hubble hugegraph-hubble loader hugegraph-loader tools hugegraph-tools
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Enhancement] unify the dependencies versions of the entire project
4 participants