-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Added more relevant constraints to the quarkus-bom #26565
Conversation
1da9688
to
612524a
Compare
@gsmet I added wildfly-elytron-* and bitbucket after talking to @sberyozkin |
bom/application/pom.xml
Outdated
@@ -14,6 +14,7 @@ | |||
<packaging>pom</packaging> | |||
|
|||
<properties> | |||
<bitbucket.version>0.7.12</bitbucket.version> |
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 one probably needs to be added to Dependabot? Or put together with the dependencies that need alignment if it needs to be aligned (together with comments indicating the need for alignment).
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.
@sberyozkin I added a direct dependency on jose4j to the module using its API directly. So it is both a direct dependency and a transitive dependency through io.smallrye:smallrye-jwt-build
. Would you like to control it in the quarkus-bom
or use whatever smallrye-jwt-build
depends on? In the later case, perhaps we shouldn't add it to the BOM, unless you think the probability of conflicts will be high. smallrye-jwt-build
is already managed, so if it comes only as a dependency of smallrye-jwt-build
there won't be a problem if don't add it to the BOM.
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.
@gsmet @sberyozkin I removed jose4j from this PR for now. Let's handle it separately from the rest.
This comment has been minimized.
This comment has been minimized.
612524a
to
88c213a
Compare
This comment has been minimized.
This comment has been minimized.
88c213a
to
59afbb0
Compare
This comment has been minimized.
This comment has been minimized.
59afbb0
to
deda35b
Compare
The Infinispan client's HealthCheckTest is failing because it's not compatible with the Hibernate Search version we are using including in our BOM. It works with 6.0.8, while the version we are including is 6.1.5. |
Failing Jobs - Building deda35b
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 Windows #- Failing: extensions/mongodb-client/deployment
! Skipped: extensions/liquibase-mongodb/deployment extensions/panache/mongodb-panache-common/deployment extensions/panache/mongodb-panache-kotlin/deployment and 8 more 📦 extensions/mongodb-client/deployment✖
✖
|
<groupId>org.hibernate.search</groupId> | ||
<artifactId>hibernate-search-util-common</artifactId> | ||
<version>${hibernate-search.version}</version> | ||
</dependency> |
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.
@yrodiere could you have a look at this when you're back? It looks like we cannot enforce Hibernate Search's version properly when using Infinispan.
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.
See this comment specifically: #26565 (comment)
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.
@gsmet wdyt about merging it a figuring out the hibernate-search/infinispan compatibility separately? Given that the rest of the constraints aren't related to that.
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.
@gsmet I can have a look but I'm not sure what I can do? The version of Infinispan bundled with Quarkus hasn't been updated to HSearch 6.1 yet. Infinispan's main
branch has been upgraded, but I don't think it's been published yet.
The only immediate solutions I see are:
- Upgrading to a newer version of Infinispan; not sure there is one.
- Downgrading Hibernate Search; that doesn't seem wise.
- Accepting we won't have matching versions. And that people can't use both Infinispan embedded and the Hibernate Search extension in the same application.
In the future, there are a few possible strategies, none of them great:
- Accepting we won't have matching versions. And that people can't use both Infinispan embedded and the Hibernate Search extension in the same application.
- Never breaking any SPI in Hibernate Search, ever again.
- Waiting for Inifinispan to be released whenever we upgrade Hibernate Search, before upgrading in Quarkus.
- Shading Hibernate Search into Infinispan Embedded.
Personally I'd favor 4. It's really supposed to be an internal dependency of Infinispan after all, since they're working hard on hiding all Hibernate Search APIs.
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.
Last time I checked there wasn't a more recent Infinispan version for us to upgrade to, unfortunately.
Never breaking the SPI (2) is a great one :) And if you do, a safe way would be to coordinate releases, so that there is combination of Hibernate Search and Infinispan that works, which is (3).
Shading is an option, although it may complicate CVE tracking and patch delivery. But maybe not that much.
As to how wise downgrading the HS version is, it depends on how often people will run into this issue, basically how critical it is to have a working combination of the two. I am not sure for long it has been broken and whether anybody has run into it yet.
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.
Despite all the complications, we shouldn't just let it go neither. Today we could document it as not being supported. Until one day we get one or more important use-cases that require both HS and Infinispan being present in the same app, at that moment we'll have to make it work somehow and starting from that moment, we will want to preserve compatibility and not have random breakages between releases. So thinking/preparing for that moment could be worth it.
@gsmet I don't think the rest of the changes should be blocked by this issue. Would you prefer to remove the comments I added about the HS/Infinispan issue from the BOM?
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.
Nope, I will merge it, I was busy on other things.
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.
not have random breakages between releases
It's not random, it's when necessary, on minor versions, in SPIs only, and that's a clearly established policy for all Hibernate projects. Let's not assume we're breaking stuff just because we don't care: we do :)
The exact reason we're having a problem is I'd been told in the past that there would not be any Infinispan Embedded in Quarkus, and since the HotRod (non-Embedded) client does not depend on Hibernate Search, the Hibernate Search dependency shouldn't have affected it at all.
So thinking/preparing for that moment could be worth it.
Certainly.
I'm back from PTO, so I'll be able to look further. First step would be to determine why the Hibernate Search dependency affects Infnispan at all; I thought Infinispan Embedded had been added to Quarkus without my knowledge, but that doesn't seem to be the case? It seems the error is really just in the HotRod client, which shouldn't be depending on Hibernate Search in the first place. I'll look into that.
What should I do to reproduce the issue; just uncomment those lines, build the relevant modules and run Quarkus + Infinispan integration tests?
Today we could document it as not being supported
Sure, documenting would be a good temporary solution.
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.
Yes, to reproduce uncomment the constraints and run io.quarkus.it.infinispan.client.HealthCheckTest.testHealthCheck
, you can see the failures from the CI above. Thanks!
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.
Thanks.
For future reference, the problem was in the test dependencies rather than in the "main" dependencies. Guillaume addressed it here: #26647
No description provided.