-
Notifications
You must be signed in to change notification settings - Fork 3k
Simplify the dev services model #48445
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
Simplify the dev services model #48445
Conversation
|
🎊 PR Preview 1a01fee has been successfully built and deployed to https://quarkus-pr-main-48445-preview.surge.sh/version/main/guides/
|
|
Quick question: does it handle the shared network config centrally? This has been quite a problem until now where we have to handle it specifically for every container (and implement it differently for each obviously :)). |
b6d90b5 to
a8afe31
Compare
| for (DevServicesRequestBuildItem serviceRequest : devServicesRequests) { | ||
| devServicesRegistry.start(serviceRequest); | ||
| } | ||
| // It would be nice to use the config source, but since we have the build item right there and this is a one-shot operation, just ask it instead |
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.
This comment needs updating
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
02336dd to
e129b90
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
039b2ec to
0c7f6f0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0c7f6f0 to
b67a540
Compare
This comment has been minimized.
This comment has been minimized.
|
|
||
| /** | ||
| * BuildItem for running dev services. | ||
| * BuildItem for discoevered (running) or to be started dev services. |
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.
| * BuildItem for discoevered (running) or to be started dev services. | |
| * BuildItem for discovered (running) or to be started dev services. |
| // We *could* get the services from the tracker, and short circuit some work. But that short circuit has some risk. | ||
| // If the matching RunningDevService was in a different classloader, we'd get a ClassCastException. |
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 think these comments are stale?
b67a540 to
83f4222
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
83f4222 to
b9c091e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b9c091e to
95994b4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
95994b4 to
4c4eb46
Compare
This comment has been minimized.
This comment has been minimized.
4c4eb46 to
a3faf65
Compare
Status for workflow
|
Status for workflow
|
| Status | Name | Step | Failures | Logs | Raw logs | Build scan |
|---|---|---|---|---|---|---|
| ✔️ | JVM Tests - JDK 17 | Logs | Raw logs | 🚧 | ||
| ✖ | JVM Tests - JDK 21 | Build |
Failures | Logs | Raw logs | 🚧 |
Full information is available in the Build summary check run.
You can consult the Develocity build scans.
Failures
⚙️ JVM Tests - JDK 21 #
- Failing: devtools/cli
📦 devtools/cli
✖ io.quarkus.cli.config.SetConfigTest.addEncryptedConfiguration line 74 - History - More details - Source on GitHub
java.lang.RuntimeException: java.lang.NegativeArraySizeException: -98
at io.smallrye.config.crypto.AESGCMNoPaddingSecretKeysHandler.decode(AESGCMNoPaddingSecretKeysHandler.java:44)
at io.smallrye.config.ExpressionConfigSourceInterceptor$1.accept(ExpressionConfigSourceInterceptor.java:79)
at io.smallrye.config.ExpressionConfigSourceInterceptor$1.accept(ExpressionConfigSourceInterceptor.java:71)
at io.smallrye.common.expression.ExpressionNode.emit(ExpressionNode.java:22)
at io.smallrye.common.expression.Expression.evaluateException(Expression.java:56)
at io.smallrye.common.expression.Expression.evaluate(Expression.java:70)
at io.smallrye.config.ExpressionConfigSourceInterceptor.getValue(ExpressionConfigSourceInterceptor.java:71)
| public DevServiceDescriptionBuildItem(String name, String description, ContainerInfo containerInfo, | ||
| Map<String, String> configs) { | ||
| this(name, description, () -> containerInfo, configs, null); | ||
| public DevServiceDescriptionBuildItem(String name, Supplier<ContainerInfo> lazyContainerInfo, |
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.
Just as an FYI, this change broke binary compatibility. I noticed this as Quarkus LangChain4j started failing, but I was unable to find the problem until now - the problem stems from https://github.com/quarkiverse/quarkus-wiremock/blob/dd0b21d3679cbd3f86d6b73026b2754c716364b4/deployment/src/main/java/io/quarkiverse/wiremock/devservice/WireMockServerProcessor.java#L84
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.
That's my bad. I suggest I prepare a PR to reintroduce that constructor.
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.
No problem at all. We can (and probably should) reintroduce that contructor, but that will create a source incompatibility (which admitedly is a smaller problem)
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.
At least we can still get it in for 3.25
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.
👌
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.
Just as an FYI, this change broke binary compatibility. I noticed this as Quarkus LangChain4j started failing, but I was unable to find the problem until now - the problem stems from https://github.com/quarkiverse/quarkus-wiremock/blob/dd0b21d3679cbd3f86d6b73026b2754c716364b4/deployment/src/main/java/io/quarkiverse/wiremock/devservice/WireMockServerProcessor.java#L84
Once again, I'm reminded that I really need to do the thing on the ecosystem CI that lists the commits that went in since the last green build. It might have made the detective work a bit easier.


Rebased on #47610 and still very much a draft
This targets ironing out issues from #47610 and provide a cleaner model for development of extensions providing dev services.
Instead of overloading the
DevServicesResultBuildItemfor discovered and to be started services, this PR creates aDevServicesRequestBuildItem. A dev services request defines, the name, the build-time config (for identification), how to start the dev service (supplier of startable), and how to extract the configuration to be injected to the app (key/supplier map).Extensions can discover running services via ContainerLocator and CompseLocator and produce
DevServicesResultBuildItemor, when they need to create a service, produceDevServicesRequestBuildItem.Changes :
RunningDevServicesTrackertoRunningDevServicesRegistry.RunnableDevServiceand moved the start/reuse logic to theDevServicesRegistryBuildItem.RunningServiceinstead of Closeable. -> see belowDevServicesRegistryBuildItemnorDevServicesRequestBuildItemhold any state. All state is in the registry andRunningServices it holds.Fixes with still some problems :
RunningService.Open for discussion :
For a given
DevServiceOwner, do we need to hold a bunch of services or just one ? Having one would simplify things further, ex. we can remove service directly by its owner.Edit: Fixes #47627