-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Port to Jersey/JAX-RS 2.0 #40
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
Conversation
|
@dflemstr Great work! Big thanks for your effort! However i don't think i should merge it as is because some docker-java client code may be tied to jersey 1.x when using third party REST APIs with jersey 1.x also. This is because there seems to be problems having jersey 1.x and jersey 2.x in the same classpath (http://jersey.576304.n2.nabble.com/Repackaging-Jersey-1-x-jars-without-jax-rs-td7580810.html). So i don't want to simply override the old jersey 1.x implementation. Rather it would be better to factor out an REST framework independent docker-java-api and provide two different implementations of it. There is a little discussion about it here: https://groups.google.com/forum/?hl=de#!topic/docker-java-dev/p_m9VIRrCeU So as soon as i factored out this docker-java-api i will merge your branch. Regards |
|
It might be worth noting that that problem is already solved in some ways. Instead of using Jersey directly, the commands in this PR use only the normal JAX-RS client API. That means that any implementation of the JAX-RS API (e.g. RestEasy, CXF, Restlet) can be dropped in instead of Jersey, and the only thing that needs to be changed is how the So if you are looking for a "REST-framework independent API" then that's exactly what JAX-RS is already. But you are correct in that it wouldn't allow you to use Jersey 1, since that library uses a proprietary Sun API. However, if I could choose, I myself would prefer to drop support for Jersey 1 and gain support for any JAX-RS 2 implementation, instead of having to create something similar to JAX-RS 2 and retain support for Jersey 1. |
|
Yes, that would be the much simpler solution but currently i don't want drop Jersey 1.x support if possible. But maybe that needs to much effort. Regardless of it i think it will be a better design to have a docker-java API that not relies on any JAX-RS (1/2) class. However, as i said already, big thanks for this PR! |
|
@dflemstr Nicely done; I like it. (Just because all of the integration tests pass doesn't necessarily mean it's 100% correct: There are definitely code coverage gaps. Nevertheless...) This is great because it breaks from the com.sun.* classes in favor of the more standard javax.* classes. @marcuslinke I'd say this is a good step in the right direction, even if we later remove the exposure of the javax.* classes in the method/command return types. If someone really wants Jersey 1.x support, they could always use 0.9.1. |
|
@marcuslinke I look forward to a more general solution for this. Meanwhile, we want to use this library at Spotify so I will maintain an internal fork. I suspect that the API won't change with your proposed solution? |
|
@dflemstr The current 0.10.0-SNAPSHOT is intended to make some major refactorings, therefore i switched from 0.9.1 to 0.10.0. It's still in progress, so unfortunately the API will change for sure, but it will include package renaming and of course the client initialization step only. So reorganizing imports and change your client setup code should be sufficient. |
|
Maybe i'm able to commit the refactored API until tomorrow, so that you could pull and integrate in your branch already. |
|
@dflemstr , @mfulgo: After thinking about it again keeping jersey 1.x support will be to much effort, so i decided to drop support for it. The most problems will be with the test cases. I really don't want to introduce abstract test cases. Nevertheless i will merge the API changes i've already done with your branch @dflemstr. |
|
@dflemstr Just merged your changes manually. |
I want to use this library in an environment where there are other libraries that depend on Jersey 2. Since Jersey 2 has better performance and a nicer API compared to Jersey 1, I thought I would just port this library to Jersey 2.
All the tests pass so I assume that this port is complete.