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.

Sign up for GitHub

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

Jump to bottom

Remove OpenshiftBaseXXXImage classes #37349

Merged
merged 2 commits into from
Dec 7, 2023
+3 −177
Merged

Remove OpenshiftBaseXXXImage classes #37349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8bc5a93
refactor: remove OpenshiftBaseXXXImage classes
iocanel Nov 28, 2023
f6f8626
Fix OpenshiftWithS2iTest
gsmet Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 0 additions & 86 deletions ...src/main/java/io/quarkus/container/image/openshift/deployment/OpenshiftBaseJavaImage.java
View file
Open in desktop

This file was deleted.

57 changes: 0 additions & 57 deletions ...c/main/java/io/quarkus/container/image/openshift/deployment/OpenshiftBaseNativeImage.java
View file
Open in desktop

This file was deleted.

32 changes: 3 additions & 29 deletions ...ent/src/main/java/io/quarkus/container/image/openshift/deployment/OpenshiftProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised the tests are passing as I had test failures when I removed the images but good if they pass.

Copy link
Member

Choose a reason for hiding this comment

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

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:

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

@gsmet I''ll have a look when I'm back from Switzerland, not before sunday.

Copy link
Member

Choose a reason for hiding this comment

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

@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.


if (config.buildStrategy == BuildStrategy.BINARY) {
// Jar directory priorities:
// 1. explicitly specified by the user.
// 2. detected via OpenshiftBaseJavaImage
// 3. fallback value
String jarDirectory = config.jarDirectory
.orElse(baseImage.map(i -> i.getJarDirectory()).orElse(config.FALLBACK_JAR_DIRECTORY));
String jarDirectory = config.jarDirectory.orElse(config.FALLBACK_JAR_DIRECTORY);
String pathToJar = concatUnixPaths(jarDirectory, jarFileName);

// If the image is known, we can define env vars for classpath, jar, lib etc.
baseImage.ifPresent(b -> {
envProducer.produce(KubernetesEnvBuildItem.createSimpleVar(b.getJarEnvVar(), pathToJar, null));
envProducer.produce(KubernetesEnvBuildItem.createSimpleVar(b.getJvmOptionsEnvVar(),
String.join(" ", config.getEffectiveJvmArguments()), null));
});

//In all other cases its the responsibility of the image to set those up correctly.
if (hasCustomJarPath || hasCustomJvmArguments) {
List<String> cmd = new ArrayList<>();
Expand All @@ -172,8 +162,6 @@ public void openshiftRequirementsJvm(ContainerImageOpenshiftConfig openshiftConf
cmd.addAll(Arrays.asList("-jar", pathToJar));
envProducer.produce(KubernetesEnvBuildItem.createSimpleVar(JAVA_APP_JAR, pathToJar, null));
commandProducer.produce(KubernetesCommandBuildItem.command(cmd));
} else if (baseImage.isEmpty()) {
envProducer.produce(KubernetesEnvBuildItem.createSimpleVar(JAVA_APP_JAR, pathToJar, null));
}
}
}
Expand Down Expand Up @@ -209,26 +197,12 @@ public void openshiftRequirementsNative(ContainerImageOpenshiftConfig openshiftC

if (config.buildStrategy == BuildStrategy.BINARY) {
builderImageProducer.produce(new BaseImageInfoBuildItem(config.baseNativeImage));
Optional<OpenshiftBaseNativeImage> baseImage = OpenshiftBaseNativeImage.findMatching(config.baseNativeImage);
// Native binary directory priorities:
// 1. explicitly specified by the user.
// 2. detected via OpenshiftBaseNativeImage
// 3. fallback value
// 2. fallback vale

String nativeBinaryDirectory = config.nativeBinaryDirectory
.orElse(baseImage.map(i -> i.getNativeBinaryDirectory()).orElse(config.FALLBACK_NATIVE_BINARY_DIRECTORY));
String nativeBinaryDirectory = config.nativeBinaryDirectory.orElse(config.FALLBACK_NATIVE_BINARY_DIRECTORY);
String pathToNativeBinary = concatUnixPaths(nativeBinaryDirectory, nativeBinaryFileName);

baseImage.ifPresent(b -> {
envProducer.produce(
KubernetesEnvBuildItem.createSimpleVar(b.getHomeDirEnvVar(), nativeBinaryDirectory, OPENSHIFT));
config.nativeArguments.ifPresent(nativeArguments -> {
envProducer.produce(KubernetesEnvBuildItem.createSimpleVar(b.getOptsEnvVar(),
String.join(" ", nativeArguments), OPENSHIFT));
});

});

if (hasCustomNativePath || hasCustomNativeArguments) {
commandProducer
.produce(KubernetesCommandBuildItem.commandWithArgs(pathToNativeBinary, config.nativeArguments.get()));
Expand Down
5 changes: 0 additions & 5 deletions ...tes/quarkus-standard-way/src/test/java/io/quarkus/it/kubernetes/OpenshiftWithS2iTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,6 @@ public void assertGeneratedResources() throws IOException {
assertThat(envVar.getName()).isEqualTo("JAVA_APP_JAR");
//assertThat(envVar.getValue()).isEqualTo("/deployments/quarkus-run.jar"); // this is flaky
});
assertThat(envVars).anySatisfy(envVar -> {
assertThat(envVar.getName()).isEqualTo("JAVA_OPTIONS");
assertThat(envVar.getValue())
.contains("-Djava.util.logging.manager=org.jboss.logmanager.LogManager");
});
});
});

Expand Down