-
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
Fix Pulsar DevService network for @QuarkusIntegrationTest for docker-image-building #44265
Fix Pulsar DevService network for @QuarkusIntegrationTest for docker-image-building #44265
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
61d1823
to
6f99bbc
Compare
This comment has been minimized.
This comment has been minimized.
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 looks good, thanks! But I prefer having @ozangunalp 's blessing too.
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.
Looks good. My comment is mostly nitpicking.
container.start(); | ||
|
||
return getRunningService(container.getContainerId(), container::close, container.getPulsarBrokerUrl(), | ||
container.getHttpServiceUrl()); | ||
var pulsarBrokerUrl = container.getPulsarBrokerUrl(); |
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'd do all this inside the PulsarContainer
.
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.
Sure, moved.
Status for workflow
|
Fixes #44264
Tested manually for project attached to #44264
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.