-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Ban and remove quarkus logger in our own source #48611
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
base: main
Are you sure you want to change the base?
Conversation
8bd93d0
to
44e9126
Compare
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: extensions/smallrye-openapi/deployment
! Skipped: devtools/bom-descriptor-json extensions/agroal/deployment extensions/elytron-security-jdbc/deployment and 58 more
📦 extensions/smallrye-openapi/deployment
✖ io.quarkus.smallrye.openapi.test.vertx.OpenApiDefaultPathTestCase.
- History - More details - Source on GitHub
java.lang.RuntimeException: Failed to start quarkus
at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
at io.quarkus.runtime.Application.start(Application.java:101)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at io.quarkus.runner.bootstrap.StartupActionImpl.run(StartupActionImpl.java:329)
at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:697)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.OutOfMemoryError: Metaspace
✖ io.quarkus.smallrye.openapi.test.vertx.SwaggerAndOpenAPIWithCommonPrefixTest.
- History - More details - Source on GitHub
java.lang.ExceptionInInitializerError
at io.quarkus.runtime.generated.StaticInitConfigCustomizer.configBuilder(Unknown Source)
at io.smallrye.config.SmallRyeConfigBuilder.build(SmallRyeConfigBuilder.java:769)
at io.quarkus.runtime.generated.Config.<clinit>(Unknown Source)
at io.quarkus.runner.ApplicationImpl.<clinit>(Unknown Source)
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:534)
at java.base/java.lang.Class.forName(Class.java:513)
✖ io.quarkus.smallrye.openapi.test.vertx.SwaggerAndOpenAPIWithCommonPrefixTest.shouldWorkEvenWithCommonPrefix
line 26
- History - More details - Source on GitHub
java.lang.AssertionError:
1 expectation failed.
Expected status code <200> but was <500>.
at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:73)
Flaky tests - Develocity
⚙️ JVM Integration Tests - JDK 17
📦 integration-tests/opentelemetry-grpc-only
✖ io.quarkus.it.opentelemetry.grpc.HelloGrpcClientTest.testHello
- History
java.lang.RuntimeException: Failed to start quarkus
-java.lang.RuntimeException
java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:668)
at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:763)
at java.base/java.util.Optional.orElseGet(Optional.java:364)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.RuntimeException: Failed to start quarkus
at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
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 this is good and we should merge it but I would like to have #48655 in first as atm ForbiddenAPIs is run on a small subpart of the tree so there might be more changes needed here once we generalize it.
I also think once my PR is in, we should start aggressively ban everything that shouldn't be used in our tree.
See discussion here: #48432 (comment)
In order to make the ban work, I explicitly list every logger method, so that
Log
itself didn't trigger the ban. An alternative would be to had to exclude Logger itself from forbidden api checking, either with a maven exclude or@SuppressForbidden
annotation. That reduces the risk of missing one method from my list, but increases the risk of missing something else banned inLog
.While getting CI green I spotted three classes which were using
Log
. Only one of them was my fault, as far as I can tell. :)In case we want it later, this is what we maven exclude looks like: