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

Wire extension updates in quarkus update #35191

Merged
merged 2 commits into from
Aug 24, 2023

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Aug 3, 2023

This is a relatively naive and simple approach but it seems to work OK.

Note that this is a preliminar effort and it is not handling all the
possibilities.
Handling the other possibilities might be problematic given how we apply
the recipes to a root module of a multi-module project.

@aloubyansky I would be interested in your feedback.

/cc @ia3andy @aloubyansky

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform labels Aug 3, 2023
@ia3andy
Copy link
Contributor

ia3andy commented Aug 9, 2023

@gsmet nice, this is exactly what I had in mind! Thanks this is one less thing in my todo list :)

@gsmet gsmet marked this pull request as ready for review August 11, 2023 12:11
@gsmet
Copy link
Member Author

gsmet commented Aug 11, 2023

@aloubyansky could you have a look at this one? I think we will want it in 3.2 maybe.

@quarkus-bot

This comment has been minimized.

for (ExtensionUpdateInfo nonPlatformExtensionsUpdate : nonPlatformExtensionsUpdates) {
if (nonPlatformExtensionsUpdate.getCurrentDep().isPlatformExtension()) {
// add, my understanding is that we should define the version? As a dependency, as a managed one?
// not completely sure how to make it work for a multi-module project?
Copy link
Member

Choose a reason for hiding this comment

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

Should we log something in this case? AFAIU, we are not applying any update in this case.

// not completely sure how to make it work for a multi-module project?
} else if (nonPlatformExtensionsUpdate.getRecommendedDependency().isPlatformExtension()) {
// remove, decide what to do here, should we remove the version given it is now managed? Will OpenRewrite support that?
// not completely sure how to make it work for a multi-module project?
Copy link
Member

Choose a reason for hiding this comment

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

Same here?

gsmet added 2 commits August 24, 2023 10:06
Note that this is a preliminar effort and it is not handling all the
possibilities.
Handling the other possibilities might be problematic given how we apply
the recipes to a root module of a multi-module project.
@gsmet gsmet force-pushed the wire-extension-updates branch from 5ed04ce to 53219e6 Compare August 24, 2023 08:06
@gsmet
Copy link
Member Author

gsmet commented Aug 24, 2023

@aloubyansky I think we should get this one in even if not perfect. I'm not sure I fully understand the two other cases so if you want something logged you would have to finish this.
Also, you are logging these at the very beginning of the process.

@aloubyansky
Copy link
Member

Ok, those are not usual cases.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 24, 2023

Failing Jobs - Building 53219e6

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 20

@gsmet gsmet merged commit ba2efea into quarkusio:main Aug 24, 2023
@quarkus-bot quarkus-bot bot added this to the 3.4 - main milestone Aug 24, 2023
@gsmet gsmet modified the milestones: 3.4 - main, 3.2.5.Final Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants