-
Notifications
You must be signed in to change notification settings - Fork 17
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
b153-Switch JAX-RS API dependencies to jakarta.ws.rs-api #500
b153-Switch JAX-RS API dependencies to jakarta.ws.rs-api #500
Conversation
e2c18ca
to
48ce39f
Compare
@jhemm2 thank you for contributing! Quite a nice round PR number for such an important contribution. There is no need to reopen a PR. I just enabled the build so that you can get the build to run every time you push your changes and check how close are we to be ready to merge the PR. I closed #501 and please proceed with this PR. Don't worry about the build failing, it's only to help us to not merge the PR into master until we know that it will build. |
@Jad-el-khoury the ECA check has passed: https://api.eclipse.org/git/eca/status/gh/eclipse/lyo/500 |
Brilliant! |
But the build is failing because there is a problem, no? @berezovskyi ! |
I have fixed this issue with my last commit, let's see what the next build says |
The last build has passed. |
Great. I tried my best to update all the affected dependencies and imports but as this change is quite large, I would appreciate someone to do some more verification and review on it. |
@jhemm2 You're the man 👍 |
Given that we are on the 6.0.0.SNAPSHOT, I believe we should be able to push the changes straight to main - once we are satisfied. You already have a branch for people to download and test with. I can try to test on some servers on my side as well. If we both manage to make it work, we can start the review process and merge the change to master. @berezovskyi ! are we missing anything in the process? |
@Jad-el-khoury yep |
Let's make a 6.0.0.alpha1 release before merging to provide projects already tracking 6.0 SNAPSHOT a bit of a fallback before the Jakarta switch? Releasing Jakarta change in 6.0 sounds good. |
We shall also update refimpl to jakarta, which requires Lyo Designer update. Usually we run simple integration tests on refimpl to chexk bigger lyo changes. |
@jhemm2 ! I would like to start with migrating the Lyo Reference Implementation as a way to validate/review this large change. Assume you have done a similar implementation, it would greatly simplify my job If you can share any hints or outline the steps needed to migrate? I planed to analyze your change here, but anything that makes this job go faster would be great. |
@Jad-el-khoury I am using only the oslc-client in my project. We are using the client to read write RM data via OSLC. |
I have just updated my project with the following dependencies and successfully ran my integration tests. The integration tests authenticate to IBM DOORS Classic as well as IBM DNG and reads/ writes modules and requirements. lyo.version=6.0.0-SNAPSHOT So from my side, at least the client part looks good. |
I have tested with Spring Boot 3.2.1 and Jersey 3.1.5. |
Thanks for the info @jhemm2 ! This will be helpful. @danlz! Really cool to hear that it works with Spring Boot. Do you think you can contribute with a example server that uses Spring Boot? Our reference Implementation do not rely on Spring Boot. It would be useful for the community to show that Lyo works with Spring (I do get that question often) |
@Jad-el-khoury It is not a rocket science ;) |
I have migrated the remaining Lyo projects to Jakarta. I have only tested very quickly on 1 sample server as per this PR. One issue remains. I wonder if anyone has any tips on how to deal with the very old oauth projects. I could manually run the following to migrate the jar files. But how can we introduce into our maven build? This link might be useful but I could not get it to work yet |
The oauth problem is the reason the build fails, so no surprises. |
And @berezovskyi I was expecting problems with Jena. I did not test properly yet but there were no complains. I noticed some special repos for Jena in the pom files. |
As @berezovskyi said, JSTL is no longer shipped with servers. Maybe you have to use a newer version of the jetty-maven-plugin? We have added JSTL to Tomcat. |
Thank you for your input. I have a proposal which seems to work. Please see the last 2 commits (combined) from this PR. Do you think the below "default" is acceptable, or should it be the other way around? By default, we only include the jstl-api with "provided" scope.
To include the jstl implementation (and jstl-api without provided scope) - run with "with-jstl-impl" profile.
For what it's worth, here are my tests before I came up with this solution. I believe it confirms your thoughts. Interesting though that Jetty12x behaves differently under this Cargo plugin. If I only include jstl-api
If I invlude both jstl-api and jstl
If I include both jstl-api and jstl (with scope "provideed")
|
If you think these last changes are acceptable, I believe this PR is ready for a final review (Special attention to all changes in the pom.xml files). |
...provider/src/test/java/org/eclipse/lyo/oslc4j/provider/json4j/test/TestInvalidTypesJson.java
Outdated
Show resolved
Hide resolved
@Jad-el-khoury in re jstl deps: My view is that we shall include -api (provided is fine for server projects but client in a standalone env will break, but it will need extra impl deps anyway in the downstream project). And then for tomcat we can include it in a profile (in LynxD), like in https://github.com/eclipse/lyo/blob/master/pom.xml#L92 (instead of specifying |
Isn't that the same as what I have already implemented? |
Yep, didn't notice. Left a question in the referenced PR. |
FYI, I pushed this PR into |
https://github.com/oslc-op/refimpl/actions/runs/8498710257 tests oslc-op/refimpl#198
https://github.com/OSLC/lyo-samples/actions/runs/8498707134 tests lyo-samples master (specifically the client project)
|
The refimpl tests are still failing with a 503 error: https://github.com/oslc-op/refimpl/actions/runs/8574115331/job/23500242159 |
Only RM was updated, so no suprises there. But if we are confident that we are almost done (and you already reviewed LyoDesigner changes). I can then re-generate all Ref-impl servers and test again. |
Now I understand. Yes, let's update the other servers because otherwise this PR is ready to merge. If we keep those tests broken, they will just break CI pipeline and will make it harder to use tests in general.
|
Thanks Jad, tests now pass: https://github.com/oslc-op/refimpl/actions/runs/8575876036/job/23505766712 |
I tried to re-run but it failed. Is it the change in the DOckerfiles that was missing? |
Yes, I think so. The values you parametrised the job with seemed OK. |
but this is the Lyo PR :) yes, we should start with it. LGTM |
Yes, I noticed that I am getting the 2 PRs confused. We are done here. |
@Jad-el-khoury Example of Lyo and Spring Boot integration. The example uses Jakarta version of Lyo. |
In the long run you should think of a Spring Boot starter for Lyo. |
Description
Updated dependencies and imports to switch to jakarta EE9 (javax vs jakarta)
remaining issues before we are ready for review
jakarta.servlet.jsp.jstl-api-3.0.0
I have removed a number of other warnings through changes in the dependencies. These need to be reviewed to make sure we have the right approach.Checklist
Issues
Closes #153 ; Closes #00
(#153)