-
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
Property to enable/disable default client in Infinispan Dev Services #39918
Property to enable/disable default client in Infinispan Dev Services #39918
Conversation
This property should be helpful @jamesnetherton @gsmet this should be backported to the latest LTS 🙏🏻 |
This comment has been minimized.
This comment has been minimized.
2d3c3a6
to
a3b2c48
Compare
This comment has been minimized.
This comment has been minimized.
a3b2c48
to
fb1cadc
Compare
Status for workflow
|
@@ -259,7 +259,7 @@ private static class QuarkusInfinispanContainer extends InfinispanContainer { | |||
|
|||
public QuarkusInfinispanContainer(String clientName, InfinispanDevServicesConfig config, | |||
LaunchMode launchMode, boolean useSharedNetwork) { | |||
super(config.imageName.orElse(IMAGE_BASENAME + ":" + Version.getMajorMinor())); | |||
super(config.imageName.orElse(IMAGE_BASENAME + ":" + Version.getVersion())); |
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.
@karesti you also changed this in this patch. What was the rationale? It doesn't look strictly related and I want to make sure that's something we want in 3.8.
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.
getMajorMinor will retrieve 14.0 while getVersion will retrieve 14.0.16.Final image
We update images to map the latest minor. However, if the user has the image locally (14.0) won't get updated. There might be bugs that are fixed on the server that will still be present in the local machine of the user for this reason. Forcing with getVersion the dev service container image, we guarantee the user is running the same version locally and on the server.
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 admit the change is kinda unrelated, but I was having some trouble so I added the change. Probably another PR would have been better
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.
OK, I think the rationale makes sense and we can backport it too.
@rsvoboda WDYT?
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.
Agree, let's have it @gsmet.
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.
So this is a problem in the end, because on product side we end up with something like quay.io/infinispan/server:14.0.27.Final-redhat-00001
I guess we need to get back to super(config.imageName.orElse(IMAGE_BASENAME + ":" + Version.getMajorMinor()));
even for main because that would be a problem for the next feature release
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.
@rsvoboda I need to check with the team, but for upstream this is the best way for dev services, otherwise during N minor versions, fixes and updates, unless we always pull the image locally, it will potentially fail. For downstream I need to check
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.
@rsvoboda I understand the issue, I'll change this back but I need to figure out how to avoid this problem in Quarkus upstream
Closes #39906