Remove OpenshiftBaseXXXImage classes #37349
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I think we have the same sort of issue with:
- https://github.com/quarkusio/quarkus/blob/main/extensions/container-image/container-image-openshift/deployment/src/main/java/io/quarkus/container/image/openshift/deployment/S2iBaseJavaImage.java
- https://github.com/quarkusio/quarkus/blob/main/extensions/container-image/container-image-s2i/deployment/src/main/java/io/quarkus/container/image/s2i/deployment/S2iBaseJavaImage.java
| @@ -146,24 +146,14 @@ public void openshiftRequirementsJvm(ContainerImageOpenshiftConfig openshiftConf | |||
| boolean hasCustomJvmArguments = config.jvmArguments.isPresent(); | |||
|
|
|||
| builderImageProducer.produce(new BaseImageInfoBuildItem(baseJvmImage)); | |||
| Optional<OpenshiftBaseJavaImage> baseImage = OpenshiftBaseJavaImage.findMatching(baseJvmImage); | |||
There was a problem hiding this comment.
I'm surprised the tests are passing as I had test failures when I removed the images but good if they pass.
There was a problem hiding this comment.
Yes so I can confirm the tests are not passing and will require some adjustments:
In integration-tests/kubernetes/quarkus-standard-way, you will have this failure:
[ERROR] Failures:
[ERROR] OpenshiftWithS2iTest.assertGeneratedResources:74
[List check single element] (1 failure)
-- failure 1 --
[List check single element] (1 failure)
-- failure 1 --
Expecting any element of:
[EnvVar(name=KUBERNETES_NAMESPACE, value=null, valueFrom=EnvVarSource(configMapKeyRef=null, fieldRef=ObjectFieldSelector(apiVersion=null, fieldPath=metadata.namespace, additionalProperties={}), resourceFieldRef=null, secretKeyRef=null, additionalProperties={}), additionalProperties={}),
EnvVar(name=JAVA_APP_JAR, value=/deployments/quarkus-run.jar, valueFrom=null, additionalProperties={})]
to satisfy the given assertions requirements but none did:
There was a problem hiding this comment.
@famod this is interesting as for GIB is concerned. GIB didn't run the integration-tests/kubernetes/quarkus-standard-way module because there is no explicit dependency in the POM. We cannot really add the dependencies to the POM because they are actually conditional and used specifically in some of the tests. I'm not completely sure how we could create a dependency there?
There was a problem hiding this comment.
@gsmet I''ll have a look when I'm back from Switzerland, not before sunday.
There was a problem hiding this comment.
@iocanel I fixed the test in a follow-up commit I just pushed but... I'm wondering if we shouldn't push the JAVA_OPTS another way? (Or maybe they are already pushed another way?) Setting the log manager is important.
I will let you finalize this.
The container-image-s2i extension is deprecated in favor of container-image-openshift and we IMHO, we should just drop it. |
OK, I can live with keeping it as is then, pending a discussion to drop the extension. We just need to fix the Thanks! |
JAVA_OPTS are not automatically added anymore. But TBH, I wonder if it's the right move to not have these JAVA_OPTS as we need to set the logger anyway.
gsmet
force-pushed
the
remove-base-openshift-images
branch
from
f91fd5d
to
f6f8626
Compare
I will drop the container-image-s2i extension in a separate PR but shouldn't we sort out https://github.com/quarkusio/quarkus/blob/main/extensions/container-image/container-image-openshift/deployment/src/main/java/io/quarkus/container/image/openshift/deployment/S2iBaseJavaImage.java in this PR? |
|
✔️ 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. |
Resolves: #36577