-
Notifications
You must be signed in to change notification settings - Fork 232
Fix jaeger-client dependency jaeger-thrift no-shadow artifact #486
Fix jaeger-client dependency jaeger-thrift no-shadow artifact #486
Conversation
Can we have a test for this? |
c615eda
to
a106b75
Compare
We do have a test in this module, but it won't catch the runtime issues, as the classpath for the test isn't the same as the classpath for a consumer of this dependency. We could certainly have an integration test consuming this artifact, though. Should we change the |
@jpkrohling @pavolloffay As with other decisions that have been made during the latest refactoring, my preference would be to remove the 'no-shadow' jar until someone actually identifies a usecase that is required. So we just include the shadow jar as the only |
The tricky part is to do that with the final pom, while still having the proper dependencies being pulled (shadow or thrift+okhttp) for the tests. |
I don't see how the test classpath is different, there is only added junit. The test should throw |
a106b75
to
18496ff
Compare
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
============================================
+ Coverage 88.27% 88.32% +0.05%
+ Complexity 499 498 -1
============================================
Files 65 65
Lines 1859 1859
Branches 242 242
============================================
+ Hits 1641 1642 +1
+ Misses 141 139 -2
- Partials 77 78 +1
Continue to review full report at Codecov.
|
You are absolutely correct, @pavolloffay. I messed up the dependencies and I even remember having this exception, but thought it was something else. |
9c96e9a
to
520d103
Compare
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
520d103
to
6e17d5f
Compare
Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de
Which problem is this PR solving?
Basically, when using the
jaeger-client
, there's a runtime exception like:Short description of the changes