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

#403 pipeline-invocation-service java17 upgrade #416

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

csun-cpointe
Copy link
Contributor

No description provided.

@@ -1,4 +1,4 @@
FROM registry.access.redhat.com/ubi9/openjdk-11-runtime:1.20 AS builder
FROM registry.access.redhat.com/ubi9/openjdk-17-runtime:1.20 AS builder
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I: Although it's the same image, use a multistage build to save more than 20 mb in space.

@@ -101,7 +101,7 @@ protected List<String> buildFinalValuesOverrides(PipelineInvocationRequest reque
List<String> args = new ArrayList<>();

args.add("--set");
args.add("spec.serviceEnabled=false");
args.add("service.enabled=false");
Copy link
Contributor Author

@csun-cpointe csun-cpointe Oct 16, 2024

Choose a reason for hiding this comment

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

I: spec.serviceEnabled has been updated to service.enabled

driver:
javaOptions: "-DKRAUSENING_BASE=/opt/spark/krausening/base -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:4747"
executor:
javaOptions: "-DKRAUSENING_BASE=/opt/spark/krausening/base -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:4747"


service:
enabled: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I: same here

{{- toYaml . | nindent 6 }}
{{- end }}
{{- end }}
Copy link
Contributor Author

@csun-cpointe csun-cpointe Oct 16, 2024

Choose a reason for hiding this comment

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

I: Including the missing change for the argocd spark-operator template.

Copy link
Contributor

@ewilkins-csi ewilkins-csi Oct 17, 2024

Choose a reason for hiding this comment

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

I: See the Known Issues for release 1.8. This may need to be backported to 1.9.

@csun-cpointe csun-cpointe linked an issue Oct 16, 2024 that may be closed by this pull request
10 tasks
@csun-cpointe csun-cpointe force-pushed the 403-pipeline-invocation-java17-upgrade branch 2 times, most recently from 31c510e to be7abcb Compare October 16, 2024 23:09
@@ -83,7 +71,7 @@
</dependency>
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-rest-client-reactive-jackson</artifactId>
<artifactId>quarkus-resteasy-reactive-jackson</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Will this also affect extensions-messaging-quarkus? And should we be using that module here instead of duplicating the messaging setup?

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked offline. It's unclear why this module was using the classic resteasy client instead of this one, but this one works and the other does not. It seems these modules have had a lot of changes in recent versions of Quarkus.

As for using extensions-messaging-quarkus, that module only provides a CDI container config. Most (all?) Quarkus apps will not rely on this, so it's unclear why this module uses Quarkus at all. It's used in alerting-teams to setup the TeamsCdiContext, but that class is unused by the service. I think that this is just unneeded code that could be removed.

@csun-cpointe csun-cpointe force-pushed the 403-pipeline-invocation-java17-upgrade branch from be7abcb to 0efcfb8 Compare October 17, 2024 17:08
@csun-cpointe csun-cpointe force-pushed the 403-pipeline-invocation-java17-upgrade branch 2 times, most recently from 37f87e1 to 4d120f6 Compare October 17, 2024 18:36
@csun-cpointe csun-cpointe force-pushed the 403-pipeline-invocation-java17-upgrade branch from 4d120f6 to 450f91c Compare October 17, 2024 18:43
@csun-cpointe csun-cpointe merged commit d8d4381 into dev Oct 17, 2024
@csun-cpointe csun-cpointe deleted the 403-pipeline-invocation-java17-upgrade branch October 17, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: JDK 17 Upgrade Support Series - Pipeline Invocation Service
3 participants