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

JavaEE to Quarkus 2 Declarative Migration Recipe #73

Merged
merged 23 commits into from
Jul 12, 2024

Conversation

ShatanikB
Copy link
Contributor

@ShatanikB ShatanikB commented Feb 4, 2024

What's changed?

Added a declarative recipe to migrate Java EE to Quarkus 2. Started with adding Quarkus BOM, Quarkus extension dependencies, removed JEE dependencies, converted some EJB annotations to CDI (Stateful, Stateless and Singleton), changed JPA annotations to work with Quarkus

What's your motivation?

Contributing to #72

Anything in particular you'd like reviewers to focus on?

General feedback on Best practices I might be missing

Anyone you would like to review specifically?

@timtebeek

Any additional context

Trying to keep it declarative. Will add imperative recipes if required.

Checklist

  • [ ] I've added unit tests to cover both positive and negative cases
    No because composite recipe. Have tested on real applications
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@timtebeek
Copy link
Contributor

Great start @ShatanikB ! Good to hear these recipes already work for you on real applications.

My suggestion for maintainability going forward is that we move your declarative recipes into a separate javaee-to-quarkus.yml file, such that it's clear from the filename what the scope is. Perhaps it would also be good to split the sinlgle large declarative recipe into two: one for the Maven & dependency changes, and another for the Java code changes. That makes it easier to reuse a part of those changes elsewhere. Your JavaEEtoQuarkus2Migration recipe can then call both of those recipes.

I've also added a test just now because that triggers some of the validations we do through our test framework. I'll work on getting the unit tests to pass on main, as those seem to have broken due to a change upstream.

github-actions[bot]

This comment was marked as outdated.

…2CodeTranformationsTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek marked this pull request as ready for review February 6, 2024 10:39
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Thanks both! From what I'm seeing this is ready for review and merge.

@gsmet
Copy link
Collaborator

gsmet commented Feb 6, 2024

Just a note that I will have a look this week.

@ShatanikB
Copy link
Contributor Author

Sorry for the newbie mistakes. For some reason, it shows that @ammachado is committing changes instead of me.

@timtebeek
Copy link
Contributor

Sorry for the newbie mistakes. For some reason, it shows that @ammachado is committing changes instead of me.

Those commits are quite confusing indeed; could you check both your user home ~/.gitconfig and repository .git/config to see if there's any [user] blocks that contain the wrong configuration values?

…s Maven only right now. Added a TODO for Gradle
@ShatanikB
Copy link
Contributor Author

Those commits are quite confusing indeed; could you check both your user home ~/.gitconfig and repository .git/config to see if there's any [user] blocks that contain the wrong configuration values?

Thanks again. My user was not configured at all on codespaces. Fixed it.

@timtebeek timtebeek requested a review from gsmet February 12, 2024 18:26
Copy link
Collaborator

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, time is scarce on my side these days.

I added some comments but it looks good overall.

src/main/resources/META-INF/rewrite/javaee-to-quarkus.yml Outdated Show resolved Hide resolved
src/main/resources/META-INF/rewrite/javaee-to-quarkus.yml Outdated Show resolved Hide resolved
Comment on lines 51 to 53
- org.openrewrite.java.dependencies.AddDependency:
groupId: io.quarkus
artifactId: quarkus-undertow
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I would add this one except if the application is explicitly using servlet.

Especially since having undertow around changes the behavior of Quarkus.

Now this can probably be done as a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added undertow to be able to use the 'SessionScoped' annotation as per this . Making the assumption that @javax.ejb.Stateful should be mapped to @javax.enterprise.context.SessionScoped in CDI

Comment on lines 54 to 59
- org.openrewrite.java.dependencies.AddDependency:
groupId: io.quarkus
artifactId: quarkus-hibernate-orm
- org.openrewrite.java.dependencies.AddDependency:
groupId: io.quarkus
artifactId: quarkus-jdbc-h2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, I don't think I would add these two except if the application makes use of persistence.

Now this can probably be done as a follow-up.

src/main/resources/META-INF/rewrite/javaee-to-quarkus.yml Outdated Show resolved Hide resolved
src/main/resources/META-INF/rewrite/javaee-to-quarkus.yml Outdated Show resolved Hide resolved
src/main/resources/META-INF/rewrite/javaee-to-quarkus.yml Outdated Show resolved Hide resolved
Comment on lines 30 to 31
displayName: Migrate JavaEE Maven Dependencies to Quarkus 2
description: Upgrade Standard JavaEE dependencies to Quarkus 2 dependencies.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a recipe locally that does exactly this (and most part of the dependency handling of this recipe). It adds the Camel Extensions for Quarkus, and the output is closer to what quarkus init generates. I can contribute it on a separate PR (but I need openrewrite/rewrite#4006 merged before that).

Copy link
Member

Choose a reason for hiding this comment

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

#4006 is now merged if you'd like to open that other PR @ammachado

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ammachado Did you have a PR for similar functionality ?

@timtebeek timtebeek self-requested a review April 14, 2024 20:15
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great work here @ShatanikB ! I've refreshed your PR with a couple of new insights since it was opened, and added small steps like removing the maven-war-plugin. Hope you agree,

It took longer than I'd wanted to see this through, as I had left space for additional review. But enough time has passed now that we'll just continue to work on the main branch if anything new feedback arrives, since you've been patient enough to get this merged.

Thanks to all involved; hope this helps drive adoption towards Quarkus from Java EE 7, and from there onwards to Quarkus 3+.

@timtebeek timtebeek merged commit 26bedfe into openrewrite:main Jul 12, 2024
2 checks passed
kmccarp pushed a commit to kmccarp/rewrite-quarkus that referenced this pull request Aug 22, 2024
* JavaEE to Quarkus 2 Declarative Migration Recipe

* Add JavaEEtoQuarkus2MigrationTest and verify pom changes

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Split Recipe to Maven Dependencies Upgrade YML and Code Tranformations YML

* Updated Tests

* Remove recipe from quarkus.yml

* Merge all three JavaEEtoQuarkus2 recipes into one file

* Update src/test/java/org/openrewrite/quarkus/quarkus2/JavaEEtoQuarkus2CodeTranformationsTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Updated Dependency Migration Description to reflect that this migrates Maven only right now. Added a TODO for Gradle

* Updated dependency versions and tests

* Move recipes and reformat tests

* Break up dependency & maven recipes; expect to remove war plugin

* Bump failsafe and surefire plugin versions

---------

Co-authored-by: Adriano Machado <[email protected]>
Co-authored-by: Tim te Beek <[email protected]>
Co-authored-by: Tim te Beek <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants