Skip to content
This repository has been archived by the owner on Jul 19, 2019. It is now read-only.

CHE-55: Adding io.fabric8 dependencies #23

Merged
merged 1 commit into from
Jan 19, 2017
Merged

Conversation

ibuziuk
Copy link
Member

@ibuziuk ibuziuk commented Dec 19, 2016

The change is coupled with PR - eclipse-che/che#3381
While building "plugin-docker" without "skip-enforce" profile I got the following error:

[WARNING] Used undeclared dependencies found:
[WARNING]    io.fabric8:kubernetes-model:jar:1.0.63:compile
[WARNING]    io.fabric8:kubernetes-client:jar:1.4.26:compile
[INFO] Add the following to your pom to correct the missing dependencies: 
[INFO] 
<dependency>
  <groupId>io.fabric8</groupId>
  <artifactId>kubernetes-model</artifactId>
  <version>1.0.63</version>
</dependency>
<dependency>
  <groupId>io.fabric8</groupId>
  <artifactId>kubernetes-client</artifactId>
  <version>1.4.26</version>
</dependency> 

In order to make it work I had to add those dependencies to pom.xml even though only "openshift-client" is required. Both kubernetes-client & openshift-client are located in the same repository and the CQ allows to proceed with checkin. However, kubernetes-model is located in different one and no CQ has been created for it. Do we need to create one even though it is not used directly ?

mvn dependency:tree

[INFO] +- io.fabric8:openshift-client:jar:1.4.26:compile
[INFO] |  \- io.fabric8:kubernetes-client:jar:1.4.26:compile
[INFO] |     +- io.fabric8:kubernetes-model:jar:1.0.63:compile
[INFO] |     |  \- com.fasterxml.jackson.module:jackson-module-jaxb-annotations:jar:2.7.5:compile
[INFO] |     +- com.squareup.okhttp3:okhttp:jar:3.4.1:compile
[INFO] |     |  \- com.squareup.okio:okio:jar:1.9.0:compile
[INFO] |     +- com.squareup.okhttp3:logging-interceptor:jar:3.4.1:compile
[INFO] |     +- com.squareup.okhttp3:okhttp-ws:jar:3.4.1:compile
[INFO] |     +- org.slf4j:jul-to-slf4j:jar:1.7.6:compile
[INFO] |     +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.5.0:compile
[INFO] |     +- com.fasterxml.jackson.core:jackson-databind:jar:2.5.0:compile
[INFO] |     |  \- com.fasterxml.jackson.core:jackson-annotations:jar:2.5.0:compile
[INFO] |     +- com.fasterxml.jackson.core:jackson-core:jar:2.5.0:compile
[INFO] |     +- io.fabric8:zjsonpatch:jar:0.2.3:compile
[INFO] |     \- com.github.mifmif:generex:jar:1.0.1:compile
[INFO] |        \- dk.brics.automaton:automaton:jar:1.11-8:compile

CQ - https://dev.eclipse.org/ipzilla/show_bug.cgi?id=12209
Jira - https://issues.jboss.org/browse/CHE-55

@skabashnyuk
Copy link
Contributor

  1. Are you sure you've made cq for all three declared decencies?
  2. Are you sure that all transitive dependencies are already approved?

@ibuziuk
Copy link
Member Author

ibuziuk commented Dec 19, 2016

@skabashnyuk For kubernetes-client & openshift-client the CQ is opened and reviewer wrote "Please proceed with checkin". For "kubernetes-model" there is no CQ opened and we do not use this library directly (kubernetes-client just depends on it). However, without specifying it in the pom.xm explicitly the build fails with:

[WARNING] Used undeclared dependencies found:
[WARNING]    io.fabric8:kubernetes-model:jar:1.0.63:compile
[INFO] Add the following to your pom to correct the missing dependencies: 
[INFO] 
<dependency>
  <groupId>io.fabric8</groupId>
  <artifactId>kubernetes-model</artifactId>
  <version>1.0.63</version>
</dependency>

Should we open a CQ for it even though it is not used directly ?

