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 adding and removing extensions from Dev UI #43840

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phillip-kruger
Copy link
Member

@phillip-kruger phillip-kruger commented Oct 12, 2024

This PR add the "quarkus add" and "quarkus remove" to Dev UI. It allows you to browse (and search) for installable extensions in Dev UI and install them from Dev UI. It also adds a remove button on the details screen on the extension if it can be uninstalled.

extension_management_dev_ui

@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 area/vertx labels Oct 12, 2024
Copy link

github-actions bot commented Oct 12, 2024

🎊 PR Preview 3cfbfcb has been successfully built and deployed to https://quarkus-pr-main-43840-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@phillip-kruger phillip-kruger force-pushed the dev-ui-add-ext branch 2 times, most recently from 7629b23 to 1a7e628 Compare October 13, 2024 22:46
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@cescoffier
Copy link
Member

Looks like it breaks a few things :-)

@phillip-kruger
Copy link
Member Author

phillip-kruger commented Oct 14, 2024

@cescoffier yes I don't know why. The tests that breaks in CI runs fine on localhost. Maybe @maxandersen can comment ?

Copy link
Member

@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.

I added a micro comment but, more importantly, before merging, I think we need to make sure this doesn't slow down the startup too much:

  • Creating an ObjectMapper can be sometimes slow. Now it might just be the first ObjectMapper creation that is slow but we need to check for it.
  • The second thing that worries me is that the test failures seem to point out the fact that you're trying to update the registry even when not actually doing anything related to your new feature. Updating the registry is slow so I definitely don't want us to pay the price of updating it every day if you don't need it - and there's a good chance you won't add a dependency or create a new app every day. To test that, you will have to drop the local registry artifact and see how it performs at startup.

Not saying these are actual issues or issues we cannot solve but we need to make sure this feature is not introducing a regression in daily life developer experience.

This review can be dropped once everything is sorted out but I just want to make sure nobody merges before we have checked the situation.

extensions/vertx-http/deployment/pom.xml Outdated Show resolved Hide resolved
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@phillip-kruger
Copy link
Member Author

The test broke because of Remove Dev. This is now fixed, let's see what CI does

@phillip-kruger

This comment was marked as outdated.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@phillip-kruger phillip-kruger force-pushed the dev-ui-add-ext branch 2 times, most recently from 741ea23 to d9fa5e4 Compare October 17, 2024 23:32
@quarkus-bot

This comment has been minimized.

@gastaldi
Copy link
Contributor

@gsmet I am not sure how to check this. You mentioned "drop the local registry artifact" - how do I do that ? I cleared my .quarkus and .jbang folders in home, but all seems fine. Any tips ?

I believe @gsmet is talking about the ~/.m2/repository/io/quarkus/registry/quarkus-non-platform-extensions/1.0-SNAPSHOT/ artifact that is downloaded once a day by the Quarkus Registry client. Try removing it and see if there is a performance impact

@phillip-kruger
Copy link
Member Author

Thanks @gastaldi .

@gsmet deleting this does make the initial mvn quarkus:dev slower as it's downloading this artifacts. Very rough estimate is it add +-2 seconds to the initial quarkus:dev. From then onward it's cached so no issue.

Maybe you can test this on you laptop and see if it's noticeable for you. For me it was not.

@gastaldi
Copy link
Contributor

@phillip-kruger can we somehow hit the registry when the UI to add an extension is opened?

@phillip-kruger
Copy link
Member Author

@gastaldi - let me play around and l'll get back here.

@quarkus-bot

This comment has been minimized.

@phillip-kruger phillip-kruger force-pushed the dev-ui-add-ext branch 2 times, most recently from 61465d2 to 2e6e1a3 Compare October 18, 2024 05:49
@phillip-kruger
Copy link
Member Author

@gastaldi @gsmet - ok I now moved that this happen when the add screen loads. So nothing on quarkus:dev anymore. I think we are good to go.
loading

@phillip-kruger phillip-kruger added triage/waiting-for-ci Ready to merge when CI successfully finishes triage/backport labels Oct 18, 2024
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 18, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit d4c6c85.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 18, 2024

Status for workflow Quarkus CI

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

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 Logs Raw logs 🚧
✔️ JVM Tests - JDK 21 Logs Raw logs 🚧
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs 🔍
Gradle Tests - JDK 17 Build ⚠️ Check → Logs Raw logs 🚧
Gradle Tests - JDK 17 Windows Build ⚠️ Check → Logs Raw logs 🚧

Full information is available in the Build summary check run.
You can consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: extensions/vertx-http/deployment 
! Skipped: devtools/cli extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 406 more

📦 extensions/vertx-http/deployment

io.quarkus.vertx.http.security.PathMatchingHttpSecurityPolicyTest. - History - More details - Source on GitHub

java.lang.RuntimeException: io.quarkus.bootstrap.BootstrapException: Failed to create the application model for io.quarkus:quarkus-vertx-http-deployment::jar:999-SNAPSHOTnull
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:736)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: io.quarkus.bootstrap.BootstrapException: Failed to create the application model for io.quarkus:quarkus-vertx-http-deployment::jar:999-SNAPSHOTnull
	at io.quarkus.bootstrap.BootstrapAppModelFactory.resolveAppModel(BootstrapAppModelFactory.java:310)
	at io.quarkus.bootstrap.app.QuarkusBootstrap.bootstrap(QuarkusBootstrap.java:138)
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:679)
	... 1 more

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 area/vertx triage/backport triage/flaky-test triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants