Skip to content
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

Fix Elasticsearch DevService network for @QuarkusIntegrationTest for docker-image-building #44120

Conversation

jgardo
Copy link
Contributor

@jgardo jgardo commented Oct 26, 2024

Fixes #43980

Tested manually for project attached to #43980

Unfortunately I couldn't find any test verifying @QuarkusIntegrationTest of created docker image...

Such tests occurred in the past, but they were dropped in PR #31295 . I suppose these tests were dropped intentionally, as time of CI build was too long.

Copy link

quarkus-bot bot commented Oct 26, 2024

/cc @gsmet (elasticsearch), @loicmathieu (elasticsearch), @marko-bekhta (elasticsearch), @yrodiere (elasticsearch)

Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

Unfortunately I couldn't find any test verifying @QuarkusIntegrationTest of created docker image...

We do test Elasticsearch dev services, but indeed not with @QuarkusIntegrationTest.

I would personally be in favor of just using dev services in most integration tests, e.g. in integration-tests/elasticsearch-rest-client, instead of relying on separate configuration.
But it's certainly outside the scope of this PR, so please don't do that here, and there might be reasons not to do it -- @geoand would know.

@yrodiere yrodiere added triage/waiting-for-ci Ready to merge when CI successfully finishes triage/backport-3.15 triage/backport labels Oct 28, 2024
@geoand
Copy link
Contributor

geoand commented Oct 28, 2024

But it's certainly outside the scope of this PR, so please don't do that here, and there might be reasons not to do it -- @geoand would know.

Totally agree. Dev services work just fine with QuarkusIntegrationTest

@yrodiere
Copy link
Member

But it's certainly outside the scope of this PR, so please don't do that here, and there might be reasons not to do it -- @geoand would know.

Totally agree. Dev services work just fine with QuarkusIntegrationTest

There might be a misunderstanding here, because that's not what I meant at all. Anyway, I created #44124 to discuss this.

@gsmet
Copy link
Member

gsmet commented Oct 28, 2024

FWIW, I remember having fixed something similar in another dev service. I wonder if it's something we need to fix globally. Definitely not in the scope of this PR, just thinking out loud.

@gsmet
Copy link
Member

gsmet commented Oct 28, 2024

To be clear, I was thinking about this PR https://github.com/quarkusio/quarkus/pull/42472/files, where we fix a slightly different issue for MongoDB but it ends up adjusting the same values for different reasons.

This comment has been minimized.

@jgardo
Copy link
Contributor Author

jgardo commented Oct 28, 2024

To be clear, I was thinking about this PR https://github.com/quarkusio/quarkus/pull/42472/files, where we fix a slightly different issue for MongoDB but it ends up adjusting the same values for different reasons.

I suppose there are some devservices to fix (at least these using io.quarkus.devservices.common.ConfigureUtil#configureSharedNetwork without storing result hostName).

Personally I also would need to fix Pulsar devservice. So should I create another issue and PR or with same issue with separate PR?

@jgardo jgardo force-pushed the bugfix/elasticsearch-devservice-network-with-it branch from a1e1209 to 8a2ab24 Compare October 28, 2024 23:23
@jgardo jgardo requested a review from yrodiere October 28, 2024 23:23
@yrodiere
Copy link
Member

yrodiere commented Oct 29, 2024

Personally I also would need to fix Pulsar devservice. So should I create another issue and PR or with same issue with separate PR?

Please open a separate PR, thanks. No need for a dedicated issue as long as the PR correctly describes the problem.

EDIT: issue => PR, sorry...

Copy link

quarkus-bot bot commented Oct 29, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 8a2ab24.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@yrodiere yrodiere merged commit 49870be into quarkusio:main Oct 29, 2024
20 checks passed
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Oct 29, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 29, 2024
@gsmet gsmet modified the milestones: 3.17 - main, 3.16.1 Oct 29, 2024
@jgardo jgardo deleted the bugfix/elasticsearch-devservice-network-with-it branch November 4, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing devservices-containing @QuarkusIntegrationTest for container-image.build=true on MacOS
4 participants