@skabashnyuk
Copy link
Contributor

if it's not needed just exclude it like this https://github.com/eclipse/che-dependencies/blob/master/pom.xml#L195-L204
what about transitive dependencies?

@ibuziuk
Copy link
Member Author

ibuziuk commented Dec 19, 2016

@skabashnyuk Do we really need to open CQ for each transitive dependency? I have done some CQs in the past and there were no requests for transitive dependencies processing. For this change we have only one CQ - https://dev.eclipse.org/ipzilla/show_bug.cgi?id=12209

@skabashnyuk
Copy link
Contributor

yes- if you adding new library and this library added new transitive dependencies.
you have a couple of ways to check it.

  1. Use dependency tree and goes over the all list of transitive dependencies and compare to the list of already approved CQs. Sometimes this method can skip some libraries.
  2. Most productive way is to compare API war before and after adding new dependencies. All new jars have to be approved with CQ

@ibuziuk
Copy link
Member Author

ibuziuk commented Dec 20, 2016

@skabashnyuk could you please take a look at -https://wiki.eclipse.org/Development_Resources/Contribution_Questionnaire#Third_Party_Libraries

As far as I am concerned, we do not need to open any CQs for transitive dependencies as long as they are not used directly. So, the only CQ that should be opened is for kubernetes-model - https://dev.eclipse.org/ipzilla/show_bug.cgi?id=12440

@skabashnyuk
Copy link
Contributor

Can you exclude all transitive dependencies that has not Che CQ?
We do need to create CQ if transitive dependency comes with assembly. Even if it not used.

@ibuziuk
Copy link
Member Author

ibuziuk commented Dec 20, 2016

@skabashnyuk probably we can exclude some of the dependencies
@l0rd the following dependencies can not be excluded for sure, so we need to open CQs for them:

  1. com.squareup.okhttp3:okhttp:jar:3.4.1
  2. com.squareup.okhttp3:logging-interceptor:jar:3.4.1
  3. com.squareup.okio:okio:jar:1.9.0

I have excluded the following dependencies and was able to build "openshift-connector" branch and run the test:

  • com.fasterxml.jackson.module:jackson-module-jaxb-annotations:jar:2.7.5
  • com.squareup.okhttp3:okhttp-ws:jar:3.4.1
  • io.fabric8:zjsonpatch
  • com.github.mifmif:generex

Need to do more testing in order to figure whether those libs are required in the runtime

@l0rd
Copy link

l0rd commented Dec 21, 2016

I've created the following CQs:

Maven artifacts com.squareup.okhttp3:okhttp:jar:3.4.1, com.squareup.okhttp3:logging-interceptor:jar:3.4.1 and com.squareup.okhttp3:okhttp-ws:jar:3.4.1 are all submodules of okttp version 3.4.1 (source code is on the same github repo for the 3 artifacts). Therefore they should not need separates CQs.

@skabashnyuk
Copy link
Contributor

@l0rd ok let us know please when they will be approved

@ibuziuk
Copy link
Member Author

ibuziuk commented Dec 21, 2016

@l0rd we need to open yet another CQ (got a bunch of NoClassDefFound error while testing):

  • io.fabric8:zjsonpatch:jar:0.2.3

@ibuziuk
Copy link
Member Author

ibuziuk commented Dec 21, 2016

@l0rd it looks like the only dependency that can be excluded is com.github.mifmif:generex:jar:1.0.1 and it's transient dk.brics.automaton:automaton:jar:1.11-8. At least I had no issues during testing. PR was updated with those lib exclusion. For all other dependencies except io.fabric8:zjsonpatch:jar:0.2.3 CQs have already been opened

@l0rd
Copy link

l0rd commented Dec 22, 2016

I've opened a new CQ for zjsonpatch: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=12451

Here is the list of current CQs. I will check a CQ as soon as with have an approval:

  • kubernetes-model Version: 1.0.65 CQ
  • jackson-module-jaxb-annotations Version: 2.7.5 CQ
  • okhttp Version: 3.4.1 CQ
  • okio Version: 1.9.0 CQ
  • zjsonpatch Version: 0.3.0 CQ
  • kubernetes-client Version: 1.4.31 CQ

