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

Replace Dev UI libs with a composite from mvnpm #41239

Closed
wants to merge 1 commit into from

Conversation

phillip-kruger
Copy link
Member

@phillip-kruger phillip-kruger commented Jun 17, 2024

This PR change the dev-ui libraries to use a composite from mvnpm that creates one jar containing all the (current) ui dependencies.

(see https://mvnpm.org/composites)

This fix 2 issues:

  1. Users are not tied to the version of a library defined in dev-ui (in the bom)
  2. Faster maven download as there is only one jar now.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/kafka-streams area/vertx labels Jun 17, 2024
@maxandersen
Copy link
Member

I grok the idea but I'm not liking it because we are doing something noone else can do afaik.

How/where are composites defined?
If a user wants to do adjustments or have their own composite is that an option?

Ie. it's under mvnnpm namespace. Not in Quarkus

@phillip-kruger
Copy link
Member Author

Anyone can do a PR on mvnpm ( the composite project). For dev ui, extension devs can still add their own libs ( increasing the chance of a version locked for a user). The idea is to manage all dev ui libs in the composite.

@phillip-kruger phillip-kruger changed the title Replace Dev UI with a composite from mvnpm Replace Dev UI libs with a composite from mvnpm Jun 17, 2024
@phillip-kruger
Copy link
Member Author

This comment has been minimized.

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.

I get that - but that means anyone needs to be ok to have the code open.

I'm not saying absolutely no to this but I would like us to:

  1. explain why this needs to be in org.mvnpm composite. why not in i.e. quarkus.io and mvnpm opens PR's against a artifact?

  2. how this works with CVE's / forks of quarkus etc.

  3. how is collisions avoided when users add another version of the lib?

  4. how can a user update a library? (I 'm assuming he can't as something in First shot at integrating mp-metrics. #3 ensures things are isolated??)

  5. what decides what goes into the composite? will it not just keep growing? how about backward compatibility if core gets split up more?

@phillip-kruger
Copy link
Member Author

phillip-kruger commented Jun 17, 2024

@maxandersen - So I can (maybe in another PR) change mvnpm to have a dynamic list of where remote composites can live. At the moment, all composite definitions are read off mvnpm/composites GH repo. If I change mvnpm to allow this to be a list, and allow adding more, we can add quarkus GH repo and move the definition there ? Will that help ? And do you think I should do this before merging this ?

Copy link

quarkus-bot bot commented Jun 18, 2024

Status for workflow Quarkus CI

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

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 17 Build Failures Logs Raw logs 🚧
✔️ JVM Tests - JDK 21 Logs Raw logs 🚧

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/opentelemetry/deployment 
! Skipped: extensions/liquibase-mongodb/deployment extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment and 55 more

📦 extensions/opentelemetry/deployment

Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.2.5:test (default-test) on project quarkus-opentelemetry-deployment:

Please refer to /home/runner/work/quarkus/quarkus/extensions/opentelemetry/deployment/target/surefire-reports for the individual test results.
Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
There was an error in the forked process


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/oidc-client-wiremock

io.quarkus.it.keycloak.OidcClientTest.testEchoAndRefreshTokensWithConcurrency - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.keycloak.OidcClientTest 1 expectation failed. Response body doesn't match expectation. Expected: "access_token_2" Actual: access_token_1 within 2 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.it.keycloak.OidcClientTest 1 expectation failed.
Response body doesn't match expectation.
Expected: "access_token_2"
  Actual: access_token_1
 within 2 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)

@phillip-kruger
Copy link
Member Author

Closing for now. I'll circle back to this at some point.

@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/kafka-streams area/vertx triage/flaky-test triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants