-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Greatly improve Quarkus update #46308
Greatly improve Quarkus update #46308
Conversation
f08f5c0
to
0103326
Compare
@jtama that should fix the issue you told me on Zulip, could you give it a try |
This comment has been minimized.
This comment has been minimized.
Works like a charm. |
4066ed7
to
3fae12f
Compare
...s/tools/message-writer/src/main/java/io/quarkus/devtools/messagewriter/MessageFormatter.java
Outdated
Show resolved
Hide resolved
0ab1637
to
c988caa
Compare
devtools/cli/src/main/java/io/quarkus/cli/update/RewriteGroup.java
Outdated
Show resolved
Hide resolved
devtools/cli/src/main/java/io/quarkus/cli/update/RewriteGroup.java
Outdated
Show resolved
Hide resolved
...ls/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/QuarkusUpdate.java
Outdated
Show resolved
Hide resolved
...ls/gradle/gradle-application-plugin/src/main/java/io/quarkus/gradle/tasks/QuarkusUpdate.java
Outdated
Show resolved
Hide resolved
24e334e
to
6443c4b
Compare
...ls-common/src/main/java/io/quarkus/devtools/commands/handlers/ProjectInfoCommandHandler.java
Show resolved
Hide resolved
9c7fb33
to
0079693
Compare
Co-authored-by: Max Rydahl Andersen <[email protected]>
0079693
to
260cc85
Compare
Status for workflow
|
@gsmet you've contributed significantly to the update tooling, so I'd like you to be aware of this change and my perspective. Generally, I don't have any substantial remarks about this change. Here is what I can say though.
The current implementation hasn't evolved much in terms of interface and presentation and I'd generally welcome this change but I also think we may re-think it later. I've been looking into a different "info reading" mechanism in the last couple of weeks, so I've been having some thoughts about this topic. |
We still have the per-module in the info, the one in the update wasn't used.
FYI we were doing the job of rectify to show some icons depending on the status. So it was a state in between just the current state and the rectify which wasn't really useful because you had to run another command to actually get the align info. Also I created an issue for this as we discussed a few days ago: "Created #46460 for follow up work"
Yeah for info, it's mostly cleaning, the update has changed a bit more. |
@aloubyansky I think we can merge this PR as it contains a lot of fixes and should be available asap. I've fixed all backward compatibility and api issues which were important. There is a plan for the info command (that we should do in another PR): #46460 Please let's not block this PR longer as it contains things that some people are waiting for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted Guillaume to review it at least on the high level
@gsmet discussed the update part on Zulip so he's aware of most of it. For the info part, this is just the premises. It's not taking more resources than the previous version which was already checking for alignement (just not showing the info by default). Guillaume, let me know if you want me to hold it longer if you want to spend more time on the review (I'll wait this afternoon to merge). |
@gsmet is that too large to be backported to 3.19? |
@ia3andy thanks for all the updates and improvements! Like the new experience of updating! It does look like
Running |
Hello, I don't think that |
If it's a regression then, imo, we should fix it. |
I don't think this has ever been working from the CLI, I haven't changed this behaviour (AFAIK). The |
I'll have to go an try some versions again, I know it wasn't working at all in the past but in more recent versions I have been able to use it afaik. I'm off this afternoon but can try out some things again tomorrow probably. |
@wjglerum it'd be great if you could confirm whether it's a regression and/or provide enough details to reproduce it. |
So just gave this another try, updating my multi module project with
However Alos ruunning the update with Maven does still work with Quarkus 3.19.2: |
Thanks for testing, it helps a lot. I will give it another second look, i must have changed something by mistake.. |
-y/-n/-N
)target/rewrite/rewrite.log
) to have a clean update logging outputupdate

info:

help:
Created #46460 for follow up work