@ibuziuk
Copy link
Member Author

ibuziuk commented Jan 5, 2017

PR has been updated due to zjsonpatch 0.2.3 copyright header issue:

[INFO] +- io.fabric8:kubernetes-client:jar:1.4.31:compile
[INFO] |  +- com.squareup.okhttp3:okhttp:jar:3.4.1:compile
[INFO] |  |  \- com.squareup.okio:okio:jar:1.9.0:compile
[INFO] |  +- com.squareup.okhttp3:logging-interceptor:jar:3.4.1:compile
[INFO] |  +- com.squareup.okhttp3:okhttp-ws:jar:3.4.1:compile
[INFO] |  +- org.slf4j:jul-to-slf4j:jar:1.7.6:compile
[INFO] |  +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.5.0:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-databind:jar:2.5.0:compile
[INFO] |  |  \- com.fasterxml.jackson.core:jackson-annotations:jar:2.5.0:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-core:jar:2.5.0:compile
[INFO] |  +- io.fabric8:zjsonpatch:jar:0.3.0:compile
[INFO] |  \- com.github.mifmif:generex:jar:1.0.1:compile
[INFO] |     \- dk.brics.automaton:automaton:jar:1.11-8:compile
[INFO] +- io.fabric8:kubernetes-model:jar:1.0.65:compile

CQs for "kubernetes-client", "kubernetes-model" & "zjsonpatch" has been updated

@skabashnyuk
Copy link
Contributor

are you going to exclude

[INFO] |  \- com.github.mifmif:generex:jar:1.0.1:compile
[INFO] |     \- dk.brics.automaton:automaton:jar:1.11-8:compile

?

@ibuziuk
Copy link
Member Author

ibuziuk commented Jan 5, 2017

@skabashnyuk yup, it is excluded

@skabashnyuk
Copy link
Contributor

Any news?

@ibuziuk
Copy link
Member Author

ibuziuk commented Jan 17, 2017

@skabashnyuk No news :-( Waiting for zjsonpatch & kubernetes-model CQ clarifications from eclipse IP team

@ibuziuk
Copy link
Member Author

ibuziuk commented Jan 17, 2017

@skabashnyuk @l0rd Good news! CQ for zjsonpatch has been approved and we got an allowance to proceed with kubernetes-model-1.0.65 checkin. So, I guess the PR can be safely merged now:

[INFO] +- io.fabric8:kubernetes-client:jar:1.4.31:compile
[INFO] |  +- com.squareup.okhttp3:okhttp:jar:3.4.1:compile
[INFO] |  |  \- com.squareup.okio:okio:jar:1.9.0:compile
[INFO] |  +- com.squareup.okhttp3:logging-interceptor:jar:3.4.1:compile
[INFO] |  +- com.squareup.okhttp3:okhttp-ws:jar:3.4.1:compile
[INFO] |  +- org.slf4j:jul-to-slf4j:jar:1.7.6:compile
[INFO] |  +- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:jar:2.5.0:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-databind:jar:2.5.0:compile
[INFO] |  |  \- com.fasterxml.jackson.core:jackson-annotations:jar:2.5.0:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-core:jar:2.5.0:compile
[INFO] |  \- io.fabric8:zjsonpatch:jar:0.3.0:compile
[INFO] +- io.fabric8:kubernetes-model:jar:1.0.65:compile
[INFO] |  \- javax.validation:validation-api:jar:1.1.0.Final:compile
[INFO] +- io.fabric8:openshift-client:jar:1.4.31:compile

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

Need to wait tomorrow before merging. (release will occur today)

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
@ibuziuk
Copy link
Member Author

ibuziuk commented Jan 18, 2017

PR has been rebased

@skabashnyuk
Copy link
Contributor

We good to go.

@l0rd l0rd merged commit 6c8dbcf into eclipse-che:master Jan 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants