-
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
Register ConfigSources directly with the builder #17483
Conversation
66e10d7
to
7634509
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 7634509
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/grpc-streaming✖ ⚙️ Maven Tests - JDK 11 #📦 integration-tests/maven✖ ⚙️ Native Tests - Data4 #📦 integration-tests/neo4j✖ ⚙️ Native Tests - HTTP #📦 integration-tests/elytron-resteasy✖ ⚙️ Native Tests - Messaging1 #📦 integration-tests/kafka-avro-apicurio2✖ 📦 integration-tests/kafka-avro✖ 📦 integration-tests/kafka-sasl✖ 📦 integration-tests/kafka-snappy✖ 📦 integration-tests/kafka-ssl✖ 📦 integration-tests/kafka-streams✖ 📦 integration-tests/kafka✖ 📦 integration-tests/reactive-messaging-kafka✖ ⚙️ Native Tests - Messaging2 #📦 integration-tests/reactive-messaging-amqp✖ ⚙️ Native Tests - Misc1 #📦 integration-tests/jgit✖ ⚙️ Native Tests - Misc2 #📦 integration-tests/mailer✖ ⚙️ Native Tests - Misc3 #📦 integration-tests/smallrye-graphql-client✖ ⚙️ Native Tests - Security1 #📦 integration-tests/elytron-security-jdbc✖ 📦 integration-tests/elytron-security-ldap✖ 📦 integration-tests/elytron-security-oauth2✖ 📦 integration-tests/elytron-security✖ ⚙️ Native Tests - Security2 #📦 integration-tests/oidc✖ |
7634509
to
39ce031
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 39ce031
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/main✖ ⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/main✖ ⚙️ JVM Tests - JDK 16 #📦 integration-tests/main✖ ⚙️ Maven Tests - JDK 11 #📦 integration-tests/maven✖ ⚙️ Native Tests - Main #📦 integration-tests/main✖ |
7f46266
to
9ee4749
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 9ee4749
Full information is available in the Build summary check run. Test Failures⚙️ Native Tests - Misc1 #📦 integration-tests/maven✖ |
9ee4749
to
a8e18aa
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building a8e18aa
Full information is available in the Build summary check run. Test Failures⚙️ Native Tests - Main #📦 integration-tests/main✖ |
a8e18aa
to
e675c46
Compare
@radcortez I rebased the PR but the failure in |
Sure, let me check. |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building e675c46
Full information is available in the Build summary check run. Test Failures⚙️ Native Tests - Main #📦 integration-tests/main✖ |
e675c46
to
860affb
Compare
860affb
to
6ad8efd
Compare
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 6ad8efd
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/resteasy-reactive-rest-client✖ |
6ad8efd
to
c474c5e
Compare
Interesting! I'll take a look on Monday |
Thanks! This also includes the work we talked about with the safe static init sources. |
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.
Great work!
I added a few comments
core/runtime/src/main/java/io/quarkus/runtime/configuration/StaticInitSafe.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/io/quarkus/deployment/builditem/StaticInitConfigSourceProviderBuildItem.java
Show resolved
Hide resolved
...loyment/src/main/java/io/quarkus/deployment/configuration/RunTimeConfigurationGenerator.java
Show resolved
Hide resolved
c474c5e
to
d6aee25
Compare
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.
Awesome, thanks!
The idea here is to register sources directly with the
SmallRyeConfigBuilder
instead of relying on theServiceLoader
. One of the main motivations is to better control what we add toConfig
, avoid adding problematic sources and avoid discovery at runtime.One other issue is related with #5492. Even with the guard added by
quarkus/core/deployment/src/main/java/io/quarkus/deployment/steps/ConfigBuildSteps.java
Lines 96 to 98 in a9d9a69
Graal is still able to reach the source entries but fails because they are not registered for reflection. Check smallrye/smallrye-config#580 and https://groups.google.com/g/quarkus-dev/c/yP7QsNRgYxY
The PR adds a check if servlet APIs and implementations are available to either include the RESTEasy config sources or rewrite them to not use their servlet-based implementation. Ideally, the RESTEasy config sources should be fixed.
It also adds the
@StaticInitSafe
annotation to mark sources that can be safely added and started during static init.