Recipes for camel-quarkus #46

JiriOndrusek · 2023-08-01T06:51:57Z

Descripition:

PR introduces a recipe - org.apache.camel.quarkus.update.CamelQuarkusMigrationRecipe - which takes care of the whole camel-quarkus migration process. This main recipe registeres all other recipes via the constructor and doNext method. If the migrated project contains any camel-quarkus dependency, the child recipes are executed. There are 4 types of camel recipes so far:
Java - recipes migrating java classes. Abstract parent of those recipes detects existence of camel-quarkus import and does nothing if there is no such import
Pom - maven recipes taking care of removed extensions (comments helping users to find replacement are created if possible)
Properties - simple recipe
Yaml - some transformation for YAML DSL feature from Camel
All migration cases are covered by JUnit tests.

Important features:

There are 2 performance optimizations implemented. None of the CQ recipe migrates the code if there is no CQ dependency in the pom. Java recipes are skipped, if the is no import of ‘org.apache.camel’ in the class.
We upgraded java in this project to have access to String blocks, which are heavily used in the tests.
PR currently does not contain gradle recipes
All recipes are grouped together into ‘CamelQuarkusMigrationRecipe`
Support of maven surefire plugin was added and the test can be executed via mvn test

Opened questions:

How to cover the upgrade process for more versions with the Java recipes avoiding compilation conflicts? (Example from the readme.md: Recipes applied for a project in version 2.7.0.Final updating to 3.1.0.Final (currentVersion=2.7, targetVersion=3.1):2.9.yaml, 3alpha.yaml and 3.1.yaml. Java recipes used in 3alpha.yaml requires camel dependencies 3.18.7 for both tests (and ideally also recipes). But java recipes from 3.1.yaml requires Camel dependencies 4.x. Having both versions 3.x and 4.x on the classpath is not possible.
Which java version should be used by this project? Because j15 brings String blocks, which are very helpful in writing junit tests, we upgraded Java. Would this be acceptable?
Is it possible to upgrade/use a more recent version of openrewrite? while developing a new recipe we realized it would be nice if we can use the most recent version since it provides many new features which will always be helpful.

Limitations:

PR does not contain any Gradle recipes.

What do you think @gsmet , @maxandersen, @ia3andy

JiriOndrusek · 2023-08-01T07:25:24Z

There are 4 minor TODOs in the PR, which does not affect functionality.

gsmet · 2023-08-01T09:14:57Z

How to cover the upgrade process for more versions with the Java recipes avoiding compilation conflicts? (Example from the readme.md: Recipes applied for a project in version 2.7.0.Final updating to 3.1.0.Final (currentVersion=2.7, targetVersion=3.1):2.9.yaml, 3alpha.yaml and 3.1.yaml. Java recipes used in 3alpha.yaml requires camel dependencies 3.18.7 for both tests (and ideally also recipes). But java recipes from 3.1.yaml requires Camel dependencies 4.x. Having both versions 3.x and 4.x on the classpath is not possible.

My understanding is that your problem is with tests? If so, I think you should have different projects that you upgrade in separate modules.
TBH, we haven't given more thoughts about the testing of the updates. I think ideally, we should have source projects, and a target project, that we could regenerate. @ia3andy did something like that for the codestarts and it's working pretty well.

Which java version should be used by this project? Because j15 brings String blocks, which are very helpful in writing junit tests, we upgraded Java. Would this be acceptable?

As mentioned in the review, maybe let's separate the projects? Also let's make sure the release target of the update jar is 11?

Is it possible to upgrade/use a more recent version of openrewrite? while developing a new recipe we realized it would be nice if we can use the most recent version since it provides many new features which will always be helpful.

Unfortunately not. They removed a feature that is critical to us. We discussed with the OpenRewrite team and we hope at some point they will reintroduce it.
We might put in place some workaround at some point but it would be a lot less practical that what we're doing right now.

gsmet

Added some comments.

gsmet · 2023-08-01T08:57:39Z

recipes/pom.xml

@@ -13,7 +13,7 @@
    <name>Quarkus Updates - Recipes</name>

    <properties>
-        <maven.compiler.release>11</maven.compiler.release>
+        <maven.compiler.release>17</maven.compiler.release>


That won't fly. We need the jar to be compatible with Java 11 as you can upgrade a Java 11 project.

Would it be possible to move the tests to a separate module?

If J11 is the only possible option, moving tests to a separate module is a solution, which allow us to use J17 for the tests.
I'll search for a guide in codestarts (@ia3andy if you can share some thoughts on the separation, that would be helpful)

temporary solution implemented

I don't see how this relates to codestarts?

gsmet · 2023-08-01T09:03:34Z

recipes/pom.xml

+        <dependency>
+            <groupId>org.apache.camel</groupId>
+            <artifactId>camel-api</artifactId>
+            <version>${camel.version}</version>


Shouldn't all the Camel dependencies be of scope test?

We are referencing them in the recipes (like using *.class.getCanonicalName() and similar). If we agree, that the camel dependencies have to stay in test scope only (which is probably the right decision), we will refactor recipes.

JiriOndrusek · 2023-08-02T11:59:07Z

Hi @gsmet ,
before there is a correct solution using recipe/test separation similarly to codestarts. would it be acceptable following temporary solution?

quarkus-updates (this PR) won't contain any camel related tests and would use J11 (if possible I'd like to keep a simple test verifying that no recipe is executed if there is no camel dependency)
new project would be introduced. This project would depend on quarkus-upgrades and would contain all tests (using j17 and camel dependencies)

Once the (final/general) separation is prepared, the tests from the temporary project would be refactored to use the final solution. This approach would allow to keep recipes + coverage even before the separation is merged (which could take some time, from the glance at the code in codestarts)

JiriOndrusek · 2023-08-03T12:13:20Z

@gsmet we fixed all suggestions as asked. Currently the majority of tests is removed from thisPR

tests will be placed in a different project as a temporary solution (until proper split between recipes & tests is proposed - which may take some time)
the only existing test verifies, that a recipe is not executed in a project with no camel-quarkus dependency.
Java is kept on version 11. No camel dependencies are present in the pom.xml.

Is this solution ok?

JiriOndrusek · 2023-08-08T08:28:10Z

recipes/pom.xml

@@ -22,6 +22,15 @@
        <rewrite-maven-plugin.version>4.46.0</rewrite-maven-plugin.version>
        <!--   for now, we don't know where to find compatibility information for the Gradle plugin -->
        <rewrite-gradle-plugin.version>5.40.6</rewrite-gradle-plugin.version>
+        <!-- If version from rewrite-recipe-bom is used, error occures during testing ->


Weird behavior is happening if the version from BOM is used for rewrite-migrate-java dependency. NoSuchMethod 'com.fasterxml.jackson.core.util.TextBuffer com.fasterxml.jackson.core.io.IOContext.constructReadConstrainedTextBuffer()' is thrown during execution of the tests. I forced version to 1.16.0 which does not cause the error. TBH I'm not completely sure why this is happening, BOM should contain compatible versions.

JiriOndrusek · 2023-08-10T13:00:33Z

Small refactor is required

JiriOndrusek · 2023-08-10T13:11:06Z

Result of the discussion on upstream chat: https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/Quarkus.20update.20tool.20.28non-core.29

All tests covering camel-quarkus migration recipes (and skipped execution in non-camel-quarkus projects) are located in recipes-unit-tests module, which uses Java 17.

I see a limitation of this solution:
• current separation does not allow to have a different dependencies for a different stream of migration (which could be solved by separation into test modules for each stream if needed)

I think that PR could be merged (and squashed into 1 commit - I kept separated commits to make reviewing easier)

@ia3andy FYI

ia3andy · 2023-08-14T12:23:55Z

recipes/src/main/resources/quarkus-updates/core/3.0.yaml

@@ -26,6 +26,13 @@
 #####
 ---
 type: specs.openrewrite.org/v1beta/recipe
+name: custom


@JiriOndrusek is that normal?

I forgot to write down a name which makes a sense - this was just my "custom" text, I'll fix it during tomorrow

I added the correct text here

@JiriOndrusek it's still the old text

I believe at some point you have removed a commit or something, make sure that you have all your changes.

Thanks good catch @ia3andy I renamed it, is it make okay now ?

ia3andy · 2023-08-14T12:27:06Z

@JiriOndrusek since we might have other extensions contributing java recipe, it would make sense to have a common package naming. something like io.quarkus.updates.camel .

JiriOndrusek · 2023-08-14T13:16:30Z

@JiriOndrusek since we might have other extensions contributing java recipe, it would make sense to have a common package naming. something like io.quarkus.updates.camel .

I agree, I'll refactor the PR

JiriOndrusek · 2023-08-15T09:18:12Z

I refactored the PR to use a package name io.quarkus.updates.camel30 (I added suffix 30, as the same is used in the quarkus/core30 and it clearly shows, that the recipe is for vesion 3.0

JiriOndrusek · 2023-08-15T09:18:51Z

@ia3andy thanks for the hints above. Now it is all fixed.

ia3andy · 2023-08-16T07:36:54Z

@ia3andy thanks for the hints above. Now it is all fixed.

I still see package org.apache.camel.quarkus.update; maybe you missed a commit?

svkcemk · 2023-08-16T12:45:46Z

@ia3andy thanks for the hints above. Now it is all fixed.

I still see package org.apache.camel.quarkus.update; maybe you missed a commit?

@ia3andy I am taking a look, thanks!

svkcemk · 2023-08-17T11:04:18Z

@ia3andy can you please take a look now? Thanks

ia3andy · 2023-08-17T14:22:16Z

@gsmet could you check? I still see the org.apache packages

ia3andy · 2023-08-17T14:23:12Z

svkcemk · 2023-08-21T10:38:27Z

@ia3andy I think we are good now, could you please take a look ? thanks!

JiriOndrusek · 2023-08-28T06:10:27Z

Thanks @svkcemk for the help! I was on PTO.
I checked the current status of the PR and there is no package org.apache.camel.quarkus.update.
(I thought that I fixed it 2 weeks ago, I might indeed skipped some commits.)
@ia3andy I'm sorry for spending so much time on this PR, but it should be OK now.

ia3andy · 2023-08-30T08:18:45Z

Thanks @svkcemk for the help! I was on PTO. I checked the current status of the PR and there is no package org.apache.camel.quarkus.update. (I thought that I fixed it 2 weeks ago, I might indeed skipped some commits.) @ia3andy I'm sorry for spending so much time on this PR, but it should be OK now.

We are waiting for a change in Quarkus core to allow having yaml extension recipes. @svkcemk is on it.

Outdated!

maxandersen · 2023-09-07T13:29:43Z

quarkusio/quarkus#35688 is the dependent PR

JiriOndrusek · 2023-09-19T06:53:49Z

@ia3andy, @maxandersen I updated (and tested locally) the PR to reflect changes from quarkusio/quarkus#35910.

All succeeded, therefore this PR can be merged.

JiriOndrusek · 2023-09-19T06:56:35Z

recipes/src/main/resources/quarkus-updates/core/3.0.yaml

+recipeList:
+  - io.quarkus.updates.camel30.CamelQuarkusMigrationRecipe
+---
+type: specs.openrewrite.org/v1beta/recipe


I found this change, which is probably not needed anymore - I will verify and remove it

JiriOndrusek · 2023-09-19T07:51:00Z

Problem is fixed.

the only work to be done is to replace simple java recipes with the yaml ones

ia3andy · 2023-09-19T07:53:40Z

@JiriOndrusek @svkcemk You did an amazing job in all this, thanks for your patience, it was a long journey, but I think having a standardized way for extensions to provide their recipes is awesome!

JiriOndrusek · 2023-09-19T10:23:42Z

@ia3andy I update PR to reference a yaml recipe from the test.

As I thought, all the migrations contains some kind of logic a condition which can not be simulated by easy yaml recipes. Therefore I can not convert any java recipe to yaml.

Here is a list with the majority of upgrades + explanation why it can not be done
<style type="text/css"></style>

Removed label on @UriEndpoint as you should use category instead.	annotation attribute value replced with the constant
Removed all asyncCallback methods on ProducerTemplate. Use asyncSend or asyncRequest instead.	addedcomment with suggestion
Removed org.apache.camel.spi.OnCamelContextStart. Use org.apache.camel.spi.OnCamelContextStarting instead.	probably not, need to add new interface and remove old one
Removed org.apache.camel.spi.OnCamelContextStop. Use org.apache.camel.spi.OnCamelContextStopping instead.	probably not, need to add new interface and remove old one
Decoupled the org.apache.camel.ExtendedCamelContext from the org.apache.camel.CamelContext.	complicated replaces of context calls, with comment exlanation
Replaced adapt() from org.apache.camel.CamelContext with getCamelContextExtension	change of ' context.adapt(ModelCamelContext.class)->((ModelCamelContext)context).' with some corner cases
Decoupled the org.apache.camel.ExtendedExchange from the org.apache.camel.Exchange.	replace + type cast of (exchange.adapt(ExtendedExchange.class) -> exchange.getExchangeExtension())
Replaced adapt() from org.apache.camel.ExtendedExchange with getExchangeExtension	replace of methods + with several corner cases
Exchange failure handling status has moved from being a property defined as ExchangePropertyKey.FAILURE_HANDLED to a member of the ExtendedExchange, accessible via `isFailureHandled()`method.	continuation of above case
Removed Discard and DiscardOldest from org.apache.camel.util.concurrent.ThreadPoolRejectedPolicy.	suggestion in comment
Removed org.apache.camel.builder.SimpleBuilder. Was mostly used internally in Camel with the Java DSL in some situations.	suggestion in comment
Moved org.apache.camel.support.IntrospectionSupport to camel-core-engine for internal use only. End users should use org.apache.camel.spi.BeanInspection instead.	change of impor, BUT not a package, only one class
Removed archetypeCatalogAsXml method from org.apache.camel.catalog.CamelCatalog.	suggestion in comment
The method configure from the interface org.apache.camel.main.Listener was removed	suggestion in comment
The type for `dumpRoutes` on `CamelContext` has changed from `boolean` to `String` to allow specifying either xml or yaml.	change of the parameter type, comment with explanatiom

JiriOndrusek · 2023-09-20T11:34:50Z

Hi @ia3andy I refactored 3 places to use yaml instead of java recipes (properties, poms & change oftype)
For the rest of the recipes it wasn't possible to use yaml, because their functionality contains handing of the parameters or more complicated stuff. There was 1 place which should be pissible to refactor, but the recipe registered in the yaml didn't work (I spent several hours to make it work), so I added a comment with TODO to this line.

From my POV the one jave recipe with todo should be reported in here (or camel-quarkus) fir further investigation, I see an option, that the recipe is not meant for the use in yaml. As it is not documented and the code of the recipe looks differently then the one which works (changeType - works vs changePackage - does not work)

ia3andy

LGTM

gsmet previously requested changes Aug 1, 2023

View reviewed changes

JiriOndrusek force-pushed the CQ-recipes-part01 branch 3 times, most recently from f15befd to a6e1eac Compare August 3, 2023 12:12

JiriOndrusek marked this pull request as ready for review August 3, 2023 12:58

JiriOndrusek force-pushed the CQ-recipes-part01 branch 2 times, most recently from f4a173c to 823f0a9 Compare August 8, 2023 08:25

JiriOndrusek commented Aug 8, 2023

View reviewed changes

JiriOndrusek force-pushed the CQ-recipes-part01 branch 3 times, most recently from 6a0ddf6 to e45e447 Compare August 10, 2023 12:45

JiriOndrusek force-pushed the CQ-recipes-part01 branch 2 times, most recently from 76754ec to 732c1c1 Compare August 10, 2023 13:09

JiriOndrusek force-pushed the CQ-recipes-part01 branch from 732c1c1 to 4379592 Compare August 10, 2023 13:23

ia3andy reviewed Aug 14, 2023

View reviewed changes

JiriOndrusek force-pushed the CQ-recipes-part01 branch from 4379592 to bc18937 Compare August 15, 2023 09:16

svkcemk force-pushed the CQ-recipes-part01 branch 2 times, most recently from 9c56dba to d0d60e0 Compare August 21, 2023 10:35

svkcemk force-pushed the CQ-recipes-part01 branch from 88d608e to f61a48c Compare August 25, 2023 15:13

JiriOndrusek force-pushed the CQ-recipes-part01 branch from f61a48c to e61b447 Compare September 19, 2023 06:51

JiriOndrusek commented Sep 19, 2023

View reviewed changes

JiriOndrusek force-pushed the CQ-recipes-part01 branch from e61b447 to 9a1f1d8 Compare September 19, 2023 07:49

JiriOndrusek force-pushed the CQ-recipes-part01 branch from 9a1f1d8 to 2cfd8ed Compare September 19, 2023 10:19

JiriOndrusek force-pushed the CQ-recipes-part01 branch from 2cfd8ed to 56f6308 Compare September 20, 2023 08:12

Recipes for camel-quarkus

3250b8c

JiriOndrusek force-pushed the CQ-recipes-part01 branch from 56f6308 to 3250b8c Compare September 20, 2023 11:29

ia3andy approved these changes Sep 20, 2023

View reviewed changes

ia3andy merged commit 17b1728 into quarkusio:main Sep 20, 2023
1 check passed

JiriOndrusek mentioned this pull request Oct 2, 2023

Camel-quarkus: Recipe for upgrade java (to 17) is missing #50

Closed

Recipes for camel-quarkus #46

Recipes for camel-quarkus #46

Conversation

JiriOndrusek commented Aug 1, 2023

JiriOndrusek commented Aug 1, 2023

gsmet commented Aug 1, 2023

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiriOndrusek commented Aug 2, 2023 • edited Loading

JiriOndrusek commented Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

JiriOndrusek commented Aug 10, 2023

JiriOndrusek commented Aug 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ia3andy commented Aug 14, 2023

JiriOndrusek commented Aug 14, 2023

JiriOndrusek commented Aug 15, 2023

JiriOndrusek commented Aug 15, 2023

ia3andy commented Aug 16, 2023

svkcemk commented Aug 16, 2023

svkcemk commented Aug 17, 2023

ia3andy commented Aug 17, 2023

ia3andy commented Aug 17, 2023

svkcemk commented Aug 21, 2023

JiriOndrusek commented Aug 28, 2023

ia3andy commented Aug 30, 2023

maxandersen commented Sep 7, 2023

JiriOndrusek commented Sep 19, 2023

Choose a reason for hiding this comment

JiriOndrusek commented Sep 19, 2023

ia3andy commented Sep 19, 2023 • edited Loading

JiriOndrusek commented Sep 19, 2023

JiriOndrusek commented Sep 20, 2023

ia3andy left a comment

Choose a reason for hiding this comment

JiriOndrusek commented Aug 2, 2023 •

edited

Loading

JiriOndrusek commented Aug 3, 2023 •

edited

Loading

ia3andy commented Sep 19, 2023 •

edited

Loading