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

Allow to specify a custom quarkus udpate recipes artifact #41618

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

ia3andy
Copy link
Contributor

@ia3andy ia3andy commented Jul 2, 2024

  • updateRecipesVersion becomes quarkusUpdateRecipes in all the tooling
  • it's still possible to specify only the version

cc @gsmet @vsevel

@quarkus-bot quarkus-bot bot added area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/maven area/platform Issues related to definition and interaction with Quarkus Platform labels Jul 2, 2024
@vsevel
Copy link
Contributor

vsevel commented Jul 2, 2024

I gave it a try, and it was successful.
with command line: mvn io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:update -N -Dstream=3.9 -DupdateRecipesCoords=com.x.y.quarkus:x-quarkus-update-recipes:3.2.0-SNAPSHOT
I can update the quarkus-resteasy-reactive dependency with the relocation rule, plus change the version in the parent pom (my rule).
here is the log:

INFO] Project [bank-quarkus] Parsing source files
[INFO] Running recipe(s)...
[WARNING] Changes have been made to pom.xml by:
[WARNING]     io.quarkus.openrewrite.Quarkus
[WARNING]         com.x.openrewrite.Quarkus
[WARNING]             org.openrewrite.maven.ChangeParentPom: {oldGroupId=com.x.y.quarkus, newGroupId=com.x.y.quarkus, oldArtifactId=lo-quarkus-parent, newArtifactId=lo-quarkus-parent, newVersion=2.10.1+395}
[WARNING]         io.quarkus.updates.core.quarkus39.Relocations
[WARNING]             org.openrewrite.java.dependencies.ChangeDependency: {oldGroupId=io.quarkus, oldArtifactId=quarkus-resteasy-reactive, newArtifactId=quarkus-rest}
[WARNING] Please review and commit the results.
[WARNING] Estimate time saved: 5m

as discussed in the zulip thread, a dedicated module x-quarkus-update-recipes needs to be defined with:

    <dependencies>
        <dependency>
            <groupId>io.quarkus</groupId>
            <artifactId>quarkus-update-recipes</artifactId>
            <version>...</version>
        </dependency>
    </dependencies>

    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-dependency-plugin</artifactId>
                <executions>
                    <execution>
                        <id>unpack-dependencies</id>
                        <phase>process-resources</phase>
                        <goals>
                            <goal>unpack-dependencies</goal>
                        </goals>
                        <configuration>
                            <outputDirectory>${project.build.directory}/classes</outputDirectory>
                            <includeScope>runtime</includeScope>
                            <includes>quarkus-updates/**</includes>
                            <excludeTransitive>true</excludeTransitive>
                        </configuration>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>

this information would be welcome in the update-quarkus guide.
as discussed also, the only drawback I see at this point is maven-ish like solution, forcing users to understand that there is an underlying artifact that we want to replace, yet grab all rule definitions and transitive dependencies.
I am happy that there is an immediate solution. may be longer term we can think about something more user friendly, such as providing a custom jar that is run in addition to the default one, instead of replacing it.

note that in my case, having to release the replacing artifact is not an issue, because I intend to add it to the monorepo that contains all of our extensions, and as such it would be released at the same time,

@gsmet
Copy link
Member

gsmet commented Jul 2, 2024

as discussed also, the only drawback I see at this point is maven-ish like solution, forcing users to understand that there is an underlying artifact that we want to replace, yet grab all rule definitions and transitive dependencies.

Again, I think we should just have a list of comma-separated artifacts. I don't think that would be a lot harder to implement?

I'm really not happy with having to augment the existing recipes, especially since for some versions, we adjust them often.

@vsevel
Copy link
Contributor

vsevel commented Jul 2, 2024

if @ia3andy is fine with it, I can try to provide an alternate PR tomorrow.

This comment has been minimized.

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2024

as discussed on zulip, @vsevel will add a new param for additional recipes coords on top of this.

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2024

@vsevel so should we merge this or not?

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 3, 2024

@gsmet could you approve this, I'll merge it only if @vsevel say he needs it..

@vsevel
Copy link
Contributor

vsevel commented Jul 3, 2024

I do not need it, but it gives more flexibility and it does not hurt.
the only thing I need minimally is covered by the other #41663 (being able to specify additional recipe artifacts)

Copy link
Member

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

lets try be backwards compatible when its this easy.

This overrules the update recipe, right? thus user must depend on quarkus updates recipe to have things work? the idea of having additional as discussed on zulip is for later?

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 4, 2024

lets try be backwards compatible when its this easy.

This overrules the update recipe, right? thus user must depend on quarkus updates recipe to have things work? the idea of having additional as discussed on zulip is for later?

It's another PR to add on top of this one: #41663

@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 4, 2024

I need to rebase it and make the renames according to @maxandersen suggestion

@ia3andy ia3andy changed the title Allow to use custom quarkus udpate recipe coords Allow to specify a custom quarkus udpate recipes artifact Jul 4, 2024
@ia3andy
Copy link
Contributor Author

ia3andy commented Jul 4, 2024

Ok I consistently renamed to:
--quarkus-update-recipes and --additional-update-recipes

@ia3andy ia3andy requested a review from maxandersen July 4, 2024 15:30
Copy link

quarkus-bot bot commented Jul 4, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 4a0da3d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/virtual-threads/grpc-virtual-threads

io.quarkus.grpc.example.streaming.VertxVirtualThreadTest.testStreamingOutputCall - History

  • INTERNAL: Half-closed without a request - io.grpc.StatusRuntimeException
io.grpc.StatusRuntimeException: INTERNAL: Half-closed without a request
	at io.grpc.Status.asRuntimeException(Status.java:533)
	at io.grpc.stub.ClientCalls$BlockingResponseStream.hasNext(ClientCalls.java:631)
	at java.base/java.util.Iterator.forEachRemaining(Iterator.java:132)
	at io.quarkus.grpc.example.streaming.VirtualThreadTestBase.testStreamingOutputCall(VirtualThreadTestBase.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.junit.QuarkusTestExtension.runExtensionMethod(QuarkusTestExtension.java:973)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestMethod(QuarkusTestExtension.java:823)

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

  • Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

Copy link
Member

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

lgtm so the dependent pr can be aligned.

is there a reason why we don't just call it --updateRecipes as for the cli shouldn't it be obvious to be quarkus ones?

@ia3andy ia3andy merged commit 1616dbe into quarkusio:main Jul 5, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 5, 2024
@ia3andy ia3andy deleted the update-coords branch July 5, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to quarkus cli (not maven/gradle/etc.) area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/maven area/platform Issues related to definition and interaction with Quarkus Platform triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants