-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Enhanced extension dependencies validation #14609
Enhanced extension dependencies validation #14609
Conversation
Thanks @aloubyansky ! @iocanel can you do the review of the client? My back log of stuff has grown significantly today and I don't want to hold up this until I can get to it |
9cf9ed9
to
c82542c
Compare
4e35f82
to
133f14e
Compare
<exclusion> | ||
<groupId>com.squareup.okhttp3</groupId> | ||
<artifactId>okhttp</artifactId> | ||
</exclusion> | ||
--> |
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.
Either we drop it the exclusion entirely or at least we add a comment explaining the why. Because this is puzzling.
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.
The changes look good to me but... we would need this PR to be organized so that the build is not broken (typically we could have a commit fixing all the issues BEFORE the commit implementing the checks).
Maybe it's already working this way but the commits look a bit suspicious :).
A PR adjusting the deps before the enhanced validation is doable. I'm still waiting for the feedback on the kubernetes client deps. |
@iocanel can you give this a look please? |
@iocanel any chance you can take a look at this? |
<dependency> | ||
<groupId>io.fabric8</groupId> | ||
<artifactId>kubernetes-client</artifactId> | ||
<exclusions> |
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.
Optional: The exclusions below should no longer be needed as of kubernetes-client 5.0.1 (currently using 5.0.2).
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.
The Kubernetes related bits look good to me.
I added an optional remark about some exclusions we could possibly remove, but given that this might be a little out of scope, we could definitely omit it for now.
133f14e
to
ca1a66b
Compare
</dependency> | ||
<dependency> | ||
<groupId>io.quarkus</groupId> | ||
<artifactId>quarkus-datasource</artifactId> |
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.
@stuartwdouglas I haven't yet looked deeper but just as a quick fix to make the enhanced extension dependency validation to pass I had to add quarkus-datasource
extension as a runtime dependency to most of the jdbc extensions. Otherwise, it would complain that the deployment artifact depends on quarkus-datasource-deployment
through quarkus-devservices-XXX
but the quarkus-datasource
isn't found among the runtime dependencies.
@gsmet the commits are re-ordered as you requested. But I still want to clarify the |
ca1a66b
to
38e17de
Compare
@gsmet This one should be good now. |
38e17de
to
734bd17
Compare
<exclusion> | ||
<groupId>com.squareup.okhttp3</groupId> | ||
<artifactId>okhttp</artifactId> | ||
</exclusion> |
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.
Why did you remove this one?
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.
As a consequence of refactoring of the kubernetes client dependencies, otherwise the tests would fail due to missing classes.
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.
Excluding that dependency results in failures like
Caused by: java.lang.NoClassDefFoundError: okio/Sink
at io.fabric8.kubernetes.client.server.mock.KubernetesServer.before(KubernetesServer.java:69)
at io.quarkus.test.kubernetes.client.KubernetesServerTestResource.initServer(KubernetesServerTestResource.java:39)
at io.quarkus.test.kubernetes.client.AbstractKubernetesTestResource.start(AbstractKubernetesTestResource.java:25)
at io.quarkus.test.common.TestResourceManager.lambda$start$1(TestResourceManager.java:105)
at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.lang.ClassNotFoundException: okio.Sink
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
…figured wrt runtime/deployment artifacts
734bd17
to
fbfcff9
Compare
As a follow up to #14463
Here is an enhanced version of the extension dependencies validator.
In addition to making sure for every dependency on the runtime extension artifact there is the corresponding dependency on the deployment extension artifact, this version also checks that:
FYI @stuartwdouglas @geoand
@geoand and @iocanel please review the kubernetes client related adjustments.