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

Bump kubernetes-client-bom from 6.13.4 to 7.0.1 #45259

Merged
merged 8 commits into from
Jan 4, 2025

Conversation

manusa
Copy link
Contributor

@manusa manusa commented Dec 23, 2024

Description

Superseded closes #42335
Fixes #44899

This PR is built on top of the changes provided by @metacosm in #42335.

The most notable change is the removal of the quarkus-test-openshift-client.
We'll need to document a migration path for those users who were relying on this module.

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/kubernetes area/testing labels Dec 23, 2024
@manusa
Copy link
Contributor Author

manusa commented Dec 24, 2024

Marking as ready to start the test suites (some tests I can't seem to run locally)

@manusa manusa marked this pull request as ready for review December 24, 2024 04:41

This comment has been minimized.

@manusa manusa force-pushed the feat/deps/kubernetes-client branch from c45403c to f6ef338 Compare December 24, 2024 04:56
@manusa manusa force-pushed the feat/deps/kubernetes-client branch from f6ef338 to ae02fe0 Compare December 24, 2024 05:50
@manusa manusa force-pushed the feat/deps/kubernetes-client branch from 942ca28 to 1bec3cd Compare December 24, 2024 08:42
@@ -26,7 +26,7 @@ public void checkEnvironment(Optional<SelectedKubernetesDeploymentTargetBuildIte
}
if (target.getEntry().getName().equals(OPENSHIFT)) {
try (var openShiftClient = kubernetesClientBuilder.buildClient().adapt(OpenShiftClient.class)) {
if (openShiftClient.isSupported()) {
if (openShiftClient.hasApiGroup("openshift.io", false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be true here? Unless I'm missing something, OpenShift being supported should mean that the openshift.io group is available on the cluster, no? Seems like in the migration the condition got inverted…

Copy link
Contributor Author

@manusa manusa Dec 24, 2024

Choose a reason for hiding this comment

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

The second argument is not negating the method but specifying that the match has to be exact or not ( true exact match, false partial match).
In this case, the meaning is that if there is any API group that partially contains this string, then we consider it to have the API group.

Copy link
Contributor

Choose a reason for hiding this comment

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

TIL! 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Was wondering when I started looking into this if we should keep these annotations and make then return a mock KubernetesServer instead to ease migration but I haven't looked too deep so maybe that wouldn't gain us much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's something I've thought and even started to do (in the spirit of trying to minimize breakages).
The problem is that the OpenShiftMockServer class no longer exists, so the breakage will remain even if we modify t to inject a KubernetesMockServer.
Please, feel free to see if you can devise some way to minimize the breakages.
This is for now the most conflicting point, since I'm not sure what the Quarkus policy requires for removing a module such as this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity what's that class for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nevermind, just saw the associated commit that explains it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It removes the default io.fabric8.kubernetes.client.vertx.VertxHttpClientFactory

With the fix introduced in v7.0.1, this class causes problems in Graal since it instantiates a Vertx instance.
The class shouldn't be necessary in Quarkus since it uses its very own QuarkusHttpClientFactory.

@manusa manusa force-pushed the feat/deps/kubernetes-client branch from 1bec3cd to 08b7c5e Compare December 24, 2024 13:56
Copy link

quarkus-bot bot commented Dec 24, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 08b7c5e.

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

You can consult the Develocity build scans.

@geoand geoand requested a review from metacosm December 30, 2024 06:36
@metacosm
Copy link
Contributor

metacosm commented Jan 3, 2025

Looks like things are working OK on QOSDK's side based on https://github.com/quarkiverse/quarkus-operator-sdk/actions/runs/12584459676/job/35074153045

@gastaldi gastaldi merged commit 72dd7dc into quarkusio:main Jan 4, 2025
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Jan 4, 2025
@manusa manusa deleted the feat/deps/kubernetes-client branch January 8, 2025 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump Kubernetes-Client to v7.0
4 participants