Skip to content

Conversation

yrodiere
Copy link
Member

@yrodiere yrodiere commented Oct 7, 2025

@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Oct 7, 2025

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@yrodiere yrodiere changed the base branch from main to 7.1 October 7, 2025 07:53
@yrodiere
Copy link
Member Author

yrodiere commented Oct 7, 2025

The benefit/risk ratio of backporting this seems reasonable, WDYT @mbellade ?

Copy link
Member

@mbellade mbellade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, the change seems pretty straightforward

@yrodiere
Copy link
Member Author

yrodiere commented Oct 7, 2025

Okay, plenty of failures... but:

  1. The Quarkus run seems to be failing because we haven't merged Use dev services instead of maven docker plugin for Hibernate Search tests quarkusio/quarkus#48512 yet (:/)
  2. TCK failures are the same ones as on the 7.1 branch: https://ci.hibernate.org/job/hibernate-orm-tck-3.2/job/7.1/34/console

Sybase failures are suspicious but ring a bell: https://ci.hibernate.org/job/hibernate-orm-pipeline/job/PR-11072/1/testReport/org.hibernate.orm.test.query.hql/LiteralTests/Build___sybase_jconn___Test___testEnumLiteralInPredicate_SessionFactoryScope_/

I'll try to have a look...


Regarding continuous improvement...

For 1 I guess we'd need someone to find time to refresh the PR and get it merged + backported. Marko is likely busy though.

For 2 there is most likely a pending Jakarta Persistence challenge, but I'll check.

@yrodiere
Copy link
Member Author

yrodiere commented Oct 7, 2025

@yrodiere yrodiere merged commit 0126999 into hibernate:7.1 Oct 7, 2025
8 of 13 checks passed
@marko-bekhta
Copy link
Member

For 1 I guess we'd need someone to find time to refresh the PR and get it merged + backported

mm there's a bit more to it ... we need to be able to start multiple ES services for integration tests to be able to switch to dev services, and that means.. we should start with a named ES clients so that we can have multiple configs, then update the ES dev service to be able to start multiple instances, and then we can refresh the PR 😕
It's on todo ... but not yet the first line 🫣 🙁

@yrodiere
Copy link
Member Author

yrodiere commented Oct 8, 2025

For 1 I guess we'd need someone to find time to refresh the PR and get it merged + backported

mm there's a bit more to it ... we need to be able to start multiple ES services for integration tests to be able to switch to dev services, and that means.. we should start with a named ES clients so that we can have multiple configs, then update the ES dev service to be able to start multiple instances, and then we can refresh the PR 😕 It's on todo ... but not yet the first line 🫣 🙁

Can't you keep the maven-docker-plugin just for the extra containers?

@marko-bekhta
Copy link
Member

I think then the "original" problem would still be there, and the plugin will be trying to manage the instances started by devservices 😔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants